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

Changes to allow keg-only glibc #13873

Merged
merged 4 commits into from Sep 19, 2022
Merged

Changes to allow keg-only glibc #13873

merged 4 commits into from Sep 19, 2022

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Sep 15, 2022

Not tested yet.

See Homebrew/homebrew-core#109998 and Homebrew/homebrew-core#110779, the latter of which is what the install changes are based on.

If it works, it'll need to be merged at the same time as Homebrew/homebrew-core#109998 (if the change moving glibc to keg-only is made).

@BrewTestBot
Copy link
Member

Review period will end on 2022-09-16 at 13:16:48 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Sep 15, 2022
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.

Makes sense to me!

@Bo98 Bo98 force-pushed the glibc-keg-only branch 2 times, most recently from d234736 to f008d3e Compare September 15, 2022 13:34
@danielnachun
Copy link
Member

Do we also need to make sure the path to the glibc headers is added to the include paths?

@Bo98
Copy link
Member Author

Bo98 commented Sep 15, 2022

I think our GCC handles that. I don't know about LLVM though.

I guess if we're adding it as a dependency everywhere, it might already be added like other dependencies are (so maybe the PATH change isn't needed...). Really the correct way would probably be how we handle glibc@2.13, but it seems like we've got away so far without.

Comment on lines 99 to 120
if ld_so.readable?
# Remove legacy symlinks
FileUtils.rm gcc_library_symlink if gcc_library_symlink.symlink?
else
Copy link
Member

Choose a reason for hiding this comment

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

Given that glibc will be keg-only:

I wonder if we should just keep using our glibc even if the host's is newer.

This will allow us to get rid of these GCC library symlinks entirely.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just keep using our glibc even if the host's is newer.

Are you saying 1) "we should use it even if the host's is newer" or 2) "we should not use it even if the host's is newer"?

Personally, I'm in strongly in favour of 2) and moderately strongly opposed to 1).

Copy link
Member

@carlocab carlocab Sep 16, 2022

Choose a reason for hiding this comment

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

It's actually neither; apologies for being unclear.

This else bit I'm commenting on above is the only part that keeps these GCC library symlinks around. This handles the case where the host Glibc is newer than ours, but the host GCC is older than our preferred version.

I was suggesting that for this case we should consider using a keg-only Glibc. I think that should allow us to get rid of these GCC symlinks entirely, which, based on previous discussion, was something we wanted to do but decided to punt on.

Whenever the host Glibc and host GCC are both not older, we should definitely not install the glibc formula.

Copy link
Member

Choose a reason for hiding this comment

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

This is a really interesting idea although @sjackman should comment when he is back on it as well. Using ldconfig as we are proposing to do would let us get rid of the GCC symlinks and hopefully move us closer towards getting rid of any use of -L#{HOMEBREW_PREFIX/}lib and -Wl,-rpath,#{HOMEBREW_PREFIX}/lib altogether.

Copy link
Member

Choose a reason for hiding this comment

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

move us closer towards getting rid of any use of -L#{HOMEBREW_PREFIX/}lib and -Wl,-rpath,#{HOMEBREW_PREFIX}/lib altogether.

Yes, I'd really like to be able to do this too.

Copy link
Member

Choose a reason for hiding this comment

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

Personal opinion is less people installing glibc the better, particularly when it comes to non-x86_64 platforms.

We don't support these. Let's not optimise for them at all until we have e.g. a means of generating portable Ruby there in CI.

Given I've not seen any objections, I'd like to merge this and Homebrew/homebrew-core#109998 as soon as today (after some testing) unless anyone is against that.

👍🏻 although I think binutils is as much if not more of a problem here.

Copy link
Member

@carlocab carlocab Sep 16, 2022

Choose a reason for hiding this comment

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

👍🏻 although I think binutils is as much if not more of a problem here.

I'm not even sure why gcc@11 depends on binutils, tbh

Or any of the GCC formulae, for that matter

Copy link
Member

Choose a reason for hiding this comment

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

I'm not even sure why gcc@11 depends on binutils, tbh

@danielnachun Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Possibly it's just there to make sure we also install brewed binutils when we install brewed gcc. If that's the case it could be changed to a global dependency. But we should make sure it's not needed for some other reason.

Copy link
Member

Choose a reason for hiding this comment

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

Possibly it's just there to make sure we also install brewed binutils when we install brewed gcc.

Why would we want to make sure of that?

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Sep 16, 2022
@BrewTestBot
Copy link
Member

Review period ended.

Library/Homebrew/extend/os/linux/install.rb Outdated Show resolved Hide resolved
Library/Homebrew/extend/os/linux/install.rb Outdated Show resolved Hide resolved
Comment on lines 89 to 92
ld_gcc_conf = ld_so_conf_d/"50-homebrew-preferred-gcc.conf"
ld_gcc_conf.atomic_write <<~EOS
#{gcc_opt_prefix}/lib/gcc/#{GCC_VERSION_SUFFIX}
EOS
Copy link
Member Author

Choose a reason for hiding this comment

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

Not too critical: but I wonder whether we should be writing this everytime or only if it doesn't exist.

If the latter: what do we do if we want to update it in the future?

@Bo98 Bo98 force-pushed the glibc-keg-only branch 4 times, most recently from 6eebf75 to fcdc376 Compare September 19, 2022 03:43
@Bo98 Bo98 merged commit b8b195c into Homebrew:master Sep 19, 2022
@Bo98 Bo98 deleted the glibc-keg-only branch September 19, 2022 07:10
@Bo98 Bo98 removed the do not merge label Sep 19, 2022
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 20, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 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.

None yet

6 participants