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

super: fix GCC linkage issue in Linux #11459

Merged
merged 1 commit into from Jun 3, 2021

Conversation

danielnachun
Copy link
Member

  • 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?

@BrewTestBot
Copy link
Member

Review period will end on 2021-06-01 at 00:00:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label May 30, 2021
@danielnachun
Copy link
Member Author

This is my second attempt to fix the issues relating to GCC linker flag ordering in the superenv in Linux. To recap why we need this:

  1. For users without a host GCC at least version 5, we have to place symlinks for the GCC 5 runtime libs in $HOMEBREW_PREFIX/lib, because we don't add RPATHs for brewed GCC 5 to our binaries built in CI. On systems with a new enough host GCC this doesn't matter because we can use the host GCC runtime libs, but on systems that need brewed GCC 5, we have to do this so that bottles built in CI can find the brewed GCC 5 runtime libs. We could add the RPATH for brewed GCC 5 when pouring bottles using patchelf.rb, but that is more complex fix and would require users to repour all bottles with GCC 5 runtime dependencies.

  2. The superenv adds -L$HOMEBREW_PREFIX/lib to the LDFLAGS when building from source. My previous attempt in cc: remove unneeded linker flag when building with GCC on Linux #10606 to fix this created breakage because I tried to remove -L$HOMEBREW_PREFIX/lib, which it turns out we still need for some things.

  3. We have a custom specs for newer GCC versions which adds the proper linker flags and RPATHS to all binaries built by that newer GCC. However, because of point 2 above, the superenv adds -L$HOMEBREW_PREFIX/lib before the linker flags added by the GCC specs file. And because of point 1 above, this means the linker finds the GCC runtime libs for GCC 5 before finding the runtime libs for newer GCC. This is possible because the sonames for GCC runtime libs are not always incremented between major versions even though the ABI is not the same (this is because the ABI is backwards compatible but not forwards compatible).

This PR implements a solution suggested by @Bo98 to add some additional logic to check if the compiler being used is GCC, and if so, to add the LDFLAG for the newer GCC before -L$HOMEBREW_PREFIX/lib. No flags are removed, and the LDFLAG we are adding is already added by the specs file. This essentially creating an extra copy of that flag and just making sure it comes before any other LDFLAGS.

I have tested this on a system with brewed GCC 5 while building a package with a newer brewed GCC, and it works as expected. Let me know if this needs further explanation as this is a particularly arcane problem!

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label May 31, 2021
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label May 31, 2021
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

BrewTestBot
BrewTestBot previously approved these changes May 31, 2021
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.

Looks good so far!

Library/Homebrew/extend/ENV/super.rb Show resolved Hide resolved
]
paths = []
if compiler.match?(GNU_GCC_REGEXP)
f = gcc_version_formula(compiler.to_s)
Copy link
Member

Choose a reason for hiding this comment

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

Need to catch the FormulaUnavailableError here in case it doesn't exist.

@danielnachun danielnachun merged commit 273b06c into Homebrew:master Jun 3, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label Jul 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 4, 2021
@danielnachun danielnachun deleted the gcc_fix branch July 11, 2022 00:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants