-
Notifications
You must be signed in to change notification settings - Fork 81
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
Add libbpf as a submodule (take 2) #168
Conversation
I tested a bunch but if folks want to experiment with the setup:
|
meson.build
Outdated
if get_option('libbpf_a') == '' | ||
message('Checking out libbpf submodule and building it...') | ||
run_command('git', 'submodule', 'update', '--init', '--recursive', check: true) | ||
run_command('make', '-C', 'libbpf/src', check: true) |
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.
Shouldn't it pass cc
and other build options (CFLGAS
, LDFLAGS
) down to make
? Also, what about build concurrency? Can we specify a reasonable -j
option by default?
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.
Doesn't cc (and the other build options) get picked up by make in the same way that meson grabs it with meson.get_compiler
? When I'm building locally if I run setup with the env CC=clang
then it builds libbpf with clang as well (as opposed to gcc which is my default).
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 we specify a reasonable -j option by default?
Sure. Is there a decent middle ground here? -j32
?
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.
Doesn't cc (and the other build options) get picked up by make in the same way that meson grabs it with
meson.get_compiler
? When I'm building locally if I run setup with theenv CC=clang
then it builds libbpf with clang as well (as opposed to gcc which is my default).
Well, depends on how the flags are specified, I think. e.g. I don't think CFLAGS is the only way to pass in compiler flags to meson. Best to explicitly set relavent enviroments as when we're calling e.g. cargo.
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 we specify a reasonable -j option by default?
Sure. Is there a decent middle ground here?
-j32
?
Add nproc
as dependency and run that to get the answer?
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.
e.g. I don't think CFLAGS is the only way to pass in compiler flags to meson
In looking at the docs, I think this is the recommended way to do this: https://mesonbuild.com/howtox.html#set-extra-compiler-and-linker-flags-from-the-outside-when-eg-building-distro-packages
I think if there are specific flags that we want to pass to either clang or gcc then we have to do that explicitly in the build file. e.g. do we want to check if the compiler is clang and pass the BPF_BASE_CFLAGS
? Maybe I'm missing something.
meson.build
Outdated
endif | ||
|
||
if libbpf_a != '' | ||
if not fs.exists(libbpf_a) |
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.
So, we end up running this once. Wouldn't it be better to have a more permanent way of detecting the default case (it can just be a bool from the above if/else block too) and run make each time?
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.
Good call. If people pull and we've update the submodule we'd want to run it again.
63b06fa
to
494e9be
Compare
This is to potentinally reduce issues with folks using different versions of libbpf at runtime. This also: - makes static linking of libbpf the default - adds steps in `meson setup` to fetch libbpf and make it
494e9be
to
499924e
Compare
LGTM.
Size diff:
|
@ptr1337 Thanks for testing! The size increase is a little sad but I guess that's expected pulling in a submodule. |
@jordalgo It seems that everything is working properly. |
This is to potentinally reduce issues with folks
using different versions of libbpf at runtime.
This also:
meson setup
to fetch libbpf and make it