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

Fix for bootstrapping llvm@13:14+gold #30954

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

wyphan
Copy link
Contributor

@wyphan wyphan commented Jun 2, 2022

Hopefully closes #29350 and #31706.

@wyphan wyphan marked this pull request as ready for review June 22, 2022 19:01
@wyphan wyphan changed the title [WIP} Fix for bootstrapping llvm@13:14+gold Fix for bootstrapping llvm@13:14+gold Jun 22, 2022
@wyphan wyphan self-assigned this Jun 22, 2022
@spackbot-app spackbot-app bot requested review from haampie and trws June 24, 2022 09:24
trws
trws previously approved these changes Jun 24, 2022
Copy link
Contributor

@trws trws left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

@trws
Copy link
Contributor

trws commented Jun 24, 2022

Hmm, looks like the headers are not found in CI. It looks like the binutils is not rebuilt though, so I'm not sure if the installed version had them. @scottwittenburg any idea why a variant change wouldn't trigger build of the dependency? https://gitlab.spack.io/spack/spack/-/jobs/2713882

@scottwittenburg
Copy link
Contributor

any idea why a variant change wouldn't trigger build of the dependency?

It looks like the e4s package preferences had already turned on +headers for binutils, so maybe this didn't affect the hash of that particular dependency.

@haampie
Copy link
Member

haampie commented Jul 28, 2022

I'm not sure if this is a good idea. We don't do this for any <pkg>-dev type system package, including python, for which headers are required when bootstrapping clingo from sources. If you mark a spec as external, you should have the equivalent to what spack would install on your system.

@wyphan
Copy link
Contributor Author

wyphan commented Jul 28, 2022

@haampie Yes, the problem is that some distros ship the headers separately as binutils-dev/devel, while the binutils package itself only contains the executables. This complicates things since Spack externals detection currently only works with executables, so spack external find might incorrectly mark binutils as external when it detects nm or readelf in PATH (and thus suggests that the system-installed version is complete, headers and all). This is why the header search is only performed if binutils is external.

@trws
Copy link
Contributor

trws commented Jul 28, 2022

We really need to do this much more generally for externals, in fact the same problem comes up for llvm when building on most debian-based distributions if the libc6-dev or multilib packages are missing, as came up in another issue not too long ago. I'm not sure if this one is particularly worth special casing, perhaps tweak it a bit to let us list a set of headers and packages to print a message for? It's, sadly, a much wider problem than just binutils.

@wyphan
Copy link
Contributor Author

wyphan commented Jul 28, 2022

@trws Is it this one? #31706

@trws
Copy link
Contributor

trws commented Jul 28, 2022

Yup.

Copy link
Contributor

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

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

Squashed and rebased to the latest develop branch

@bernhardkaindl bernhardkaindl self-assigned this Mar 20, 2024
Copy link
Contributor

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

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

It correctly alerts me to install the missing packages on my host, but fails in CI as discussed above already.

I agree that we'd need generic header checks for external packages,
or add external binutils only with "~headers" like I propose in #40214 as a quick fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

llvm+gold with external binutils build failure because of missing plugin-api.h header
6 participants