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

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

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@akalin
Copy link
Contributor

akalin commented Mar 24, 2018

(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

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md
@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Mar 24, 2018

This is the follow-up promised in #4337 (comment) .

# see __fish_construct_path in config.fish and
# https://opensource.apple.com/source/shell_cmds/shell_cmds-203/path_helper/path_helper.c.auto.html .

function __quote -d "single-quote arguments, escaping characters as necessary"

This comment has been minimized.

@akalin

akalin Mar 24, 2018

Contributor

I couldn't find a built-in way to quote a string. Thought string escape would do it, but it does something different.

This comment has been minimized.

@faho

faho Mar 25, 2018

Member

string escape will do what you want - escape a string so that it'll pass through source unharmed. It just doesn't always use single-quotes. Instead it falls back to using \\ for more complicated things.

However, I'm not quite sure why this path_helper function exists at all - you're not using it anywhere as far as I can see, and since it would run in the fish process, there's no need to output the set statements - instead, you could just directly run them. Which would also remove the need for quoting.

This comment has been minimized.

@akalin

akalin Mar 25, 2018

Contributor

It's just a driver for debugging the path_helper logic without having to start shells and examine environment variables. I can remove it if needed.

This comment has been minimized.

@faho

faho Mar 25, 2018

Member

Yes, please! This looks like something to invoke, and it really shouldn't be.

This comment has been minimized.

@akalin

akalin Mar 25, 2018

Contributor

Done! Please take another look.


function __quote -d "single-quote arguments, escaping characters as necessary"
for x in $argv
set -l quoted (string replace -a -r '([\\\\\'])' '\\\\\\\\$1' $x)

This comment has been minimized.

@akalin

akalin Mar 24, 2018

Contributor

Not exactly sure why I need to triple-escape the \ in \\\\\\\\$1; need to escape once because it's in a single-quoted string, another because it's a replacement string for a regexp, but I can't think of a reason for a third escape.

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Mar 24, 2018

Pinging original main reviewer @krader1961.

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Mar 24, 2018

Hmm, not sure why invocation/login-interactive.invoke is failing. Seems to fail on master for me, so might be unrelated. Never mind, should be fixed now.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Mar 24, 2018

@ridiculousfish is probably the best reviewer here.

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Mar 25, 2018

Thanks for doing this!

Can you please catch me up on the rationale? /usr/libexec/path_helper is meant to be consumed by shells, not exported by them, so I don't understand why we would want to implement it in fish?

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Mar 25, 2018

Sure thing! My previous PR #4337 took the approach of parsing path_helper's output. However, that turned out to be more complicated and brittle, for a few reasons:

  • path_helper prints out lines meant to be evaluated in csh or sh. So fish would have to parse those lines, which is more complicated than just reading the same files that path_helper does. Someone did suggest just eval'ing the csh output, but that's just scary.
  • path_helper tries to escape its output, but does so half-heartedly (in particular, it doesn't handle spaces). So we'd have to unescape that output, which makes it more complicated than just parsing the files.
  • By implementing the logic in fish, we avoid a fork.

If you compare this PR to #4337 , this one does seem simpler to understand. The danger of our behavior diverging from path_helper is minimal, since path_helper has barely changed since it was first implemented.

However, if you prefer the other approach, I could always resurrect #4337 .

end

for x in $result
echo $x

This comment has been minimized.

@faho

faho Mar 26, 2018

Member

Note: This entire for-loop could also be replaced with set -xg $argv[1] $result - then the outer set becomes unnecessary.

This comment has been minimized.

@akalin

akalin Mar 28, 2018

Contributor

Thanks, done!

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 26, 2018

This looks quite nice to me.

Unfortunately, I can't really test it, so I'll leave that up to @ridiculousfish!

@faho faho added this to the fish-3.0 milestone Mar 26, 2018

@faho faho added the enhancement label Mar 26, 2018

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Mar 26, 2018

@akalin thanks for the explanation. I'm on board with emulating path_helper, but I don't understand the reason behind the second change, the "add a path_helper command to reproduce the behavior of
/usr/libexec/path_helper for fish". Who will call this function?

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Mar 28, 2018

@ridiculousfish Per @faho's suggestion, I removed the path_helper function; I had it around only for debugging. Please take another look!

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Mar 28, 2018

Looks good. Squash-merged as adbaddf. Thanks!!

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Mar 30, 2018

Note that this changes the behaviour from running for all shells to running for login shells only. This matches the behaviour for other shells but breaks sessions run using iTerm2's "run command" option, for example.

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Mar 30, 2018

@zanchey how do people using other shells solve the problem? Perhaps we could expose a function for people to call (that does what this PR does for a login shell) if they want that behavior?

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Mar 31, 2018

Nice spotting @zanchey, I switched it back to running for all shells in c0f832a.

@akalin

This comment has been minimized.

Copy link
Contributor

akalin commented Apr 1, 2018

Hi, just to be clear, I made it login-shell-only to match the behavior of sh/bash (since the call to path_helper is in /etc/profile). So it's intended that fish deviate from that and read the paths for every shell?

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Apr 1, 2018

Nothing is really intended. I reverted that because it looked like an undocumented behavior change, and zanchey observed that it broke a use case ("Run Command" in iTerm2).

Nobody has made a case for what the right behavior should be, so it's better to not break any users who might be depending on the existing behavior.

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