-
Notifications
You must be signed in to change notification settings - Fork 55
chore: Generate bindings based upon rust-version
#124
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
Conversation
|
Good catch! I'm leaning toward making bindgen generate pre-2024 compatible code instead; there's no good reason for us to require 2024 imo. I assume that's possible? I'll look into it. |
Actually, I think this is because I am not familiar with using bindgen myself, it seems that you can have it configured to generate bindings for a given MSRV?: rust-lang/rust-bindgen#3049 In that thread they later discuss using Some earlier discussion in that thread describes avoiding newer versions of Rust for the bindgen target due to issues, and then later bumping it once fixes landed for their usage. I'm not familiar enough with |
|
Yeah I'm definitely not a bindgen expert but I figured there must be a way. If you feel like figuring out how I'm happy to merge! Otherwise I'll research it. |
|
This looks fine, the build failure looks like some buggy CI. I don't mind debugging that, you've already done enough :) |
|
Ok I've followed this advice to leverage the Cargo ENV
Which should parse a string value into the expected I went with I'm not sure what the actual MSRV is for That said, the |
Thanks, I have no clue about the CI failure cause sorry 😓 Maybe it's related to my last comment about newer Rust possibly not being happy? (I think this would only be when a project uses |
rust-version to Cargo.tomlrust-version
If you are referring to this CI failure: https://github.com/libbpf/libbpf-sys/actions/runs/15938909087/job/44963986439?pr=124#step:6:2159 Then it's because the generated Rust binding file is outdated. The new version will have to be commited. |
83fa8ec to
3db71b5
Compare
|
Required for bindings change to `unsafe extern`.
Sounds fine to me. For what it's worth, |
|
Sorry for the noise. I'm currently stuck on the fact that, on my machine, running diff --git a/src/bindings.rs b/src/bindings.rs
index dea2158..504e5ac 100644
--- a/src/bindings.rs
+++ b/src/bindings.rs
@@ -7112,8 +7112,8 @@ unsafe extern "C" {
opts: *mut bpf_token_create_opts,
) -> ::std::os::raw::c_int;
}
-pub type va_list = __builtin_va_list;
pub type __gnuc_va_list = __builtin_va_list;
+pub type va_list = __builtin_va_list;
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
pub struct btf_header {Which is... confusing. I'll keep researching |
I've hit that once or twice on PRs where I actively modified |
| index % 8 | ||
| }; | ||
| let mask = 1 << bit_index; | ||
| if val { byte | mask } else { byte & !mask } |
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.
Bindgen also can't seem to decide if it wants this on one line or split across multiple. It flattened this a few commits ago, only to again explode it in this commit. Not a blocker to merge this, but odd.
|
Ah, got it. Bindgen intends to have deterministic output, but only for the same input, and that includes system C headers: rust-lang/rust-bindgen#1605 So the fix is to just make sure rebuild.sh and CI use the same distro, version, and architecture. |
|
I'll cut a release shortly – does CI work reliably now? I suppose we'll find out, but I could never seem to convince it to publish in the past. |
My last five |
TL;DR:
Cargo.toml. This benefits users and the cargo resolver going forward.editionto2021given the minimum version of rust is well beyond1.56?UPDATE: As later discussed, prior to this PR bindings were generated for the latest Rust toolchain, whereas this PR has been adjusted to lower that to permit building on Rust prior to 1.82.
Required for bindings change to
unsafe extern:unsafe extern(introduced with Rust 1.82 in Oct 2024).1.5.1release oflibbpf-sys(May 2025), so anyone using the Nov 20241.5.0release (without explicitly pinning it) has had their minimum required Rust also bumped.rust-versionhelpful at communicating this information to the resolver, users will not need to troubleshoot the build failure to identify the minimum Rust toolchain required.On Rust toolchains prior to 1.82, builds for
[email protected]would fail with the following:NOTE: Feb 2025 released Rust 1.85 which introduced the Rust 2024 edition. There's technically no need to raise the version of rust required to 1.85, but bumping the rust edition to 2024 this may be required AFAIK (and may require some additional work). Given Rust 2021 edition was from Oct 2021 with Rust 1.56.0 and the required
rust-versionto build this crate, you might as well consider bumping that though?I have not contributed a CI update, but ideally you would run tests for your minimum supported rust version to catch these sort of implicit bumps.
rust-versionis configured for a crate, the resolver should be able to fallback to earlier releases (when semver constraint is compatible).rust-versionis added toCargo.tomlhere, falling back to1.5.1as the prior release will of course still expect Rust 1.82, but if it had been configured for that release instead, the fallback to1.5.0would have worked.