support having `rust_target` track rust-toolchain.toml

[sellout]

I recently ran into an issue on a repo that has rustc pinned to 1.81 with a rust-toolchain.toml file because of a regression in 1.82.

bindgen on that repo was generating bindings for 1.83, which included output that 1.81 doesn’t understand.

Adding rust_target(RustTarget::stable(81, 0)) was easy enough once I discovered it, but once 1.85 (which fixes the regression) is stable, we’ll update rust-toolchain.toml to point at that release, at which point we’ll have to update build.rs with rust_target(RustTarget::stable(85, 0)).

It would be great if there were something like RustTarget::toolchain_toml() to indicate that it should extract the target version from that file. It feels like that should maybe even be the default (if such a file exists), but I’m happy either way.

[pvdrz]

I think it'd be great if we could always parse the right msrv from somewhere instead of having to set it up by hand.

My only concern is that there are several places where you can set the msrv. For example, rust-version inside your Cargo.toml.

I think what you propose is of having a method that does the parsing is a good addition, specially because it doesn't work automagically and we might already add toml as a dependency in the near future.

[sellout]

I think it'd be great if we could always parse the right msrv from somewhere instead of having to set it up by hand.

I was going to respond that I don’t think MSRV is the right thing, but on reflection I think it is. The general assumption is that if it works on rustc 1.n, it’ll work on 1.n+m. Not always true (as in my case), but it often holds, and so you want bindings.rs to not generate anything that wouldn’t compile with that minimum version.

My only concern is that there are several places where you can set the msrv. For example, rust-version inside your Cargo.toml.

I think of rust-toolchain.toml as complementary to MSRV. It’s more for reproducibility. If rust_target defaulted to rust-version and paid no attention to rust-toolchain.toml, I would be content.

I think what you propose is of having a method that does the parsing is a good addition, specially because it doesn't work automagically and we might already add toml as a dependency in the near future.

Yeah, I’m not opposed to my original suggestion 😅, and I can especially see applications eschew rust-version in favor of rust-toolchain.toml, but it seems like rust-version should be preferred if it’s set, and otherwise rust-toolchain.toml … and then latest stable.

And to reiterate: I’d be happy if just rust-version were supported. I think that sounds like the right place.

[ydirson]

Rustup documents a number of possible ways to set the toolchain version, it does not look right to duplicate the logic anywhere. OTOH Cargo sets env variables, among which build.rs receives the path to the compiler as RUSTC, and thus $RUSTC --version would look like a cheap and reliable way to get the target toolchain version?

[mulkieran]

I'm all in favor of using rust-version in Cargo.toml as the default rust target for bindgen also.

[mulkieran]

Since I realized rust-version value is exported as CARGO_PKG_RUST_VERSION and that can be parsed using an idiom like .rust_target(env!("CARGO_PKG_RUST_VERSION").parse().expect("valid")) I just don't think it is a big deal.

[sellout]

@mulkieran Just to clarify, you’re saying that this issue isn’t a priority, because it’s easy to specify that bindgen should track rust-version using the CARGO_PKG_RUST_VERSION environment variable?

That makes sense to me. I’m happy with that approach. I still think bindgen could do it by default, but having that snippet means I don’t have to think about it.

[mulkieran]

@mulkieran Just to clarify, you’re saying that this issue isn’t a priority, because it’s easy to specify that bindgen should track rust-version using the CARGO_PKG_RUST_VERSION environment variable?

That makes sense to me. I’m happy with that approach. I still think bindgen could do it by default, but having that snippet means I don’t have to think about it.

Exactly. I think that works well enough for my purposes since it doesn't have to be maintained independent of the rust-version value in Cargo.toml.

[reneleonhardt]

So, rust-toolchain[.toml] isn't being considered even as a fallback? Seems odd having to define it two times if the requirement is already configured...