Skip to content
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!: do not remove items from PATH in POSIX entrypoint #1521

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

hyperupcall
Copy link
Contributor

@hyperupcall hyperupcall commented Mar 28, 2023

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

@hyperupcall hyperupcall requested a review from a team as a code owner March 28, 2023 05:37
@hyperupcall hyperupcall changed the title fix: No longer munge path in POSIX entrypoint fix!: No longer munge path in POSIX entrypoint Mar 28, 2023
@hyperupcall hyperupcall changed the title fix!: No longer munge path in POSIX entrypoint fix!: No longer remove items in PATH in POSIX entrypoint Mar 28, 2023
@jthegedus
Copy link
Contributor

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

asdf.sh Show resolved Hide resolved
Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jthegedus jthegedus changed the title fix!: No longer remove items in PATH in POSIX entrypoint fix!: do not remove items from PATH in POSIX entrypoint Mar 28, 2023
@jthegedus jthegedus merged commit b6d0ca2 into asdf-vm:master Mar 28, 2023
@hyperupcall
Copy link
Contributor Author

hyperupcall commented Mar 28, 2023

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 🤷 .

@hyperupcall hyperupcall deleted the posix-entrypoint-tweaks branch March 28, 2023 06:59
@jthegedus
Copy link
Contributor

jthegedus commented Mar 28, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

Copy link
Member

@Stratus3D Stratus3D left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

willnorris added a commit to willnorris/dotfiles that referenced this pull request May 21, 2023
Previously, loading ASDF_DIR/asdf.sh rewrote ASDF's placement in PATH,
which required only loading it in root shells, and loading
ASDF_DIR/lib/asdf.sh in subshells.

asdf reworked asdf.sh in asdf-vm/asdf/#1480 and asdf-vm/asdf#1521 and
removed ASDF_DIR/lib/asdf.sh in asdf-vm/asdf#1525, which makes this
workaround no longer effective.

Instead, manually add ASDF_DIR/bin and ASDF_DATA_DIR/shim to PATH
directly in root shells and load ASDF_DIR/asdf.sh in all shells (since
it no longer messes with PATH).
@gabrielsoldani
Copy link

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
Copy link
Member

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

@gabrielsoldani
Copy link

gabrielsoldani commented Jun 15, 2023 via email

@hyperupcall
Copy link
Contributor Author

hyperupcall commented Jun 15, 2023

@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
Copy link
Member

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
Copy link
Contributor

jthegedus commented Jun 25, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants