Allow HOMEBREW_PREFIX replacement in external patches

[pkryger]

This updates how external patches are applied, making them equivalent with embedded patches. That is, with this the "@@HOMEBREW_PREFIX@@" string in external patches will be updated to a value of HOMEBREW_PREFIX variable.

I found an issue opened by @gaborbernat (#15925), and realised I have encountered the same problem before (IIRC, back then I ended up using embedded patch). But I think this is still a good feature to have.

@MikeMcQuaid has closed the issue, but indicated he was willing to accept a PR.

Technical detail: I followed what has been done for embedded patches, but avoided slurping the whole file into memory. Opted for falling back to line-by-line IO. This changes semantic slightly as now an Errno::ENOENT is thrown from the body when patch file is missing. I was not sure if it should be caught and translated to BuildError. For the first cut I opted for test update. Note, there are tests for BuildError.

Please let me know if you have any comments.

This fixes #15925

---

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

---

[Bo98]

I wonder if this will break existing patches: https://github.com/search?q=repo%3AHomebrew%2Fformula-patches+HOMEBREW_PREFIX&type=code.

[MikeMcQuaid]

Thanks so much @pkryger, this is a really great PR!

Would love some thoughts from other @Homebrew/maintainers and @homebrew/core on this as it'll affect them too.

[pkryger]

I wonder if this will break existing patches: https://github.com/search?q=repo%3AHomebrew%2Fformula-patches+HOMEBREW_PREFIX&type=code.

Good point. I guess it will. Seems like these formulae needed that functionality after all (at least glib did). I wonder if it worth to proceed, and say use @@HOMEBREW_PREFIX@@ as a marker. That way these places won't be affected (need to check them first though).

[MikeMcQuaid]

I wonder if this will break existing patches: github.com/search?q=repo%3AHomebrew%2Fformula-patches+HOMEBREW_PREFIX&type=code.

Yes, seems that way. Also seems like @@HOMEBREW_PREFIX@@ is already replaced which makes me wonder if this PR is needed at all? Is this a missing documentation issue rather than missing functionality?

[pkryger]

Yes, seems that way. Also seems like @@HOMEBREW_PREFIX@@ is already replaced which makes me wonder if this PR is needed at all? Is this a missing documentation issue rather than missing functionality?

It can be only a documentation issue indeed. I updated the PR to use existing markers (i.e., with double @). That way it's backward compatible with affected formulae. But happy to revert to updating documentation only.

[cho-m]

Also seems like @@HOMEBREW_PREFIX@@ is already replaced which makes me wonder if this PR is needed at all? Is this a missing documentation issue rather than missing functionality?

It's not replaced as we follow up with inreplace, e.g. https://github.com/Homebrew/homebrew-core/blob/c5838ab1072db0ddacd2bf4f4c1626027d57ce21/Formula/p/python%403.13.rb#L181-L182

---

That way these places won't be affected (need to check them first though).

They (i.e. the corresponding formulae) will be affected anyways as inreplace will fail by default if there are no replacements. If change is done, will need to set audit_result: false until available in stable brew tag and then drop the inreplace.

[pkryger]

Given the issues above I can see a few paths forward, each of them with slightly different outcome. I am happy to pursue any of them, but I lack of experience in brew development to decide which of them can be deemed the best one for the project. I can see at least a few options. Please let me know if you think there are more options that are worth discussing and pursuing. Likewise, if you like one of the proposals, please let me know as well.

Option 1: Use HOMEBREW_PREFIX in external patches

This could be achieved by synchronising the change with:

• update to Homebrew/homebrew-patches to use the HOMEBREW_PREFIX,
• and update to Homebrew/homebrew-core to remove inreplace calls.

I think I'd like the end result the most. It brings homogeneous behaviour between embedded and external patches, making them that tiny bit easier to use. Although it seems like the most invasive change, as it will affect any potential tap out there in the wild. I skimmed through usage of Homebrew/homebrew-core-like calls on GitHub, and it seems it's mostly in some version of clone of existing formulae. But it's only one place where taps can be stored. Not sure neither how to communicate such a change neither if communication only will be enough.

Option 2: Use @@HOMEBREW_PREFIX@@ in external patches

It would make the change slightly easier (especially with the audit_result: false tip @cho-m brought up), as there won't be a need to synchronise PRs. Reversion of the audit_result: false change could be postponed after relevant inreplace calls are removed from Homebrew/homebrew-core. Or, perhaps a bit longer to give maintainers of existing taps time to adjust.

Alternatively, the change could be done with PR synchronisation like the option 1. In this variant the audit_result: false could still be introduced to help with external taps migration.

The end result will won't be as neat, as it would require two different strings to be used. One for external and anther one for internal patch. It will make patch management a bit more difficult, i.e., moving patch between internal and external would require a bit more work. Yet it is less risky, as it seems to have a smaller potential of breaking users workflows.

Option 3: Use a new, distinct tag in external patches, i.e., {{HOMEBREW_PREFIX}}

It would make change even easier, as there would be no need for any synchronisation neither with Homebrew/homebrew-core nor with Homebrew/homebrew-patches. These could be updated to use the new mechanism after the change is merged.

The end result will be similar to the option 2, however the change will be even less risky, as it seems that the {{HOMEBREW_PREFIX}} is not used on GitHub. Other taps are not public on GitHub are still subjected to breakage, but - I guess - that usage patterns will be similar to public GitHub. What bothers me slightly is proliferation of these tags. While, I guess the @@HOMEBREW_PREFIX@@ would be less frequent with time (especially if we decide to update Homebrew/homebrew-core and Homebrew/homebrew-patches , it's still not certain if it will eventually become extinct.

Option 4. Encourage using inreplace method.

This would only require documentation change: just provide an example, how the value of HOMEBREW_PREFIX can be used in external patches. The example could either use@@HOMEBREW_PREFIX@@ like Homebrew/homebrew-core does, or use the same as for internal patches (i.e., just HOMEBREW_PREFIX). An extra tip can be added suggesting user that they are free to use any string of their choice to preserve patch integrity.

The end result will preserve existing functionality, yet it will require a bit of more work from users. This, however, seems like the least risky option, as there will be no functional change in code.

[MikeMcQuaid]

Thanks for your patience and comprehensive spelling out of the options here @pkryger, I'm grateful and impressed ❤️

Option 2: Use @@HOMEBREW_PREFIX@@ in external patches

This is my preferred option, I think it's nicer to be consistent here, especially when, if it breaks anything, it will be both undocumented behaviour and seems more likely than not to achieve the desired result anyway.

If change is done, will need to set audit_result: false until available in stable brew tag and then drop the inreplace.

This makes sense to me @cho-m and seems like the best balance.

@pkryger hopefully this lets you move forward! Let me know if not and if you need any help. Thanks again!

[pkryger]

@MikeMcQuaid Thanks for following up! I have submitted the last commit to disable the audit_result in inreplace f31e93a), which I think should be the last thing to do for this bit of work. Let me know if you need anything else before merging it.

Have very little experience with Homebrew development, and even less so with its release. Could you please advise me how can I know that the change made it to a stable brew tag, so I can submit a follow up PR to remove inreplace calls in in the Homebrew/hombrew-cask?

[MikeMcQuaid]

Thanks @pkryger, great work here!