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

VENDORIZE: add feature vendored #498

Conversation

mmullin-halcyon
Copy link

Add the feature vendored to turn on that feature in libbpf-sys see: libbpf/libbpf-sys#59

This feature will get libbpf-sys to do all the static compilation rather than the application developer statically compiling these libraries themselves, or relying on their distribution to provide static versions of libelf and zlib. The "static" feature still exists for those who still wish it, as compiling libelf adds significant compilation time.

Vendored was added to libbpf-sys due to some major distributions no longer shipping statically compiled versions of libelf (eg Fedora).

CHANGELOG NOTE: feature "vendored" allows libelf and zlib to be statically linked. Completely static binaries can now be compiled via the command RUSTFLAGS='-C target-feature=+crt-static' cargo build --target x86_64-unknown-linux-gnu.
CHANGELOG NOTE: Feature "vendored" will not work with musl.

Signed-off-by: Michael Mullin mmullin@halcyon.ai

Add the feature vendored to turn on that feature in libbpf-sys
see: libbpf/libbpf-sys#59

This feature will get libbpf-sys to do all the static compilation rather
than the application developer statically compiling these libraries
themselves, or relying on their distribution to provide static versions
of libelf and zlib. The "static" feature still exists for those who still
wish it, as compiling libelf adds significant compilation time.

Vendored was added to libbpf-sys due to some major distributions no longer
shipping statically compiled versions of libelf (eg Fedora).

CHANGELOG NOTE: feature "vendored" allows libelf and zlib to be statically
linked. Completely static binaries can now be compiled via the command
`RUSTFLAGS='-C target-feature=+crt-static' cargo build --target
x86_64-unknown-linux-gnu`. This feature will not work with musl.

signed-off-by: Michael Mullin <mmullin@halcyon.ai>
@danielocfb
Copy link
Collaborator

Thanks for the pull request! This is fine in isolation, but I feel like the perceived novendor / vendored overlap is confusing (admittedly that's confusing on the libbpf-sys side already, and I should have noticed that earlier). Would it make sense / be possible to do the following (in libbpf-rs):

  • rename the novendor feature to vendored-libbpf and enable it by default (not sure if we can map the positive feature to a negative one in libbpf-sys easily, though, off hand, as I think negative features are generally frowned upon)
  • rename vendored to vendored-elf-zlib and leave it disabled by default

What do you think?

@danielocfb danielocfb self-requested a review July 5, 2023 20:47
@danielocfb danielocfb self-assigned this Jul 5, 2023
@mmullin-halcyon
Copy link
Author

mmullin-halcyon commented Jul 5, 2023

* rename the `novendor` feature to `vendored-libbpf` and enable it by default (not sure if we can map the positive feature to a negative one in `libbpf-sys` easily, though, off hand, as I think negative features are generally frowned upon)

* rename `vendored` to `vendored-elf-zlib` and leave it disabled by default

Something like?

[features]
default = ["libbpf-sys/novendor"]
vendored-libbpf = [] # requires default-features = false
vendored-libbpf-full = ["libbpf-sys/vendored"] # requires default-features = false
static = ["libbpf-sys/static"] # requires default-features = false

Where:
default - means all libraries are dynamic linked and provided by filesystem (current novendor)
vendored-libbpf - means static libbpf compiled by libbpf-sys, elf/zlib are dynamic from filesystem (current default)
vendored-libbpf-full - means static libbpf/elf/zlib are all provided by libbpf-sys (proposed in this MR vendored)
static - means libbpf-sys provides static libbpf, filesystem provides static versions of elf/zlib (current static)

It's a bit annoying to need to set default-features = false. vendored-libbpf is also a placebo feature that does nothing (and isn't needed).

The weird case is actually static, as libbpf-sys would still be providing the libbpf.a static lib. There is currently no way to get a static libbpf.a library external to libbpf-sys.

I don't particularly like this due to the default-features=false, I'd rather stick to default, novendor, static, and vendored. vendored could be renamed to vendored-libbpf-deps to remove confusion about vendored vs novendor.

I understand why you would want "all external dynamic libs" to be the default though, as it's more logical to start from zero and add, rather than start from something and remove. it's sort of like counting 0,1,2,3 rather than -1,0,1,2.

We could ask @alexforster if he would be amenable to changing libbpf-sys' flags around such that
default : no compilation, libbpf/elf/zlib dynamic libraries from filesystem (ie current novendor)
vendored : libbpf static compiled by libbpf-sys, elf/zlib dynamic from filesystem (ie current default)
vendored-static : libbpf static compiled by libbpf-sys, elf/zlib static from filesystem (ie current static)
vendored-full : libbpf/elf/zlib all compiled by libbpf-sys static (ie current vendored)
static : no compilation libbpf/elf/zlib all static libraries from the filesystem (new feature).

afxdp, rebpf, and xsk-rs all seem to be dead projects, so I don't think there would be too much problem with this

I am prepared to do this work, Alex would need to give approval because of the other (possibly dead) projects

libbpf-rs could then mimic the libbpf-sys features.

@danielocfb
Copy link
Collaborator

danielocfb commented Jul 5, 2023

Where:
default - means all libraries are dynamic linked and provided by filesystem (current novendor)
vendored-libbpf - means static libbpf compiled by libbpf-sys, elf/zlib are dynamic from filesystem (current default)
vendored-libbpf-full - means static libbpf/elf/zlib are all provided by libbpf-sys (proposed in this MR vendored)
static - means libbpf-sys provides static libbpf, filesystem provides static versions of elf/zlib (current static)

Yes, something like that.

It's a bit annoying to need to set default-features = false. vendored-libbpf is also a placebo feature that does nothing (and isn't needed).

Indeed, that is unfortunate and not nice. I was trying to keep changes to libbpf-rs for the time being, but I can see that that may not be possible if we want to do it cleanly.

I understand why you would want "all external dynamic libs" to be the default though, as it's more logical to start from zero and add, rather than start from something and remove. it's sort of like counting 0,1,2,3 rather than -1,0,1,2.

I was honestly just trying to preserve the existing behavior, at least how I understand it.

default : no compilation, libbpf/elf/zlib dynamic libraries from filesystem (ie current novendor)
vendored : libbpf static compiled by libbpf-sys, elf/zlib dynamic from filesystem (ie current default)
vendored-static : libbpf static compiled by libbpf-sys, elf/zlib static from filesystem (ie current static)
vendored-full : libbpf/elf/zlib all compiled by libbpf-sys static (ie current vendored)
static : no compilation libbpf/elf/zlib all static libraries from the filesystem (new feature).

In my opinion vendoring and linking are completely orthogonal. Conceptually we can vendor a library and then link it statically or dynamically or we can link it statically or dynamically from the system. In practice dynamic linking when vendoring may not be that practical due to tooling issues (search path and whatever other complications there may be), though I don't believe there is anything inherently undesirable or impossible about it.

With that in mind, in my opinion, and given the logic we already have, I'd envision:

[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 = []

This way, in my opinion there is very little confusion and everything is selectable without restriction. There would also be no negation. Why would anybody link elf statically but not zlib? Dunno. Not feeling strongly about the granularity, but to me we are already having a separation between what we do for libbpf vs. elf & zlib. So this proposal seems like a logical succession.

afxdp, rebpf, and xsk-rs all seem to be dead projects, so I don't think there would be too much problem with this

I don't believe this would necessarily be breaking. We could conceivably keep the existing features around and just deprecate them (that may be most appropriate for a 1.0 lib). That would complicate the logic somewhat, but we could have one build time check for mutual exclusion of the two feature sets (let's call the current set of features v1 and the new addition v2 features).

