bottle.rb: add workaround to make gcc have cellar :any by danielnachun · Pull Request #11899 · Homebrew/brew

danielnachun · 2021-08-21T23:45:19Z

…LINKS

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?

This is a followup to https://github.com/Homebrew/linuxbrew-core/pull/23361. I followed the suggestion from @iMichka to add a regex to exclude the nonrelocatable strings inside the GCC formulae. For many years :any was manually added to the GCC cellar block on Linux so we know it works. This should guarantee this happens from now on.

BrewTestBot · 2021-08-21T23:45:32Z

Review period will end on 2021-08-24 at 00:00:00 UTC.

Bo98 · 2021-08-22T01:10:07Z

ALLOWABLE_HOMEBREW_REPOSITORY_LINKS is purely about exceptions to the HOMEBREW_REPOSITORY rule, which hard blocks rather than simply mark as non-relocatable.

danielnachun · 2021-08-22T01:13:46Z

Okay, what's the best place to add this exception? Perhaps in the ignores regex:

ignores = [%r{/include/|\.(c|cc|cpp|h|hpp)$}]

I was able to change that to

ignores = [%r{/include/|\.(c|cc|cpp|h|hpp)$|#{cellar}/gcc}]

and it also made the gcc bottles relocatable.

Bo98 · 2021-08-22T01:23:58Z

Something like that, yes.

Can you clarify here what the references are? We don't have the issue on macOS so I'm interested what might be different.

Bo98 · 2021-08-22T01:27:17Z

Oh sorry, we do have the issue on macOS. Someone must have manually changed the macOS line in the PR you linked.

danielnachun · 2021-08-22T01:27:44Z

Yeah, let me post the strings below. I'm happy to explore other solutions even outside of bottle.rb - this was just a starting point.

Warning: String '/home/linuxbrew/.linuxbrew/Cellar' still exists in these files:
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/gcc-ranlib-5
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/' at offset 0x12160
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/' at offset 0x121a0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0x121e0
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/gcc-nm-5
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/' at offset 0x12160
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/' at offset 0x121a0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0x121e0
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/x86_64-unknown-linux-gnu-c++-5
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0x7aef8
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/' at offset 0x7af38
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/' at offset 0x7af78
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7' at offset 0x82f90
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/gcc-ar-5
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/' at offset 0x12160
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/' at offset 0x121a0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0x121e0
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/x86_64-unknown-linux-gnu-gcc-5.5.0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0x79ef8
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/' at offset 0x79f38
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/' at offset 0x79f78
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7' at offset 0x81f50
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/gfortran-5
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0x7aef8
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/' at offset 0x7af38
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/' at offset 0x7af78
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7' at offset 0x83110
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/cpp-5
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0x7aef8
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/' at offset 0x7af38
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/bin/' at offset 0x7af78
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7' at offset 0x82ff0
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/x86_64-unknown-linux-gnu/5.5.0/f951
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0x112c380
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/' at offset 0x112c3e0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include' at offset 0x112c420
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include' at offset 0x112c778
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/include' at offset 0x112c7e0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include-fixed' at offset 0x112c818
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7' at offset 0x1140300
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/x86_64-unknown-linux-gnu/5.5.0/cc1plus
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0xde1540
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/' at offset 0xde15a0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include' at offset 0xde15e0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include' at offset 0xde1938
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/include' at offset 0xde19a0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include-fixed' at offset 0xde19d8
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7' at offset 0x11988e0
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/x86_64-unknown-linux-gnu/5.5.0/cc1obj
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0xd041a0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/' at offset 0xd04200
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include' at offset 0xd04240
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include' at offset 0xd04598
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/include' at offset 0xd04600
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include-fixed' at offset 0xd04638
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7' at offset 0x10bb6c0
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/x86_64-unknown-linux-gnu/5.5.0/cc1
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0xcd8b40
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/' at offset 0xcd8ba0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include' at offset 0xcd8be0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include' at offset 0xcd8f38
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/include' at offset 0xcd8fa0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include-fixed' at offset 0xcd8fd8
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7' at offset 0x1090080
/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/libexec/gcc/x86_64-unknown-linux-gnu/5.5.0/cc1objplus
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/' at offset 0xe0da20
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/' at offset 0xe0da80
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include' at offset 0xe0dac0
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include' at offset 0xe0de18
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/include' at offset 0xe0de80
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include-fixed' at offset 0xe0deb8
 --> match '/home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7' at offset 0x11c4da0

danielnachun · 2021-08-22T01:29:00Z

Oh sorry, we do have the issue on macOS. Someone must have manually changed the macOS line in the PR you linked.

Interesting. Anyway, GCC is designed to be portable so these strings should be ignored regardless of OS.

danielnachun · 2021-08-23T03:30:05Z

I pushed a change to add something to the ignore regex that I know will work for GCC. Hopefully this is a better solution.

iMichka · 2021-08-23T09:48:09Z

GCC is designed to be portable

Everyone says so, but is this officially documented upstream?

Looking at the long list of /home/linuxbrew/.linuxbrew/Cellar/ paths in the binaries, somethings tells me that stuff might still break in unexpected fashion (and hard to debug maybe) in non-default prefixes?

Bo98 · 2021-08-23T12:30:50Z

The documentation of GCC_EXEC_PREFIX somewhat describes a behaviour where the full include paths (e.g. /home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7/lib/gcc/5/gcc/x86_64-unknown-linux-gnu/5.5.0/include) have the "configured prefix" (which should be /home/linuxbrew/.linuxbrew/Cellar/gcc@5/5.5.0_7) swapped with the value of that env (the env itself is determined automatically if not specified).

That doesn't cover everything but that's about as far as I know regarding the matter.

I pushed a change to add something to the ignore regex that I know will work for GCC

Maybe we should actually limit this to the GCC formulae so that a potential dependent that includes GCC prefixes is still flagged.

MikeMcQuaid

This is far too broad for me for now. I'd like to see this scoped to specific i.e. gcc formulae, comments added and, ideally, we long-term fix this in a way that makes sense for other formulae.

BrewTestBot · 2021-08-24T00:04:35Z

Review period ended.

danielnachun · 2021-08-24T01:14:37Z

Thanks for the additional feedback. A few more thoughts:

I think @Bo98 is right that GCC figures out the right prefix automatically at run time. This thread on the GCC mailing list also says clearly that a GCC installation can be moved so long as the whole directory tree is moved together (which we are doing here): https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed. I'd also like to hear from @sjackman on this when he has a chance to reply, as he has a lot of knowledge on GCC.
I would definitely like to restrict this to GCC formulae. If anyone has suggestions on the best way to do this I'm happy to change it accordingly. I would rather avoid even messing with ignores and just having a hard override for GCC on Linux here. GCC is special on Linux because pretty much everything depends on it, so making it specific is reasonable.
I think the ultimate long term solution for this is Use binary patching to improve relocatability of bottles #10846 and I'm almost certain that would work for GCC as it's already used by Anaconda, but I want to wait for the core merge to finish before putting serious work into that. For now I'm just hoping we can find a safe, simple workaround because GCC is a critical dependency and a large percentage of the Linux user base has to use non-default prefixes. We get a lot of support issues right now from users who are needlessly strugglingly to build GCC from source when it can be used as a bottle.

MikeMcQuaid · 2021-08-24T12:49:28Z

I think @Bo98 is right that GCC figures out the right prefix automatically at run time. This thread on the GCC mailing list also says clearly that a GCC installation can be moved so long as the whole directory tree is moved together (which we are doing here): https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed. I'd also like to hear from @sjackman on this when he has a chance to reply, as he has a lot of knowledge on GCC.

Cool, this is exactly what I'd like to see as a comment in the source.

I would definitely like to restrict this to GCC formulae. If anyone has suggestions on the best way to do this I'm happy to change it accordingly. I would rather avoid even messing with ignores and just having a hard override for GCC on Linux here. GCC is special on Linux because pretty much everything depends on it, so making it specific is reasonable.

I'd still like to see some sort of "ignores" just in case we accidentally add a hardcoded path through our formula changes and don't notice.

3. I think the ultimate long term solution for this is Use binary patching to improve relocatability of bottles #10846 and I'm almost certain that would work for GCC as it's already used by Anaconda, but I want to wait for the core merge to finish before putting serious work into that. For now I'm just hoping we can find a safe, simple workaround because GCC is a critical dependency and a large percentage of the Linux user base has to use non-default prefixes. We get a lot of support issues right now from users who are needlessly strugglingly to build GCC from source when it can be used as a bottle.

I agree that could be a good long-term solution. I'm more optimistic it will be for these sorts of cases where we know they are already working on Anaconda than globally.

MikeMcQuaid · 2021-08-24T12:53:20Z

Library/Homebrew/dev-cmd/bottle.rb

        # Ignore matches to source code, which is not required at run time.
        # These matches may be caused by debugging symbols.
-        ignores = [%r{/include/|\.(c|cc|cpp|h|hpp)$}]
+        ignores = [%r{/include/|\.(c|cc|cpp|h|hpp)$|#{cellar}/gcc}]


Suggested change

ignores = [%r{/include/|\.(c|cc|cpp|h|hpp)$|#{cellar}/gcc}]

ignores = [%r{/include/|\.(c|cc|cpp|h|hpp)$}]

# GCC installation can be moved so long as the whole directory tree is moved together (which we are doing here):

# https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.

if Formula["gcc"].versioned_formulae.map(&:name).include?(name)

ignores << %r{#{Regexp.escape(cellar)}/gcc/}

end

Thanks, this is super helpful! One quick question, will Formula["gcc"].versioned_formulae.map also include the current version of GCC (GCC 11)? Because we'd want to include that as well.

I made this change but I hit a weird problem when testing with gcc@5 - it's not returned by Formula["gcc"].versioned_formulae.map! This is the list of formulae that are loaded by that map:

/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/gcc.rb /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/gcc@8.rb /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/gcc@6.rb /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/gcc@9.rb /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/gcc@4.9.rb /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/gcc@10.rb /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/gcc@7.rb

gcc@5 might be excluded from that list because of its special status as the CI version of gcc.

Separately from that, I had to add a begin/rescue block to catch FormulaUnavailableError, but now I'm getting a brew style failure I don't really understand:
Metrics/AbcSize: Assignment Branch Condition size for bottle_formula is too high. [<83, 255, 81> 280.1/280]

I made this change but I hit a weird problem when testing with gcc@5 - it's not returned by Formula["gcc"].versioned_formulae.map! This is the list of formulae that are loaded by that map:

I think it'd be worth trying to fix that. Sorry for the yak shave.

Separately from that, I had to add a begin/rescue block to catch FormulaUnavailableError, but now I'm getting a brew style failure I don't really understand:

Basically that's saying "move some logic into a new method".

I think it'd be worth trying to fix that. Sorry for the yak shave.

Definitely agreed that we should fix that. It's not clear to me how gcc@5 is even being excluded from that list in the first place, as Formula.versioned_formulae just seems to find the formulae based on filenames. I dig around to found out how this happening.

Basically that's saying "move some logic into a new method".

Okay I will make a new method that returns an additional array of regexes to be appended to ignores.

I made some progress in figuring out why gcc@5 was missing from the list of versioned formulae produced by Formula.versioned_formulae. It is not specific to gcc@5. When I tried to use brew bottle with gcc@9, for example, it skipped gcc@9 but did load the other versions, including gcc@5. I am puzzled as to how the version of gcc I'm bottling with brew bottle could have any influence on the output of versioned_formulae, given that we are calling the method for Formula["gcc"], and not the specific version of gcc being bottled.

I am wondering if this is somehow related to next if versioned_path == path in

def versioned_formulae Pathname.glob(path.to_s.gsub(/(@[\d.]+)?\.rb$/, "@*.rb")).map do |versioned_path| next if versioned_path == path Formula[versioned_path.basename(".rb").to_s] rescue FormulaUnavailableError nil end.compact.sort_by(&:version).reverse end

but when I comment that line out, the result is the same.

@danielnachun Thanks for digging in. Think it's fine to just hack around this for now.

sjackman · 2021-08-26T23:14:09Z

I think @Bo98 is right that GCC figures out the right prefix automatically at run time. This thread on the GCC mailing list also says clearly that a GCC installation can be moved so long as the whole directory tree is moved together (which we are doing here): https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed. I'd also like to hear from @sjackman on this when he has a chance to reply, as he has a lot of knowledge on GCC.

Your understanding is correct. Also true for binutils, which is currently flagged as non-relocatable, but is in fact similarly relocatable. Having formulae in the compiler toolchain flagged as relocatable is particularly helpful to the user experience of bootstrapping a new Homebrew on Linux installation.

danielnachun · 2021-08-27T00:09:24Z

I think @Bo98 is right that GCC figures out the right prefix automatically at run time. This thread on the GCC mailing list also says clearly that a GCC installation can be moved so long as the whole directory tree is moved together (which we are doing here): https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed. I'd also like to hear from @sjackman on this when he has a chance to reply, as he has a lot of knowledge on GCC.

Your understanding is correct. Also true for binutils, which is currently flagged as non-relocatable, but is in fact similarly relocatable. Having formulae in the compiler toolchain flagged as relocatable is particularly helpful to the user experience of bootstrapping a new Homebrew on Linux installation.

I'll take a look at the binary strings in binutils and see if I can make another regex for those. binutils falls under the same umbrella of "essential" Linux dependencies and its great to know that it's also actually relocatable!

MikeMcQuaid · 2021-08-30T14:21:46Z

Your understanding is correct. Also true for binutils, which is currently flagged as non-relocatable, but is in fact similarly relocatable. Having formulae in the compiler toolchain flagged as relocatable is particularly helpful to the user experience of bootstrapping a new Homebrew on Linux installation.

I'm fine with us adding more and more of these formula-specific tweaks to bottling logic and then reverting them on user complaints. We may want to move them to a dedicated file i.e. bottle_formula_tweaks.rb or something.

carlocab · 2021-09-02T14:10:58Z

Library/Homebrew/dev-cmd/bottle.rb

        ignores = [%r{/include/|\.(c|cc|cpp|h|hpp)$}]
+
+        # GCC installation can be moved so long as the whole directory tree is moved together:
+        # https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.


Unfortunately this advice is only true on Linux, so this exception needs to be Linux-only.

github-actions · 2021-09-27T20:49:56Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

danielnachun · 2021-10-18T01:25:23Z

I made a few changes here to pass brew style, and to deal with the fact that Formula.versioned_formulae wasn't behaving as expected. Now I've used Version.formula_optionally_versioned_regex to do a regex match to the formula name that should handle versions properly. I had to move a lot of the logic into a new method in order to pass the AbcSize audit, but I tested gcc and binutils and it works like it did before.

MikeMcQuaid · 2021-10-18T15:19:57Z

Library/Homebrew/dev-cmd/bottle.rb

+      ignores = %r{#{Regexp.escape(cellar)}/gcc} if
+        Version.formula_optionally_versioned_regex(:gcc).match(f.name)
+      ignores = %r{#{Regexp.escape(cellar)}/binutils} if
+        Version.formula_optionally_versioned_regex(:binutils).match(f.name)


Don't use postfix if if it can't fit on one line. This can either be an if/elsif or case given that these are exclusive cases.

Library/Homebrew/dev-cmd/bottle.rb

carlocab · 2021-10-18T18:33:26Z

Library/Homebrew/dev-cmd/bottle.rb

+        %r{#{Regexp.escape(cellar)}/binutils}
+      end
+    end
+    ignores ||= []


Wait, do we really want to add an empty array to the ignores array (ignores in bottle_formula, not ignores in add_additional_ignores) when we're not bottling gcc or binutils on Linux?

We probably don't need to add an empty array. Can it just be nil instead?

It can. My recent suggestion, before the edit, returned nils with OS.mac? or when the formula isn't gcc or binutils. You can return an empty array if you make sure this method always returns an array, and you change the << to += below, though.

MikeMcQuaid · 2021-10-18T18:57:11Z

Library/Homebrew/dev-cmd/bottle.rb

+  def add_additional_ignores(f, cellar)
+    # On Linux, GCC installation can be moved so long as the whole directory tree is moved together:
+    # https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.
+    # binutils is relocatable for the same reason: https://github.com/Homebrew/brew/pull/11899#issuecomment-906804451.
+    ignores = if OS.linux?
+      case f.name
+      when Version.formula_optionally_versioned_regex(:gcc)
+        %r{#{Regexp.escape(cellar)}/gcc}
+      when Version.formula_optionally_versioned_regex(:binutils)
+        %r{#{Regexp.escape(cellar)}/binutils}
+      end
+    end
+    ignores ||= []


Suggested change

def add_additional_ignores(f, cellar)

# On Linux, GCC installation can be moved so long as the whole directory tree is moved together:

# https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.

# binutils is relocatable for the same reason: https://github.com/Homebrew/brew/pull/11899#issuecomment-906804451.

ignores = if OS.linux?

case f.name

when Version.formula_optionally_versioned_regex(:gcc)

%r{#{Regexp.escape(cellar)}/gcc}

when Version.formula_optionally_versioned_regex(:binutils)

%r{#{Regexp.escape(cellar)}/binutils}

end

end

ignores ||= []

def formula_ignores(f, cellar)

ignores = if OS.linux?

case f.name

# On Linux, GCC installation can be moved so long as the whole directory tree is moved together:

# https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.

when Version.formula_optionally_versioned_regex(:gcc)

[%r{#{Regexp.escape(cellar)}/gcc/}]

# binutils is relocatable for the same reason: https://github.com/Homebrew/brew/pull/11899#issuecomment-906804451.

when Version.formula_optionally_versioned_regex(:binutils)

[%r{#{Regexp.escape(cellar)}/binutils/}]

else

[]

end

end.freeze

MikeMcQuaid · 2021-10-18T18:58:08Z

Library/Homebrew/dev-cmd/bottle.rb

+        # Add additional workarounds to ignore
+        ignores << add_additional_ignores(f, cellar)


Suggested change

# Add additional workarounds to ignore

ignores << add_additional_ignores(f, cellar)

# Add any formula-specific workarounds

ignores += add_additional_ignores(f, cellar)

MikeMcQuaid · 2021-10-18T18:58:31Z

Library/Homebrew/dev-cmd/bottle.rb

+        # Add additional workarounds to ignore
+        ignores << add_additional_ignores(f, cellar)
+
        any_go_deps = f.deps.any? do |dep|


I think it might be nice to figure out how to put this into the function above, too.

Yes, I was thinking that would make sense. I'll make the change.

carlocab

Sorry for flip-flopping on the comments. I think this should work.

carlocab · 2021-10-18T18:57:58Z

Library/Homebrew/dev-cmd/bottle.rb

+  def add_additional_ignores(f, cellar)
+    # On Linux, GCC installation can be moved so long as the whole directory tree is moved together:
+    # https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.
+    # binutils is relocatable for the same reason: https://github.com/Homebrew/brew/pull/11899#issuecomment-906804451.
+    ignores = if OS.linux?
+      case f.name
+      when Version.formula_optionally_versioned_regex(:gcc)
+        %r{#{Regexp.escape(cellar)}/gcc}
+      when Version.formula_optionally_versioned_regex(:binutils)
+        %r{#{Regexp.escape(cellar)}/binutils}
+      end
+    end
+    ignores ||= []
+    ignores
+  end


Suggested change

def add_additional_ignores(f, cellar)

# On Linux, GCC installation can be moved so long as the whole directory tree is moved together:

# https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.

# binutils is relocatable for the same reason: https://github.com/Homebrew/brew/pull/11899#issuecomment-906804451.

ignores = if OS.linux?

case f.name

when Version.formula_optionally_versioned_regex(:gcc)

%r{#{Regexp.escape(cellar)}/gcc}

when Version.formula_optionally_versioned_regex(:binutils)

%r{#{Regexp.escape(cellar)}/binutils}

end

end

ignores ||= []

ignores

end

# On Linux, GCC installation can be moved so long as the whole directory tree is moved together:

# https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.

# binutils is relocatable for the same reason: https://github.com/Homebrew/brew/pull/11899#issuecomment-906804451.

def gcc_or_binutils_regex_if_applicable(f, cellar)

return [] unless OS.linux?

case f.name

when Version.formula_optionally_versioned_regex(:gcc)

[%r{#{Regexp.escape(cellar)}/gcc}]

when Version.formula_optionally_versioned_regex(:binutils)

[%r{#{Regexp.escape(cellar)}/binutils}]

end

end

carlocab · 2021-10-18T18:58:18Z

Library/Homebrew/dev-cmd/bottle.rb

        ignores = [%r{/include/|\.(c|cc|cpp|h|hpp)$}]
+
+        # Add additional workarounds to ignore
+        ignores << add_additional_ignores(f, cellar)


Suggested change

ignores << add_additional_ignores(f, cellar)

ignores += gcc_or_binutils_regex_if_applicable(f, cellar)

Needs a +=

Yea, returning an Array instead of a NilClass or Regexp is cleaner. Amended my suggestion.

carlocab · 2021-10-18T19:17:50Z

Library/Homebrew/dev-cmd/bottle.rb

+        %r{#{Regexp.escape(cellar)}/gcc}
+      when Version.formula_optionally_versioned_regex(:binutils)
+        %r{#{Regexp.escape(cellar)}/binutils}


These need to be Arrays and not Regexps (wrap them in [,]) for += to work.

Library/Homebrew/dev-cmd/bottle.rb

MikeMcQuaid · 2021-10-18T19:31:35Z

Library/Homebrew/dev-cmd/bottle.rb

+  def add_regex_if_applicable(f, cellar, ignores)
+    # Ignore matches to go keg
+    any_go_deps = f.deps.any? do |dep|
+      dep.name =~ Version.formula_optionally_versioned_regex(:go)
+    end
+    if any_go_deps
+      go_regex =
+        Version.formula_optionally_versioned_regex(:go, full: false)
+      ignores += [%r{#{Regexp.escape(HOMEBREW_CELLAR)}/#{go_regex}/[\d.]+/libexec]}]
+    end
+
+    # On Linux, GCC installation can be moved so long as the whole directory tree is moved together:
+    # https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.
+    # binutils is relocatable for the same reason: https://github.com/Homebrew/brew/pull/11899#issuecomment-906804451.
+    ignores += if OS.linux?
+      case f.name
+      when Version.formula_optionally_versioned_regex(:gcc)
+        [%r{#{Regexp.escape(cellar)}/gcc}]
+      when Version.formula_optionally_versioned_regex(:binutils)
+        [%r{#{Regexp.escape(cellar)}/binutils}]
+      end
+    end
+    ignores
+  end


Suggested change

def add_regex_if_applicable(f, cellar, ignores)

# Ignore matches to go keg

any_go_deps = f.deps.any? do |dep|

dep.name =~ Version.formula_optionally_versioned_regex(:go)

end

if any_go_deps

go_regex =

Version.formula_optionally_versioned_regex(:go, full: false)

ignores += [%r{#{Regexp.escape(HOMEBREW_CELLAR)}/#{go_regex}/[\d.]+/libexec]}]

end

# On Linux, GCC installation can be moved so long as the whole directory tree is moved together:

# https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.

# binutils is relocatable for the same reason: https://github.com/Homebrew/brew/pull/11899#issuecomment-906804451.

ignores += if OS.linux?

case f.name

when Version.formula_optionally_versioned_regex(:gcc)

[%r{#{Regexp.escape(cellar)}/gcc}]

when Version.formula_optionally_versioned_regex(:binutils)

[%r{#{Regexp.escape(cellar)}/binutils}]

end

end

ignores

end

def formula_ignores(f, cellar)

ignores = []

# Ignore matches to go keg

# TODO: explain why

any_go_deps = f.deps.any? do |dep|

dep.name =~ Version.formula_optionally_versioned_regex(:go)

end

if any_go_deps

go_regex = Version.formula_optionally_versioned_regex(:go, full: false)

ignores << %r{#{Regexp.escape(HOMEBREW_CELLAR)}/#{go_regex}/[\d.]+/libexec]}

end

ignores << if OS.linux?

case f.name

# On Linux, GCC installation can be moved so long as the whole directory tree is moved together:

# https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.

when Version.formula_optionally_versioned_regex(:gcc)

%r{#{Regexp.escape(cellar)}/gcc}

# binutils is relocatable for the same reason: https://github.com/Homebrew/brew/pull/11899#issuecomment-906804451.

when Version.formula_optionally_versioned_regex(:binutils)

%r{#{Regexp.escape(cellar)}/binutils}

end

end

ignores

end

MikeMcQuaid · 2021-10-18T19:32:11Z

Library/Homebrew/dev-cmd/bottle.rb

+        # Add additional workarounds to ignore
+        ignores += add_regex_if_applicable(f, cellar, ignores)


Suggested change

# Add additional workarounds to ignore

ignores += add_regex_if_applicable(f, cellar, ignores)

# Add formula-specific workarounds

ignores += formula_ignores(f, cellar)

MikeMcQuaid · 2021-10-18T20:13:44Z

Library/Homebrew/dev-cmd/bottle.rb

+      go_regex =
+        Version.formula_optionally_versioned_regex(:go, full: false)


Suggested change

go_regex =

Version.formula_optionally_versioned_regex(:go, full: false)

go_regex = Version.formula_optionally_versioned_regex(:go, full: false)

MikeMcQuaid · 2021-10-18T20:15:09Z

Library/Homebrew/dev-cmd/bottle.rb

+    # On Linux, GCC installation can be moved so long as the whole directory tree is moved together:
+    # https://gcc-help.gcc.gnu.narkive.com/GnwuCA7l/moving-gcc-from-the-installation-path-is-it-allowed.
+    # binutils is relocatable for the same reason: https://github.com/Homebrew/brew/pull/11899#issuecomment-906804451.


Please move down these comments so it's easier to see what parts specifically refer to gcc/binutils and so adding more formulae in future won't look like they are referred to by these comments. Ideally I'd consider duplicating the if OS.linux? on each regex line and doing ignores.compact so it doesn't require as much reorganisation when other formulae are added here

I moved things around as per your suggestions and now it should be easier to add new formulae as needed. I think it makes sense to keep the go exception separate because it is related to the dependencies of the formula rather than the formula name. But for future additions which rely on the formula name, they should be added to the case block with if OS.linux? or if OS.mac? added as needed.

MikeMcQuaid · 2021-10-20T07:59:13Z

Looks great, thanks @danielnachun and sorry for all the back and forth!

danielnachun · 2021-10-20T08:35:52Z

Thanks @MikeMcQuaid!

carlocab · 2021-10-24T08:14:45Z

Related? Homebrew/homebrew-core@56dd835

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 21, 2021

danielnachun requested review from iMichka and sjackman August 21, 2021 23:45

danielnachun force-pushed the gcc_relocation_fix branch from e272e04 to 54a84ae Compare August 22, 2021 18:54

danielnachun changed the title ~~bottle.rb: add regex for gcc cellar to ALLOWABLE_HOMEBREW_REPOSITORY_…~~ bottle.rb: add workaround to make gcc have cellar :any Aug 23, 2021

MikeMcQuaid requested changes Aug 23, 2021

View reviewed changes

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 24, 2021

BrewTestBot approved these changes Aug 24, 2021

View reviewed changes

MikeMcQuaid reviewed Aug 24, 2021

View reviewed changes

danielnachun force-pushed the gcc_relocation_fix branch 3 times, most recently from a8ea1b4 to 3e2fc14 Compare August 26, 2021 05:26

carlocab reviewed Sep 2, 2021

View reviewed changes

danielnachun force-pushed the gcc_relocation_fix branch from 3e2fc14 to f775b5d Compare October 18, 2021 01:03

MikeMcQuaid reviewed Oct 18, 2021

View reviewed changes

danielnachun force-pushed the gcc_relocation_fix branch from f775b5d to 1a0d01f Compare October 18, 2021 18:00

carlocab reviewed Oct 18, 2021

View reviewed changes

Library/Homebrew/dev-cmd/bottle.rb Show resolved Hide resolved

danielnachun force-pushed the gcc_relocation_fix branch 3 times, most recently from 8be080d to 39174d8 Compare October 18, 2021 18:29

carlocab reviewed Oct 18, 2021

View reviewed changes

MikeMcQuaid reviewed Oct 18, 2021

View reviewed changes

carlocab reviewed Oct 18, 2021

View reviewed changes

danielnachun force-pushed the gcc_relocation_fix branch from 39174d8 to ff7f81d Compare October 18, 2021 19:08

carlocab reviewed Oct 18, 2021

View reviewed changes

danielnachun force-pushed the gcc_relocation_fix branch 2 times, most recently from a5b3a2a to 8bff652 Compare October 18, 2021 19:22

danielnachun commented Oct 18, 2021

View reviewed changes

Library/Homebrew/dev-cmd/bottle.rb Show resolved Hide resolved

MikeMcQuaid reviewed Oct 18, 2021

View reviewed changes

danielnachun force-pushed the gcc_relocation_fix branch from 8bff652 to 9c7bce0 Compare October 18, 2021 19:55

MikeMcQuaid reviewed Oct 18, 2021

View reviewed changes

bottle.rb: add workaround to make gcc have cellar :any

0323c3a

danielnachun force-pushed the gcc_relocation_fix branch from 9c7bce0 to 0323c3a Compare October 19, 2021 19:18

MikeMcQuaid approved these changes Oct 20, 2021

View reviewed changes

MikeMcQuaid merged commit e771571 into Homebrew:master Oct 20, 2021

danielnachun mentioned this pull request Oct 20, 2021

gcc: rebuild as a relocatable bottle Homebrew/homebrew-core#87666

Closed

6 tasks

github-actions bot added the outdated PR was locked due to age label Nov 24, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2021

danielnachun deleted the gcc_relocation_fix branch July 11, 2022 00:18

		# Add additional workarounds to ignore
		ignores << add_additional_ignores(f, cellar)

	ignores << add_additional_ignores(f, cellar)
	ignores += gcc_or_binutils_regex_if_applicable(f, cellar)

		# Add additional workarounds to ignore
		ignores += add_regex_if_applicable(f, cellar, ignores)

		go_regex =
		Version.formula_optionally_versioned_regex(:go, full: false)

	go_regex =
	Version.formula_optionally_versioned_regex(:go, full: false)
	go_regex = Version.formula_optionally_versioned_regex(:go, full: false)

Uh oh!

Conversation

danielnachun commented Aug 21, 2021

Uh oh!

BrewTestBot commented Aug 21, 2021

Uh oh!

Bo98 commented Aug 22, 2021

Uh oh!

danielnachun commented Aug 22, 2021

Uh oh!

Bo98 commented Aug 22, 2021

Uh oh!

Bo98 commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielnachun commented Aug 22, 2021

Uh oh!

danielnachun commented Aug 22, 2021

Uh oh!

danielnachun commented Aug 23, 2021

Uh oh!

iMichka commented Aug 23, 2021

Uh oh!

Bo98 commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

BrewTestBot commented Aug 24, 2021

Uh oh!

danielnachun commented Aug 24, 2021

Uh oh!

MikeMcQuaid commented Aug 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielnachun Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjackman commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielnachun commented Aug 27, 2021

Uh oh!

MikeMcQuaid commented Aug 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 27, 2021

Uh oh!

danielnachun commented Oct 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlocab Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Bo98 commented Aug 22, 2021 •

edited

Loading

Bo98 commented Aug 23, 2021 •

edited

Loading

danielnachun Aug 26, 2021 •

edited

Loading

sjackman commented Aug 26, 2021 •

edited

Loading

carlocab Oct 18, 2021 •

edited

Loading

carlocab Oct 18, 2021 •

edited

Loading

carlocab Oct 18, 2021 •

edited

Loading

carlocab Oct 18, 2021 •

edited

Loading