bug: PATH order situationally broken on MacOS

[merrilymeredith]

Describe the Bug

Currently on main on MacOS, starting a login shell with an asdf environment already set up will leave asdf at the end of the path. This behavior changed with #1521. The trouble is thanks to /etc/zprofile and path_helper, which shuffles the inherited path, then asdf now avoids moving itself back to the front. zprofile doen't appear to be protected by SIP, but I imagine it gets clobbered by updates occasionally, if users were asked to change it.

Steps to Reproduce

If you open a terminal, for example, then tmux or exec into tmux, it starts login shells by default, and those will have system and other paths moved ahead of asdf. This is an example just running a child shells explicitly, with child shells indented for clarity:

❯ which ruby
/Users/meredith/.asdf/shims/ruby
❯ zsh
  ❯ which ruby
  /Users/meredith/.asdf/shims/ruby
  ❯ exit
❯ zsh --login
  ❯ which ruby
  /usr/bin/ruby
  ❯ logout
❯ which ruby
/Users/meredith/.asdf/shims/ruby
❯ exec zsh --login
❯ which ruby
/usr/bin/ruby
❯ logout

If you set up a terminal profile that starts tmux directly, on the other hand, asdf isn't added to the path beforehand, and the login shells started inside that session don't have this issue.

Expected Behaviour

This is an awkward situation thanks to the MacOS behavior, but I'd expect asdf to keep path priority with a normal install without requiring workarounds. I think I'm following main because I have an extreme legacy checkout, but this could cause a lot of surprises when the change hits a release.

Actual Behaviour

Detailed above.

Environment

OS:
Darwin mhoward-mbp 21.6.0 Darwin Kernel Version 21.6.0: Mon Dec 19 20:46:01 PST 2022; root:xnu-8020.240.18~2/RELEASE_ARM64_T8101 arm64

SHELL:
zsh 5.9 (arm-apple-darwin21.3.0)

BASH VERSION:
5.2.15(1)-release

ASDF VERSION:
v0.11.3-684f4f0

ASDF INTERNAL VARIABLES:
ASDF_DEFAULT_TOOL_VERSIONS_FILENAME=.tool-versions
ASDF_DATA_DIR=/Users/meredith/.asdf
ASDF_DIR=/Users/meredith/.asdf
ASDF_CONFIG_FILE=/Users/meredith/.asdfrc

ASDF INSTALLED PLUGINS:
deno                         https://github.com/asdf-community/asdf-deno.git master 87442fe
elixir                       https://github.com/asdf-vm/asdf-elixir master 1693b35
erlang                       https://github.com/asdf-vm/asdf-erlang master 0d402e6
nim                          https://github.com/rfrancis/asdf-nim.git master 4c0ce68
nodejs                       https://github.com/asdf-vm/asdf-nodejs master 644ada3
perl                         https://github.com/ouest/asdf-perl.git main f1b2040
ruby                         https://github.com/asdf-vm/asdf-ruby master f167b05
rust                         https://github.com/code-lever/asdf-rust.git master 0c88f99

asdf plugins affected (if relevant)

No response

[jthegedus]

@merrilymeredith Thanks for the report.

this could cause a lot of surprises when the change hits a release

This is why we haven't published a release for this yet, we were sure there would be some issues. It is marked as a "BREAKING CHANGE", so this shouldn't surprise people who read the updates before installing them.

@merrilymeredith I am not sure how we should proceed here. The only way I see us being able to guard against other tooling moving PATH is by us always modifying PATH, which just adds to the problem.

@hyperupcall @Stratus3D what are your thoughts on how we handle this?

[hyperupcall]

I think there are two ways to fix the issue:

1. Introduce an env vars ASDF_FORCE_PREPEND= to force asdf.sh to prepend the asdf values to the start of the PATH.

With this option, it is clear that this behavior deviates from the default of other shells like Elvish, Fish, etc. We only need to add this to asdf.sh because as far as I know path_helper only works with sh and bash-compatible shells (including zsh). It must be an environment variable rather than a flag because POSIX doesn't define the behavior of passing extra arguments to .. Since path_helper outputs for sh, we need to cover all bases. We also need to make documentation clear that this variable ideally should not be exported and only used like ASDF_FORCE_PREPEND= . asdf.sh

rbenv ended up doing something like this (paths were duplicated rather than being moved to the front).

2. Instruct users how to fix the broken behavior of path_helper

Fundamentally, there must be no asdf entries in the PATH when path_helper is ran. Some fixes seem to include:

A.

This SO post showing to empty PATH before sourcing /etc/profile, which will eventually run path_helper. Of course, this will need to be modified for newer macOS versions that use Zsh. Another example of this fix is here

if [ -f /etc/profile ]; then
    PATH=""
    source /etc/profile
fi

B.

Mentioned here, another way of fixing this bug without modifying /etc/z?profile is making path_helper no longer an executable:

sudo chmod ugo-x /usr/libexec/path_helper

C.

rvm seems to fix this by appending their PATH-appending logic to ~/.zlogin instead of ~/.zprofile.

I don't have a Mac, so I am unable to test these fixes, but I imagine the last 2C one would be the cleanest

[jthegedus]

I tried to argue that this is a user problem rather than rbenv's, but I
can't fix everybody shell init when they report bugs. Instead, let's
revert to simpler times in rbenv where we just roll along with the
duplication and don't ask any questions.

From the rbenv change.

I don't have a Mac, so I am unable to test these fixes, but I imagine the last one would be the cleanest

Do you mean option 2.C?

rvm seems to fix this by appending their PATH-appending logic to ~/.zlogin instead of ~/.zprofile.

[hyperupcall]

Do you mean option 2.C?

Yeah, I mean option 2C - it seems to be the cleanest. Since it doesn't require doing weird hacks like PATH= or removing the executable bit from path_helper, that other installed apps may depend on?

[hyperupcall]

I just made a PR (#1560) that implements option 1 from this thread. It is the most straightforward way to fix things from the perspective of a user, especially if they are new to the whole shell thing.

[Stratus3D]

Option 1 does seem like the easiest solution right now. None of these seem particular elegant, but #1560 seems like the most pragmatic.