Skip to content
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

Revert "workflows/tests: set HOMEBREW_RELOCATABLE_INSTALL_NAMES" #141081

Merged

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Sep 1, 2023

This reverts commit 83ef417.

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This has broken app bundle creation via CMake and Qt, e.g.

In current state, breakages to common use cases are worse then advantages from enabling relocatable names. I recall various projects using Homebrew for app bundling which may now be negatively impacted.

Until we figure out a way of resolving reported issues (which may require modifying qt/cmake to support rewrites of these modified RPATHs), we should disable relocatable names and fix regressions.

@cho-m cho-m requested a review from carlocab September 1, 2023 06:29
@cho-m cho-m requested review from MikeMcQuaid and a team as code owners September 1, 2023 06:29
@github-actions github-actions bot added automerge-skip `brew pr-automerge` will skip this pull request workflows PR modifies GitHub Actions workflow files labels Sep 1, 2023
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be unset in those specific formulae instead or in brew test-bot where e.g. qt or cmake are in the dependency tree?

@cho-m
Copy link
Member Author

cho-m commented Sep 1, 2023

Could this be unset in those specific formulae instead or in brew test-bot where e.g. qt or cmake are in the dependency tree?

I think the issue can impact any formulae that user wants to link in their CMake project and then distribute as app bundle. For example, https://github.com/orgs/Homebrew/discussions/4735 is for harfbuzz's RPATHs which is meson-based formula. The more formulae we bottle with rewritten RPATHs, the more users will hit this.

As I recall, CMake app bundle copies over dylibs into Framework directory and then RPATH rewrites them while copying over recursive dylibs. Seems like Homebrew's rewrites are interfering with CMake's rewrites.

Not sure yet if there is anything that can be done on our side without checking how the CMake/Qt app bundle logic works.

@MikeMcQuaid
Copy link
Member

Not sure yet if there is anything that can be done on our side without checking how the CMake/Qt app bundle logic works.

This is something we can do, it's all open source 😁. That said, you've convinced me we should just revert this for now.

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Sep 1, 2023
Merged via the queue into Homebrew:master with commit c2b0b00 Sep 1, 2023
@cho-m cho-m deleted the revert-relocatable-install-names branch September 1, 2023 22:41
@ZhongRuoyu ZhongRuoyu mentioned this pull request Sep 2, 2023
6 tasks
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 2, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request outdated PR was locked due to age workflows PR modifies GitHub Actions workflow files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants