Make bottle relocation more specific

[MikeMcQuaid]

Provide a detailed description of the proposed feature

Don't rewrite paths such as e.g. $foo/build/usr/local/bin. For example, see objc-run: https://github.com/iljaiwas/objc-run/blob/55f257300154201a40f3631b06d8131dfe47a5a2/objc-run#L110

See discussion in Homebrew/homebrew-core#75943 (comment)

This would require changing at least replace_text_in_files (

brew/Library/Homebrew/keg_relocate.rb

Line 69 in eae3558

def replace_text_in_files(relocation, files: nil)

) and each_unique_file_matching (

brew/Library/Homebrew/keg_relocate.rb

Line 112 in eae3558

def each_unique_file_matching(string)

) in keg_relocate.rb.

What is the motivation for the feature?

Improve the number of bottles that are relocated and all:.

How will the feature be relevant to at least 90% of Homebrew users?

Reduce the need to redownload bottles.

What alternatives to the feature have been considered?

The status quo.

[Rylan12]

Is this just to protect against cases where one of the relocation path regexes matches in the middle of a path or are there more cases that need fixing?

If so, can we just add \b to the beginning of the old_* relocation strings in replace_locations_with_placeholders

brew/Library/Homebrew/keg_relocate.rb

Lines 39 to 52 in eae3558

	def replace_locations_with_placeholders
	relocation = Relocation.new(
	old_prefix: HOMEBREW_PREFIX.to_s,
	old_cellar: HOMEBREW_CELLAR.to_s,
	new_prefix: PREFIX_PLACEHOLDER,
	new_cellar: CELLAR_PLACEHOLDER,
	old_repository: HOMEBREW_REPOSITORY.to_s,
	new_repository: REPOSITORY_PLACEHOLDER,
	old_library: HOMEBREW_LIBRARY.to_s,
	new_library: LIBRARY_PLACEHOLDER,
	)
	relocate_dynamic_linkage(relocation)
	replace_text_in_files(relocation)
	end

[MikeMcQuaid]

Is this just to protect against cases where one of the relocation path regexes matches in the middle of a path

Yes, I think so for now.

or are there more cases that need fixing?

Probably but: we don't know what they are yet 😁

If so, can we just add \b to the beginning of the old_* relocation strings in replace_locations_with_placeholders

Something like that makes sense to me. I'm not sure I fully understand what \b does and whether it would handle /, ", or the beginning of line like we'd expect?

[Rylan12]

I'm not sure I fully understand what \b does and whether it would handle /, ", or the beginning of line like we'd expect?

Good point. it don't think it will match " but I'm pretty sure it does match / so probably best to go with your suggestion from Homebrew/homebrew-core#76394 (comment):

/[^a-z]?#{HOMEBREW_PREFIX}/

[MikeMcQuaid]

It's worth noting e.g. https://github.com/Homebrew/homebrew-core/blob/a32f8fd08629a24537a01af1e57556007a5fbc29/Formula/fontconfig.rb#L23-L28

(and similar cases in other formulae)

Arguably any/all of these are a sign that the bottle relocation logic needs fixed.

[Rylan12]

Now that #11332 has been merged, I've opened Homebrew/homebrew-core#77187 and Homebrew/homebrew-core#77188 to remove the pour_bottle? blocks from objc-run and pkg-config.

There are still 20 other formulae in homebrew/core that have pour_bottle? blocks but these fall into four categories:

1. ocaml: I'm currently investigating why this is needs the pour_bottle? block and will hopefully soon know whether it's solved by keg_relocate: only replace matches at the start of a path #11332 as well or not (looks like it isn't)
2. percona-server needs datadir == var/"mysql"
3. gcc and llvm formulae require the Xcode CLT to be installed on macOS:

   pour_bottle? do
     on_macos do
       reason "The bottle needs the Xcode CLT to be installed."
       satisfy { MacOS::CLT.installed? }
     end
   end

4. python formulae require the Apple Command Line Tools to be installed on macOS:

   pour_bottle? do
     on_macos do
       reason <<~EOS
         The bottle needs the Apple Command Line Tools to be installed.
           You can install them, if desired, with:
             xcode-select --install
       EOS
       satisfy { MacOS::CLT.installed? }
     end
   end

