Make novendor feature the default

[mmullin-halcyon]

Make novendor feature the default
Merge vendored feature into static

There is now only two settings

• default (formerly novendor, all libs are dynamic)
• static (formerly vendored, all libs are static)

Side Effect: Clients of libbpf-sys will now compile zlib and elfutils as part of the libbpf-sys project. This has the benefit of ensuring that the libraries are well suited for the rust bindings, but takes extra time during compilation

[mmullin-halcyon]

I was hoping the .github/workflow/ci.yml would cause a CI run. I do not know how to run these myself.

[danielocfb]

I was hoping the .github/workflow/ci.yml would cause a CI run. I do not know how to run these myself.

I'd think we should enable support for running pull requests, not just pushes, @alexforster ?

[danielocfb]

I am fine with this change, though I am somewhat worried about the backwards compat story (but will leave the call to Alex). But the provided interface should be sufficient for libbpf-rs's intents and purposes (I hope).

---

That being said, I still firmly believe that conceptually (read: outside of my libbpf-rs tinted view) this crate should not make any policy decisions such as vendoring one library implies vendoring another or vendoring one library implies static linking. As such,

[features]
default = ['vendored-libbpf'] # current behavior, we can keep it or change it; just a proposal
vendored = ['vendored-libbpf', 'vendored-elf', 'vendored-zlib']
vendored-libbpf = []
vendored-elf = []
vendored-zlib = []
static = ['static-libbpf', 'static-elf', 'static-zlib']
static-libbpf = []
static-elf = []
static-zlib = []

is the right approach, in my opinion (earlier discussion). This is in line with libbpf-sys not building any opinionated abstractions on top of what the C library provides.

(I won't push for that as I don't use libbpf-sys in anything but libbpf-rs, and as I explained above, for libbpf-rs the proposed interface should suffice)

[alexforster]

History of novendor is here: #18, #47

I don't think it's used much, but it is used, so I can't just remove it. But, if I'm understanding what you're doing here, it seems like maybe we can just keep it and make it a no-op?

[alexforster]

[features]
default = ['vendored-libbpf'] # current behavior, we can keep it or change it; just a proposal
vendored = ['vendored-libbpf', 'vendored-elf', 'vendored-zlib']
vendored-libbpf = []
vendored-elf = []
vendored-zlib = []
static = ['static-libbpf', 'static-elf', 'static-zlib']
static-libbpf = []
static-elf = []
static-zlib = []

is the right approach, in my opinion (earlier discussion). This is in line with libbpf-sys not building any opinionated abstractions on top of what the C library provides.

I do like & agree with this, but I'm not going to have time to do the work myself and I don't feel strongly enough to insist that it be done like this.

Though (again, if I'm understanding this PR correctly) it seems like we could merge this PR and come back later to add the more granular control without breaking backward compatibility. Do you agree @mmullin-halcyon?

[mmullin-halcyon]

I'm wondering if there is a theoretical problem with mixing and matching vendored libraries with non-vendored libraries.

EG. without any of these changes, could a binary compiled with libbpf.a but with libelf.so and libz.so have issues if libelf.so has a breaking update shipped by a distribution provider?

Or if we mix&match vendorized deps, could we have a binary compiled with libbpf.a+libelf.a but libz.so have issues if libz.so has a breaking update?

Is this risk higher or lower than a binary which is completely non-vendored having troubles if libbpf.so ever updates?

[alexforster]

if we mix&match vendorized deps, could we have...

Oh yeah, I bet there are all sorts of ways a user might get in trouble by trying to mix these. But my general philosophy here is: have sane defaults, and allow users an escape hatch if they need it. I don't want to presume I know all the reasons someone might want to statically compile libelf and libbpf but not libz, or whatever.

[danielocfb]

Thanks for sharing your opinion, @alexforster. Yes, I agree we could add the more elaborate features later. So as per my understanding the only change for this PR to be ready is to re-introduce the novendor feature and make it a no-op, correct?

[mmullin-halcyon]

@alexforster Any comments?

[danielocfb]

@alexforster Any comments?

@alexforster can this pull request be moved forward?

[anakryiko]

It seems like all the people involved are fine with this, including @alexforster. So I'm going ahead and merging this, as it's been pending for a while. Until we cut a release on crates.io this is easily revertable, if necessary. @alexforster please yell if you disagree.

[alexforster]

Sorry, thanks for stepping in. I'm still somewhat concerned that we're going to break people by changing the default. I'm hesitant to veto because of that though, because the vast majority of users of this crate only pull it in transitively. Is there any way we can proactively get the ecosystem ready before publishing a release?

[mmullin-halcyon]

@alexforster The only thing I can suggest is that this change requires a Major semantic version update. Major semantic versioning should protect all consumers who are using libbpf-sys = "1" or 1.2 etc.

... I think... I've been doing rust for 5 years now and I still get confused on the semantic versioning stuff.

The issue is that some consumers may have an expectation of "backported bug/security fixes" to a 1.x branch for 3 to 6 months or so. I think most developers using bpf understand that it is a fast moving technology and they would understand a statement of "version 1.x is deprecated, please upgrade to 2.x for further bug fixes"

[alexforster]

Yeah, I know we could also semver bump, but I reeeally like that we currently match major and minor semver with libbpf itself, and only deviate in patch. (A completely OCD thing, I know :)

[anakryiko]

@alexforster I do agree that matching major versions between libbpf-sys and libbpf are good for minimizing confusion.

As for breakage. It seems like libbpf-rs is the biggest consumer of libbpf-sys, and @danielocfb is saying that he'll be able to accommodate this change just fine.

There are a bunch of other dependencies on crates.io (https://crates.io/crates/libbpf-sys/reverse_dependencies), and comparing to libbpf-rs, they are much smaller in terms of downloads. So I think majority of libbpf-sys uses are going through libbpf-rs, and should have much smaller volume. So maybe this change won't cause much churn?

Separately, to minimize the need for customer to do any changes, maybe we should just leave vendored feature, but make it just an alias for static.

If neither of the above works, what should we do to unblock libbpf-sys release?

[danielocfb]

So thinking about this a bit more...it's fine that libbpf-rs can release a new version that updates the minimum libbpf-sys. HOWEVER, that will not address previous minor release. And it appears those will now be broken:

--- Cargo.toml
+++ Cargo.toml
@@ -9,3 +9,6 @@ members = [
   "examples/tproxy",
 ]
 resolver = "2"
+
+[patch.crates-io]
+libbpf-sys = { git = "https://github.com/libbpf/libbpf-sys.git", rev = "c043865bc069c886b5ac1f87a62d2c2a48f3ce70" }

error[E0425]: cannot find value `API_HEADERS` in crate `libbpf_sys`
  --> libbpf-cargo/src/build.rs:62:45
   |
62 |     for (filename, contents) in libbpf_sys::API_HEADERS.iter() {
   |                                             ^^^^^^^^^^^ not found in `libbpf_sys`
   |
note: found an item that was configured out
  --> /home/muellerd/.cargo/git/checkouts/libbpf-sys-c1a3c506327d30bf/c04386/src/lib.rs:20:11
   |
20 | pub const API_HEADERS: [(&'static str, &'static str); 10] = [
   |           ^^^^^^^^^^^
   = note: the item is gated behind the `static` feature

For more information about this error, try `rustc --explain E0425`.

I'd say...let's just cut a new major version. It's really not worth it dealing with this fallout. That's exactly what semantic versioning is about and supposed to help with.

[danielocfb]

I'd say...let's just cut a new major version. It's really not worth it dealing with this fallout. That's exactly what semantic versioning is about and supposed to help with.

Though actually that could also be problematic. Similar to what happened as part of the libc-calypse (0.1 -> 0.2 upgrade), if a program has libbpf-sys twice in its dependency tree, I think the build will break if only one is upgraded, because you can't link against two different version of the same system lib (but I think that's what would be implied). That's much more hypothetical here than it is for libc, though, because this entire scenario is less likely by virtue of libbpf-sys being a much less commonly used crate.

[alexforster]

I mean, with the caveat that I forget why we even needed to change the default vendoring strategy... can we just not? I love the new more granular features, but I'm not quite sure why those mean we need to break semver.

I personally have codebases that will be broken by this change, and I also personally directly depend on both libbpf-sys and libbpf-rs (I needed to break out of the abstraction for some reason). Maintainer-me feels uncomfortable releasing a change that will break user-me.

[danielocfb]

I mean, with the caveat that I forget why we even needed to change the default vendoring strategy... can we just not?

I suppose @mmullin-halcyon 's reasoning is explained a bit over in libbpf-rs. But he can weigh in here as well. Basically, the default-features = false seems to be the sticking point for changing defaults.

It seems as if we can preserve existing behavior and, based on what we encountered so far and @alexforster 's stance, I am fine with it.

That being said -- and I know it's shifting the goal post a bit -- but I discussed this more with @anakryiko and not to put words in his mouth but I think we are both confused by what is changing and to what. Not that we don't understand after reading commit messages, code and comments, just that it's non obvious and needs to be looked up all the time. So in our discussion we basically converged back on my earlier proposal, which makes it very explicit what is going on:

[features]
default = ['vendored-libbpf'] # current behavior, we can keep it or change it; just a proposal
vendored = ['vendored-libbpf', 'vendored-elf', 'vendored-zlib']
vendored-libbpf = []
vendored-elf = []
vendored-zlib = []
static = ['static-libbpf', 'static-elf', 'static-zlib']
static-libbpf = []
static-elf = []
static-zlib = []

So here is my proposal: while we are reworking these features let's do it as above. @alexforster seemed fine with it and @anakryiko expressed his support as well. I'd be happy to implement it.
We'd configure things so that the current default behavior is preserved (which I believe is what I laid out but will double check). So we would solely change feature names, but not defaults. I'll have to think a bit more about the novendor case, I suppose and if everything pans out in the end. I understand that's not exactly what @mmullin-halcyon created this PR for, but would it be a path forward for you nevertheless (i.e., would that work for you)?

[alexforster]

I'm on board with that.

[masmullin2000]

Goals:
My primary goal with all of this work has been to enable a fully statically compiled final binary to be buildable on any gnu-based distribution (musl+libbpf is a separate can of worms).

Unfortunately for my goal, Fedora no longer distributes libelf as a static library; thus it must be compiled sometime during the development process. This can be done either as a setup process where the developer manually downloads libelf, and compiles it by hand, or it could be done by libbpf-sys and allow developers to not worry about the libelf compilation process (which isn't fun).

Though it imposes a maintenance cost upon the libbpf-sys project, I expect that many other users of the libbpf-rs/libbpf-sys environment desire to have a static final application without the difficulties of manually installing static libelf. The competing project Aya explains that creating a fully stand alone binary is one of their important features (https://github.com/aya-rs/aya).

The addition of zlib into libbpf-sys is somewhat gold plating. I believe that the major distributions still supply a static version of this library in their package manager, but there is the risk that distributions might not continue to do so, similar to how Fedora stopped shipping static libelf.

My secondary goal is to enable speedy compiles for non-release builds and speedy cargo checks. Unfortunately for this goal, the compilation of libelf is quite slow (zlib and libbpf compilation time is negligible), so it's important to me to have compilation of libelf be enabled/disabled by a feature flag without having to poke around in the Cargo.toml of my project.

Thoughts:
So long as I can enable a static application for release mode, and have a separate compile process which does not compile libelf, I'm happy. I would really like to not need to edit my Cargo.toml to switch between these two goals.

I think that separating out libelf from zlib is more effort than it's worth, and I don't really understand the reason why libbpf should ever use a vendored .so, but I admit that this might simply be my lack of imagination.

[alexforster]

Can you use this to choose flags based on debug/release?

rust-lang/cargo#2328