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

Use unversioned GCC for runtime libraries when required. #13916

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Use unversioned GCC for runtime libraries when required. #13916

merged 1 commit into from
Sep 27, 2022

Conversation

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

See discussion at Homebrew/homebrew-core#110010.

@BrewTestBot
Copy link
Member

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

@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Sep 22, 2022
@BrewTestBot
Copy link
Member

Review period ended.

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.

Not sure I agree with this. Doesn't this make it fairly likely you'll need gcc@11 and gcc installed?

@carlocab
Copy link
Member Author

carlocab commented Sep 23, 2022

I'm a little confused by that, since this is my understanding of the conclusion of our discussion at Homebrew/homebrew-core#110010.

In any case:

This PR does not change the likelihood of needing both gcc and gcc@11 installed. Some users already need both without this change, and this PR does not change anything for them. (Nor does it change anything right now for users who currently don't need both installed.)

What this PR does change is the likelihood of needing three different versions of GCC installed. In particular, this PR makes sure that brew never forces you to have three versions installed, whereas not merging this PR will result in some users having three versions of GCC forced on them when GCC 13 comes along. (This happens, for instance, if you currently need both gcc@11 and gcc@12 without this change, and then gcc is upgraded to GCC 13 and you have a formula with a dependency on gcc.)

That said: needing multiple GCCs is not likely at all, at least for users who only use bottles. This is something you can always do in the default prefix (whenever a bottle is available). With this PR, you'll only ever need a second GCC if you build from source. On the other hand, without this PR, some bottle users will need two GCCs (gcc@12 and gcc) installed, even if they don't build from source.

@MikeMcQuaid
Copy link
Member

I'm a little confused by that, since this is my understanding of the conclusion of our discussion at Homebrew/homebrew-core#110010.

Sorry about that.

What this PR does change is the likelihood of needing three different versions of GCC installed. In particular, this PR makes sure that brew never forces you to have three versions installed, whereas not merging this PR will result in some users having three versions of GCC forced on them when GCC 13 comes along. (This happens, for instance, if you currently need both gcc@11 and gcc@12 without this change, and then gcc is upgraded to GCC 13 and you have a formula with a dependency on gcc.)

Yeh, I'd rather we nailed down what we want to do here unless GCC 13 is landing imminently.

I'm ok with this change if we also consider bumping the compiler to gcc@12 and then ideally do something like allowing either gcc@12 or higher to be installed for folks that only need it at runtime and just gcc@12 for folks that need it from building from source.

I think we share the goal of minimising GCC versions: I think one for both runtime and building from source cases seems ideal.

@carlocab
Copy link
Member Author

Ok, here's an idea:

We make the compiler dependency a "soft" dependency, in the sense that brew install will still proceed without it installed (and use the unversioned gcc to compile), but build failures (and maybe brew doctor) will suggest a brew install gcc@11. [*]

This way, we make it a bit easier to make sure that users ever only need one GCC installed, but in the event that they need a second one, we nudge them towards the version we use in CI to maximise the probability of a successful source build.

[*] brew install proceeding and using the unversioned gcc may actually already be the current behaviour.

@MikeMcQuaid
Copy link
Member

This way, we make it a bit easier to make sure that users ever only need one GCC installed, but in the event that they need a second one, we nudge them towards the version we use in CI to maximise the probability of a successful source build.

I'm suggesting we use GCC@12 in CI for build, too, given the weird Ubuntu libstdc++/compiler mismatch and the fact that we can just brew install gcc-12 or whatever it is in all our base images.

@carlocab
Copy link
Member Author

I'm suggesting we use GCC@12 in CI for build, too, given the weird Ubuntu libstdc++/compiler mismatch and the fact that we can just brew install gcc-12 or whatever it is in all our base images.

I don't want to use brew to install that, but maybe we can just apt install gcc-12 or something.

@MikeMcQuaid
Copy link
Member

I don't want to use brew to install that, but maybe we can just apt install gcc-12 or something.

@carlocab In our workers: sure. When we install GCC for users: we should install the same one we use in CI for consistency. If we're going to install GCC for build and runtime on the same machine: these should ideally be the same GCC. If we're going to install GCC for runtime libraries only: I don't really care what version it is. If we're going to install GCC for build only: it should be the same we use in CI.

Does that makes sense? Do you agree?

@carlocab
Copy link
Member Author

If we're going to install GCC for runtime libraries only: I don't really care what version it is. If we're going to install GCC for build only: it should be the same we use in CI.

Yes, I agree with this.

However, I think I'd like to punt on making the GCC we build with GCC 12 -- that will require a bit more care and coordination to avoid disruption to Homebrew/core, and I think merging this will unblock Homebrew/actions#308. We need Homebrew/actions#308 to unbreak Homebrew/core -- our publish workflow has broken because brew is not in PATH. https://github.com/Homebrew/homebrew-core/actions/runs/3132629345/jobs/5085168240#step:3:18

@danielnachun
Copy link
Member

I do think we should just move to use GCC 12 to build everything. There's really no point in using GCC 11 given that the GCC 12 runtime libraries are needed anyway.

I'm ambivalent about using the latest GCC at run time instead of what we use in CI. Either we should use GCC 12 for building and runtime of C/C++ programs, and the newest GCC for building and runtime of Fortran, or we use the newest version for all runtimes, and install GCC 12 when building C/C++ programs from source. Both of those solutions mean the user only ever has to have 2 versions of GCC installed, thought the latter choice does mean fewer users will need 2 versions installed.

@danielnachun
Copy link
Member

Having said the above comments, I think we should probably merge this now so that we can fix the existing breakages.

@carlocab
Copy link
Member Author

I rebased to make sure tests run against the tip of master, but that turned out to be a bad idea...

Could not find 'brew' command in PATH.

https://github.com/Homebrew/brew/actions/runs/3133106631/jobs/5086140404

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.

Ok, let's merge to unblock things.

@MikeMcQuaid MikeMcQuaid merged commit 79a3a82 into Homebrew:master Sep 27, 2022
@carlocab carlocab deleted the unversioned-gcc-runtime branch September 27, 2022 08:22
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 28, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
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.

4 participants