LnL7 / nix-darwin

nix modules for darwin
MIT License
2.43k stars 407 forks source link

$PATH is broken for fish shell #122

Open grossbart opened 5 years ago

grossbart commented 5 years ago

I updated nix-darwin today and had to rollback to a version from October because my $PATH was broken (using fish shell).

Before (correct):

/Users/gape/.nix-profile/bin
/run/current-system/sw/bin
/nix/var/nix/profiles/default/bin
/usr/local/bin
/usr/bin
/bin
/usr/sbin
/sbin

After (broken):

/Users/gape/.local/bin
/Users/gape/.local/bin
/Users/gape/.nix-profile/bin
/usr/local/bin
/usr/bin
/bin
/usr/sbin
/sbin
/Users/gape/.nix-profile/bin
/run/current-system/sw/bin
/nix/var/nix/profiles/default/bin

The important part being that /run/currenty-system/sw/bin comes too late and the Nix provided binaries are not picked up correctly, resolving to /usr/bin/git for example. I'm also not sure where all this duplication is coming from …

I tried to get to the source of this error but was not able to figure it out exactly. What worked was to switch to an older generation and then to the newest generation again: in this case, the $PATH was set correctly. But when I exit fish shell and enter it again, the $PATH is broken.

It could be that this problem was introduced in https://github.com/LnL7/nix-darwin/commit/676ef103771aa3fc4b150290294b8ad5610d2750#diff-02a3bd02a5cdf5583b2e516a0e92d58a because the version of nix-darwin that worked for me was from 2018-10-17 and this is the major change that happened to the environment a few days later. But then maybe not because no one else has this problem?

jhenahan commented 5 years ago

Also hitting this after updating from a version from last week. Haven't narrowed down the issue, yet, but it seems like environment.systemPath isn't being picked up by fish after export.

LnL7 commented 5 years ago

The same thing happens for all shells so I'm not sure what the issue is.

grossbart commented 5 years ago

I spent some time looking for this bug and wanted to post a small update, even though I have not found the source of the problem nor a solution …

I went through all the commits since October and could not find anything that looked particularly problematic with regards to this problem. The one commit that caught my eye (and that I mentioned in the initial post), introduced this:

# Prevent this file from being sourced by child shells.
export __NIX_DARWIN_SET_ENVIRONMENT_DONE=1

This seems suspect to me because it introduces a global env variable that might be unpredictable. But then I went into the generated nixos-env-preinit.fish file and echoed the PATH. I could not find a problem there, it was all working correctly.

I also changed the tests to test for the sequence of the Nix paths and then the system paths in order and that was also correct.

But whenever I start a shell in the newest nix-darwin, my PATH is wrong as described above. So there has to be some other hook somewhere that works on this. I will keep looking …

A last thing: if I would find a place where I think it goes wrong, how would I test this on my system? Would I have to build the installer and run that from my local repo?

yurrriq commented 5 years ago

For the record, I'm having similar issues, it looks like.

LnL7 commented 5 years ago

I'm not a fish user so I don't know what to look for but I'd take a look at https://github.com/NixOS/nixpkgs/pull/45784. Also verifying the following might help to narrow down the problem:

  1. Is this reproducible with nixpkgs-18.09-darwin or only nixpkgs-unstable/master?
  2. Does this also happen on nixos-unstable, could just be hidden because /usr is almost empty there.
yurrriq commented 5 years ago

Here are my pinned nixpkgs and nix-darwin:

{
  "owner": "NixOS",
  "repo": "nixpkgs-channels",
  "branch": "nixpkgs-18.09-darwin",
  "rev": "0b471f71fada5f1d9bb31037a5dfb1faa83134ba",
  "sha256": "148vh3602ckm1vbqgs07fxwpdla62h37wspgy0bkcycqdavh7ra5"
}
{
  "owner": "LnL7",
  "repo": "nix-darwin",
  "branch": "master",
  "rev": "629fa534988b7a402f4278c9445f0e20e5f03973",
  "sha256": "033xwz0gaqc89khbs1l3lxii5v54sqqyzag55xjyqrmaz7bbancy"
}

and the resulting $PATH:

$ for p in $PATH; echo $p; end
/Users/me/.rvm/gems/jruby-9.2.5.0/bin
/Users/me/.rvm/gems/jruby-9.2.5.0@global/bin
/Users/me/.rvm/rubies/jruby-9.2.5.0/bin
/Users/me/.rvm/bin
/Users/me/bin
/bin
/Users/me/.nix-profile/bin
/run/current-system/sw/bin
/nix/var/nix/profiles/default/bin
/usr/local/bin
/usr/bin
/bin
/usr/sbin
/sbin

For me, it looks like it's just /bin that's too early, and to be honest, I'm not sure that wasn't the case prior to October.

thefloweringash commented 5 years ago

I had a look at this, and I think the problem is that Fish on nix-darwin runs its initializers in an incompatible order.

On macOS, there's a path_helper system that allows adding to paths via /etc/paths.d/. For example, /etc/paths.d/40-XQuartz includes the line /opt/X11/bin. Part of its behaviour, is that anything it manages is placed before the existing values of PATH. From my reading of fish's startup, the fish implementation also has the same behaviour.

$ PATH=/bin:/sentinel:/sbin /usr/libexec/path_helper -s
PATH="/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/sentinel"; export PATH;

Logging the startup of a fish shell (without $__NIX_DARWIN_SET_ENVIRONMENT_DONE, to simulate a clean environment), shows that the nix-darwin path configuration is done before path helper.

$ fish --interactive --login -d3 -c 'echo $PATH' 2>&1  | cat -n | grep -e 'path_helper' -e 'fenv source'
   645  <3> fish: Created job 4 from command 'fenv source /nix/store/li0kdlly65x6a3n5h7jwn2l71v6f7zkk-set-environment' with pgrp -2
   691  <3> fish: Created job 3 from command 'command -sq /usr/libexec/path_helper' with pgrp -2

Nixpkgs does some additional configuration of fish to setup the environment very early, even before any of fish's main configuration is applied. Nix-darwin uses this same setup to inject the PATH variable. The eventual result is that the PATH is applied, then fish re-orders it in it's path_helper implementation.

A possible solution would be to stash the initial value of PATH in the pre-init, and restore it at the top of the regular init. I'll work on a PR.

While we're here: child shells

The behaviour around __NIX_DARWIN_SET_ENVIRONMENT_DONE should only apply to child shells, and is necessary for things like nix run to work. Prior to this change, every shell would explicitly reset PATH to the global configured value, and clobber whatever setup had been done by nix run, or user config in the parent shell, etc.

Fish also makes the questionable choice of running their path_helper on all shells. This is a departure from macOS, which only runs in login shells via /etc/profile. Even if nix-darwin does the right thing, this will cause all child shells to have their path's shuffled, breaking things like nix run[1].


[1] Presently nix run defaults to bash, so this would only be a problem when running nix run -c fish.

thefloweringash commented 5 years ago

I think the required configuration point is missing in fish. I've filed an issue upstream at https://github.com/fish-shell/fish-shell/issues/5641.

grossbart commented 5 years ago

Thanks for the research, @thefloweringash. This rabbit hole gets deeper and deeper 😓 …

yurrriq commented 5 years ago

I ended up doing this:

for p in /run/current-system/sw/bin ~/bin
    if not contains $p $fish_user_paths
        set -g fish_user_paths $p $fish_user_paths
    end
end

... in my programs.fish.shellInit, as per @mqudsi's suggestion, and now my stuff works well enough.

🤷‍♂️ YMMV

fish, version 3.0.1

grossbart commented 5 years ago

Thanks for the pointer, @yurrriq, this works for me, too. I'm on fish 3.0.0 and without this I still had the problem. It's hackish, but it works … This is what I added to my .nixpkgs/darwin-configuration.nix:

  programs.fish.enable = true;
  programs.fish.shellInit = ''
    for p in /run/current-system/sw/bin
      if not contains $p $fish_user_paths
        set -g fish_user_paths $p $fish_user_paths
      end
    end
  '';
LnL7 commented 5 years ago

If that also works for the version of fish in 18.09 something similar could be added to the module by default.

yurrriq commented 5 years ago

I'm using 18.09 (NixOS/nixpkgs@46d3867a08a9206685e2b6a8e19f5ad9f6ab4b39) on NixOS and it seems to work for fish 2.7.1 too.

what-the-functor commented 5 years ago

I also ran into this. Fish version 3.0.2. @yurrriq's fix works for me, as well.

teehemkay commented 4 years ago

I have the following function definition in programs.fish.shellInit

      function __nix_darwin_fish_macos_fix_path -d "reorder path prioritizing darwin-nix paths"
        # Fish initialization for login shells rebuilds $PATH by emulating calling /usr/libexec/path_helper
        # which results in nix-darwin additional paths being appended (instead of prependd) to $PATH
        # (see https://github.com/fish-shell/fish-shell/blob/90547a861a13806cdfcf479e279527b2cb18c922/share/config.fish#L171)
        # As a workaround we re-source the nix-darwin environment again if necessary
        if test $PATH[1] = '/usr/local/bin'
          set fish_function_path ${pkgs.fish-foreign-env}/share/fish-foreign-env/functions $fish_function_path
          # source the NixOS environment config again
          fenv source ${config.system.build.setEnvironment}
          set -e fish_function_path[1]
        end
      end
      '';

then I just invoke __nix_darwin_fish_macos_fix_path at the top of programs.fish.interactiveShellInit.

The first test in the function may need to be adapted though.

HTH

anka-213 commented 4 years ago

Here's a couple of suggestions for solutions from faho on https://github.com/fish-shell/fish-shell/issues/7142#issuecomment-647166217

  • Remove the offending paths from /etc/paths or $fish_user_paths - this is probably not viable for nix
  • Add the nix paths to /etc/paths - this might be viable for nix-darwin
  • Use a configuration snippet. Call it "00-nix.fish", put the path manipulation there. This is clean, simple and does what you want. It can be kept out of sight of users by putting it in the system conf.d
  • Use $fish_user_paths - this is more visible to the user and can cause issues should they erase the variable
  • Use /etc/fish/config.fish - this is semi-visible to the user, and only works after the snippets, so it's probably not viable
  • Patch share/config.fish to add in $PATH changes later - note that we don't want __fish_build_paths later because that's needed to find our functions, of which we execute a few in share/config.fish

I'm not sure which of these (if any) is most viable.

happysalada commented 3 years ago

For me a simple fix was setting environment.systemPath = [ /run/current-system/sw/bin ]; working with all shells

webframp commented 3 years ago

Seeing this same problem with zsh

lilyball commented 3 years ago

We should be able to stuff the Nix stuff into fish_user_paths to get around this. My existing nixpkgs fish configuration looks like

{
  fish = super.fish.override {
    fishEnvPreInit = sourceBash: ''
      set -l oldPath $PATH
    '' + sourceBash profilePath + ''
      for elt in $PATH
        if not contains -- $elt $oldPath
          set -ag fish_user_paths $elt
        end
      end
      set PATH $oldPath
    '';
  };
}

It records the $PATH before sourcing the Nix environment, then walks the new $PATH and copies anything new into fish_user_paths, then restores the old $PATH. This code runs in the equivalent spot as fish/nixos-env-preinit.fish. So we could do the same thing here. The downside of course is the user then has to avoid overwriting fish_user_paths, but there's no other obvious workaround as $__fish_data_dir/config.fish slaps the macOS default paths on the front, calls the function to move fish_user_paths to the front, and then sources the conf.d/* files, and there's no opportunity to run code after the PATH modification and before ~/.config/fish/conf.d/*` is sourced.

Perhaps the best action here is to add it to fish_user_paths, but to not remove it from $PATH. It looks like any entry in fish_user_paths that already existed in $PATH is still moved to the front of $PATH but is not marked as something to be deleted when fish_user_paths is reset. This way users who unconditionally overwrite fish_user_paths will retain their Nix environment, but we'll have successfully protected it from the /usr/libexec/path_helper code.

And by doing it this way, the $PATH will be correct during the sourcing of the conf.d files.

lilyball commented 3 years ago

I just tested this by hacking my config file with mkBefore and mkAfter and it seems to work, although it turns out the nix-darwin environment includes /usr/bin:/bin:… so all of those end up in $fish_user_paths, which is a bit weird. I suppose I could explicitly list those to be skipped.

lilyball commented 3 years ago

Here's my hack:

{ config, lib, ... }:

with lib;

let
  cfg = config.programs.fish;
in
{
  config = mkIf cfg.enable {
    environment.etc."fish/nixos-env-preinit.fish".text = mkMerge [
      (mkBefore ''
        set -l oldPath $PATH
      '')
      (mkAfter ''
        for elt in $PATH
          if not contains -- $elt $oldPath /usr/local/bin /usr/bin /bin /usr/sbin /sbin
            set -ag fish_user_paths $elt
          end
        end
        set -el oldPath
      '')
    ];
  };
}

My resulting $PATH looks correct, and clearing $fish_user_paths afterwards doesn't break it.

winterqt commented 2 years ago

Here's another hack that increases compatibility a bit in certain cases (like login shells):

  environment.etc."paths" = {
    text = concatStringsSep "\n" ([ "/Users/winter/.nix-profile/bin" ] ++ (remove "$HOME/.nix-profile/bin" (splitString ":" config.environment.systemPath)));
    knownSha256Hashes = [
      "cdfc5a48233b2f44bc18da0cf5e26df47e9424820793d53886aa175dfbca7896"
    ];
  };

Obviously only works for single user systems, replace /Users/winter with your home directory.

pingiun commented 2 years ago

In https://github.com/fish-shell/fish-shell/blob/e066715127fc0a515e7a8c66a2b7b21531bd500e/share/config.fish#L199 I think this change is needed:

             # Merge in any existing path elements
             for existing_entry in $$argv[1]
                 if not contains -- $existing_entry $result
-                    set -a result $existing_entry
+                    set -p result $existing_entry
                 end
             end

When I hacked this into my file in /nix/store, the resulting path was

; echo $PATH
/nix/var/nix/profiles/default/bin /run/current-system/sw/bin /Users/jelle/.nix-profile/bin /usr/local/bin /usr/bin /bin /usr/sbin /sbin /Library/Apple/usr/bin /Applications/VMware Fusion Tech Preview.app/Contents/Public /usr/local/MacGPG2/bin

which seems like it's completely correct.

lilyball commented 2 years ago

@pingiun That reverses the order of the existing entries. Also this code here is mimicking the behavior of /usr/libexec/path_helper. I'm not actually sure why path_helper prepends the paths though, especially since its manpage says it's supposed to append them. Perhaps because appending then runs into ordering issues, where the paths it sets are supposed to have a particular order but any that already exist in PATH would screw that up.

This issue affects nested bash login shells too (since it uses path_helper directly), so it's not just Fish.

The discussion in https://github.com/fish-shell/fish-shell/issues/7142 (thank you @anka-213) goes into detail about the problem here. I just commented as I think a real solution does unfortunately require modifying share/config.fish, though I'm not optimistic about getting any upstream changes. And sadly we cannot simply add a file to /etc/paths.d/ as any paths installed that way come after the system paths in /etc/paths.

Until such time as we get an upstream change in Fish, I think our only real solution is to patch share/config.fish to insert the hook we need. The hook can be a simple emit __fish_config_macos_set_env_post immediately after the __fish_macos_set_env call, and nix-darwin can define a function to look for that in nixos-env-preinit.fish and use it to fix up the path. This does mean changing the Fish derivation though, which means recompiling, so this patch would have to go into nixpkgs.

lilyball commented 2 years ago

As suspected, there is resistance to adding this hook into the upstream fish-shell (https://github.com/fish-shell/fish-shell/issues/7142#issuecomment-972160140). We'll need to patch share/config.fish in nixpkgs instead.

lilyball commented 2 years ago

Fish also makes the questionable choice of running their path_helper on all shells. This is a departure from macOS, which only runs in login shells via /etc/profile.

Just to note, this behavior was fixed in Fish 3.1.0.

andrewhamon commented 2 years ago

@lilyball This still seems to be a problem for me even on Fish 3.3.1

Fish version:

fish --version
fish, version 3.3.1

Path in Fish:

/usr/local/bin
/usr/bin
/bin
/usr/sbin
/sbin
/Library/Apple/usr/bin
/usr/local/munki
/Users/andrewhamon/.nix-profile/bin
/run/current-system/sw/bin
/nix/var/nix/profiles/default/bin

Path in ZSH:

/Users/andrewhamon/.nix-profile/bin
/nix/var/nix/profiles/default/bin
/Users/andrewhamon/.nix-profile/bin
/run/current-system/sw/bin
/nix/var/nix/profiles/default/bin
/usr/local/bin
/usr/bin
/usr/sbin
/bin
/sbin
lilyball commented 2 years ago

@andrewhamon I was responding specifically to the text I quoted, about Fish running the path_helper logic on non-login shells. That's the part that was fixed in Fish 3.1.0, and means that if the login shell behavior can be worked around, then nested non-login shells will continue to work.

andrewhamon commented 2 years ago

I see, sorry for misinterpreting 👍

PaulGrandperrin commented 2 years ago

For the record, here's my code if someone has a similar setup to me.

My setup: fish configured in home-manager, deployed on both macOS/nix-darwin and Linux/NixOS (https://github.com/PaulGrandperrin/nix-systems)

The code:

loginShellInit = "fish_add_path --move --prepend --path $HOME/.nix-profile/bin /run/wrappers/bin /etc/profiles/per-user/$USER/bin /nix/var/nix/profiles/default/bin /run/current-system/sw/bin";
mathieupost commented 1 year ago

For me this worked:

loginShellInit = ''for p in (string split " " $NIX_PROFILES); fish_add_path --prepend --move $p/bin; end'';

Needed to use the split function because $NIX_PROFILES is stored as a concatenated string instead of a list.

cdfa commented 1 year ago

I was having the same issue with fish installed through pure nixpkgs (with nix-env -iA). For solving the specific problem of a program from /usr/local/bin/ overriding one in a nix shell, it was sufficient to clear the $fish_user_paths variable with set --erase fish_user_paths.

Keep in mind that /Users/colin/.nix-profile/bin and /nix/var/nix/profiles/default/bin will still be prepended though.

mathieupost commented 1 year ago

Not sure what's changed/fixed, but after reinstalling nix and nix-darwin I don't have this problem anymore. Even without any of the workarounds above.

lilyball commented 1 year ago

@mathieupost The Nix installer was updated to support Fish. IIRC it adds a file to fish's conf.d.

mathieupost commented 1 year ago

Perfect! So I guess this can be closed then? Maybe if someone else who had the same problem can confirm?

anka-213 commented 1 year ago

Is there a way to fix this issue without having to make a clean reinstall? It would probably take a lot of time to reinstall everything

layday commented 1 year ago

It looks like the fish conf was added in 2.11.1 which is the version installed by the installer but nix upgrade-nix points to 2.11.0 a couple of months after 2.11.1 was released and the nix package on nixpkgs seems to be stuck on 2.11.0 too.

mamciek commented 1 year ago

Not sure what's changed/fixed, but after reinstalling nix and nix-darwin I don't have this problem anymore. Even without any of the workarounds above.

I was doing a fresh install of home-manager and nix-darwin today and the problem still exists.

mathieupost commented 1 year ago

@mamciek Did you try completely reinstalling nix? As noted by @layday, the installer of nix itself was updated to include a nix conf.

mamciek commented 1 year ago

Yes - I removed /nix folder completly. And then installed nix, then nix-darwin. I was executing all the commands in default OSX zsh shell (my fish was removed with /nix).

mathieupost commented 1 year ago

I had another MacBook laying around with the same issue, so I tried uninstalling and reinstalling nix on that one as well and it also solved the issue there. These are the steps I did, maybe it helps:

mamciek commented 1 year ago

I had another reinstall, this time it included fresh OSX reinstall as well. I don't have this problem now.

layday commented 1 year ago

Until the time you update Nix and you are downgraded to 2.11.0 ;)

mathieupost commented 1 year ago

Until the time you update Nix and you are downgraded to 2.11.0 ;)

❯ nix --version
nix (Nix) 2.11.0

Still works, as the fix is in the install script ;)

layday commented 1 year ago

Hmm, perhaps it's something that Home Manager is doing.

layday commented 1 year ago

Right, the installer will update fish config files in place. That is, this'll only work if you have previously installed fish and did not wipe its config files. Considering that macOS doesn't come with fish, and if you intend to use nix as a replacement for homebrew/macports, i.e. if you are gonna install fish with nix, then you need to be on nix 2.11.1 to be able to source the new nix profile scripts for fish, which are generated when building nix. If you've upgraded (downgraded) nix after a fresh install, you can switch back to the 2.11.1 generation with nix-env.

tacomilkshake commented 1 year ago

If you have nix 2.11.1 or later installed in your user profile (via nix-env or nix profile), nix will only add paths for binaries installed in your user profile and the root user's user profile.

This won't add paths for system installed packages--which is absolutely needed. Unfortunately, you need to add /run/current-system/sw/bin/ after the paths added by nix 2.11.1. This means you have to provide an override so that all three nix binary paths are in the correct order (user, root, system), overriding any benefit from the 2.11.1 fixes.

So, nix 2.11.1 is still broken with fish on Darwin.

To patch this, I added this to the top of my ~/.config/fish/config.fish:

### Add nix binary paths to the PATH
# Perhaps someday will be fixed in nix or nix-darwin itself
if test (uname) = Darwin
    fish_add_path --prepend --global "$HOME/.nix-profile/bin" /nix/var/nix/profiles/default/bin /run/current-system/sw/bin
end

I included the test for Darwin as this only occurs on my darwin systems. nixos + fish paths are set just fine.

arichtman commented 1 year ago

I've encountered this too - just on the stock zsh

nix files

arichtman@macbookpro ~ % nix --version
nix (Nix) 2.13.2
arichtman@macbookpro ~ % uname -a 
Darwin macbookpro 21.6.0 Darwin Kernel Version 21.6.0: Mon Dec 19 20:44:01 PST 2022; root:xnu-8020.240.18~2/RELEASE_X86_64 x86_64
mathieupost commented 1 year ago

On my new MacBook it unfortunately doesn't work anymore, so I'm back to this workaround

khaneliman commented 1 year ago

If you have nix 2.11.1 or later installed in your user profile (via nix-env or nix profile), nix will only add paths for binaries installed in your user profile and the root user's user profile.

This won't add paths for system installed packages--which is absolutely needed. Unfortunately, you need to add /run/current-system/sw/bin/ after the paths added by nix 2.11.1. This means you have to provide an override so that all three nix binary paths are in the correct order (user, root, system), overriding any benefit from the 2.11.1 fixes.

So, nix 2.11.1 is still broken with fish on Darwin.

To patch this, I added this to the top of my ~/.config/fish/config.fish:

### Add nix binary paths to the PATH
# Perhaps someday will be fixed in nix or nix-darwin itself
if test (uname) = Darwin
    fish_add_path --prepend --global "$HOME/.nix-profile/bin" /nix/var/nix/profiles/default/bin /run/current-system/sw/bin
end

I included the test for Darwin as this only occurs on my darwin systems. nixos + fish paths are set just fine.

Fresh install of nix and nix-darwin and this worked for fixing it for me. Thanks :)