New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

home-manager:init at 2017-10-11 #30330

Merged

peterhoeg merged 12 commits into NixOS:master from pasqui23:home-manager

Dec 30, 2017

Contributor

pasqui23 commented Oct 11, 2017 •

edited

Motivation for this change

home-manager is a manager of user environments using nix

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

pasqui23 added 2 commits

October 11, 2017 23:55


          home-manager:init at 2017-10-11

66f9f05


          adding wrapper for NIX_PATH

8d06775

bjornfor requested a review from rycee

October 12, 2017 06:16

rycee reviewed

View reviewed changes

pkgs/applications/misc/home-manager/default.nix Outdated

+                    --subst-var-by MODULES_PATH '${modulesPathStr}' \
+                    --subst-var-by HOME_MANAGER_EXPR_PATH "${dot}.nix"
+                  wrapProgram $out/bin/home-manager --prefix NIX_PATH : home-manager=${src}/modules

Member

rycee Oct 12, 2017

Instead of setting NIX_PATH here could set

--subst-var-by MODULES_PATH '${if modulesPath == null then "${src}/modules" else modulesPath}'

above. Then a user could override the path to the modules, if they want, but by default it should use the downloaded ones.

rycee reviewed

View reviewed changes

pkgs/top-level/all-packages.nix Outdated

		@@ -1115,6 +1115,8 @@ with pkgs;

		hid-listen = callPackage ../tools/misc/hid-listen { };

		home-manager = callPackage ../applications/misc/home-manager {};

Member

rycee Oct 12, 2017

I think Home Manager would be considered a tool rather than an application since it is not intended for interactive use.

Member

rycee commented Oct 12, 2017

It would be really nice to have Home Manager be installable directly from Nixpkgs but I've avoided it thus far due to the general difficulty posed by keeping the modules up-to-date. Basically, whenever something changes in the Home Manager modules we'd have to bump the version of the Nixpkgs package. Maybe it is not a big problem for Nixpkgs unstable but once it ends up in a stable version things get trickier, I guess.

rycee reviewed

View reviewed changes

pkgs/applications/misc/home-manager/default.nix Outdated

+              #Taken directly from
+              #https://github.com/rycee/home-manager/blob/9c1b3735b402346533449efc741f191d6ef578dd/home-manager/default.nix
+              { pkgs

Member

rycee Oct 12, 2017

The arguments should be made more "standard". I.e., instead of pulling everything from pkgs, should include stdenv, fetchFromGitHub, bash, coreutils, etc. here.

rycee reviewed

View reviewed changes

pkgs/applications/misc/home-manager/default.nix Outdated

+              pkgs.stdenv.mkDerivation rec{
+                name = "home-manager-${version}";
+                version="2017-10-11";

Member

rycee Oct 12, 2017

Should have spaces around =.

Member

peterhoeg commented Oct 13, 2017

I understand this is taken verbatim from upstream, but it really requires quite a bit of cleanup to be ready for merging.


          Integrate rycee's suggestions

74a2c4a

Contributor Author

pasqui23 commented Oct 13, 2017

Just integrated @rycee's suggestions.
As for nixos stable,it would just pin to a stable revision of the repository,just like any other program that is fetched from GitHub.

Thus the nixpkgs stable and unstable would point to different revisions,and the stable one would be frozen to where it was when nixpkgs was frozen.


          Merge branch 'master' of https://github.com/NixOS/nixpkgs into home-m…

036f1b7

…anager

peterhoeg requested changes

View reviewed changes

Member

peterhoeg left a comment

Most of the issues are minor.

pkgs/tools/package-management/home-manager/default.nix Outdated

+              #Taken directly from
+              #https://github.com/rycee/home-manager/blob/9c1b3735b402346533449efc741f191d6ef578dd/home-manager/default.nix
+              { pkgs,bash,coreutils,less,stdenv,makeWrapper,fetchFromGitHub

Member

peterhoeg Oct 14, 2017

We don't need pkgs
We typically put a space after comma ,

pkgs/tools/package-management/home-manager/default.nix Outdated

+              , modulesPath ? null
+              }:
+              stdenv.mkDerivation rec{

Member

peterhoeg Oct 14, 2017

Space after rec

pkgs/tools/package-management/home-manager/default.nix Outdated

+              stdenv.mkDerivation rec{
+                name = "home-manager-${version}";
+                version = "2017-10-11";
+                src=fetchFromGitHub{

Member

peterhoeg Oct 14, 2017

Space before and after =

pkgs/tools/package-management/home-manager/default.nix Outdated

+                  rev = "7e6f3364bcf0a0ec838aa4853f550a9a7b5ed027";
+                  sha256 = "04wl0jpha19fsnyc5a8y4pkss9by468k0i5w5bjswnaz792yji6v";
+                };
+                buildInputs=[makeWrapper];

Member

peterhoeg Oct 14, 2017

nativeBuildInputs not buildInputs for makeWrapper
= spaces
Space after [ and before ].

pkgs/tools/package-management/home-manager/default.nix Outdated

+                  sha256 = "04wl0jpha19fsnyc5a8y4pkss9by468k0i5w5bjswnaz792yji6v";
+                };
+                buildInputs=[makeWrapper];
+                phases = [ "installPhase" ];

Member

peterhoeg Oct 14, 2017

You probably in general don't want to override phases. Instead specifically exclude the phases you don't need. In this case that will probably be dontBuild = true;.

pkgs/tools/package-management/home-manager/default.nix Outdated

+                '';
+                meta = with stdenv.lib; {
+                  description = "A user environment configurator";

Member

peterhoeg Oct 14, 2017

How about license?

pkgs/tools/package-management/home-manager/default.nix Outdated

+                meta = with stdenv.lib; {
+                  description = "A user environment configurator";
+                  maintainers = [ maintainers.rycee ];

Member

peterhoeg Oct 14, 2017

Should be maintainers = with maintainers; [ rycee ];

pasqui23 and others added 4 commits

October 14, 2017 11:23


          Integrate peterhoeg's suggestions

27189d4


          Merge branch 'home-manager' of https://github.com/pasqui23/nixpkgs in…

…to home-manager


          corrected typos

7a5f41d


          Merge https://github.com/NixOS/nixpkgs into home-manager

4530e3d

Contributor Author

pasqui23 commented Oct 21, 2017

Ok why the the Travis build is failing?

Member

rycee commented Oct 21, 2017

The Travis failure seems unrelated to your PR.

Contributor Author

pasqui23 commented Nov 22, 2017

Ok,I am trying to update home-manager but with the following default.nix i get only error: file 'home-manager/home-manager/home-manager.nix' was not found in the Nix search path (add it using $NIX_PATH or -I) when doing home-manager switch


{ bash, coreutils, less, stdenv, makeWrapper, fetchFromGitHub

  # Extra path to the Home Manager modules. If set then this path will
  # be tried before `$HOME/.config/nixpkgs/home-manager/modules` and
  # `$HOME/.nixpkgs/home-manager/modules`.
, modulesPath ? null
  # Extra path to Home Manager. If set then this path will be tried
  # before `$HOME/.config/nixpkgs/home-manager` and
  # `$HOME/.nixpkgs/home-manager`.
, confPath ? ""
}:

stdenv.mkDerivation rec {
  name = "home-manager-${version}";
  version = "2017-11-22";
  src = fetchFromGitHub{
    owner = "rycee";
    repo = "home-manager";
    rev = "3c875267afdac6348e5e7177df10364db8bb5edd";
    sha256 = "0wz73dsjpa4847n3j24lm7s9il622pgmhrqmjibzka9bk5wl4wzs";
  };
  nativeBuildInputs = [ makeWrapper ];
  dontBuild = true;

  installPhase = let
    dot = "${src}/home-manager/home-manager" ;
    mod = if modulesPath == null then "${src}/modules" else modulesPath;

  in ''
    install -v -D -m755 ${dot} $out/bin/home-manager

    substituteInPlace $out/bin/home-manager \
      --subst-var-by bash "${bash}" \
      --subst-var-by coreutils "${coreutils}" \
      --subst-var-by less "${less}" \
      --subst-var-by MODULES_PATH '${mod}:${dot}.nix' \
      --subst-var-by HOME_MANAGER_EXPR_PATH "${dot}.nix"\
      --subst-var-by HOME_MANAGER_PATH '${confPath}'

  '';

  meta = with stdenv.lib; {
    description = "A user environment configurator";
    maintainers = with maintainers; [ rycee ];
    platforms = platforms.linux;
    license = licenses.mit;
  };
}


          home-manager: 2017-10-11 -> 2017-11-22

a80e7c7

Member

rycee commented Nov 22, 2017

HM was in nix-community/home-manager@bf3a8c6 changed to only use HOME_MANAGER_PATH. I.e. the substitutions MODULES_PATH and HOME_MANAGER_EXPR_PATH were removed. So to fix your issue I believe you can change the line

--subst-var-by HOME_MANAGER_PATH '${confPath}'

to read

--subst-var-by HOME_MANAGER_PATH '${src}'

This change was made to support the new installation instructions and also using Home Manager as a NixOS module.

My suggestion is still to install HM separately from Nixpkgs for now and use the programs.home-manager.enable option to automatically keep it up-to-date.


          home-manager: 2017-10-11 -> 2017-11-22,corrected envieroment variable…

db72861

… HOME_MANAGER_PATH

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels


          home-manager:2017-11-22 -> 2017-12-7

2f74b37

Contributor Author

pasqui23 commented Dec 24, 2017

I would like if @peterhoeg could review this pr,now that all automatic checks have passed

Member

peterhoeg commented Dec 29, 2017

Just a few minor nitpicks:

a) the documentation at the top isn't correct - it isn't taken verbatim but adapted from
b) the spacing is odd - sometimes you have no blank lines between attributes and others (like inside the installPhase) you have extra blank lines


          home-manager:corrected style

8ba84de

peterhoeg merged commit 0dd7d21 into NixOS:master

Member

peterhoeg commented Dec 30, 2017

Thanks @pasqui23 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment