Fix broken novendor feature

[jirutka]

$ cargo test --features novendor

  = note: /usr/lib/gcc/x86_64-alpine-linux-musl/12.2.1/../../../../x86_64-alpine-linux-musl/bin/ld: target/debug/deps/tests-81d77a8d8e24cc4f.v49whvrqu4l5awr.rcgu.o: in function `tests::tests::test':
          tests/tests.rs:19: undefined reference to `libbpf_set_print'
          collect2: error: ld returned 1 exit status

  = help: some `extern` functions couldn't be found; some native libraries may need to be installed or have their path specified
  = note: use the `-l` flag to specify native libraries to link
  = note: use the `cargo:rustc-link-lib` directive to specify the native libraries to link with Cargo (see https://doc.rust-lang.org/cargo/reference/build-scripts.html#cargorustc-link-libkindname)

[alexforster]

Hmm. This feature was added in this commit by @danobi, and apparently broken by me in this commit when I removed bindings.c (since it was no longer necessary).

I have a few questions:

1. If this change broke novendor, then I'm confused about how Rust ever knew to link with libbpf in the first place. It looks to me like we were only using pkg_config to find the correct include path. Was the cc crate using this to infer that it should add -lbpf?
2. Before I broke novendor, were we linking statically or dynamically? Or, rather, I guess my real question is: should this be "cargo:rustc-link-lib=bpf" or "cargo:rustc-link-lib=static=bpf"? If we should be trying to statically link, then we probably also need to output "cargo:rustc-link-lib=static=z" and "cargo:rustc-link-lib=static=elf".

[jirutka]

If this change broke novendor, then I'm confused about how Rust ever knew to link with libbpf in the first place.

When novendor is disabled (default), you build the vendored libbpf and tell rustc to link it statically here.
You haven’t noticed it because it’s not covered in the CI job and apparently no one has actually used this feature until now (or just didn’t report it).
You can see the test failure here.

Before I broke novendor, were we linking statically or dynamically?

Statically.

Or, rather, I guess my real question is: should this be "cargo:rustc-link-lib=bpf" or "cargo:rustc-link-lib=static=bpf"?

The former when novendor is enabled (i.e. linking against system-provided libbpf), the latter when novendor is disabled (i.e. statically linking vendored libbpf).

If we should be trying to statically link, then we probably also need to output "cargo:rustc-link-lib=static=z" and "cargo:rustc-link-lib=static=elf".

This would require more changes than just adding static to these commands.

[alexforster]

When novendor is disabled (default), you build the vendored libbpf and tell rustc to link it statically here.

Right, I get how it works without novendor, but I'm confused about how it ever could have worked with novendor enabled.

You haven’t noticed it because it’s not covered in the CI job and apparently no one has actually used this feature until now (or just didn’t report it).

Oh right, maybe that's the answer to my confusion: this feature never worked to begin with?

Before I broke novendor, were we linking statically or dynamically?

Statically.

Well, guess if this feature was always broken, then we weren't linking at all :)

Or, rather, I guess my real question is: should this be "cargo:rustc-link-lib=bpf" or "cargo:rustc-link-lib=static=bpf"?

The former when novendor is enabled (i.e. linking against system-provided libbpf), the latter when novendor is disabled (i.e. statically linking vendored libbpf).

If we should be trying to statically link, then we probably also need to output "cargo:rustc-link-lib=static=z" and "cargo:rustc-link-lib=static=elf".

This would require more changes than just adding static to these commands.

To clarify: do we care about supporting the case where the system-provided libbpf is static? If so, and if we aren't using pkgconfig, then I think we'd have to manually link in libz and libelf, since libbpf transitively depends on them.

But, if we don't care about the case where there's a static system-provided libbpf, then this PR looks fine as-is.

[jirutka]

Oh right, maybe that's the answer to my confusion: this feature never worked to begin with?

I think so.

To clarify: do we care about supporting the case where the system-provided libbpf is static?

Ah, I understand what you mean now, I overlooked library_prefix() before. Fixed now. (EDIT: not yet, one more fix)

[jirutka]

To clarify: do we care about supporting the case where the system-provided libbpf is static?

This would actually require more changes. And I would start by rewriting it to use pkg-config. However, to be honest, I’ve already spent too much time on this.

[danobi]

I think it worked at one point. pkgs.org reports a novendor version being shipped in fedora: https://pkgs.org/search/?q=libbpf-sys

I also seem to recall dynamic linking with libbpf working at some point (at least when I developed the patch).