Overhaul vendoring/linking features #74

danielocfb · 2023-12-06T17:21:12Z

Please see #64 for a discussion and motivation and check individual commits for change descriptions.

This reverts commit ef6ca89.

The make_libelf() and make_zlib() function are not used unless the "vendored" feature is enabled, but there is little benefit in not even defining them in that case. On the contrary: it means that we may miss out on warnings or clippy suggestions if the corresponding feature is not enabled. Let's just use a runtime feature check to guard invocation, but have the code be present unconditionally. Signed-off-by: Daniel Müller <deso@posteo.net>

Address some issues reported by clippy. Signed-off-by: Daniel Müller <deso@posteo.net>

Similar to what we do for building the other libraries, factor out a function, make_libbpf(), dedicated to building libbpf. Signed-off-by: Daniel Müller <deso@posteo.net>

With an upcoming change the novendor feature will be deprecated and should no longer be used. It will be replaced by building the library without any features. To express that in CI, switch from having a "features" argument to a more general "args", which will simply hold --no-default-features for the case where no features are desired (but for now no semantic change is made). Signed-off-by: Daniel Müller <deso@posteo.net>

We probably should suppress the output of the commands we run when we check for their existence, as it's vastly confusing to see nonsense such as ``` autoreconf: error: 'configure.ac' is required make: *** No targets specified and no makefile found. Stop. Please specify at least one package name on the command line. autopoint: *** Missing configure.in or configure.ac, please cd to your package first. autopoint: *** Stop. <stdin>:1: premature EOF bison: missing operand Try 'bison --help' for more information. Usage: gawk [POSIX or GNU style options] -f progfile [--] file ... Usage: gawk [POSIX or GNU style options] [--] 'program' file ... POSIX options: GNU long options: (standard) -f progfile --file=progfile -F fs --field-separator=fs -v var=val --assign=var=val [...] ``` show up on virtually every build script failure. Redirect stdout & stderr to /dev/null to prevent this noise from showing up. Signed-off-by: Daniel Müller <deso@posteo.net>

danielocfb · 2023-12-06T17:29:05Z

Uhm, let me figure out the CI stuff.

We need to re-run the build script when the LD_LIBRARY_PATH environment variable changes, or we may end up with a build not picking up any of the libraries correctly. Signed-off-by: Daniel Müller <deso@posteo.net>

This change overhauls the vendoring & static linking features that the library exposes. Please refer to the larger discussion [0] for additional context, but in short: we introduce separate features for vendoring/linking each of the three system library dependencies: libbpf, libelf, zlib. "static" and "vendored" meta-features are still available, which apply to all three libraries in unison. The remaining dependencies are expressed declaratively via dependent features. E.g., because zlib is only a dependency of libbpf (and not a direct one), linking it statically implies linking libbpf statically. In the future, this design would make it possible to enable additional configurations. For example, currently vendoring any library implies linking it statically, because we only build the static version. This is more of a simplification than a strict requirement and if needed, we could support dynamic linking when using a vendored copy. We could alternatively move to a model where we err out on combinations that make little sense/are risky/whatever. Doing so could be beneficial if we ever were to loosen some of those dependencies down the line to minimize chance of breakage. I am unsure how likely that really is or whether it would be cause for concern, and I generally like the declarative nature of "this feature depends on this other" in Cargo.toml, but we could change that if we feel strongly. The default features mirror the previous default and no behavior change should occur. The existing novendor feature is kept but deprecated and should be removed in the future (a warning will be printed as part of the build). It was certainly one of the main causes of confusion where novendor and vendored could co-exist and it was hard to understand what would or wouldn't happen. In the new world, users are advised to simply build with all features disabled. I tested everything on a binary depending on libbpf-sys with various features enabled and spot checked the expected dynamic library dependencies. We could enshrine that as a CI step, but given that this logic is expected to change infrequently I didn't go down that road. [0] libbpf#64 (comment) Closes: libbpf#70 Signed-off-by: Daniel Müller <deso@posteo.net>

danielocfb · 2023-12-06T19:45:37Z

@mmullin-halcyon I would think that in combination with Alex' suggestion these changes should be able to support your use case, but I'd recommend you give them a try (and/or review).

mmullin-halcyon · 2023-12-06T19:49:10Z

I will look at this ASAP (probably tomorrow evening).

danielocfb · 2023-12-06T20:02:32Z

One more meta-comment: the current proposal enables dependent features as necessary. E.g., linking libelf statically enables static linking of libbpf. We could alternatively move to a model where we err out on combinations that make little sense/are risky/whatever. Doing so could be beneficial if we ever were to loosen some of those dependencies down the line to minimize chance of breakage. I am unsure how likely that really is or whether it would be cause for concern, and I generally like the declarative nature of "this feature depends on this other" in Cargo.toml. But we could change that if we feel strongly.

mmullin-halcyon · 2023-12-08T00:28:58Z

I am having a strange problem.

using
https://github.com/mmullin-halcyon/libbpf-rs/tree/origin/story/vendorize_dependencies (and changing a few flags)
and this feature-overhaul branch. I am enabling both "static" and "vendored" (see bottom of comment)

I compile with the following command

RUSTFLAGS='-C target-feature=+crt-static' \
		cargo build --target x86_64-unknown-linux-gnu -r --features "vendored"

note: vendored for my binary is turning on static and vendored on libbpf-sys

My end binary is no longer static against libc.

ldd target/x86_64-unknown-linux-gnu/release/REDACTED
	linux-vdso.so.1 (0x00007ffe661bf000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f3fbee46000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f3fbed65000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f3fbe61e000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f3fbee81000)

From the log of libbpf-sys

Using feature vendored-libbpf=true
Using feature vendored-libelf=true
Using feature vendored-zlib=true
Using feature static-libbpf=true
Using feature static-libelf=true
Using feature static-zlib=true

I can't see anywhere in the code which should cause this problem. Please give me some more time before submitting to sus-out what is going on.

mmullin-halcyon · 2023-12-08T00:48:55Z

OK, all clear. TL;DR something on my end was amiss.

Not sure what I did, but I did some cargo update -p libbpf-sys; cargo update -p libbpf-rs; cargo clean and deleted items from my ./cargo/ area, and now things are fully static once again using these changes

Now let me actually review the code :)

mmullin-halcyon

I did not test on AARCH64, but I suspect that the libelf static compile kludge on that platform is still needed.

build.rs

mmullin-halcyon · 2023-12-08T18:45:45Z

LGTM

src/lib.rs

alexforster · 2023-12-10T21:36:56Z

.github/workflows/ci.yml

I'm not fluent in Github CI – does the "matrix" now test all the various feature combinations?

It's only an adjustment of the combinations we already build. That's what I tried to convey with

I tested everything on a binary depending on libbpf-sys with various
features enabled and spot checked the expected dynamic library
dependencies. We could enshrine that as a CI step, but given that this
logic is expected to change infrequently I didn't go down that road.

Do let me know if you want to have all possible combinations tested in CI and to what degree.

It's okay, I was just trying to make sure I understood what was changing.

alexforster · 2023-12-10T21:37:13Z

This is awesome! Left some questions/comments.

We could alternatively move to a model where we err out on combinations that make little sense/are risky/whatever.
I generally like the declarative nature of "this feature depends on this other" in Cargo.toml.

I like it too, I'm fine with it the way it is.

alexforster · 2023-12-11T18:30:28Z

I'm happy if y'all are happy. Ready to merge?

danielocfb · 2023-12-11T18:31:50Z

I'm happy if y'all are happy. Ready to merge?

Ready from my side, thanks!

danielocfb · 2023-12-27T17:20:56Z

Hi @alexforster, would you be able to cut a release containing the changes and the libbpf bump?

d-e-s-o added 6 commits November 29, 2023 14:20

Revert "Make novendor feature the default"

8514b6e

This reverts commit ef6ca89.

Address clippy reported issues

222db03

Address some issues reported by clippy. Signed-off-by: Daniel Müller <deso@posteo.net>

Factor out make_libbpf() helper function

8c2a9d9

Similar to what we do for building the other libraries, factor out a function, make_libbpf(), dedicated to building libbpf. Signed-off-by: Daniel Müller <deso@posteo.net>

danielocfb marked this pull request as draft December 6, 2023 17:29

d-e-s-o added 2 commits December 6, 2023 09:44

Re-run build script when LD_LIBRARY_PATH changes

627c2ac

We need to re-run the build script when the LD_LIBRARY_PATH environment variable changes, or we may end up with a build not picking up any of the libraries correctly. Signed-off-by: Daniel Müller <deso@posteo.net>

danielocfb force-pushed the topic/feature-overhaul branch 2 times, most recently from 8e159d6 to 095e7f9 Compare December 6, 2023 19:42

danielocfb marked this pull request as ready for review December 6, 2023 19:43

danielocfb mentioned this pull request Dec 6, 2023

Possibility to build/link with non-system & non-vendored libbpf? #70

Closed

danielocfb force-pushed the topic/feature-overhaul branch from 095e7f9 to c4bc391 Compare December 6, 2023 20:12

mmullin-halcyon suggested changes Dec 8, 2023

View reviewed changes

build.rs Outdated Show resolved Hide resolved

danielocfb force-pushed the topic/feature-overhaul branch from c4bc391 to 97a32ea Compare December 8, 2023 18:38

alexforster added the enhancement New feature or request label Dec 10, 2023

alexforster self-assigned this Dec 10, 2023

alexforster reviewed Dec 10, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

alexforster reviewed Dec 10, 2023

View reviewed changes

danielocfb force-pushed the topic/feature-overhaul branch from 97a32ea to eac7ce4 Compare December 11, 2023 17:06

alexforster merged commit 5282b15 into libbpf:master Dec 11, 2023
8 checks passed

danielocfb deleted the topic/feature-overhaul branch December 11, 2023 18:33

danielocfb mentioned this pull request Jan 24, 2024

Update libbpf (C) in libbpf-{rs,sys} cargo crate to 1.3.0 libbpf/libbpf-rs#654

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul vendoring/linking features #74

Overhaul vendoring/linking features #74

danielocfb commented Dec 6, 2023

danielocfb commented Dec 6, 2023

danielocfb commented Dec 6, 2023

mmullin-halcyon commented Dec 6, 2023

danielocfb commented Dec 6, 2023

mmullin-halcyon commented Dec 8, 2023

mmullin-halcyon commented Dec 8, 2023 •

edited

Loading

mmullin-halcyon left a comment

mmullin-halcyon commented Dec 8, 2023

alexforster Dec 10, 2023

danielocfb Dec 11, 2023

alexforster Dec 11, 2023

alexforster commented Dec 10, 2023

alexforster commented Dec 11, 2023

danielocfb commented Dec 11, 2023

danielocfb commented Dec 27, 2023

Overhaul vendoring/linking features #74

Overhaul vendoring/linking features #74

Conversation

danielocfb commented Dec 6, 2023

danielocfb commented Dec 6, 2023

danielocfb commented Dec 6, 2023

mmullin-halcyon commented Dec 6, 2023

danielocfb commented Dec 6, 2023

mmullin-halcyon commented Dec 8, 2023

mmullin-halcyon commented Dec 8, 2023 • edited Loading

mmullin-halcyon left a comment

Choose a reason for hiding this comment

mmullin-halcyon commented Dec 8, 2023

alexforster Dec 10, 2023

Choose a reason for hiding this comment

danielocfb Dec 11, 2023

Choose a reason for hiding this comment

alexforster Dec 11, 2023

Choose a reason for hiding this comment

alexforster commented Dec 10, 2023

alexforster commented Dec 11, 2023

danielocfb commented Dec 11, 2023

danielocfb commented Dec 27, 2023

mmullin-halcyon commented Dec 8, 2023 •

edited

Loading