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

docs: mention shebang rewrite in the formula cookbook #11244

Merged
merged 7 commits into from May 10, 2021

Conversation

Moisan
Copy link
Member

@Moisan Moisan commented Apr 24, 2021

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

I was recently working on a formula involving the use of rewrite_shebang and I noticed there was no mention of it in the documentation. This is a very simple paragraph about it, I am open to go into more detail if needed.

@BrewTestBot
Copy link
Member

Review period will end on 2021-04-27 at 00:00:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Apr 24, 2021
docs/Formula-Cookbook.md Outdated Show resolved Hide resolved
Co-authored-by: Rylan Polster <rslpolster@gmail.com>
Rylan12
Rylan12 previously approved these changes Apr 25, 2021
Copy link
Member

@Rylan12 Rylan12 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. Thanks!

@@ -750,6 +750,10 @@ ln_s libexec/"name", bin

The symlinks created by [`install_symlink`](https://rubydoc.brew.sh/Pathname#install_symlink-instance_method) are guaranteed to be relative. `ln_s` will only produce a relative symlink when given a relative path.

### Rewriting a script shebang

In some cases, the formula goal is to install a script written in an interpreted language, such as Python or Perl, to be used as an executable. Homebrew provides a `rewrite_shebang` method to rewrite the shebang of a script to point to the correct interpreter path. For example, the [`icdiff` formula](https://github.com/Homebrew/homebrew-core/blob/7beae5ab57c65249403699b2b0700fbccf14e6cb/Formula/icdiff.rb#L16) uses such utility.
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth adding a mention of what it's rewritten from/to/why?

Copy link
Member Author

Choose a reason for hiding this comment

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

I committed some changes to cover this.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Would be useful to mention that you probably need to include Language::{Python,Perl}::Shebang or similar to use this.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Apr 27, 2021
@BrewTestBot
Copy link
Member

Review period ended.

BrewTestBot
BrewTestBot previously approved these changes Apr 27, 2021
@Moisan
Copy link
Member Author

Moisan commented Apr 27, 2021

@carlocab : I was debating between describing the different parts needed to use the utility or if pointing to an example was enough.

@carlocab
Copy link
Member

I think pointing to the example is mostly enough, just that I suspect many readers will miss the include Language::Python::Shebang part in the example.

BrewTestBot
BrewTestBot previously approved these changes Apr 27, 2021
@Moisan
Copy link
Member Author

Moisan commented Apr 27, 2021

👍 I added a sentence to cover this.

docs/Formula-Cookbook.md Outdated Show resolved Hide resolved
Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
BrewTestBot
BrewTestBot previously approved these changes Apr 27, 2021
carlocab
carlocab previously approved these changes Apr 27, 2021
Rylan12
Rylan12 previously approved these changes Apr 27, 2021
@@ -750,6 +750,12 @@ ln_s libexec/"name", bin

The symlinks created by [`install_symlink`](https://rubydoc.brew.sh/Pathname#install_symlink-instance_method) are guaranteed to be relative. `ln_s` will only produce a relative symlink when given a relative path.

### Rewriting a script shebang

Some formulae install executable scripts written in an interpreted language such as Python or Perl. Homebrew provides a `rewrite_shebang` method to rewrite the shebang of a script. This replaces a script's original interpreter path with the one installed by Homebrew and guarantees that the correct interpreter is used at execution time.
Copy link
Member

Choose a reason for hiding this comment

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

This replaces a script's original interpreter path with the one installed by Homebrew

I'm not sure this is correct? It depends on the relevant OS, I think? CC @Bo98

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes. It's not entirely accurate. I think it depends on the dependencies a formula declares.

Copy link
Member

Choose a reason for hiding this comment

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

For Python it's right since we don't allow system Python. For Perl, it is indeed not correct as if uses_from_macos "perl" is used then it will use system Perl as the shebang on macOS.

It's perhaps also worth clarifying that rewrite_shebang is only needed where build system doesn't already do it (it's not needed under many pip and ExtUtils::MakeMaker cases).

@Moisan Moisan dismissed stale reviews from Rylan12, carlocab, and BrewTestBot via 665b864 April 27, 2021 15:39
BrewTestBot
BrewTestBot previously approved these changes Apr 27, 2021
docs/Formula-Cookbook.md Outdated Show resolved Hide resolved
Co-authored-by: Rylan Polster <rslpolster@gmail.com>
@MikeMcQuaid MikeMcQuaid merged commit c6ac648 into Homebrew:master May 10, 2021
@MikeMcQuaid
Copy link
Member

Thanks @Moisan!

@Moisan Moisan deleted the docs_shebang branch May 12, 2021 13:10
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2021
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

6 participants