-
Notifications
You must be signed in to change notification settings - Fork 95
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
Makefile: improvements #28
Conversation
Showing the benefits of this arrangement... Checking if libbpfgo builds with libbpf v0.2:
Builds and libbpfgo_test.go works with one compile warning. Now checking perfbuffers selftest with v0.2:
Now checking perfbuffers selftest with v0.4.0
Both v0.2 and v0.4.0 works with perfbuffers selftest. |
I get the following error:
I'm not sure why it can't find |
Would u mind trying it again ?
I'm using $(abspath ..) for the ../ directory, let's see if that works out in your setup. |
Oh, I just realized you have:
You might have to do a git submodule deinit --all --force and then checkout this repo. Then git submodule init libbpf again. I changed upstream libbpf location to ./libbpf and kept all tests inside ./libbpf/. I'm in doubt of ./selftest/build as it is not a selftest but a 'bpf' dependency for the build test (we could have it as ./libbpfgo_test.bpf.c, for example). Anyway, it is just a suggestion on better organizing the tree and having a way to test regressions and bisections in an easier way. The ./selftest/*/Makefile can call a "run.sh" script inside the test directory (like you had).. for now I have only done 'perftest' as example. |
Alright the change you pushed appears to have fixed it! I'm now getting linker errors which I suspect have to do with my environment, but haven't tracked it down yet:
|
Okay, so.. if you are compiling the libbpfgo-static, then you have to make sure your If you are compiling libbpfgo-dynamic (which is the default, and the one I think you are referring to) then you have to make sure the OS you are using (fedora ?) has at least the package libbpf v0.4.0. In my case, for example, I'm using:
BUT the default version for libbpf0 package (and libbpf-dev) is:
So I had to bring the 0.4.0-1ubuntu1 version from Ubuntu Impish (the -devel version) to make sure it contained the following libbpf upstream commit:
and
|
My libbpf submodule is certainly on a new enough commit, I just don't know why the self test compile isn't linking against it properly, it appears to be pointing to it. |
These happened to be:
when I had 0.3.0 from the OS, thus the previous comment. But, now I see that you had:
... so... maybe the env variables are not being taken into consideration ? Then what will be used will be the |
Ah ok I finally got it working. I had divergent packages for libbpf and libbpf-static on my system. I uninstalled and installed manually from source. But you make a good point, this means that the self tests are building against my environment instead of the git submodule. Investigating |
Alright now that I have my environment issues figured out, this looks very good to me! Can you please:
|
Yes! I'm glad you liked it. I'll fix the RUN and improve instructions how to have this working. Thanks for reviewing it! |
Also no reason to keep the 'old' directories. I like this system better :-) |
Definitely! It was kept as a reference until the +1 only. Will do. |
This comment has been minimized.
This comment has been minimized.
Ha... I got it.. (why you are having issues). I did a small change to libbpfgo.go to have "-lbpf", that made it to ALWAYS link to shared libbpf. If i remove that, I can have static or dynamic linking.
But then I need to find a way to automatically dynamic link to libbpf when just doing 'go build & go install' without having to manually specify CGO_LDFLAGS containing "-lbpf". |
Yup that makes sense. We should avoid having |
Alright, this is ready if you don't find anything else worth doing together in this PR. Thanks for reviewing @grantseltzer |
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.
Just a couple comments, but otherwise this is great!
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.
made it halfway through and had to drop it, will continue next time
Closes: #33 $ make libbpfgo-static-test ./libbpfgo.go:790:15: errptrError format %s has arg pid of wrong type int ./libbpfgo.go:802:14: Sprintf format %s has arg offset of wrong type uint32
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.
There is a small orthogonal, to this change, problem: I'm removing CFLAGS #cgo pragma from libbpfgo.go so we can have full dynamic and static builds for the library test, consumers (local) and selftests. Problem is... without:
An end user of this module (through go.mod -> require) won't be able to compile it locally (even if having libbpf package installed). Problem raises exactly there:
Do either @itaysk or @grantseltzer have a better idea how to solve this ? |
Unless they explicitly specify the path to the libraries, right?
What do you think about keeping |
Yes this is what I meant when making the comment. Sorry, I thought it was obvious. (Please very I'm correct, but I think this is how it works today) BTW - maybe we need a sanity test for that use case but not a blocker for this PR |
To be honest I think the libbpfgo package should use CGO PKG-CONFIG stanza. This would make very easy for all distros to use it as they all rely in PKG-CONFIG for CFLAGS/LDFLAGS:
Problem is, by adding any stanza 'by default' we lose the static/dynamic capability (I could not figure a way to override them).
I think 'go build' will have to work out of the box so users will simply include it and use it without worrying much. I have seen things such as go generate that perhaps could help us out with this, but haven't dig into it yet =(. |
Alright, I'm returning the:
it works for the dynamic and static builds. For now the 'go build' won't work out-of-the-box because it will miss -lbpf LDFLAG. Maybe we can open an issue later to check if we can use 'go generate', something like having wrappers such as this example here: |
I'm working on this (low priority, as we've spoken). I'm porting my portablebpf example to libbpfgo and I was thinking in keeping both in an 'examples/' folder, with a good Readme.md (for example). This go towards with what @grantseltzer was saying about keeping documentation in a single place (don't want to keep in my personal repo as it will be interesting for libbpfgo). It shall explain how to use libbpfgo both dynamically and statically for a simple portable libbpfgo project. |
Currently you will find the following GNU Makefile rules: | Makefile Rule | Description | |--------------------------|-----------------------------------| | all | builds libbpfgo (dynamic) | | clean | cleans entire tree | | selftest | builds all selftests (static) | | selftest-run | runs all selftests (static) | * libbpf dynamically linked (libbpf from OS) | Makefile Rule | Description | |--------------------------|-----------------------------------| | libbpfgo-dynamic | builds dynamic libbpfgo (libbpf) | | libbpfgo-dynamic-test | 'go test' with dynamic libbpfgo | | selftest-dynamic | build tests with dynamic libbpfgo | | selftest-dynamic-run | run tests using dynamic libbpfgo | * statically compiled (libbpf submodule) | Makefile Rule | Description | |--------------------------|-----------------------------------| | libbpfgo-static | builds static libbpfgo (libbpf) | | libbpfgo-static-test | 'go test' with static libbpfgo | | selftest-static | build tests with static libbpfgo | | selftest-static-run | run tests using static libbpfgo | * examples $ make libbpfgo-static => libbpfgo statically linked with libbpf $ make -C selftest/perfbuffers => single selftest build (static libbpf) $ make -C selftest/perfbuffers run-dynamic => single selftest run (dynamic libbpf) $ make selftest-static-run => will build & run all static selftests > Note 01: dynamic builds need your OS to have a *recent enough* libbpf package (and its headers) installed. Sometimes, recent features might require the use of backported OS packages in order for your OS to contain latest *libbpf* features (sometimes required by libbpfgo). > Note 02: static builds need `git submodule init` first. Make sure to sync the *libbpf* git submodule before trying to statically compile or test the *libbpfgo* repository.
All static binaries (and selftests) were being built statically against libbpf only. This change makes them full static binaries.
I'm using the following to test this change:
|
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.
just want to make sure, if someone just imports libbpfgo into a go program and |
Since this commit:
the cgo LDFLAGS stanza did not have -lbpf. I guess tracee is dealing with that on its own. Any other consumers might be doing the same. I'd say we're good (wih no regressions) but this is something we have to address (the 'go install' out of the box in a dynamic build AND allowing the static builds). What about merging this one, to unblock @grantseltzer's other commits, and we can address this in a merge on its own ? |
👍 |
Makefile: improvements for the entire tree
Currently you will find the following GNU Makefile rules: