-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
docs: updates the node formula example to use the node installed by Homebrew #17539
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,7 +102,19 @@ class Foo < Formula | |
|
||
def install | ||
system "npm", "install", *Language::Node.std_npm_install_args(libexec) | ||
bin.install_symlink Dir["#{libexec}/bin/*"] | ||
|
||
# "link" the executable `foo`, ensuring that the version of node installed by Homebrew is used | ||
# (the created `foo` will set the ENV before exec'ing your executable) | ||
env = { PATH: "#{HOMEBREW_PREFIX/"bin"}:$PATH" } | ||
(bin/"foo").write_env_script "#{libexec}/bin/foo", env | ||
|
||
# Uncomment if you simply want to symlink the executable – note that this means the first | ||
# `node` on the PATH will be used (not necessarily the one Homebrew installed) | ||
# bin.install_symlink Dir["#{libexec}/bin/*"] | ||
|
||
# Uncomment if you want to write the completion scripts for bash, fish, and zsh (assuming | ||
# your executable has a command "completion" which returns a completion script) | ||
# generate_completions_from_executable(libexec/"bin/foo", "completion", base_name: 'foo') | ||
Comment on lines
+114
to
+117
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How many formulae does this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure but personally when I was writing a formula I didn't know that this was an option (adding completions) and I also did not know that the (In my case, my CLI already supported completions – so it was as simple as knowing that it's possible and easy to have Homebrew set it up) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like 387 formulae use However, out of the 173 formulae that have Of those 10, only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Note that when you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given this is used by <10% of Node formulae: I'm not sure it's worth documenting specifically here. |
||
end | ||
|
||
test do | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which/how many formulae do this each of these ways? CC @homebrew/core for thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't know when you'd ever want it this way – it's just how it was documented previously. At the very least, if the formula has a
depends_on
tonode
then I'd expect it to always use thewrite_env_script
to force the usage of the Homebrew-installednode
.yarn will use the first
node
on the path but I'd imagine this is an outlier (and it is also not callingbin.install_symlink
– but it is not trying to always use homebrew's node)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the 173 formulae with
require "language/node"
:5 use
write_env_script
, although only 3 use it in the exact way suggested here:protoc-gen-grpc-web
system "npm", "install", *Language::Node.std_npm_install_args(libexec)
so maybe it shouldn't countmongosh
code-server
bcoin
emscripten
On the other hand, there are 155 formulae that use
bin.install_symlink
and 145 of those usebin.install_symlink Dir[libexec/"foo"]
. So, the original way this is written seems to be by far the most prevalent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Rylan12!
Given the prevalence here: I think this may be worth opening a Homebrew/core issue to discuss if we want to migrate the approach here. I don't think it makes sense to have the documentation recommend the opposite of what most formulae are doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened it here – Homebrew/homebrew-core#176257 – apologies if that was not the way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's perfect, thanks!