fix!: do not remove items from PATH in POSIX entrypoint

[hyperupcall]

Summary

The PATH issue was briefly mentioned in the original POSIX entrypoint PR, but it was not fixed there.

The issue is that previously asdf modified the PATH variable, and removed the asdf bin or asdf shims directory (from PATH) if they existed. Besides not being good practice (in my opinion), this had issues since this behavior would only run under Bash and ZSH (POSIX doesn't support substitution in parameter expansion and I intentionally didn't add this behavior to KSH for simplicity). Since the same script could change PATH differently (when ran under different shells), this can be confusing behavior. Also, neither the Elvish, or Nushell scripts remove items from the PATH, this also could be confusing.

These changes make it so the POSIX entrypoint match the Elvish, Fish, and Nushell behavior by only adding the bin and shims directory if they aren't already in the path.

Tested with Zsh, Bash, Dash, Ksh, Mksh

Edit: These changes don't match Fish behavior, I didn't read the asdf.fish correctly

[jthegedus]

I can't quite remember the scenario under which we added the "removal and then add" of items to PATH, but it solved some problem... 🤷

Given this is effectively part of #1480 and a breaking change alongside it, I will merge so they can be tested together by those utilising the repo HEAD

[jthegedus]

LGTM, let's merge to let people test this alongside #1480

[hyperupcall]

I can't quite remember the scenario under which we added the "removal and then add" of items to PATH, but it solved some problem... shrug

I suppose it's possible a user in an interactive shell sources asdf.sh with the intention of making sure the asdf bin and shims have priority (prepended first in PATH). I guess it can come up if they are adding other directories to PATH and they suddenly want to have asdf shims/binaries to take priority.

If we want to support that behavior, then in my opinion we can just unconditionally append to the path (removing the case statement). That way it will work even if the user it in Dash or Ksh or whatever. I guess that'll make PATH slightly longer, but 🤷 .

[jthegedus]

I suppose it's possible a user in an interactive shell sources asdf.sh with the intention of making sure the asdf bin and shims have priority (prepended first in PATH). I guess it can come up if they are adding other directories to PATH and they suddenly want to have asdf shims/binaries to take priority.

Having asdf prepended to PATH seems to be the preferred behaviour from our existing and historic user base. See this recent discussion #1496 (comment)

I guess that'll make PATH slightly longer, but 🤷 .

The introduction of the preprend to PATH was to resolve issues people had with over population of PATH. Many people cared about having a clean PATH, so appending data to PATH and unnecessarily increasing PATHs length is not preffered.

This is a reason as to why I was suggesting we have an entrypoint script for each Shell we support, even if they have a 90% overlap in implementation, this way simplifications to the code to ensure it works across all Shells does not interfere with existing behaviours of other Shells.

So yes, I think we will need to modify this to prepend as it did previously and to align with Elvish/Fish/Nushell behaviour. Whether or not we remove from PATH first really depends on how people source asdf, so that we only prepend once or if we re-run this code a lot, only prepend if it is not in PATH. Again, I am fuzzy on why we did the removal and then addition instead of just not adding if it was already in PATH.

[hyperupcall]

I was a bit confused reading your response but I think that's because we have different definitions of prepend.

When I use prepend, I'm strictly talking about the act of adding strings to the beginning of PATH, conditionally (ie only if string is not somewhere in PATH) or unconditionally. But when you use the word, you mean that the string must always be at the beginning after prepending, correct?

I think even with the definition I think you're using I'm still confused but I think I understand the outcome you prefer.

This is a reason as to why I was suggesting we have an entrypoint script for each Shell we support, even if they have a 90% overlap in implementation, this way simplifications to the code to ensure it works across all Shells does not interfere with existing behaviours of other Shells.

I see what you mean now and I apologize for that.

So yes, I think we will need to modify this to prepend as it did previously and to align with Elvish/Fish/Nushell behaviour.

I checked the scripts again and I'm more confused since those three shells don't seem to have consistent behavior?:

• POSIX Shell/Bash/Zsh Past Behavior AND Fish Current behavior:

  • String is put at beginning of path always. If string is already in PATH and is not at the beginning, it is removed.

• POSIX Shell/Bash/Zsh Current Behavior AND Nushell/Elvish current behavior:

  • String is put at beginning of PATH, but only if the string isn't already in PATH

Both methods keep a "clean" path, but only the first ensures that asdf remains at the beginning of PATH if adsf.<ext> is sourced.

The first one is what we want to go for, right? I should revert most of these Bash/etc. changes, and change Nushell and Elvish so their behavior matches Fish and the old Bash behavior?

[jthegedus]

Sorry for the confusion. My incorrect recollection was that Nushell/Elvish were implemented the same as Fish.

String is put at beginning of PATH, but only if the string isn't already in PATH

I prefer this method as it preserves user's custom configuration if they decide to add asdf to PATH themselves.

In my opinion there is no need to revert.

I think we just need to update Fish to align with everything else and perhaps update the docs to communicate we prepend dirs if not present in PATH.

[hyperupcall]

In my opinion there is no need to revert.

👍

I think we just need to update Fish to align with everything else and perhaps update the docs to communicate we prepend dirs if not present in PATH

Sounds good! I can make a PR that does that

[Stratus3D]

String is put at beginning of PATH, but only if the string isn't already in PATH

I think you made the right choice in selecting this approach.

[gabrielsoldani]

Have you considered using sed to remove PATH entries and maintain the previous behavior?

I agree the new behavior is better, but I don't find the lack of pattern substitution in POSIX variable expansion a compelling argument to introduce breaking changes when it could just run POSIX sed to do the replacement instead.

[Stratus3D]

@gabrielsoldani is this a breaking change? Both this PR and #1480 (comment) are labeled fixes.

[gabrielsoldani]

@gabrielsoldani is this a breaking change? Both this PR and #1480 (comment) are labeled fixes.

Yeah. Previous to this change, on bash and zsh, `asdf.sh` checked if the `PATH` already contained the bin and shims directories, and if it did, it removed them and prepended them, ensuring asdf is first in the `PATH` after sourcing the file. Now, to get the same behavior in zsh and bash, you need to set `ASDF_FORCE_PREPEND` to `yes` (it defaults to `no`, except on macOS where it defaults to `yes`, so this is not a breaking change for macOS) (see #1560). To avoid the breaking change it could simply default to `yes` everywhere.

[hyperupcall]

@gabrielsoldani The changes were made to make the behavior consistent across shells (the ones asdf supports) and user expectations and other tooling. If every tool automatically forcefully made themselves the first entry in the PATH, the user would have no control when they should.

I only mention the non-POSIX pattern substitution because that adds to the inconsistent behavior (sourcing same file, different results).

Nothing was changed solely because pattern substitution in variable expansions is not in POSIX - as you can see in the final asdf.sh included in v0.12.0, the behavior was implemented for non-Bash, non-Zsh shells (without using sed, since that is too slow).

Your points about this being a breaking change (this PR, #1521, unrelated to #1480) are correct and were discussed, which is why I authored #1560 so users could explicitly choose with the variable. But yes, it does not default to yes on non-macOS platforms for the reasons I mentioned above.

[Stratus3D]

To avoid the breaking change it could simply default to yes everywhere.

This is a good point. It is breaking because we changed the default. But I think this is a necessary change. I'm going to catch up on what was discussed in #1579 next.

[jthegedus]

@gabrielsoldani is this a breaking change? Both this PR and #1480 (comment) are labeled fixes.

@Stratus3D The ! denotes that this is breaking. From the release-please docs:

The most important prefixes you should have in mind are:
fix: which represents bug fixes, and correlates to a SemVer patch.
feat: which represents a new feature, and correlates to a SemVer minor.
feat!:, or fix!:, refactor!:, etc., which represent a breaking change (indicated by the !) and will result in a SemVer major.

We also have the setting for the release-please-action:

bump-minor-pre-major: Should breaking changes before 1.0.0 produce minor bumps?

Set to true. So anything with ! produces minor version bumps not major.

Hope that clarifies the linked fix!: changes.