One more thing (very general comment): I don't know if we necessarily need to plumb all those features through libbpf-rs as well (though I am not objecting to it). From what I understand it is common practice to honor environment variables that could be set for all kinds of build stuff. E.g., https://github.com/kornelski/rust-lcms2-sys/blob/db7ead3536f062f2a1057da48cd256ac2181253c/src/build.rs#L22

Also, yet more generally: I found https://kornel.ski/rust-sys-crate to be a good summary of good practices in this realm.

Anyway, let's wait for @alexforster's take.

Edit:

I am prepared to do this work, Alex would need to give approval because of the other (possibly dead) projects

👍

@anakryiko
Copy link
Member

With that in mind, in my opinion, and given the logic we already have, I'd envision:

[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 = []

This way, in my opinion there is very little confusion and everything is selectable without restriction. There would also be no negation. Why would anybody link elf statically but not zlib? Dunno. Not feeling strongly about the granularity, but to me we are already having a separation between what we do for libbpf vs. elf & zlib. So this proposal seems like a logical succession.

Does "vendoring" mean providing as part of Rust library? I'm just wondering what's the point of vendoring, if one doesn't link against it? I might be missing something.

But for the above, I think it's an overkill in terms of all the options available to the users. While theoretically there might be some combination of statically linking one subset of libraries while dynamically linking another, I think in practice it's more all-or-nothing: one either goes for dynamic linking and then libelf, libz, and libbpf should be .so and linked dynamically, or you go all-in on static linking and would like libelf.a+libz.a+libbpf.a all statically linked to not have to rely on global environment providing them (and at good versions).

Also, if I was a user of libbpf/libbpf-rs. I wouldn't want to care all that much that there is elf and zlib (and maybe some other one in the future) dependencies to libbpf. I'd like to say "just please statically link all the needed stuff so I don't have to care or think about this".

So I wonder if maybe we should just provide 2 or 3 options: no-vendoring (all dynamically linked), all vendoring (libbpf+libz+libelf all vendored and statically linked), and maybe for backwards compat/compile time saving, libbpf-only-vendoring (current default, as far as I understand). Naming are completely for demo purposes, of course.

In short, too many options and knobs might actually hurt :) Especially for Rust folks that are not that familiar with horrors of C/C++ world of linking hell.

@mmullin-halcyon
Copy link
Author

I am in favour of a simplistic "we provide no libraries, get shared libs from your distribution" vs "we provide everything statically linked"

The super easy solution is:
libbpf-rs' default config can remain the same (which means libbpf-sys still provides a libbpf.a... I don't think that this is a big deal), and the "static" libbpf-rs configuration can point to libbpf-sys' "vendored" configuration. "novendor" configuration can still exist for those who want it (I think novendor is needed for alpine linux).

The proper solution is:
libbpf-sys changes the default to it's current "novendor", and has a single config option for "static" which statically vendorizes all three dependent libraries. libbpf-rs then matches libbpf-sys' configuration.

@danielocfb
Copy link
Collaborator

The proper solution is:
libbpf-sys changes the default to it's current "novendor", and has a single config option for "static" which statically vendorizes all three dependent libraries. libbpf-rs then matches libbpf-sys' configuration.

I'd be okay with that.

@danielocfb
Copy link
Collaborator

I'd be okay with that.

Sorry, I meant to quote both the "super easy solution" and the "proper solution". I am fine with both, as long as we don't have novendor and vendored exposed in libbpf-rs.

@alexforster
Copy link
Member

alexforster commented Jul 8, 2023

Haven't read this whole thread but in general I'm happy to go along with whatever y'all decide. One caveat: we do need to keep the existing functionality of the novendor and static features in libbpf-sys, even if they just turn into aliases for other features. The vendored feature is new enough that we can probably get away with renaming/removing it if needed.

@danielocfb
Copy link
Collaborator

@mmullin-halcyon are you still waiting on input on this PR?

@mmullin-halcyon
Copy link
Author

@mmullin-halcyon are you still waiting on input on this PR?

No. I simply had a busy weekend and haven't had time to dedicate. I will be on this tonight. Sorry.

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.

4 participants