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/linkage_checker: remove gcc from undeclared_deps #13796

Merged
merged 2 commits into from
Sep 5, 2022

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Sep 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 is causing warnings all over Homebrew/homebrew-core.

For example, there are dozens of warnings about this in the following CI
run: https://github.com/Homebrew/homebrew-core/runs/8116257943?check_suite_focus=true

This is causing warnings all over Homebrew/homebrew-core.

For example, there are dozens of warnings about this in the following CI
run:

    https://github.com/Homebrew/homebrew-core/runs/8116257943?check_suite_focus=true
@BrewTestBot
Copy link
Member

Review period will end on 2022-09-05 at 06:32:23 UTC.

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

@carlocab @danielnachun @iMichka @sjackman I'd like to understand a little more why this is happening. Are these opportunistic linkages? Are these actually using the GCC formula but it's just added as a dependency in a different way? Will this be resolved when we rebuild bottles on 22.04?

If this is temporary until 22.04 migration: I'm fine with adding this now with a Ruby comment noting when it should be removed. If this is not temporary: can we work out how to resolve this properly?

@carlocab
Copy link
Member Author

carlocab commented Sep 2, 2022

This is an anticipated consequence of #13659.

This is not quite opportunistic linkage, nor is it a dependency -- at least, not in the same way we usually think of those terms.

This is probably best described as "optional" linkage: the formulae that throw these warnings will use gcc whenever it is installed, but they don't need gcc to be installed to function.

Will this be resolved when we rebuild bottles on 22.04?

If by resolving this you mean removing the undeclared dependency exception, then one way of doing this would be to:

  1. Revert linux/super: add unversioned GCC lib directory to RPATH #13659
  2. Remove all (or, at least, almost all) Linux-only GCC dependencies in Homebrew/core
  3. Rebuild all the affected bottles
  4. Ensure that no formula that has dependents ever declares a Linux-only GCC dependency

Step 2 above is, from my understanding, planned, and will be enabled by building bottles on 22.04. Step 3 will likely happen eventually.

There is some dispute about step 4, since some Linux maintainers want the flexibility to build with the gcc formula, and would like to roll back the current audits we have in place to prevent this.

Step 4, however, is crucial. One major reason for doing #13659 is to remove the need to rebuild all of formula foo's dependents whenever foo is rebuilt using Homebrew gcc. This happens because, when foo is built with gcc, foo acquires a dependency on gcc's runtime libraries, and its dependents must then also use the same runtime libraries or else they break. #13659 makes it so that those dependents can use gcc's runtime libraries without being rebuilt.

An alternative to the above would be to make the gcc linkage check a bit more sophisticated by examining the objects that seem to link with gcc to check if they indeed need the latest gcc or can manage with our preferred GCC's runtime libraries.

I know @sjackman had ideas about doing something like this for libstdc++ -- I'm not sure, however, if this is enough to cover every type of "optional" linkage with gcc, since there could be linkage to other runtime libraries as well (though libstdc++ will be the most common).

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, I'm fine as-is given the explanation. Would love to see an inline Ruby comment summarising this situation and explaining why with a TODO that links to an issue describing the work that needs to be done. Thanks again @carlocab!

@danielnachun
Copy link
Member

This happens because, when foo is built with gcc, foo acquires a dependency on gcc's runtime libraries, and its dependents must then also use the same runtime libraries or else they break. #13659 makes it so that those dependents can use gcc's runtime libraries without being rebuilt.

I think #13659 is a great idea and we should keep it. Although I think we should avoid using brewed GCC whenever possible, in the rare case it may be needed preventing the dependency explosion is critical.

Step 2 above is, from my understanding, planned, and will be enabled by building bottles on 22.04. Step 3 will likely happen eventually.

I will be working as much as possible to get rid of all the Linux-only gcc dependencies as quickly as possible once the migration is done. That will get rid of the problem unless we eventually have a formula that needs to be built with brewed GCC and has C++ dependents.

An alternative to the above would be to make the gcc linkage check a bit more sophisticated by examining the objects that seem to link with gcc to check if they indeed need the latest gcc or can manage with our preferred GCC's runtime libraries.

I know @sjackman had ideas about doing something like this for libstdc++ -- I'm not sure, however, if this is enough to cover every type of "optional" linkage with gcc, since there could be linkage to other runtime libraries as well (though libstdc++ will be the most common).

This is the proper solution and I will go over the example @sjackman gave on doing this to see how to implement it best. The logic essentially boils down to finding the newest symbol ABI version in the bottle's binaries and making sure it is not newer than what is provided by GCC 11. It only applies to 4 libraries libstdc++, libgomp, libgcc_s and libatomic, and for a given bottle we should only check the libraries among those 4 that are actually linked.

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

Review period ended.

@carlocab carlocab merged commit a2c23f0 into Homebrew:master Sep 5, 2022
@carlocab carlocab deleted the linux-gcc-linkage branch September 5, 2022 08:05
# See discussions at:
# https://github.com/Homebrew/brew/pull/13659
# https://github.com/Homebrew/brew/pull/13796
# TODO: Find a nicer way to handle this. (e.g. examining the ELF file to determine the required libstdc++.)
Copy link
Member

Choose a reason for hiding this comment

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

@danielnachun can you tackle this post 3.6.0? Please open a tracking issue if that'd be helpful for you to remember 👍🏻

Thanks ❤️

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

4 participants