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

bpftool: Pass additional compile flags as EXTRA_CFLAGS, not CFLAGS #52

Merged
merged 2 commits into from
Oct 2, 2023

Conversation

qmonnet
Copy link
Contributor

@qmonnet qmonnet commented Oct 2, 2023

Bpftool expects users to pass additional compilation flags via EXTRA_CFLAGS, not via CFLAGS. This is because when compiled from the kernel repo, CFLAGS are usually overwritten by the build system.

When we build retsnoop with CFLAGS=--static, the static flag is passed down to bpftool's Makefile. Then we hit an issue: --static is ignored for feature probing at build time, but taken into account for the build itself. Under certain conditions this results in a discrepency and bpftool's Makefile believes LLVM support is available (given it tested without the --static flag) whereas it is not set up for static builds.

Let's address this by passing retsnoop's CFLAGS via bpftool's EXTRA_CFLAGS, and resetting its CFLAGS. We must pass the CFLAGS to the "make" invocation through the environment, and not as a command line argument, to avoid overriding the CFLAGS values that we set inside of bpftool's Makefile.

Related: #51 (comment)

Bpftool expects users to pass additional compilation flags via
EXTRA_CFLAGS, not via CFLAGS. This is because when compiled from the
kernel repo, CFLAGS are usually overwritten by the build system.

When we build retsnoop with CFLAGS=--static, the static flag is passed
down to bpftool's Makefile. Then we hit an issue: --static is ignored
for feature probing at build time, but taken into account for the build
itself. Under certain conditions this results in a discrepency and
bpftool's Makefile believes LLVM support is available (given it tested
without the --static flag) whereas it is not set up for static builds.

Let's address this by passing retsnoop's CFLAGS via bpftool's
EXTRA_CFLAGS, and resetting its CFLAGS. We must pass the CFLAGS to the
"make" invocation through the environment, and not as a command line
argument, to avoid overriding the CFLAGS values that we set inside of
bpftool's Makefile.

Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Test dynamic and static builds for retsnoop on PRs. Test on amd64 only
for now - but this should be easily extensible with the arm64 steps from
the release workflow if necessary in the future.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
Copy link
Contributor Author

qmonnet commented Oct 2, 2023

Bonus: I added a CI workflow to build retsnoop on pull requests, let me know if you prefer to discard it.

@anakryiko
Copy link
Owner

Awesome, thanks a lot for CI build tests. Adding arm64 would definitely be great. I also wonder if static vs dynamic build would be better to split, so they can go in parallel? But yeah, this is awesome, thanks!

@anakryiko anakryiko merged commit 9dc72e1 into anakryiko:master Oct 2, 2023
2 checks passed
@qmonnet qmonnet deleted the pr/bpftool-make-fix branch October 2, 2023 20:33
@qmonnet
Copy link
Contributor Author

qmonnet commented Oct 2, 2023

I also wonder if static vs dynamic build would be better to split, so they can go in parallel?

I wondered too, but thought this would need to install the deps on a second runner and was not sure it was worth it. But yes, it would certainly go faster to run them in parallel.

@anakryiko
Copy link
Owner

btw, I just created a draft release using CI action (https://github.com/anakryiko/retsnoop/actions/runs/6385199025), it all worked well this time. I'll be using this for the next release, thanks for your help!

@qmonnet
Copy link
Contributor Author

qmonnet commented Oct 2, 2023

Great to hear! Did the workflow_dispatch work as you expected? Maybe I should use it too for bpftool

@anakryiko
Copy link
Owner

Yep, I had to add workflow_dispatch and it worked for master. I tried for v0.9.7 tag, but that failed because we don't have all the fixes. I'll give it another try when I'm ready for new release and let you know if something is not working :)

I actually couldn't figure out how to do release without workflow_dispatch. What was the anticipated way to do this?

@qmonnet
Copy link
Contributor Author

qmonnet commented Oct 2, 2023

I actually couldn't figure out how to do release without workflow_dispatch. What was the anticipated way to do this?

Before commit ad63eba, the workflow should have triggered every time someone pushes a tag matching the pattern ** (meaning, any tag). On my fork, where I tested the workflow, you can see (under the Actions tab) that the objects associated to the events for the older runs of the workflow were tags, test05 and the like.

It should have created a draft release automatically for the tag associated with the push event. I'm not sure how it retrieves a tag from workflow_dispatch, by the way.

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.

2 participants