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

use clang 13 #488

Merged
merged 3 commits into from
Nov 23, 2021
Merged

use clang 13 #488

merged 3 commits into from
Nov 23, 2021

Conversation

lmb
Copy link
Collaborator

@lmb lmb commented Nov 16, 2021

clang 13 has been out for a while and is now available on apt.llvm.org. Switch testdata, etc. to the new version. We still need clang-9 on CI since bpf2go tests rely on it.

@ti-mo ti-mo self-requested a review November 16, 2021 18:58
@ti-mo
Copy link
Collaborator

ti-mo commented Nov 16, 2021

Since we have the llvm (and Go) builder image now, could we consider running the bpf2go tests containerized on CI? I can't get a working clang9 build on Arch anymore since a few weeks now (my libffi is too new) and not sure if this will be fixed. As a result, I have to skip the bpf2go tests for now locally.

@lmb
Copy link
Collaborator Author

lmb commented Nov 17, 2021

Yeah we can consider doing that. The downside is that builds will probably take longer due to the 500MiB container image we need to pull. Maybe a good middle ground is to add another makefile target to use the container but not use that on CI yet? Your local workflow could be make test. Either way, can we tackle that in a separate PR?

@ti-mo
Copy link
Collaborator

ti-mo commented Nov 18, 2021

My problem is mostly that go test is currently broken on my machine. Couldn't we use the clang binary in the Go test suite (should point to something on CI, right?) and a fixed version in the Makefile, since that runs containerized and its output needs to be stable?

@lmb
Copy link
Collaborator Author

lmb commented Nov 18, 2021

That's why I added 76957b9 to explain why we require clang-9. It's not called by the Makefile (unfortunately, since that already runs in the container).

Some options: skip the tests if clang-9 is missing (kinda suboptimal); use an env var to override the version we test against (less meh, also not great); bump the minimum version and hope we don't introduce incompatibilities.

Do you mind if we merge this PR though? I need clang-13 for some bpf2go stuff.

@ti-mo ti-mo merged commit 8df2d8a into master Nov 23, 2021
@ti-mo ti-mo deleted the lmb/clang-13 branch November 23, 2021 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants