Skip to content
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.

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

home-environment: allow adding to $PATH #1292

Merged
merged 1 commit into from
Oct 5, 2020

Conversation

afontaine
Copy link
Contributor

Description

Sometimes I'll have some things installed to my dot files that provide
a collection of executables (doom-emacs, org-capture), or need a
$HOME/bin or similar to keep a handful of quick scripts and wanted to
be able to do it decrlaratively.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

@afontaine afontaine requested a review from rycee as a code owner May 30, 2020 21:35
@afontaine
Copy link
Contributor Author

Hmm looks like the build failed because fetching etex failed, which is very unrelated. Can we try restarting the build to see if the etex servers are awake?

@turion
Copy link

turion commented Jun 4, 2020

Does that work for you locally?

I tried home.sessionVariables.PATH = "$PATH:/foo/bar";, which should have given me an equivalent result (and it does produce the same .nix-profile/etc/profile.d/hm-session-vars.sh as yours), but PATH is not modified by that.

@turion
Copy link

turion commented Jun 4, 2020

Hmm looks like the build failed because fetching etex failed, which is very unrelated. Can we try restarting the build to see if the etex servers are awake?

Either @rycee can restart the build on Travis or you can make some change and force-push, that might trigger the build as well.

@turion
Copy link

turion commented Jun 4, 2020

I tried home.sessionVariables.PATH = "$PATH:/foo/bar";, which should have given me an equivalent result (and it does produce the same .nix-profile/etc/profile.d/hm-session-vars.sh as yours), but PATH is not modified by that.

Hmm, I think I'm wrong and that does indeed work. I just needed to restart my system.

modules/home-environment.nix Outdated Show resolved Hide resolved
modules/home-environment.nix Outdated Show resolved Hide resolved
@teto
Copy link
Collaborator

teto commented Jun 29, 2020

For reference, Nixos has the weird limited option:

environment.homeBinInPath
     Include ~/bin/ in $PATH.

what if you want to prepend to PATH instead ? can't you just modify home.sessionVariables instead ? I don't believe we need a specific option for that.

@turion
Copy link

turion commented Jun 29, 2020

@teto

what if you want to prepend to PATH instead ?

Does that have a different effect:

can't you just modify home.sessionVariables instead ? I don't believe we need a specific option for that.

See my previous comment: #1292 (comment) It doesn't work straightforwardly.

@teto
Copy link
Collaborator

teto commented Jun 29, 2020

Hmm, I think I'm wrong and that does indeed work. I just needed to restart my system.

Seems like it worked ?! this behaves no differently: if you change sessions variables, you have to relog, otherwise, you could change it into your shell variables.

@turion
Copy link

turion commented Jun 29, 2020

Hmm, I think I'm wrong and that does indeed work. I just needed to restart my system.

Seems like it worked ?!

You're right, I forgot!

@afontaine
Copy link
Contributor Author

@turion sorry for letting this stall for so long, but I've updated the documentation a bit more and fixed the default example.

As for why this and not just home.sessionVariables.PATH, this approach allows users to break declarations apart and use them across files/modules/etc if you aren't about having one long file for home-manager.

I have ~/.config/emacs/bin in my path for Doom Emacs (as it is not yet very nix-able), and .git/safe/../../bin, primarily for working with non-nix'd projects. These belong in the emacs set-up file I have and the git setup file, respectively.

The prepend/append issue is interesting, but I'm not sure what the best approach is to this beyond complicating this even further with something like {path = "some/path"; prepend = true;}

@turion
Copy link

turion commented Jul 20, 2020

I think this is a good option to add.

@rycee probably your input is needed here :)

@teto
Copy link
Collaborator

teto commented Jul 20, 2020

one can already adjust the PATH via export PATH=/my/bin:${PATH} so this looks overengineered ?

@afontaine
Copy link
Contributor Author

afontaine commented Jul 20, 2020

one can already adjust the PATH via export PATH=/my/bin:${PATH} so this looks overengineered ?

@teto How? Where? Through sessionVariables? in myShell.initExtra?

If in either of those options, how do we append to it across multiple nix files? do I need to keep track of all the path adjustments I want to make and keep them in one location? That feels strange.

Admittedly, it does feel slightly overkill, but at the same time, the current solutions feel... lacking. I'm not really sure what the best solution is though.

@turion
Copy link

turion commented Jul 20, 2020

I don't think this is overkill or overengineered. It abstracts adding directories to the PATH from the actual mechanism. It's good that one can configure the PATH variable without having to read the home-manager source code in order to understand where and when and how hm-session-vars.sh is called. And it probably does no harm because it has a unit test.

@rycee
Copy link
Member

rycee commented Jul 20, 2020

Perhaps it would suffice to fix the type of sessionVariables to be a attrsOf instead of attrs with a list value being concatenated by :? Something like

diff --git a/modules/home-environment.nix b/modules/home-environment.nix
index 209df4ff..7691e6a1 100644
--- a/modules/home-environment.nix
+++ b/modules/home-environment.nix
@@ -228,7 +228,11 @@ in
 
     home.sessionVariables = mkOption {
       default = {};
-      type = types.attrs;
+      type = with types;
+        let
+          primitive = either bool (either int str);
+          var = either primitive (listOf str);
+        in attrsOf var;
       example = { EDITOR = "emacs"; GS_OPTIONS = "-sPAPERSIZE=a4"; };
       description = ''
         Environment variables to always set at login.
diff --git a/modules/lib/shell.nix b/modules/lib/shell.nix
index 5e5743f5..7b57596d 100644
--- a/modules/lib/shell.nix
+++ b/modules/lib/shell.nix
@@ -2,7 +2,10 @@
 
 rec {
   # Produces a Bourne shell like variable export statement.
-  export = n: v: ''export ${n}="${toString v}"'';
+  export = let
+    toShell = v:
+      if lib.isList v then lib.concatStringsSep ":" v else toString v;
+  in n: v: ''export ${n}="${toShell v}"'';
 
   # Given an attribute set containing shell variable names and their
   # assignment, this function produces a string containing an export

Then the configuration

home.sessionVariables = lib.mkMerge [
  (lib.mkAfter {
    PATH = ["$PATH"];
  })
  {
    PATH = ["$HOME/path1"];
  }
  {
    PATH = ["$HOME/path2"];
  }
];

would place

export PATH="$HOME/path1:$HOME/path2:$PATH"

in hm-session-vars.sh.

It wouldn't correctly handle an empty PATH variable, though.

@berbiche
Copy link
Member

@rycee Quick question: why would you use either when listOf is available? Are the documentation/error strings clearer?

@rycee
Copy link
Member

rycee commented Jul 20, 2020

@berbiche I assume you mean types.oneOf. It's mainly out of habit. Probably better to use oneOf since it is a more stylish and the generated code should be identical to using either explicitly.

@afontaine
Copy link
Contributor Author

Perhaps it would suffice to fix the type of sessionVariables to be a
attrsOf instead of attrs with a list value being concatenated by :?

This was my initial thought as well, @rycee, but I am relatively new to writing nix so
I'm not too familiar with how to go about it. I initially just started by trying
to edit toShell to work with lists as well.

It wouldn't correctly handle an empty PATH variable, though.

This was my big blocker though. PATH is such a frustratingly special case, and
I couldn't find an elegant way to inject a :$PATH into the variable building
logic, so it felt easier to split it out into a separate option. It is also
required here unfortunately 😞

@turion
Copy link

turion commented Jul 21, 2020

Perhaps it would suffice to fix the type of sessionVariables to be a attrsOf instead of attrs with a list value being concatenated by :?

But there are lots of session variables that aren't paths.

modules/home-environment.nix Outdated Show resolved Hide resolved
modules/home-environment.nix Outdated Show resolved Hide resolved
@afontaine afontaine force-pushed the add-home-session-path branch 2 times, most recently from 04276f3 to 8f024ed Compare August 13, 2020 04:23
@afontaine
Copy link
Contributor Author

@rycee is there anything I can change here to move this along, aside from merge conflicts?

modules/home-environment.nix Outdated Show resolved Hide resolved
modules/misc/news.nix Outdated Show resolved Hide resolved
This option allows adding additional entries to `PATH`.
@rycee rycee merged commit 0006da1 into nix-community:master Oct 5, 2020
@rycee
Copy link
Member

rycee commented Oct 5, 2020

I made some minor changes and merged to master. Thanks for the contribution!

@afontaine
Copy link
Contributor Author

awesome, thanks @rycee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants