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

keg_relocate (linux): prepend gcc/lib/current to RPATH when needed #13631

Merged
merged 3 commits into from
Aug 2, 2022
Merged

keg_relocate (linux): prepend gcc/lib/current to RPATH when needed #13631

merged 3 commits into from
Aug 2, 2022

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Aug 2, 2022

  • 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 should help keep bottles that require GCC working when
Homebrew/homebrew-core#106755 is merged.

This only works on freshly-poured bottles. Previously installed bottles
will still break on systems with a host GCC older than GCC 11.


I've marked this as draft since this isn't ready to merge yet. For one thing, this line probably needs explanation in a comment. But I've opened this to suggest a potential way to minimise the pain of breaking all these Linux bottles for the GCC 12 migration.

As noted above, this fix only applies for bottles poured after brew includes this change. If desired, we could add a check to brew doctor that recommends a brew install foo if foo would benefit from having their RPATH fixed up by this change.

This should help keep bottles that require GCC working when
Homebrew/homebrew-core#106755 is merged.

This only works on freshly-poured bottles. Previously installed bottles
will still break on systems with a host GCC older than GCC 11.
@carlocab carlocab requested review from MikeMcQuaid and a team August 2, 2022 03:09
@BrewTestBot
Copy link
Member

Review period will end on 2022-08-03 at 03:09:28 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 2, 2022
@carlocab
Copy link
Member Author

carlocab commented Aug 2, 2022

It's also worth noting that this will likely fix most, if not all, Linux-only GCC dependents in CI (as bottles are always freshly poured).

Also add comment explaining what this line does, along with a TODO
suggesting a replacement once we've shipped the GCC PR.
@carlocab carlocab marked this pull request as ready for review August 2, 2022 05:33
@carlocab
Copy link
Member Author

carlocab commented Aug 2, 2022

It's worth noting that this replaces linkage to any Homebrew GCC, even if they're not the latest, with linkage to the newest Homebrew GCC.

This is safe to do only if GCC libraries are always backwards-compatible. My understanding is that they should be. At the very least, libstdc++ and libgcc_s should be.

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.

Fine to 🚢 as-is but one question.

@fxcoudert
Copy link
Member

This is safe to do only if GCC libraries are always backwards-compatible.

GCC libraries are properly versioned, so any non-backward compatible change will mean the library version will have changed. So this should not be a problem.

@carlocab carlocab added the critical Critical change which should be shipped as soon as possible. label Aug 2, 2022
@carlocab carlocab enabled auto-merge August 2, 2022 11:10
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 2, 2022
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@carlocab
Copy link
Member Author

carlocab commented Aug 2, 2022

Ok, I think I'm happy to merge this.

Also, with this change in hand, we can then use a trick similar to what we did for the Linuxbrew migration to migrate installed Linux bottles to GCC 12:

LINUXBREW_CORE_MIGRATION_LIST.each do |name|
begin
formula = Formula[name]
rescue FormulaUnavailableError
next
end
next unless formula.any_version_installed?
keg = formula.installed_kegs.last
tab = Tab.for_keg(keg)
# force a `brew upgrade` from the linuxbrew-core version to the homebrew-core version (even if lower)
tab.source["versions"]["version_scheme"] = -1
tab.write
end
Settings.write "linuxbrewmigrated", true
end

I think this should result in an almost pain-free migration experience for most Linux users. (Aside from the bottles that have already been lost due to changes independent from bottle removal.)

Thoughts? We'll need to get the timing of tagged releases for brew, etc, right, of course.

@MikeMcQuaid
Copy link
Member

Also, with this change in hand, we can then use a trick similar to what we did for the Linuxbrew migration to migrate installed Linux bottles to GCC 12:

Seems like a great idea 👍🏻

@carlocab carlocab merged commit 716136b into Homebrew:master Aug 2, 2022
@carlocab carlocab deleted the gcc-rpaths branch August 2, 2022 12:54
@carlocab
Copy link
Member Author

carlocab commented Aug 2, 2022

Ok, I guess we'll want those Linux bottles back then. Homebrew/homebrew-core#107148

@github-actions github-actions bot added the outdated PR was locked due to age label Sep 2, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2022
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.

5 participants