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

Fish subshell changes PATH order #5456

Closed
whwkong opened this issue Dec 31, 2018 · 32 comments · Fixed by #5767
Closed

Fish subshell changes PATH order #5456

whwkong opened this issue Dec 31, 2018 · 32 comments · Fixed by #5767
Labels
bug
Milestone

Comments

@whwkong
Copy link

@whwkong whwkong commented Dec 31, 2018

I use Pyenv which places its shims at the head of the PATH.
I also use Pipenv whose documentation recommends a certain shell-configuration.
pipenv shell --fancy spawns a subshell...however, the order of the PATH variable is different in the subshell.

I've given a simplified example below:

$ fish --version
fish, version 3.0.0

$ uname -a
Darwin MacBook-Pro.local 18.2.0 Darwin Kernel Version 18.2.0: Mon Nov 12 20:24:46 PST 2018; root:xnu-4903.231.4~2/RELEASE_X86_64 x86_64

$ echo $TERM
xterm-256color

I created a simple config.fish to demonstrate the issue.
Steps to repro:

$ cat ~/.config/fish/config.fish
if status --is-login
    set -gx PATH ~/.pyenv/shims $PATH
end

# re-start shell (let me know if there's an easy one-liner spawn a new fish shell with an encapsulated `config.fish`)
$ echo $PATH
/Users/whwkong/.pyenv/shims /usr/local/bin /usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/git/bin

$ fish
Welcome to fish, the friendly interactive shell
Type help for instructions on how to use fish

$ echo $PATH
 /usr/local/bin /usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/git/bin /Users/whwkong/.pyenv/shims

# which affects the Python version of choice
$ which -a python
python is /usr/local/bin/python
python is /usr/bin/python
python is /Users/thepathunfolds/.pyenv/shims/python

The set -gx command in config.fish is not executed in the sub-shell, but I would normally expect that the new sub-shell simply inherits the global PATH variable.

From #4192, and #4336 I understand that Fish adds in the paths from /etc/paths and /etc/paths.d.

Is there a way to enforce the order that these paths are added, so that they are appended, rather than prepended?

@Darkshadow2
Copy link

@Darkshadow2 Darkshadow2 commented Jan 1, 2019

The reason it's getting put at the end is that it is indeed in the inherited PATH, but the script that does the loading from /etc/paths and /etc/paths.d also reorders it so that it matches the order from them, and anything extra gets placed at the end. If you change the above test from is-login to is-interactive and do the same steps as above, you'll see your PATH will be /Users/whwkong/.pyenv/shims /usr/local/bin /usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/git/bin /Users/whwkong/.pyenv/shims and if you then launched another fish shell under that one, it would have a second /Users/whwkong/.pyenv/shims at the end.

You should probably just change it to a test to see if it's already part of the PATH and remove it if so and then just put it in the front (no tests to see if if's a login shell or interactive one).

@whwkong
Copy link
Author

@whwkong whwkong commented Jan 1, 2019

Good enough for me. Thank you for the explanation and recommendation!

@bjoernpollex-sc
Copy link

@bjoernpollex-sc bjoernpollex-sc commented Jan 3, 2019

@Darkshadow2 I don't understand the logic here - isn't this reordering only supposed to happen for login shells? The current behavior seems to be that this always happens. When I explicitly spawn a sub-shell, I want to environment the be inherited unmodified, yet there seems to be no way to do so.

@Darkshadow2
Copy link

@Darkshadow2 Darkshadow2 commented Jan 3, 2019

It's set to always happen, and looks like it has been that way (with one brief exception) since it was added. It's in $__fish_data_dir/config.fish.

@kreisys
Copy link

@kreisys kreisys commented Jan 4, 2019

This is really irritating. I started hitting this after fish 3.0.0 dropped. I use Nix so it's very important to me to be able to have the Nix paths at the very beginning. Without it I keep falling back to the macOS bundled version of vim (which is dark and sad) or even worse, the BSD versions of cli utils! Is there any way around this?

@whwkong
Copy link
Author

@whwkong whwkong commented Jan 5, 2019

I'm going to re-open this issue as this is a problem for fish 3.0.

bash does not have this issue.
I tested this for fish 2.7.1 and it also does not have this issue : the paths from /etc/paths, /etc/paths.d are appended at the end.

My current work-around solution is hackish, and other people are having this issue with fish 3.0 as well.

(edit: if it is critical that your subshell PATH be consistent, then I simply suggest switching back to fish 2.x until this is addressed). Note that fish does not simply inherit the PATH for sub-shells - it constructs the path... and if you have fish_user_paths set, it will still be prepended to PATH.

@whwkong whwkong reopened this Jan 5, 2019
@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Jan 5, 2019

I think I know what is happening here, but I am not seeing any change in behavior between fish 2.7.1 and fish 3.0.

The problem is that you are directly manipulating PATH, which makes it hard for fish to know what's the originally exported PATH, what is fish's customized view of PATH, and what the correct order of PATH entries is.

Replace

if status --is-login
    set -gx PATH ~/.pyenv/shims $PATH
end

with

if status --is-login
    set -g fish_user_paths ~/.pyenv/shims fish_user_paths
end

Or if fish is never the login shell, you can use the following:

if not contains ~/.pyenv/shims $fish_user_paths
    set -g fish_user_paths ~/.pyenv/shims $fish_user_paths
end

@whwkong
Copy link
Author

@whwkong whwkong commented Jan 6, 2019

I'm finding that fish 2.7.1 and fish 3.0 construct paths for sub-shells differently.

With the following config.fish:

$ cat ~/.config/fish/config.fish
if status --is-login
    set -gx PATH ~/.pyenv/shims $PATH
end
$ fish --version
fish, version 2.7.1

$ echo $PATH
/Users/thepathunfolds/.pyenv/shims /usr/local/bin /usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/git/bin

# launch sub-shell
$ fish
$ echo $PATH 
/Users/thepathunfolds/.pyenv/shims /usr/local/bin /usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/git/bin
# shim path is at the front
$ fish --version
fish, version 3.0.0

$ echo $PATH
/Users/thepathunfolds/.pyenv/shims /usr/local/bin /usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/git/bin

# launch sub-shell 
$ fish
$ echo $PATH
/usr/local/bin /usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/git/bin /Users/thepathunfolds/.pyenv/shims
# shim path is now at the end

@mqudsi mqudsi removed the question label Jan 11, 2019
@zanchey zanchey added this to the fish-future milestone Jan 11, 2019
@zanchey zanchey added the bug label Jan 11, 2019
@faho faho mentioned this issue Jan 12, 2019
7 tasks
@psagers
Copy link

@psagers psagers commented Jan 13, 2019

In order to get my Python environment going again, I worked around this with the following function:

function path-reordered -d "Echos the elements of $PATH, reordered such that custom paths precede standard system paths. This can be used to work around a fish 3.0.0 bug in which sub-shells fail to respect path ordering."
  set -l sys_paths

  for p in $PATH
      if string match -q -r '^/usr/|^/s?bin(/|$)' $p
          set -a sys_paths $p
      else
          echo $p
      end
  end

  for p in $sys_paths; echo $p; end
end

My config.fish now looks like:

if status --is-login
     set PATH $HOME/.local/bin $PATH
     # etc.
else
     set PATH (path-reordered)
end

Your definition of what constitutes a system path may vary.

faho added a commit to faho/fish-shell that referenced this issue Jan 16, 2019
@faho
Copy link
Member

@faho faho commented Jan 16, 2019

Please try 4bf5288 from my "etc-paths-keep-order" branch. That makes it so, when we read from /etc/paths{,.d}, we only append paths that aren't already there, so the order should stay stable.

@psagers
Copy link

@psagers psagers commented Jan 17, 2019

That resolved the narrow issue, although it still seems a bit off. I removed all of the path customizations I had in my ~/.config/fish/config.fish and opened a new Terminal window with and without the change. My /etc/paths contains:

/usr/local/bin
/usr/bin
/bin
/usr/sbin
/sbin

Stock fish 3.0.0:

~> echo $PATH
/usr/local/bin /usr/bin /bin /usr/sbin /sbin

With change:

~> echo $PATH
/usr/bin /bin /usr/local/bin /usr/sbin /sbin

@faho
Copy link
Member

@faho faho commented Jan 17, 2019

That resolved the narrow issue, although it still seems a bit off.

@psagers: What's the path in bash?

This $PATH looks like fish inherits "/usr/bin /bin /usr/local/bin", and then appends "/usr/sbin /sbin" from /etc/paths. Which seems alright?

If that's not acceptable, then this would actually require us to only do path_helper once instead of keeping the order. One possibility would be to only do it for login shells (AFAIK macOS terminal.app starts login shells by default?), another to export a variable to inhibit it.

I have no idea how bash does this. Under what conditions is path_helper started there? Can we replicate those?

@psagers
Copy link

@psagers psagers commented Jan 17, 2019

I actually tried bash as well and (again, after moving ~/.profile) got the expected /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin. /bin/sh --noprofile gives me /usr/bin:/bin.

I mostly work on FreeBSD, so I'm learning some new things about macOS here. The bash/csh invocations of /usr/libexec/path_helper reside in /etc/profile and /etc/csh.login respectively, so should only get executed in login shells. And yes, Terminal does appear to spawn login shells. That looks like it may be the root discrepancy here.

Updated: I changed the conditional to if command -sq /usr/libexec/path_helper; and status --is-login and so far everything looks good.

@tgray
Copy link
Contributor

@tgray tgray commented Feb 24, 2019

I'm also having this problem. I set my $PATH in .bashrc, then start up fish. All my $PATH modifications were inherited no problem with fish 2.x. I just upgrade to 3.x and now the system default path (and paths out of /etc/paths.d - see below) appear before all my modifications.

/usr/local/bin /usr/bin /bin /usr/sbin /sbin

noahfrederick added a commit to noahfrederick/dots that referenced this issue Feb 24, 2019
@ridiculousfish ridiculousfish removed this from the fish-future milestone Feb 25, 2019
@ridiculousfish ridiculousfish added this to the fish 3.1.0 milestone Feb 25, 2019
@ridiculousfish

This comment has been minimized.

@SolitudeSF

This comment has been minimized.

@ridiculousfish

This comment has been minimized.

@SolitudeSF

This comment has been minimized.

@techalchemy

This comment has been minimized.

@SolitudeSF

This comment has been minimized.

@SolitudeSF

This comment has been minimized.

@ridiculousfish

This comment has been minimized.

@faho
Copy link
Member

@faho faho commented Mar 26, 2019

So, we now already only read /etc/paths in login shells (which macOS terminals tend to run, AFAIK), but I believe we still need the change to keep the order.

I've submitted #5767, but I can't actually test it, so I'm going to have to have someone on macOS do that for me. Ideally several people with different configurations.

@faho faho closed this in #5767 Mar 30, 2019
faho added a commit that referenced this issue Mar 30, 2019
* Keep the order for $PATH and $MANPATH when reading /etc/paths

Fixes #5456.
@faho faho mentioned this issue Mar 30, 2019
yurrriq added a commit to yurrriq/dotfiles that referenced this issue Apr 9, 2019
@jerryzhoujw
Copy link

@jerryzhoujw jerryzhoujw commented Jun 17, 2019

@faho I have got the same problem, may I help to test ? I wonder how to install the 3.0.3, I have install 3.0.2 with Installer.

Updated:

I have flowed

mkdir build; cd build
cmake ..
make
sudo make install

and replace fish fish_indent into usr/local/bin, and restart computer.

$ fish version
> fish, version 3.0.2-1298-g5b5887ea-dirty

and the result of echo $PATH and fish & echo $PATH still different order.

@sjuxax
Copy link
Contributor

@sjuxax sjuxax commented Jul 2, 2019

Had this collision today installing https://github.com/kennethreitz/fish-pipenv on Linux. The race described in the documentation there appears to be occurring, though I haven't really debugged it. https://github.com/kennethreitz/fish-pipenv/blob/master/conf.d/pipenv.fish#L3 calls command -s to see if pipenv is in the user's path, and since it isn't when fish loads that file, pipenv hard-codes it to return a pointer to the manual.

The fundamental issue re: pipenv seems to be that there's not a reliable way for it to check whether the user's final $PATH will contain the requested object or not. The macOS/general unstable path order thing seems like an independent issue. Unless fish explicitly instructs that command shouldn't be used in conf.d, I'd expect it to reliably search whichever paths the fully-started shell would know.

Any thoughts? cc @kennethreitz @jorgebucaran

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Jul 4, 2019

@sjuxax happy to try to help, I had trouble following your description. I understand this has something to do with the fact that ~/.config/fish/config.d files are sourced before ~/.config/fish/config.fish but maybe you can state it more clearly? Does this have to do with path_helper or is PATH being set in config.fish or is it something else?

@sjuxax
Copy link
Contributor

@sjuxax sjuxax commented Jul 5, 2019

I was referencing the explanation in the fish-pipenv README (also discussed at sentriz/fish-pipenv#1). In particular:

The situation with fish shell is that it executes scripts in the /Users/user/.config/fish/config.d folder before executing config.fish and the pipenv package creates a link in the config.d folder hence it is executed before config.fish.

Basically, the fish-pipenv plugin calls command -s to look for a pipenv executable on the user's path, and if doesn't find it, it creates a function called pipenv telling the user to install pipenv and try again. If the pipenv executable is made available on the path by a later part of the shell initialization, e.g., a line in config.fish, then the pipenv function will still be set and shadow the executable. This makes for a confusing situation for the user, who sees a message telling them to install pipenv even though which pipenv returns a valid executable on their path.

Ultimately the plugin should probably handle this itself by checking the path at runtime, but it raises questions around the semantics of command -s for plugin authors, since path elements that are added as part of the shell's standard initialization process may or may not be available based on the order which fish decides to load plugins and configs. It'd be ideal to clarify whether this is intended behavior or not.

@faho
Copy link
Member

@faho faho commented Jul 5, 2019

It'd be ideal to clarify whether this is intended behavior or not.

Yes, it is intended.

We have three options when it comes to conf.d snippets vs config.fish:

  • Snippets first, then config.fish - this means the user's ideas take precedence, allowing them to override stuff from snippets they got elsewhere
  • Config.fish first, then snippets - this means snippets can rely on config.fish, but means users need to either insert later snippets or modify the snippets in-place to override
  • Some scheme to have some snippets first, some later - this is quite the overcomplication

And we picked 1.

Ideally, you wouldn't rely on the user's configuration - like you said, it should just check the path at runtime (or just try it?). Checking first is a bit of a premature optimization - command -s is fast.

If you absolutely could not, you'd have to tell the user to insert their configuration for e.g. $PATH in a snippet that's sourced early - 00-path.fish or something.

But either way, none of that is really related to this issue - if you still have more questions related to this, I'd encourage you to open another one.

lilyball referenced this issue Jul 11, 2019
Just replace the issue number because this is effectively a
replacement of the previous.

[ci skip]
@zanchey
Copy link
Member

@zanchey zanchey commented Sep 16, 2019

For posterity, as I have just spent twenty minutes trying to understand the changes here:

  • #5767 was reverted as #5956
  • #5637 prevented this problem and was the "true fix"

@lilyball
Copy link
Contributor

@lilyball lilyball commented Sep 17, 2019

That sounds about right.

fricklerhandwerk added a commit to fricklerhandwerk/.config that referenced this issue Oct 28, 2019
the script did not work on macOS because `fish` prepends the contents of
`/etc/paths` and `/etc/paths.d/*` to `$PATH` [1], resulting in BSD
`readlink` to be used instead of the one from `nixpkgs`. this is fixed
on the master branch [2], but the patch is staged for 3.1.0 (!), while
the most recent release is 3.0.2.

well, also the script was badly broken, I never ran it since changing it
the last time...

[1]: fish-shell/fish-shell#5456
[2]: fish-shell/fish-shell#5637
@archywillhe
Copy link

@archywillhe archywillhe commented Dec 30, 2019

@zanchey wished I've seen your comment before spending 20min trying to figure out where is the fix

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.