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

shellenv.sh / fish shell: Move Brew PATHs to front if they exist (add -m arg to fish_add_path) #18304

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

WinkelCode
Copy link
Contributor

@WinkelCode WinkelCode commented Sep 10, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

(Unchecked steps aren't applicable here, in my opinion)


This PR addresses an issue when a PATH variable with Homebrew paths is inherited, modified, and then brew shellenv is evaluated again. In this case, the expected behavior is that Homebrew directories should be placed at the front of the PATH. However, since -m (move component) isn’t specified, fish_add_path doesn’t modify the variable, this can, for example, cause binaries in /usr/bin (like XCode CLI tools' Python) to take precedence.

I encountered this in VSCode, where eval (/opt/homebrew/bin/brew shellenv fish) in my config.fish didn’t properly update the PATH, as VSCode's environment resolution already included the paths, just not at the top. This change ensures $HOMEBREW_PREFIX/bin and $HOMEBREW_PREFIX/sbin are always placed first in PATH.

Correction:

It's not actually VSCode's fault that the Brew PATH components aren't at the top, they are, but when fish thinks it's a login shell (which it does when run in VSCode's terminal), it will add some PATH components before the inherited ones, causing the issue. Nonetheless, based on the behavior for other shells, it seems like the behavior with -m specified, which is what this PR does, would be what is intended for brew shellenv.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

This seems consistent with what we do for other shells 👍🏻

@carlocab
Copy link
Member

Thanks!

@carlocab carlocab merged commit c5d09b4 into Homebrew:master Sep 12, 2024
27 checks passed
@branchvincent
Copy link
Member

branchvincent commented Sep 12, 2024

👍 agree with this as well!

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