-
-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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
Change all node formulas to use Homebrew-installed node rather than the first node on the path #176257
Comments
@Homebrew/core thoughts here? Seems fairly sensible to me but I may be missing something. |
env = { PATH: "#{HOMEBREW_PREFIX/"bin"}:$PATH" }
(bin/"foo").write_env_script "#{libexec}/bin/foo", env If this pattern is used enough, it feels like we should have a simpler way to specify it (I don't think this pattern is necessarily unique to node formulae...). Maybe have a function that returns the env hash?: (bin/"foo").write_env_script "#{libexec}/bin/foo", homebrew_bin_env (not a fan of |
I don't think that pattern is commonly used. Couldn't find any usage for More common are prepending homebrew-core/Formula/b/bcoin.rb Line 36 in 0fdd17a
homebrew-core/Formula/m/mongosh.rb Line 24 in 0fdd17a
Or shebang modification homebrew-core/Formula/a/apify-cli.rb Line 32 in 0fdd17a
homebrew-core/Formula/l/lando-cli.rb Lines 31 to 32 in 0fdd17a
I guess the original approach didn't require manually checking executable names as everything in EDIT: Also, Though, something similar to Java formulae logic could be used instead that generates env scripts for everything in bin.install Dir["#{libexec}/bin/*"]
bin.env_script_all_files libexec/"bin", Language::Java.overridable_java_home_env EDIT: On general topic, I agree that it does make sense to use a specific dependency. This is in alignment with how we handle other languages, e.g.
|
We automatically rewrite shebangs for some stuff (Python, Perl, ?), right? Doing this for NodeJS (and perhaps Ruby, etc.) too would make more sense to me than requiring DSL changes in hundreds of formulae.
Yeh, this.
Ok, yeh, so it's handled by upstream in these cases. Feels like if |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Hi @MikeMcQuaid, unfortunately this change broke setups that relied on version managers (like https://github.com/nvm-sh/nvm). I installed pnpm through Brew, which has a dependency on Node, so that was also installed through Brew. Different projects in my company rely on different versions of Node, all of which are defined by Is there a way to disable this so that I'm not forced to stop using Brew to manage my pnpm installation or have to use There are also others who have been confused by this behaviour and thought it was pnpm's fault. See: pnpm/pnpm#8384 |
I second the comment by @SenseiMarv. Suddenly pnpm is broken on systems where node versions are managed with fnm too. |
I think the fix should be that the shebang rewrite only happens if node is a required dependency. For the examples above (and also for yarn formula), node is not required - but is necessary to build and/or test. Or have a way for the formula to opt out of the rewrite - but I'd expect that to only be the case in situations where node is not a required dependency |
This seems reasonable to me, and can already be handled in specific formulae. See |
@ctaintor I don't understand the intricacies of what is rewritten and when. I only can say that this change broke every setup where node versions are managed through nvm/fnm or similar, so I believe it should be reverted asap. yarn, pnpm, you name it. |
I agree: this is the correct fix here.
@carlocab I'm a bit 😢 that that
We're going to fix forward instead of reverting. |
Yeah this isn't good. Cleaner is not supposed to have false positives so if we're going to start adding skip_clean everywhere then I don't think the cleaner is the correct place for this logic to be. My approval on the PR was conditional on that. I note that pnpm does not have a runtime dependency on Node, so that's perhaps where the bug is - it definitely should not be adjusting the shebang in that scenario. |
Otherwise, we rewrite this even when we have a e.g. build or test dependency on NodeJS. See context in: Homebrew/homebrew-core#176257 (comment)
Have opened Homebrew/brew#17993 for this. |
Otherwise, we rewrite this even when we have a e.g. build or test dependency on NodeJS. See context in: Homebrew/homebrew-core#176257 (comment)
This is in response to the discussion on updating the documentation for Node for Formula Authors. @MikeMcQuaid suggested opening a discussion here.
To summarize - most node formulas in
homebrew-core
add a binary by symlinking, likely because this is how the current documentation suggests it be done. However, using a symlink will mean that the firstnode
binary on yourPATH
will be used and this may not be the one which was installed by the formula viadepends_on "node"
. This is problematic if the binary installed by the formula requires newer versions of node or if the architecture of the Homebrew-node differs from that which is installed on the PATH (e.g. arm64 vs x86, which is what made me realize this problem).To fix this, it is suggested that all formulas which include
require "language/node"
and have adepends_on "node"
be changed, removing something likebin.install_symlink Dir["#{libexec}/bin/*"]
and instead doing something like:Verification
brew doctor
output saysYour system is ready to brew.
and am still able to reproduce my issue.brew update
and am still able to reproduce my issue.brew doctor
and that did not fix my problem.What were you trying to do (and why)?
Consistently run a CLI which was installed by a homebrew node formula.
What happened (include all command output)?
In my case, a user was getting an error which was related to npm not having installed an x86 version of a dependency. The reason they were getting this error is because they had an x86 version of node on their PATH while the Homebrew formula had installed and used an arm64 version of node, thus meaning that npm had only installed the arm64 version of a dependency.
What did you expect to happen?
I expected that the formula would use the Homebrew-installed node and not fail due to something the user had installed elsewhere.
Step-by-step reproduction instructions (by running
brew
commands)The text was updated successfully, but these errors were encountered: