fish-shell
fish-shell copied to clipboard
fish-shell
`__fish_macos_set_env` does not handle lines with colons
Using: fish 3.7.1 macOS Sonoma 14.5 Ghostty terminal
Please help me understand what https://github.com/fish-shell/fish-shell/blob/master/share/config.fish#L190-L192 is doing, as its behaviour seems incorrect to me.
I use Ghostty terminal, which appends itself to MANPATH. fish inherits MANPATH as:
:/Applications/Ghostty.app/Contents/Resources/ghostty/../man
fish then runs https://github.com/fish-shell/fish-shell/blob/master/share/config.fish#L190-L192, which sets MANPATH to:
/usr/share/man:/usr/local/share/man::/Applications/Ghostty.app/Contents/Resources/ghostty/../man
That is, fish prepends the paths from /etc/manpaths to MANPATH. (My /etc/manpaths.d/ is empty.)
man manpath says:
IMPLEMENTATION NOTES
The manpath utility constructs the manual path from two sources:
1. From each component of the user's PATH for the first of:
- pathname/man
- pathname/MAN
- If pathname ends with /bin: pathname/../share/man and pathname/../man
2. The configuration files listed in the FILES section for MANPATH entries.
The information from these locations is then concatenated together.
If the -L flag is set, the manpath utility will search the configuration files listed in the FILES section for MANLOCALE entries.
My default /etc/man.conf already contains everything in /etc/manpaths (i.e. /usr/share/man and /usr/local/share/man), so fish is prepending paths that already exist in man's search path.
Effectively, fish is moving those two paths from 2) and putting them before 1), but only if MANPATH is non-empty. This means that the system man pages in these 2 paths override anything in your PATH. Why is fish doing this?
For context, some Homebrew packages shadow macOS built-ins, so Homebrew installs them under alternate names (e.g. gsed if you install gnu-sed). If you add /usr/local/opt/gnu-sed/libexec/gnubin to your PATH, then sed will run GNU sed, and man sed should show the man page for GNU sed. This doesn't work in fish, and I think this is why.
As the comment says: This is what macOS' path_helper does in login shells, so we do the same thing.
It seems weird to me that Ghosttty would set $MANPATH just to its own path, in systems I know this would override the system default manpath.
It seems weird to me that Ghosttty would set $MANPATH just to its own path, in systems I know this would override the system default manpath.
@faho I missed a : in my original post (now edited). Ghostty appends itself to MANPATH.
As the comment says: This is what macOS' path_helper does in login shells, so we do the same thing.
__fish_macos_set_env leaves appended values as appended, but I noticed that path_helper converts appended values to prepended values. You can notice this in zsh, for example, which runs path_helper on macOS.
@faho Is __fish_macos_set_env supposed to mirror path_helper's behaviour exactly? I'm not sure if this difference is intentional or not.
__fish_macos_set_env leaves appended values as appended, but I noticed that path_helper converts appended values to prepended values
The source we have as well as this github mirror appends.
Our code seems to be a fairly straightforward translation of that - read /etc/manpath, read files in /etc/manpaths.d/, read $MANPATH, append each directory not already included.
If you're saying that doesn't happen, I'm going to need some more information about that, because given the code I don't see a difference.
If I'm reading the path_helper source correctly, append_path_segment drops empty segments. So when processing Ghostty's :/Applications/Ghostty.app/Contents/Resources/ghostty/../man, the leading : is effectively dropped.
So it appends /usr/share/man, /usr/local/share/man, and then /Applications/Ghostty.app/Contents/Resources/ghostty/../man, resulting in:
/usr/share/man:/usr/local/share/man:/Applications/Ghostty.app/Contents/Resources/ghostty/../man:
This is what I meant by path_helper converting appended values to prepended values -- the order is preserved, but Ghostty's path gets moved before the trailing : (I'm actually not sure where the trailing : is being added).
Because fish processes the empty segments, you get (notice the :: in the middle and no trailing :):
/usr/share/man:/usr/local/share/man::/Applications/Ghostty.app/Contents/Resources/ghostty/../man
This honestly makes more sense to me, but it just doesn't match path_helper.
I haven't tested, but there should be a similar discrepancy if $MANPATH contains a :: in the middle.
Our code also drops empty segments:
https://github.com/fish-shell/fish-shell/blob/325b51aca0974bd5409c6007e8e98b49a68f03c8/share/config.fish#L172-L173
Oh, hmm. I have no idea then. Somehow I am getting
/usr/share/man:/usr/local/share/man::/Applications/Ghostty.app/Contents/Resources/ghostty/../man
from fish, and I don't know why. zsh (which I believe actually runs path_helper) gives me
/usr/share/man:/usr/local/share/man:/Applications/Ghostty.app/Contents/Resources/ghostty/../man:
Hold on, the entry is ":/Applications/Ghostty.app/Contents/Resources/ghostty/../man".
That's gonna be read as one line, and then it's gonna be added as one line, then fish's "path variable" magic turns it into two entries.
We expect the path files to have one path per line instead of the awkward ":" gunk, and this breaks that assumption.
I'm going to reopen this for someone on macos to write a patch that handles ":" in path lines because I can't test it.
I would like to take a jab at this if it’s unclaimed. I encountered this issue today when trying ghostty too. If it’s game, can I have a little guidance on where to start?
Sure. In macos_set_env: https://github.com/fish-shell/fish-shell/blob/a579abb81bf883486f8d89181ce99ba291d84893/share/config.fish#L170-L175
You probably want to string split : before you check if the entry is already included.
This seems to work with Ghostty, although I'm not really confident about the nested while loops:
while read -l entry
string split : $entry | while read -l split_entry
if not contains -- $split_entry $result
test -n "$split_entry"
and set -a result $split_entry
end
end
end <$path_file
I tried passing -d : to while, but that wasn't doing what I expected it to do. With the nested while, all leading/trailing colons are removed.
Funny thing tho that I noticed is that the behaviour between fish and zsh are inverted in my case compared to how @injust is experiencing it. Starting Ghostty with command set to zsh --login -interactive gives me:
$ echo $MANPATH
/Users/redacted/.zi/polaris/man::/usr/share/man:/usr/local/share/man:/Applications/Ghostty.app/Contents/Resources/ghostty/../man
while for fish, I get the following:
$ echo $MANPATH
/usr/share/man /usr/local/share/man /Applications/Ghostty.app/Contents/Resources/ghostty/../man
This revelation also made me find out that I was using zsh in Ghostty and not fish as I thought I was.
although I'm not really confident about the nested while loops:
You don't need them if you redirect to string:
string split : <$path_file | while read
or use a for-loop instead:
for entry in (string split : <$path_file)
That for-loop sits better with my eyes imo. Here's the final version:
for path_file in $argv[2] $argv[3]/*
if test -f $path_file
for entry in (string split : <$path_file)
if not contains -- $entry $result
test -n "$entry"
and set -a result $entry
end
end
end
end
I'm using fish 4.0.0 now, which includes https://github.com/fish-shell/fish-shell/commit/95d61ea0fbd90ed01b6ea39a790ca0d1b6a14689 and so this should be fixed.
But the MANPATH behaviour hasn't changed and I'm still getting the same MANPATH as https://github.com/fish-shell/fish-shell/issues/10684#issuecomment-2564331523.
@westernwontons Is this fixed for you in Ghostty? I'm not sure if I've done something wrong elsewhere.
I'm using fish 4.0.0 now, which includes https://github.com/fish-shell/fish-shell/commit/95d61ea0fbd90ed01b6ea39a790ca0d1b6a14689
It doesn't. It appears we never committed it to the 4.0 branch.
Picking it for 4.0.1 now.
But the MANPATH behaviour hasn't changed and I'm still getting the same MANPATH as #10684 (comment).
I'm using fish 4.0.2 (which hopefully includes the fix) but my MANPATH still doesn't seem correct.
can you insert some debugging statements around __fish_macos_set_env in share/config.fish (check inputs, outputs etc.)
On fish 4.1.2 now, with this added to the beginning and end of __fish_macos_set_env:
if test "$argv[1]" = MANPATH
echo \$MANPATH = "$MANPATH"
end
outputs:
$MANPATH = :/Applications/Ghostty.app/Contents/Resources/ghostty/../man
$MANPATH = /usr/share/man:/usr/local/share/man::/Applications/Ghostty.app/Contents/Resources/ghostty/../man
With completely unconfigured zsh, I get:
$MANPATH = /usr/share/man:/usr/local/share/man:/Applications/Ghostty.app/Contents/Resources/ghostty/../man:
With completely unconfigured zsh, I get:
Confirmed that I get the same result when running path_helper from fish
> MANPATH=:/Applications/Ghostty.app/Contents/Resources/ghostty/../man /usr/libexec/path_helper | grep MANPATH
MANPATH="/usr/share/man:/usr/local/share/man:/Applications/Ghostty.app/Contents/Resources/ghostty/../man:"; export MANPATH;
So the only difference is that we're putting the second colon first instead of last
ok I think I'm close to a fix for the extra middle colon, but where does the extra colon at the end come from? https://raw.githubusercontent.com/apple-opensource-mirror/shell_cmds/4ea11424b80a117ad1e401867b1453bb2362feb3/path_helper/path_helper.c doesn't seem to set it
Given MANPATH=:/my/humble/manpath, the last paragraph of construct_path should execute as
result = /usr/share/man:/usr/local/share/man str = :/my/humble/manpath str = \0/my/humble/manpath append_path_segment(&result, str); // no-op str = /my/humble/manpath append_path_segment(&result, str); result = /usr/share/man:/usr/local/share/man:/my/humble/manpath
ok I think I'm close to a fix for the extra middle colon, but where does the extra colon at the end come from? apple-opensource-mirror/shell_cmds@
4ea1142/path_helper/path_helper.c (raw) doesn't seem to set it
I've been wondering the exact same thing...
When I compile the code myself, the output is different from the path_helper that ships with macOS...
$ MANPATH= ./a.out | grep MANPATH
MANPATH="/usr/share/man:/usr/local/share/man"; export MANPATH;
$ MANPATH= /usr/libexec/path_helper | grep MANPATH
MANPATH="/usr/share/man:/usr/local/share/man:"; export MANPATH;
Seems like https://raw.githubusercontent.com/apple-opensource-mirror/shell_cmds/4ea11424b80a117ad1e401867b1453bb2362feb3/path_helper/path_helper.c is outdated.
https://github.com/apple-oss-distributions/shell_cmds/blob/main/path_helper/path_helper.c gives me behaviour matching the /usr/libexec/path_helper shipped with macOS. It explicitly adds the trailing colon in main().
nice, that's certainly easier than trying to attach a debugger ;)
Makes me wonder why we don't use path_helper directly..
We could do eval "$(/usr/libexec/path_helper -c)" since we have a compatibility wrapper for csh's setenv.
But path_helper doesn't do any quoting, so quotes or dollar signs in PATH could inject code.. so I guess we either do some sanitization or stick to our reimplementation until Apple adds path_helper -f
The more I dig into this, the more I feel like path_helper is a PITA...
It turns MANPATH=:foo (something that is appended to man's default search path) into MANPATH=/usr/share/man:/usr/local/share/man:foo: (which is now prepended to man's search path).
The prepend basically blows away anything that comes from man's default search path (which takes your $PATH into consideration).
😭
it basically blows away anything that comes from man's default search path
Right, and surprisingly the default search path has some components interpsersed between /usr/local/share/man (which is the 1st entry below) and /usr/share/man (the 3rd), God knows why 🤷
$ manpath
/usr/local/share/man:/System/Cryptexes/App/usr/share/man:/usr/share/man:/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/share/man:/Library/Developer/CommandLineTools/usr/share/man
which takes your $PATH into consideration
not sure how
I pushed a "fix" to the faithful-macos-manpath branch.
Not sure if we should take it though,
it matches macOS behavior but it seems just wrong.
fish's current behavior, i.e.
MANPATH=:/Applications/Ghostty.app/Contents/Resources/ghostty/../man fish -lc 'echo $MANPATH'
/usr/share/man:/usr/local/share/man::/Applications/Ghostty.app/Contents/Resources/ghostty/../man
seems exactly what the user intended to happen - the system default manpath is inserted before the Ghostty one.
(God knows why /usr/share/man etc. must be prepended(!) explicitly rather than leaving this to the default manpath..)
it basically blows away anything that comes from man's default search path
Right, and surprisingly the default search path has some components interpsersed between
/usr/local/share/man(which is the 1st entry below) and/usr/share/man(the 3rd), God knows why 🤷$ manpath /usr/local/share/man:/System/Cryptexes/App/usr/share/man:/usr/share/man:/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/share/man:/Library/Developer/CommandLineTools/usr/share/manwhich takes your $PATH into consideration
not sure how
The manpath man page:
IMPLEMENTATION NOTES
The manpath utility constructs the manual path from two sources:
1. From each component of the user's PATH for the first of:
- pathname/man
- pathname/MAN
- If pathname ends with /bin: pathname/../share/man and pathname/../man
2. The configuration files listed in the FILES section for MANPATH entries.
The information from these locations is then concatenated together.
If the -L flag is set, the manpath utility will search the configuration files listed in the FILES section for MANLOCALE entries.
I pushed a "fix" to the
faithful-macos-manpathbranch. Not sure if we should take it though, it matches macOS behavior but it seems just wrong.fish's current behavior, i.e.
MANPATH=:/Applications/Ghostty.app/Contents/Resources/ghostty/../man fish -lc 'echo $MANPATH' /usr/share/man:/usr/local/share/man::/Applications/Ghostty.app/Contents/Resources/ghostty/../manseems exactly what the user intended to happen - the system default manpath is inserted before the Ghostty one. (God knows why
/usr/share/manetc. must be prepended(!) explicitly rather than leaving this to the default manpath..)
Agreed, it seems incorrect that path_helper just discards empty segments when parsing the existing $MANPATH.