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

Add Apple Silicon support #13

Merged
merged 3 commits into from
Apr 13, 2022
Merged

Conversation

lunij
Copy link
Contributor

@lunij lunij commented Oct 30, 2021

This is an addition to the PR #12 by @nemoDreamer.

Newer installations of Homebrew use /opt/homebrew/ instead of /usr/local/.

The difference is the if type -q brew condition that didn't work for me on my M1. type can only find brew when it is already part of the $PATH variable, but to be part of the $PATH variable this fish script needs to run first.

In other words: This fish script adds necessary paths used by brew to $PATH to make brew accessible. Only after this script did its job, commands like type brew and which brew can finally work.

nemoDreamer and others added 2 commits June 14, 2021 19:48
With this change, a path is added to `$PATH` only if it is not already
part of `$PATH` and contains a file called `brew`. Also, the path is
added in the beginning of `$PATH` to ensure it has precedence over
system paths, otherwise system apps like `ruby` and `python` would be
preferred over the Homebrew variants. It should be the other way around.
Copy link
Member

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

Hey, @lunij, thanks for contributing!

How about reporting the absence of brew at the end, after all existing paths are appended to $PATH? This may not only maintain the behavior in that regard but also help troubleshooting issues.

@lunij
Copy link
Contributor Author

lunij commented Nov 1, 2021

How about reporting the absence of brew at the end [...]

Yes, I did it.

In addition I changed the algorithm to one for loop only. Now, a path is added to $PATH only if it is not already part of $PATH and contains a file called brew. Also, the path is added in the beginning of $PATH to ensure it has precedence over system paths, otherwise system apps like ruby and python would be preferred over the Homebrew variants. It should be the other way around.

@scorphus
Copy link
Member

scorphus commented Dec 20, 2021

How about reporting the absence of brew at the end [...]

Yes, I did it.

Thank you!

In addition I changed the algorithm to one for loop only. Now, a path is added to $PATH only if it is not already part of $PATH and contains a file called brew. [...]

Nice! We need to make sure contains is supported in older Fish releases.

[...] Also, the path is added in the beginning of $PATH to ensure it has precedence over system paths, otherwise system apps like ruby and python would be preferred over the Homebrew variants. It should be the other way around.

It's usually a good idea to give system apps preference. Perhaps not when it's the user's shell (whereas in bash and scripts not child of the user's shell process, system's apps would still be preferred — which could also cause confusion other than failures).

I don't really know all implications as I always append (instead of prepend) brew path to PATH — and use virtual environments for pretty much everything (ruby, python, etc.)

Still, this is a radical change that might affect many users who have it the other way around. Maybe that should still be the default behavior and a the plugin could support some sort of a flag to indicate that brew paths should be prepended.

What are your thoughts, @bobthecow?

Copy link

@isinyaaa isinyaaa left a comment

Choose a reason for hiding this comment

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

Hi, folks. I've been trying to set this up on my machine and was having issues until I noticed that if for some reason, the user is already appending to PATH somewhere else, this script will yell needlessly.

conf.d/brew.fish Outdated
else
end

if not set -q brew_found

Choose a reason for hiding this comment

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

My suggestion would be:

Suggested change
if not set -q brew_found
if not set -q brew_found; and not command -q brew

@misimpso
Copy link

misimpso commented Apr 5, 2022

I was having the problem of brew not being available on the path, and fish complaining with the "type -q" conditional check failing. This fix, along with the first fix from this issue, fixed my problem.

@scorphus scorphus force-pushed the marc/apple-silicon branch 2 times, most recently from d361f27 to 0021f7a Compare April 13, 2022 17:34
@scorphus
Copy link
Member

Thanks, @lunij! :shipit:

@scorphus scorphus merged commit 0021f7a into oh-my-fish:master Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants