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

linux: use preferred_gcc instead of gcc #10569

Merged
merged 1 commit into from
Feb 10, 2021
Merged

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented Feb 8, 2021

  • 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?
  • Have you successfully run brew man locally and committed any changes?

@BrewTestBot
Copy link
Member

Review period will end on 2021-02-09 at 19:09:44 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 8, 2021
@iMichka
Copy link
Member Author

iMichka commented Feb 8, 2021

I need to run some sanity checks locally before shipping this. Proposed by @sjackman

Library/Homebrew/extend/ENV/shared.rb Outdated Show resolved Hide resolved
Library/Homebrew/extend/os/linux/keg_relocate.rb Outdated Show resolved Hide resolved
@sjackman
Copy link
Member

sjackman commented Feb 8, 2021

In extend/os/linux/linkage_checker.rb

-@undeclared_deps -= ["gcc", "glibc"]
+@undeclared_deps -= [preferred_compiler, "glibc"]

@sjackman
Copy link
Member

sjackman commented Feb 8, 2021

In extend/os/linux/system_config.rb

-["glibc", "gcc", "xorg"].each do |f|
+["glibc", preferred_compiler, "xorg"].each do |f|

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.

👍🏻 once this is 💚. I really like how minimal and clean this change is ❤️

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 9, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@iMichka
Copy link
Member Author

iMichka commented Feb 9, 2021

I had to stub gcc@5 in the tests to make it pass. That's the only test change I had to make.

@sjackman
Copy link
Member

I did some quick tests in the homebrew/brew Docker image.
brew install -sv hello uses /usr/bin/gcc-9 provided by the host
brew install gcc@5 && brew install -sv hello uses /home/linuxbrew/.linuxbrew/bin/gcc-5

Currently gcc and gcc@5 conflict, because they both provide GCC 5 until https://github.com/Homebrew/linuxbrew-core/pull/21380 is merged.

I need to think about migration issues for existing users that have gcc installed, because their host system is old (GCC < 5). We want it to smoothly install gcc@5 for those users (and upgrade gcc to 10, but I'm less concerned about that). I don't believe gcc@5 will be installed automatically, but I need to test that.

@sjackman
Copy link
Member

We want it to smoothly install gcc@5 for those users (and upgrade gcc to 10, but I'm less concerned about that). I don't believe gcc@5 will be installed automatically, but I need to test that.

This sort of works! Once gcc is upgraded to GCC 10 and no longer provides ~/.linuxbrew/opt/gcc/bin/gcc, when the user has no other executable named gcc version ≥ 5 in their PATH, then Homebrew will install preferred_gcc, aka gcc@5.

@iMichka
Copy link
Member Author

iMichka commented Feb 10, 2021

Regarding the transition phase from gcc to gcc-5 in linuxbrew-core: stuff might get broken for complex cases, especially for people which do not have gcc-5 installed. But I think we can guide them to run some commands manually to clean the mess up if anything goes wrong. But your tests tend to demonstrate that there will be no issue.

@iMichka iMichka merged commit e0a9ec9 into Homebrew:master Feb 10, 2021
@iMichka iMichka deleted the gcc2 branch February 10, 2021 11:20
@sjackman
Copy link
Member

Thank you, Michka!

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 13, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants