KEMBAR78
Add formula DSL support for omitting install name rewriting for `@rpath/*` install names by Copilot · Pull Request #20801 · Homebrew/brew · GitHub
Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Oct 2, 2025

  • Add preserve_rpath DSL method to Formula class (similar to skip_clean)
  • Store preserve_rpath setting in Formula class instance variable
  • Modify dylib_id_for in Library/Homebrew/extend/os/mac/keg_relocate.rb to skip rewriting @rpath install names when DSL is enabled
  • Add accessor method in Keg class to check formula's preserve_rpath setting
  • Add tests for the new functionality
  • Run tests to validate changes
  • Fix Sorbet type errors
Original prompt

This section details on the original issue you should resolve

<issue_title>Add formula DSL support for omitting install name rewriting for @rpath/* install names</issue_title>
<issue_description>### Verification

Provide a detailed description of the proposed feature

Currently, we rewrite all library install names to something like opt_prefix/library_install_name.basename.

if file.dylib?
id = relocated_name_for(file.dylib_id, relocation)
change_dylib_id(id, file)
end

Typically, these will be because the install name refers to a Cellar path, e.g., prefix/library_install_name.basename. The install name rewriting allows us to avoid unnecessary rebuilds of dependents with version/revision bumps.

This install name rewriting isn't needed when the install name starts with @rpath, because these will typically not hardcode a Cellar path reference that will need to be rebuilt on version/revision bumps.

Formulae should support some sort of DSL that makes the code in keg_relocate.rb skip rewriting an install name if it starts with @rpath.

What is the motivation for the feature?

Our rewriting of @rpath-prefixed install names breaks macdeployqt. See Homebrew/discussions#2823.

This is arguably an upstream bug, but convincing upstream to fix it is unlikely to succeed without a reproduction of the issue outside of Homebrew, and I don't think anyone has the time or energy to do this. Even if one had such a reproducer, it would likely still be rather difficult to convince upstream to fix this short of providing a patch.

Such a DSL could also replace a very old workaround in Rust:

  def post_install
    Dir["#{lib}/rustlib/**/*.dylib"].each do |dylib|
      chmod 0664, dylib
      MachO::Tools.change_dylib_id(dylib, "@rpath/#{File.basename(dylib)}")
      MachO.codesign!(dylib) if Hardware::CPU.arm?
      chmod 0444, dylib
    end
  end

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

It probably won't be, but it would likely be relevant to our users who use Qt, which form a non-negligible fraction of our user base.

What alternatives to the feature have been considered?

See alternatives described in this comment.</issue_description>

Comments on the Issue (you are @copilot in this section)

@carlocab Also, to explain why we want an additional DSL rather than just skipping the rewriting of `@rpath/*` install names entirely:

Typically, these will make linking with libraries harder. For example, if libfoo.dylib has an absolute path for its install name, linking with libfoo only requires

-L$HOMEBREW_PREFIX/lib -lfoo

However, if libfoo has @rpath/libfoo.dylib for its install name, one would need some variation of

-L$HOMEBREW_PREFIX/lib -lfoo -Wl,-rpath,$HOMEBREW_PREFIX/lib

which is likely to trip up many users and result in may issues/discussion posts created.

If we had such a DSL, the above would not be a consideration for Rust (cargo handles linker invocation), and isn't likely to be a significant problem for Qt (since upstream ships libraries/frameworks with install names that start with @rpath in their pre-built binaries).</comment_new>
<comment_new>@MikeMcQuaid
Thanks @carlocab. Agree with everything you've said here and the desire for this for both Qt and Rust.</comment_new>
<comment_new>@MikeMcQuaid
@pawsaw We're waiting for someone to implement it.</comment_new>
<comment_new>@Bo98
Here's a case that doesn't involve @rpath: Swift ships dylibs that point to /usr/lib/swift/libswift* rather than the installed file. This is intentional due to the unique way that Swift ships with the OS but new features can still be back deployed to older OS.

My initial thought was to skip anything that didn't start with HOMEBREW_PREFIX or the build directory. @rpath however is hard and do acknowledge the DSL argument here.</comment_new>

Fixes #15354

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@MikeMcQuaid
Copy link
Member

@copilot try again

@Copilot
Copy link
Contributor Author

Copilot AI commented Oct 3, 2025

@copilot try again

Fixed Sorbet type errors in commit 6d444d0. The implementation now uses T.unsafe(::Formula) to handle the Formula lookup in the macOS-specific keg_relocate module, and removed the incorrect typed: strict annotation from the test file.

Copilot finished work on behalf of MikeMcQuaid October 3, 2025 07:24
Co-authored-by: MikeMcQuaid <125011+MikeMcQuaid@users.noreply.github.com>
@MikeMcQuaid MikeMcQuaid force-pushed the copilot/fix-fab7d5fc-dabe-4a66-8fc4-ed32200d3d65 branch from 6d444d0 to 2bd0b81 Compare October 6, 2025 16:28
@MikeMcQuaid MikeMcQuaid changed the title [WIP] Add formula DSL support for omitting install name rewriting for @rpath/* install names Add formula DSL support for omitting install name rewriting for @rpath/* install names Oct 6, 2025
@MikeMcQuaid MikeMcQuaid marked this pull request as ready for review October 6, 2025 16:30
@MikeMcQuaid MikeMcQuaid enabled auto-merge October 6, 2025 16:30
@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Oct 6, 2025
Merged via the queue into main with commit b5225c1 Oct 6, 2025
38 checks passed
@MikeMcQuaid MikeMcQuaid deleted the copilot/fix-fab7d5fc-dabe-4a66-8fc4-ed32200d3d65 branch October 6, 2025 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add formula DSL support for omitting install name rewriting for @rpath/* install names

3 participants