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

bindgen 0.70 uses offset_of on Rust 1.76 #2960

Closed
juntyr opened this issue Oct 20, 2024 · 9 comments
Closed

bindgen 0.70 uses offset_of on Rust 1.76 #2960

juntyr opened this issue Oct 20, 2024 · 9 comments

Comments

@juntyr
Copy link

juntyr commented Oct 20, 2024

My weekly CI on my MSRV 1.76 started failing because a dependency bumped bindgen to 1.70, which now includes offset_of! in the bindings despite being still unstable. It looks like #2787 introduced the extra checks, though at a quick glance that PR looks fine.

juntyr added a commit to juntyr/numcodecs-rs that referenced this issue Oct 20, 2024
@kulp
Copy link
Member

kulp commented Oct 23, 2024

@juntyr,

(I think you mean bindgen 0.70 and not 1.70.)

In the PR you linked, I think #2787 (comment) might be helpful:

The MSRV of the generated code is configurable with Builder::rust_target. If using Bindgen 0.70 and Rust < 1.77.0 then the relevant target will need to be set.

@juntyr juntyr changed the title bindgen 1.70 uses offset_of on Rust 1.76 bindgen 0.70 uses offset_of on Rust 1.76 Oct 23, 2024
@juntyr
Copy link
Author

juntyr commented Oct 23, 2024

(I think you mean bindgen 0.70 and not 1.70.)

Whoops :)

The MSRV of the generated code is configurable with Builder::rust_target. If using Bindgen 0.70 and Rust < 1.77.0 then the relevant target will need to be set.

How do I set the target when bindgen runs in the build script of a dependency I don’t control?

@pvdrz
Copy link
Contributor

pvdrz commented Oct 24, 2024

that's on your dependency's yard. If they want to produce bindings for a lower msrv then they have to do it themselves.

@juntyr
Copy link
Author

juntyr commented Oct 24, 2024

I think at least the CHANGELOG should be updated to mention that 0.70 contains an on-by-default MSRV bump for generated code.

I know that in the end it was my dependency’s fault for bumping bindgen in a minor release so that a simple cargo update suddenly broke my build. But it still feels like a call-out in the CHANGELOG might have prevented this.

@pvdrz
Copy link
Contributor

pvdrz commented Oct 24, 2024

@juntyr I understand your point, definitely we could include such comments in the future. However, they might be rather confusing as there are 3 different MSRVs: One to compile bindgen, one to compile bindgen-cli and one for the generated code.

That being said, if someone maintains a library with an MSRV guarantee and you're bumping dependencies without checking if your MSRV guarantees hold, it is as you said, their fault. Specially since you can use

builder.rust_target(RustTarget::Stable_1_73)

to be sure that if there's a MSRV guarantee breakage is actually a bug in bindgen.

@barakugav
Copy link

The rust-version field in bindgen Cargo.toml should be 1.77

@pvdrz
Copy link
Contributor

pvdrz commented Nov 1, 2024

The rust-version field in bindgen Cargo.toml should be 1.77

no:

$ cargo +1.70.0 build -p bindgen
   Compiling proc-macro2 v1.0.60
   Compiling glob v0.3.1
   Compiling memchr v2.5.0
   Compiling quote v1.0.28
   Compiling unicode-ident v1.0.6
   Compiling prettyplease v0.2.7
   Compiling libc v0.2.159
   Compiling cfg-if v1.0.0
   Compiling log v0.4.17
   Compiling minimal-lexical v0.2.1
   Compiling either v1.8.1
   Compiling regex-syntax v0.6.28
   Compiling libloading v0.7.4
   Compiling bindgen v0.70.1 (/home/christian/Workspace/bindgen/bindgen)
   Compiling itertools v0.13.0
   Compiling bitflags v2.2.1
   Compiling shlex v1.3.0
   Compiling rustc-hash v1.1.0
   Compiling clang-sys v1.4.0
   Compiling nom v7.1.3
   Compiling regex v1.7.1
   Compiling syn v2.0.18
   Compiling cexpr v0.6.0
    Finished dev [unoptimized + debuginfo] target(s) in 6.47s

@barakugav
Copy link

@pvdrz You dont get the error when compiling, but the generated code of bindgen uses the offset_of! macro, which was added in rust 1.77 https://doc.rust-lang.org/nightly/core/mem/macro.offset_of.html

@pvdrz
Copy link
Contributor

pvdrz commented Nov 1, 2024

Please @barakugav read the thread carefully before writing comments like these

the generated code of bindgen uses the offset_of! macro, which was added in rust 1.77

yes, this is true. This doesn't mean that bindgen requires Rust 1.77 to compile. If you want to control the target Rust version of the generated bindings you must use the rust_target method to guarantee that your bindings compile for the right Rust version.

You can use bindgen with Rust 1.70 and generate bindings that should be guaranteed to work on any Rust version from 1.33 to the latest stable and nightly,

@juntyr juntyr closed this as not planned Won't fix, can't repro, duplicate, stale Nov 22, 2024
ojeda added a commit to ojeda/linux that referenced this issue Nov 23, 2024
Each `bindgen` release may upgrade the list of Rust targets. For instance,
currently, in their master branch [1], the latest ones are:

    Nightly => {
        vectorcall_abi: #124485,
        ptr_metadata: #81513,
        layout_for_ptr: #69835,
    },
    Stable_1_77(77) => { offset_of: #106655 },
    Stable_1_73(73) => { thiscall_abi: #42202 },
    Stable_1_71(71) => { c_unwind_abi: #106075 },
    Stable_1_68(68) => { abi_efiapi: #105795 },

By default, the highest stable release in their list is used, and users
are expected to set one if they need to support older Rust versions
(e.g. see [2]).

Thus, over time, new Rust features are used by default, and at some
point, it is likely that `bindgen` will emit Rust code that requires a
Rust version higher than our minimum (or perhaps enabling an unstable
feature). Currently, there is no problem because the maximum they have,
as seen above, is Rust 1.77.0, and our current minimum is Rust 1.78.0.

Therefore, set a Rust target explicitly now to prevent going forward in
time too much and thus getting potential build failures at some point.

Since we also support a minimum `bindgen` version, and since `bindgen`
does not support passing unknown Rust target versions, we need to use
the list of our minimum `bindgen` version, rather than the latest. So,
since `bindgen` 0.65.1 had this list [3], we need to use Rust 1.68.0:

    /// Rust stable 1.64
    ///  * `core_ffi_c` ([Tracking issue](rust-lang/rust#94501))
    => Stable_1_64 => 1.64;
    /// Rust stable 1.68
    ///  * `abi_efiapi` calling convention ([Tracking issue](rust-lang/rust#65815))
    => Stable_1_68 => 1.68;
    /// Nightly rust
    ///  * `thiscall` calling convention ([Tracking issue](rust-lang/rust#42202))
    ///  * `vectorcall` calling convention (no tracking issue)
    ///  * `c_unwind` calling convention ([Tracking issue](rust-lang/rust#74990))
    => Nightly => nightly;

    ...

    /// Latest stable release of Rust
    pub const LATEST_STABLE_RUST: RustTarget = RustTarget::Stable_1_68;

Thus add the `--rust-target 1.68` parameter. Add a comment as well
explaining this.

An alternative would be to use the currently running (i.e. actual) `rustc`
and `bindgen` versions to pick a "better" Rust target version. However,
that would introduce more moving parts depending on the user setup and
is also more complex to implement.

Cc: Christian Poveda <git@pvdrz.com>
Cc: Emilio Cobos Álvarez <emilio@crisal.io>
Link: https://github.com/rust-lang/rust-bindgen/blob/21c60f473f4e824d4aa9b2b508056320d474b110/bindgen/features.rs#L97-L105 [1]
Link: rust-lang/rust-bindgen#2960 [2]
Link: https://github.com/rust-lang/rust-bindgen/blob/7d243056d335fdc4537f7bca73c06d01aae24ddc/bindgen/features.rs#L131-L150 [3]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
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

No branches or pull requests

4 participants