-
Notifications
You must be signed in to change notification settings - Fork 380
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
build: use clang-13 to build bpf programs #171
Conversation
context: . | ||
file: ./Dockerfile.clang | ||
push: true | ||
platforms: linux/amd64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add linux/arm64
here?
This PR is currently blocked by a verifier failure on 5.4 for the large exec prog. I'll post the logs here tomorrow. |
4a118d1
to
da194d9
Compare
Addressing Chance's feedback [0]. [0]: #171 (comment) Signed-off-by: William Findlay <[email protected]>
0d27e30
to
da194d9
Compare
NB: this is blocked on #221 |
tested on 5.4 and 4.19 seems ok note I see following fails on 4.19:
but I see them with the old clang as well, so I'll check on those separately |
da194d9
to
2a092c8
Compare
2a092c8
to
3da9fa9
Compare
3da9fa9
to
2fbae30
Compare
2fbae30
to
137967d
Compare
137967d
to
4fb8b70
Compare
ok, would be great if we could do that as follow up ;-) I'll put that on my todo list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just some minor comments.
4fb8b70
to
96905cd
Compare
One other thought: Maybe the "right" thing to do is to have the Dockerfile and deployment job live in a separate repo. I'm not in love with the way it works now where we have to push to build the new image then push again to update the CI jobs to use the new builder (my fault since I wrote the first draft, but my intention was always to move it over to something like this and maybe it makes sense to just do it now). What do you think @kkourt @olsajiri? |
96905cd
to
275f994
Compare
Use apt-get instead of apt to silence warnings. Remove tools-install target because it no longer exists. This re-introduces a previously reverted commit. Signed-off-by: Jiri Olsa <[email protected]> Signed-off-by: William Findlay <[email protected]>
Introduce a new Docker image that provides a suitable clang version for compiling our BPF programs. This re-introduces a previously reverted commit. Signed-off-by: William Findlay <[email protected]>
275f994
to
b315284
Compare
If I understand correctly, this is about having to do a first version of PR to figure out the image tag, and then update the PR with the produced image tag? No strong opinions either way. Having a different repo is sensible, but having everything in the same repo is simpler. Fwiw, this is also how cilium is doing it: https://docs.cilium.io/en/latest/contributing/development/images/ (cilium is different in that it requires clang in its runtime image as well.) |
$(CONTAINER_ENGINE) stop ${id} | ||
.PHONY: image-clang | ||
image-clang: | ||
$(CONTAINER_ENGINE) build -f Dockerfile.clang --build-arg VERSION=1:13.0.1-2ubuntu2 -t "cilium/clang:${DOCKER_IMAGE_TAG}" . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reading https://docs.cilium.io/en/latest/contributing/development/images/, how about we use cilium/tetragon-clang for the image name? I think it would be less confusing for folk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@willfindlay please address this comment if needed with follow up.
This needs more testing on additional kernels before we merge. For example, while testing locally I noticed some verifier errors on 5.4 that we should fix first.