5. libgccjit and swift-format both also need the CLT but its not restricted to just on macOS

First of all, is there really a difference between 3 and 4? I think 4 gives better messaging so it should probably be used consistently (see my suggestion below).

Second, for number 5, should these be enclosed in on_macos blocks? You can't install the CLT on Linux (at least I assume) so I wonder if they work on Linux or not.

I don't think we can really avoid the CLT requirement but I wonder if we should add a shortcut to require the CLT because it seems that the same pour_bottle? block is used in a bunch of places.

Maybe one of these could be a shortcut (obviously allowing existing pour_bottle? syntax to continue:

pour_bottle? :require_clt
pour_bottle? :if_clt_installed
pour_bottle? if: :clt_installed

[carlocab]

I'd been contemplating making MacOS::CLT.installed? return true on anything that's not macOS, but I suspect your suggestion of a pour_bottle? DSL might be better.

[MikeMcQuaid]

Now that #11332 has been merged, I've opened Homebrew/homebrew-core#77187 and Homebrew/homebrew-core#77188 to remove the pour_bottle? blocks from objc-run and pkg-config.

🎉

• ocaml: I'm currently investigating why this is needs the pour_bottle? block and will hopefully soon know whether it's solved by keg_relocate: only replace matches at the start of a path #11332 as well or not (looks like it isn't)

😭 but thanks for checking

First of all, is there really a difference between 3 and 4? I think 4 gives better messaging so it should probably be used consistently (see my suggestion below).

Agreed!

I don't think we can really avoid the CLT requirement but I wonder if we should add a shortcut to require the CLT because it seems that the same pour_bottle? block is used in a bunch of places.

Maybe one of these could be a shortcut (obviously allowing existing pour_bottle? syntax to continue:

I like this! Maybe pour_bottle? :clt_required or pour_bottle? if: :clt_installed?

[MikeMcQuaid]

I'd been contemplating making MacOS::CLT.installed? return true on anything that's not macOS, but I suspect your suggestion of a pour_bottle? DSL might be better.

I'd like (eventually) to get to a point where MacOS and MacOS::CLT classes aren't even defined on Linux (as they aren't in the "generic OS" layer).

[Rylan12]

Okay, here's an update on ocaml. The pour_bottle? block was added in 2016 in Homebrew/homebrew-core#586 (specifically Homebrew/homebrew-core@a995aa2).

Looking at the contents of the bin directory, it seems that there are several references to /usr/local/lib. It looks to me like we intentionally don't search for references to /usr/local/lib because it's not a Homebrew-only directory (is that correct?).

There are also references to /usr/local/bin, many (but not all) through shebangs: #!/usr/local/bin/ocamlrun. We also don't check for /usr/local/bin (for what I presume is the same reason). I do know, though, that we have some handling of shebangs (I know that @Bo98 did some work on them in #11286). I don't know much about how what we do with shebangs in terms of relocation. Is this soemthing that should be looked into (i.e. a shebang is found pointing to HOMEBREW_PREFIX/bin/foo where foo belongs to a formula)?

[Rylan12]

I like this! Maybe pour_bottle? :clt_required or pour_bottle? if: :clt_installed?

I like these. No real preference from me; I think they're both very clear. I'll spend some time soon working on this

[MikeMcQuaid]

Looking at the contents of the bin directory, it seems that there are several references to /usr/local/lib. It looks to me like we intentionally don't search for references to /usr/local/lib because it's not a Homebrew-only directory (is that correct?).
We also don't check for /usr/local/bin (for what I presume is the same reason).

Correct!

There are also references to /usr/local/bin, many (but not all) through shebangs: #!/usr/local/bin/ocamlrun.

We should probably rewrite these so it works when unlinked.

Is this soemthing that should be looked into (i.e. a shebang is found pointing to HOMEBREW_PREFIX/bin/foo where foo belongs to a formula)?

Yes, that would make sense to do I think 👍🏻. Should be relatively easy to look at bottle time whether it's a symlink into a cellar and, if so, replace it with a opt path.

[MikeMcQuaid]

Closing this as considering it mostly done.