Skip to content

docs: recommend using libexec.glob() for symlinking binaries from npm modules #21318

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.

Sign up for GitHub

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

Jump to bottom
Merged
p-linnane merged 2 commits into from
Dec 25, 2025
Merged

docs: recommend using libexec.glob() for symlinking binaries from npm modules #21318

p-linnane merged 2 commits into from
Dec 25, 2025

Conversation

@sushichan044
Copy link
Contributor

@sushichan044 sushichan044 commented Dec 24, 2025

Reflect the pattern commonly used in newer npm module formulae

  • 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 lgtm (style, typechecking and tests) with your changes locally?

Copilot AI review requested due to automatic review settings December 24, 2025 11:52
Copilot AI reviewed Dec 24, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the documentation to recommend using the libexec.glob() method instead of Dir[] for symlinking binaries from npm modules, aligning with patterns used in newer npm module formulae.

  • Changes the recommended approach for symlinking npm module executables from Dir["#{libexec}/bin/*"] to libexec.glob("bin/*")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sushichan044
Copy link
Contributor Author

sushichan044 commented Dec 24, 2025

While inspecting several npm module formulae, I noticed cases where Dir["#{libexec}/bin/*"] was replaced with libexec.glob("bin/*")

Since calling .glob() appears to be the recommended approach now, I will update the documentation to recommend using .glob().

@sushichan044
docs: recommend using libexec.glob for symlinking
6fd5326 
Reflect the pattern commonly used in newer npm module formulae
@sushichan044 sushichan044 force-pushed the 202512242045-libexec-glob branch from ff36750 to 6fd5326 Compare December 24, 2025 11:57
@sushichan044
Copy link
Contributor Author

sushichan044 commented Dec 24, 2025

CI failed due to timeout
https://github.com/Homebrew/brew/actions/runs/20485670211/job/58867570370?pr=21318#step:11:79

ZhongRuoyu
ZhongRuoyu approved these changes Dec 24, 2025
View reviewed changes
Copy link
Member

@ZhongRuoyu ZhongRuoyu left a comment

Choose a reason for hiding this comment

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

Thanks!

@p-linnane
Copy link
Member

p-linnane commented Dec 24, 2025

If we're going this route, we should also update formula_creator:

brew/Library/Homebrew/formula_creator.rb

Lines 244 to 246 in ff14abc

<% elsif @mode == :node %>
system "npm", "install", *std_npm_args
bin.install_symlink Dir["\#{libexec}/bin/*"]

@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Dec 24, 2025

If we're going this route, we should also update formula_creator:

Yup 👍🏻

@sushichan044
fix: update template to use libexec.glob()
e71dfda 
This reflecting documentation changes from Node-for-Formula-Authors.md
@sushichan044 sushichan044 force-pushed the 202512242045-libexec-glob branch from 098ac73 to e71dfda Compare December 24, 2025 23:10
@sushichan044
Copy link
Contributor Author

sushichan044 commented Dec 24, 2025

@p-linnane
Thank you for the review. I missed this because I was only doing a simple search.

It has been fixed in e71dfda.

p-linnane
p-linnane approved these changes Dec 25, 2025
View reviewed changes
Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

Thanks!

@p-linnane p-linnane added this pull request to the merge queue Dec 25, 2025
Merged via the queue into Homebrew:main with commit ca1fdac Dec 25, 2025
36 checks passed
@p-linnane p-linnane mentioned this pull request Dec 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@ZhongRuoyu ZhongRuoyu ZhongRuoyu approved these changes

@bevanjkay bevanjkay bevanjkay approved these changes

@p-linnane p-linnane p-linnane approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sushichan044 @p-linnane @MikeMcQuaid @ZhongRuoyu @bevanjkay