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

support having rust_target track rust-toolchain.toml #3049

Open
sellout opened this issue Dec 9, 2024 · 3 comments
Open

support having rust_target track rust-toolchain.toml #3049

sellout opened this issue Dec 9, 2024 · 3 comments
Assignees

Comments

@sellout
Copy link

sellout commented Dec 9, 2024

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
Copy link
Contributor

pvdrz commented Dec 10, 2024

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.

@pvdrz pvdrz self-assigned this Dec 10, 2024
@sellout
Copy link
Author

sellout commented Dec 10, 2024

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
Copy link

ydirson commented Jan 6, 2025

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?

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

3 participants