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

node: add shebang rewriting #15861

Merged
merged 5 commits into from Aug 15, 2023

Conversation

samford
Copy link
Member

@samford samford commented Aug 12, 2023

  • 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?

Formulae that depend on node sometimes contain files that use a shebang like #!/usr/bin/env node and this can lead to issues when the node in a user's environment isn't brewed node.

For example, some node modules are compiled when the formula is built but if the user's node is a different major version than brew's node, the differing NODE_MODULE_VERSION can produce an error when certain parts of the application are used. The formula may build and test fine and the issue may only become apparent when more of the application is exercised (e.g., Homebrew/homebrew-core#139389).

This adds a Language::Node::Shebang module, which allows us to use rewrite_shebang detected_node_shebang, ... in formulae to address this type of issue. I borrowed from the existing Perl and Python implementations but omitted logic that doesn't seem to apply to node (though do let me know if I've overlooked anything).

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Might be good to mention it here in the docs as well.

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.

Looks great, nice work @samford! One nit.

Library/Homebrew/language/node.rb Outdated Show resolved Hide resolved
@samford
Copy link
Member Author

samford commented Aug 15, 2023

Might be good to mention it here in the docs as well.

Thanks for the heads up. The docs use non-exhaustive wording (i.e., "such as Python or Perl"), so that's technically still correct. I can update the text if we think it will be beneficial (e.g., "such as Python, Perl or Node") but I'm not sure if it would be helpful or just more verbose.


I've pushed a commit that incorporates the above feedback and extracts the length of the longest matching shebang string into a constant. I also created a constant for the regex, so it's not recreated every time #node_shebang_rewrite_info is called.

While I was at it, I reworked the existing Shebang modules for Perl and Python to use the previously-mentioned setup (adding constants and also type signatures) and to better align with each other (e.g., adding a perl_shebang_rewrite_info method). Lastly, I expanded the tests for the Shebang modules to hit 100% coverage. That said, if we would prefer to handle the Perl/Python changes in a separate PR, I can pare this down to the Node changes.

Library/Homebrew/language/python.rb Outdated Show resolved Hide resolved
@apainintheneck
Copy link
Contributor

How worried should we be about name clashes from these mixins? They now all have constants with the same names so if you included multiple of them in your formula (something that is technically possible but not present at the moment in core) at least one of them would get overwritten, right?

I also don't think we should really be that worried about the regexes being redefined each time since it doesn't seem to be a hot codepath and should really only be needed once for a handful of formula (67 by my count) during bottling. I guess what I'm saying is that I'm partial to how it was before.

@apainintheneck apainintheneck self-requested a review August 15, 2023 04:41
Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

This needs to be changed so that the mixins can't overwrite each others constants. Probably the easiest things to do would be either prefix the constant with the language like PYTHON_SHEBANG_REGEX or use inline regexes like before.

Formulae that depend on `node` sometimes contain files that use a
shebang like `#!/usr/bin/env node` and this can lead to issues when
the `node` in a user's environment isn't brewed `node`.

For example, some node modules are compiled when the formula is built
but if the user's `node` is a different major version than brew's
`node`, the differing `NODE_MODULE_VERSION` can produce an error when
certain parts of the application are used. The formula may build and
test fine and the issue may only become apparent when more of the
application is exercised.

This adds a `Language::Node::Shebang` module (borrowing from the
existing Perl and Python examples), which allows us to use
`rewrite_shebang detected_node_shebang, ...` in formulae to address
this type of issue.
This primarily reworks `Language::Perl::Shebang` to use constants for
the shebang regex and max length (like the previous Node commit) and
to extract the `RewriteInfo` call into a separate method (like Python
and Node).

Besides that, this also adds type signatures to the methods.
This reworks `Language::Python::Shebang` to use constants for
the shebang regex and max length (like the previous Node commit).
Besides that, this also adds type signatures to the existing methods.
This brings coverage of `Language::Perl::Shebang` to 100%.
This brings coverage of `Language::Python::Shebang` to 100%.
Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

Thanks for updating it!

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 PR is AWESOME 😍. Really love how you've not only added new functionality here @samford but also cleaned up a bunch of related additional code.

@MikeMcQuaid MikeMcQuaid merged commit 8c460bd into Homebrew:master Aug 15, 2023
24 checks passed
@samford samford deleted the node-add-shebang-rewriting branch August 15, 2023 13:06
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 15, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants