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 incorrectly sets a default $PATH on macOS #4336

Closed
akalin opened this Issue Aug 17, 2017 · 19 comments

Comments

Projects
None yet
6 participants
@akalin
Copy link
Contributor

akalin commented Aug 17, 2017

Fish version:

$ fish --version
fish, version 2.6.0

OS version:

system_profiler SPSoftwareDataType
Software:

    System Software Overview:

      System Version: macOS 10.12.6 (16G29)
      Kernel Version: Darwin 16.7.0
      Boot Volume: Root
      Boot Mode: Normal
      Secure Virtual Memory: Enabled
      System Integrity Protection: Enabled

Using Terminal.app:

$ echo $TERM
xterm-256color

Here's a repro (with default shell set to /bin/bash):

akalin$ echo $PATH
/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/usr/local/homebrew/bin:/Applications/Keybase.app/Contents/SharedSupport/bin:/usr/local/MacGPG2/bin:/Library/TeX/texbin

$ sh -c 'env HOME=$(mktemp -d) fish'
Welcome to fish, the friendly interactive shell

 /U/akalin> echo $PATH
/usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/homebrew/bin /Applications/Keybase.app/Contents/SharedSupport/bin /usr/local/MacGPG2/bin /Library/TeX/texbin /usr/local/bin

Here's the output of path_helper (which reads /etc/paths and /etc/paths.d/*):

> /usr/libexec/path_helper 
PATH="/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/usr/local/homebrew/bin:/Applications/Keybase.app/Contents/SharedSupport/bin:/usr/local/MacGPG2/bin:/Library/TeX/texbin"; export PATH;

The discussion in #4192 says that fish simply inherits its $PATH variable, but that doesn't seem to be the case!

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Aug 17, 2017

In addition, there seems to be some possibly-related weird phenomenon with inherited PATH variables:

> echo $PATH
/usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/homebrew/bin /Applications/Keybase.app/Contents/SharedSupport/bin /usr/local/MacGPG2/bin /Library/TeX/texbin /usr/local/bin

> set -x PATH $PATH APPEND

> echo $PATH
/usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/homebrew/bin /Applications/Keybase.app/Contents/SharedSupport/bin /usr/local/MacGPG2/bin /Library/TeX/texbin /usr/local/bin APPEND

> sh -c 'env HOME=$(mktemp -d) fish'
Welcome to fish, the friendly interactive shell
> echo $PATH
APPEND /usr/bin /bin /usr/sbin /sbin /opt/X11/bin /usr/local/homebrew/bin /Applications/Keybase.app/Contents/SharedSupport/bin /usr/local/MacGPG2/bin /Library/TeX/texbin /usr/local/bin

The APPEND somehow moved from the end of the $PATH to the beginning!

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 17, 2017

Fish does import PATH from the environment it inherits. However, configuration scripts can then modify the PATH. If you look at $__fish_datadir/config.fish you'll see that it contains this block of code:

# OS X-ism: Load the path files out of /etc/paths and /etc/paths.d/*
set -g __fish_tmp_path $PATH
function __fish_load_path_helper_paths
    # We want to rearrange the path to reflect this order. Delete that path component if it exists a
    # Since we are prepending but want to preserve the order of the input file, we reverse the array
    set __fish_tmp_path $__fish_tmp_path[-1..1]
    while read -l new_path_comp
        if test -d $new_path_comp
            set -l where (contains -i -- $new_path_comp $__fish_tmp_path)
            and set -e __fish_tmp_path[$where]
            set __fish_tmp_path $new_path_comp $__fish_tmp_path
        end
    end
    set __fish_tmp_path $__fish_tmp_path[-1..1]
end
test -r /etc/paths
and __fish_load_path_helper_paths </etc/paths
for pathfile in /etc/paths.d/*
    __fish_load_path_helper_paths <$pathfile
end
set -xg PATH $__fish_tmp_path
set -e __fish_tmp_path
functions -e __fish_load_path_helper_paths

So I conclude that you have /usr/local/bin as the last entry in /etc/paths and/or one of the files in /etc/paths.d.

@krader1961 krader1961 added the question label Aug 17, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 17, 2017

The APPEND somehow moved from the end of the $PATH to the beginning!

That's because that's the only PATH entry not in the path config files. This is standard macOS behavior. Personally I don't care for that spooky action at a distance dynamic setting of PATH. So I simply set PATH the way I want it in my ~/.config/fish/config.fish script if it's a login shell.

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Aug 17, 2017

Thank you, this explains it! I missed that there was another config.fish in $__fish_datadir. I did indeed have a file in /etc/paths.d that had /usr/local/bin as the only entry (shakes fist at wolframscript).

And yes, I'm finding myself disliking this spooky action at a distance also. :)

@akalin akalin closed this Aug 17, 2017

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Aug 18, 2017

Actually, after looking over the code and relevant commits/issues, and experimenting with bash and path_helper I think the relevant section in $__fish_datadir/config.fish is buggy.

path_helper seems to do deduplication on paths, and it prefers the position in /etc/paths. So even if I had /usr/local/bin in a file in /etc/paths.d, $PATH should still have /usr/local/bin first, assuming the default contents of /etc/paths which puts /usr/local/bin first.

The invocation of path_helper is in /etc/profile, so that means that /etc/paths and /etc/paths.d are read during login shells (i.e., new tabs/windows in Terminal.app). Furthermore, $PATH is clobbered in /etc/profile, but path_helper reads $PATH and seems to prepend the paths from /etc/paths and /etc/paths.d, getting rid of any subsequent duplicates. It doesn't seem to test whether the directories exist.

Of course, even when $PATH contains duplicates, bash (like many other shells, I assume) acts as if the earliest one takes precedence.

Finally, the comment at https://github.com/fish-shell/fish-shell/blob/master/share/config.fish#L128 seems to agree with the above:

We want to rearrange the path to reflect this order [from /etc/paths and /etc/paths.d/*]. Delete that path component if it exists and then prepend it.

However, that block of code tries to be clever by reversing, appending, and then reversing again. But that only works if it reads the input in reverse order also! It isn't, but that is mostly "cancelled" by the other bug, which is that it's actually still prepending $new_path_comp. However, this means that if $PATH is A B C D E P Q R S, and /etc/path and /etc/paths.d/* contains A B C D E, then the new $PATH will be P Q R S A B C D E, when from everything above it should be A B C D E P Q R S. Furthermore, if /etc/path and /etc/paths.d/* contains A B C D E A, and the old $PATH is empty, then the new $PATH will be B C D E A, when it should be A B C D E.

Finally, that block of code should really be inside a status --is-login block.

Does the above sound right to you? cc @ridiculousfish, since he wrote the original code.

So to fix the above I suggest the following:

  1. Put the reading of /etc/paths and /etc/paths.d/* inside a status --is-login block.
  2. Fix __fish_load_path_helper_paths to read from /etc/paths, /etc/paths.d/*, and $PATH, in that order, ignoring duplicates.
  3. Remove the test for non-existent directories. (Since they may come into existence later...)

@akalin akalin reopened this Aug 18, 2017

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Aug 18, 2017

I'll want to fix this locally, so I can probably send a pull request.

@akalin akalin referenced this issue Aug 18, 2017

Closed

Fix reading of /etc/paths on OS X #4337

0 of 3 tasks complete
@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Aug 18, 2017

Ugh, playing around with path_helper a bit more, it appears it clobbers $PATH, but it also reads $PATH. Investigating more...

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Aug 18, 2017

Okay, I think I now understand the behavior of path_helper. Updated comment above and the pull request.

@krader1961 krader1961 added enhancement and removed question labels Aug 18, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 18, 2017

FYI, Once you ignore the recent changes to fix the style of the code and minor improvements you'll see that most of the logic is based on issue #417 as implemented by commit d99c2cb.

@krader1961 krader1961 added this to the fish-3.0 milestone Aug 18, 2017

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Aug 18, 2017

Ah, you're right. The only thing that it misses, I think, is that the code in #417 clobbers $PATH, and d99c2cb appends to it.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 18, 2017

Okay, it's a major release only change. If we're only going to do this for login shells would it make more sense to use /usr/libexec/path_helper? On my macOS server it clocks in at 5 ms. The comment in issue #417 about wanting to avoid forks (i.e., launching external processes) is generally a good idea but seems counter-productive here given how cheap it is to use that command. Especially when it is one command of many run when launching a login shell. We also have precedent for doing this type of thing. See the invocation of dircolors in the share/functions/ls.fish script. I'd be curious to know how much faster the custom fish script is. I'm betting it is less than an order of magnitude. In which case it isn't worth the complexity. Especially since the other macOS shells already pay that cost.

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Aug 18, 2017

Hmm, yeah, if we're not opposed to forking, it might be cleaner. The only downside is that path_helper outputs a couple of bash commands that we'd have to parse, which might be hard to do without forking some more. I'll try it out...

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 18, 2017

The only downside is that path_helper outputs a couple of bash commands that we'd have to parse, ...

But so does dircolors which we handle in the share/functions/ls.fish script. The same approach should work here.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 18, 2017

Also, while we want to, as a general rule, avoiding running external commands if the same result can be achieved solely with fish builtins there are many situations where the external command is the better choice. I'm pretty confident this is one of those situations since we should not be reinventing the wheel given that using the current wheel, /usr/libexec/path_helper, is very fast (0.005 seconds on my system).

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Aug 18, 2017

Ah, taking a look at ls.fish, you're right! Sorry, I wasn't that familiar with fish's string manipulation commands.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Aug 18, 2017

#927 might be worth reviewing as well.

@floam

This comment has been minimized.

Copy link
Member

floam commented Aug 19, 2017

The only downside is that path_helper outputs a couple of bash commands that we'd have to parse, which might be hard to do without forking some more. I'll try it out...

It can also do csh-compatilble output, which works in fish:

$ /usr/libexec/path_helper -c
setenv PATH "/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin";

@krader1961 krader1961 changed the title fish moves /usr/local/bin to the end of $PATH in macOS fish incorrectly sets a default $PATH on macOS Aug 19, 2017

@azag0

This comment has been minimized.

Copy link

azag0 commented Feb 10, 2018

I'll just add another UX. I use both bash and fish, so I set my PATH in ~/.profile, and start fish with bash -lc fish. Currently, __fish_load_path_helper_paths mangles the order in my PATH, so I simply comment out that part of config.fish.

@akalin akalin referenced this issue Mar 24, 2018

Closed

Fix reading of /etc/paths on OS X #4852

0 of 3 tasks complete

ridiculousfish added a commit that referenced this issue Mar 28, 2018

Fix reading of /etc/paths on OS X
(and /etc/paths.d/*)

Do so by emulating the behavior of /usr/libexec/path_helper for login
shells, matching the behavior in /etc/profile.

Also add a path_helper command to reproduce the behavior of
/usr/libexec/path_helper for fish.

This also handles setting MANPATH if necessary.

Fixes issue #4336
@faho

This comment has been minimized.

Copy link
Member

faho commented Apr 10, 2018

This was fixed by merging #4852. Thanks @akalin!

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