Use Clang from cilium-builder image to build BPF code in CI #31754
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/test |
1 similar comment
|
/test |
gentoo-root
requested review from
lambdanis,
bimmlerd,
borkmann,
aanm and
christarazi
christarazi
approved these changes
Apr 3, 2024
bimmlerd
reviewed
Apr 4, 2024
bimmlerd
approved these changes
Apr 4, 2024
lambdanis
approved these changes
Apr 4, 2024
joestringer
approved these changes
Apr 5, 2024
There was a problem hiding this comment.
We may want to get some input from developers who run macOS, particularly for the docs change here - We may be able to tweak the bash script to be more portable. But otherwise this PR LGTM from build and github-sec angles.
I'm still on the fence about the timeout, but I don't feel strongly enough to push back / request changes on that front.
joestringer
added
the
dont-merge/needs-rebase
gentoo-root
force-pushed
the
pr/max/ci-clang-builder
branch
from
cfd16a5
to
6fb773d
Compare
|
/test |
gentoo-root
removed
the
dont-merge/needs-rebase
joestringer
reviewed
Apr 8, 2024
Remove the hardcoded LLVM 10.0 version. Move to building documentation and running CI actions in a reproducible way using the cilium-builder image with the appropriate version of LLVM. Remove the now unneeded step set_clang_dir from lint-bpf-checks.yaml. Unfortunately, ginkgo build test in lint-build-commits.yaml can't be converted that simply (because cilium-builder images don't contain an LLVM toolchain for the host, only for BPF), so leave it as is for now and keep in mind to upgrade the LLVM version there on upgrades. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
Currently, the verifier test is using Clang from Cilium release images, and the integration tests use hardcoded Clang 10.0.0 releases. Convert the verifier test to extract Clang from the relevant cilium-builder image instead. Unfortunately, the integration tests can't be converted that simply (because cilium-builder images don't contain an LLVM toolchain for the host, only for BPF), so leave it as is for now and keep in mind to upgrade the LLVM version there on upgrades. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
gentoo-root
force-pushed
the
pr/max/ci-clang-builder
branch
from
6fb773d
to
8bf977a
Compare
|
/test |
maintainer-s-little-helper
bot
added
the
ready-to-merge
|
🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Many workflows in the CI use a hardcoded Clang 10.0 version, which in addition differs from the actual Clang used by Cilium, because the latter has some patches on top. Migrate to building inside cilium-builder or extracting clang from cilium-builder where possible in a straightforward way. Some gaps remain for the future work.
Also clean up a duplicating workflow and some leftovers from #31526 (previous work on integrating cilium-builder into the workflow that runs
make run_bpf_tests).Field-tested in #31729, where an LLVM upgrade revealed workflows that were tied to Clang 10.