Skip to content

Don't relocate Apple Silicon bottles for default prefix #19247

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

Open
1 task done
MikeMcQuaid opened this issue Feb 5, 2025 · 7 comments · May be fixed by #19384
Open
1 task done

Don't relocate Apple Silicon bottles for default prefix #19247

MikeMcQuaid opened this issue Feb 5, 2025 · 7 comments · May be fixed by #19384
Labels
features New features help wanted We want help addressing this

Comments

@MikeMcQuaid
Copy link
Member

Verification

Provide a detailed description of the proposed feature

We should avoid using e.g. @@HOMEBREW_PREFIX@@ etc. relocation replacements on Apple Silicon (and maybe Linux x86_64/arm64 but not/never macOS Intel).

These were needed initially because replacing /usr/local on macOS Intel was far too wide-reaching with too many false positives and negatives.

What is the motivation for the feature?

This would:

  • simplify and speed up pouring on our widest used platform(s)
  • potentially enable codesigning of homebrew-core packages for Apple Silicon (where we require cask code-signing anyway)
    • code-signing is broken by relocation

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

Increased speed and security for homebrew-core.

What alternatives to the feature have been considered?

  • Doing nothing
  • Having another tap that signs already-relocated bottles
@MikeMcQuaid MikeMcQuaid added features New features help wanted We want help addressing this labels Feb 5, 2025
@cho-m
Copy link
Member

cho-m commented Feb 25, 2025

Would this be specific to binaries (or bottles containing binaries)?

Mainly as we use placeholder in text files to create all bottles.

@MikeMcQuaid
Copy link
Member Author

@cho-m Yes, it should be binary specific and be indicated somehow in a bottle/tab/manifest so we don't break :all bottles. Anything that needs code signed would, by definition, not be possible to be :all.

samuelarogbonlo added a commit to samuelarogbonlo/brew that referenced this issue Feb 26, 2025
This change skips the relocation process for bottles on Apple Silicon Macs
when using the default prefix (/opt/homebrew). Relocation was initially
needed for Intel Macs to avoid wide-reaching replacements with false positives,
but is unnecessary for Apple Silicon where the default prefix is unique.

Benefits:
- Simplifies and speeds up bottle pouring on Apple Silicon
- Potentially enables code-signing of homebrew-core packages
- Improves security by avoiding binary modification post-build

A --force-bottle-relocation flag is added for edge cases where
relocation might still be needed.

Fixes Homebrew#19247
@samuelarogbonlo samuelarogbonlo linked a pull request Feb 26, 2025 that will close this issue
7 tasks
samuelarogbonlo added a commit to samuelarogbonlo/brew that referenced this issue Mar 10, 2025
1. Add binary relocation detection using RubyMacho
2. Update skip_relocation_for_apple_silicon? to consider binary analysis
3. Track relocation status in bottle tab metadata
4. Update keg relocation methods to use new functionality

This implements the remaining suggestions from issue Homebrew#19247.
@cho-m
Copy link
Member

cho-m commented Mar 15, 2025

and maybe Linux x86_64/arm64

This can help for formulae corrupted by binary patching, e.g.

Only current option here is manual hacks to bypass the relocation logic (e.g. wrap the binary inside an archive so that brew bottle cannot see binary). Even non-relocatable bottles still use patchelf.rb and break binary.

Alternative would be to add DSL similar to NixOS to prevent patching, e.g. https://nixos.org/manual/nixpkgs/unstable/#var-stdenv-dontPatchELF

@Bo98
Copy link
Member

Bo98 commented Mar 17, 2025

While I do agree it'd be ideal to use patchelf less (as it is fundamentally a hack), there's quite a difference between optimising the common path and suppressing a bug. We'll never be able to eliminate patchelf entirely for everyone, so suppressing it for the default path to avoid CI flagging genuine bugs is harmful as it will still be a problem that will affect some users. The fact CI is catching that bug is a good thing and it does raise a point that it's perhaps worth keeping a test for in brew test-bot should we make Linux not patch by default. The variables that require patching is more on Linux than macOS: different dynamic linker locations (such as brewed glibc) can trigger a patch rather than being just something for different prefixes.

(This is not me saying to ignore those problems - I just don't think suppressing it in CI is the correct approach. DSL is perhaps better depending on how it works for end users - not entirely clear to me how that works for e.g. a brewed glibc case.)


As an aside, to help out on those issues: I've looked into the protobuf examples just now and do see what's causing the issue - just not entirely sure yet what made it suddenly regress given it was working a couple months ago. Could do with further bisecting.

@cho-m
Copy link
Member

cho-m commented Mar 17, 2025

The fact CI is catching that bug is a good thing and it does raise a point that it's perhaps worth keeping a test for in brew test-bot should we make Linux not patch by default.

I do think we should still test the relocation path as part of CI run if the bottle is relocatable.

Though in the case that the bottle is not relocatable, I think bypassing some checks are fine. The main use case of patching non-relocatable bottle would be the --force-bottle usage which I would consider a lower supported tier / unsupported case.


Taking examples of SBCL-created executables and Bazel, in Homebrew/core we did end up skipping the test on Linux and just shipped broken Linux bottles, which seems worse to me than if we had shipped a bottle with non-patched binaries that only worked on default prefix.


Essentially would be nicer to match support tiers (#19248), e.g.

  1. Prioritize default prefix and avoid patchelf that could corrupt bottle. This will reduce chances of accidentally shipping broken bottles.
  2. Test relocation in CI for relocatable bottle. If it fails, we should have DSL to mark it as non-relocatable (currently we rely on bottle DSL with some pour_bottle? only_if: :default_prefix though a more specific DSL would be cleaner if we want to detail broken if patchelf-ed, etc.)
  3. User is on their own if they try to pour non-relocatable bottle in non-default prefix

This will require more work to handle than ARM macOS case so can split this off into a separate issue tracker to consider after ARM macOS is done.

@MikeMcQuaid
Copy link
Member Author

  1. Prioritize default prefix and avoid patchelf that could corrupt bottle. This will reduce chances of accidentally shipping broken bottles.

I agree with this.

I don't think it makes sense for us to be talking about support levels and at the same time say that we need to ensure sufficient CI coverage of lower support levels that are, almost by definition, going to have lower coverage.

Relatedly, I don't think any maintainers should be or feel obligated to do work for unsupported configurations affecting a minority of users. If some maintainers want to: that's fine.


The "relocate by default" came about from days when relocating a /usr/local prefix (and repository, before ./Homebrew was used) simply wasn't reliable enough. It definitely is now, though, and @cho-m's argument adds weight to this.

@MikeMcQuaid
Copy link
Member Author

The main use case of patching non-relocatable bottle would be the --force-bottle usage which I would consider a lower supported tier / unsupported case.

@cho-m Do you think it's worth adding this to #19641 to discuss support tiers? If so: where do you think it fits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
features New features help wanted We want help addressing this
Projects
None yet
3 participants