rust-lang / rustup

The Rust toolchain installer
https://rust-lang.github.io/rustup/
Apache License 2.0
6.15k stars 886 forks source link

Consider supporting MSRV in rustup #1484

Open anp opened 6 years ago

anp commented 6 years ago

See comment below.

Original post: Currently I commit `rust-toolchain` files for most of my projects so that I and contributors have less manual work to do (check README, run commands, etc). It works well in combination with the RLS too. That said, the same way that exact semver bounds in Cargo.toml create many duplicated deps in builds, my observation of having had this habit for a while is that I end up with many more rust toolchains installed than I really need to work on all of my projects. Many of the older toolchains are pinned at a specific stable version -- not because I didn't want to support newer stable releases, but because I wanted to make sure that contributors are building with at least a particular minimum version. My instinct here is to propose that adding a `+` to the end of a toolchain specifier will allow rustup to satisfy the toolchain requirement with the provided version or any newer version from the same release channel (nightly-foo-+ will only give newer nightlies, 1.28.0+ will only give newer stables). I haven't discussed this with anyone before, so I'm very open to the idea that this proposal misses important needs and would love to hear feedback on the idea. A quick search of the issue tracker didn't turn up any previous discussions, please forgive me if I'm retreading old ground!
rofrol commented 5 years ago

This ^^^

kinnison commented 4 years ago

Hi @anp and @rofrol

I'm sorry nobody got back to you before now. Is this something you still would like?

if not, please could you close the issue. If it's still potentially useful then I'd like to understand your use cases so I can judge the best approach.

Thanks,

Daniel.

anp commented 4 years ago

Will rustup respect the minimum Rust version RFC that was recently approved? If so, I think that would address my main use case.

kinnison commented 4 years ago

I had not seen that RFC, no right now "no" but it looks very interesting. If you're sure that would make the most sense, then I think I'll retitle this issue "Consider supporting MSRV in rustup" so we can work out what, if anything, rustup can usefully do to make this possible.

illicitonion commented 1 year ago

Now that Cargo supports rust-version as an optional Cargo.toml field (since 1.56.0), and is starting to seriously consider factoring MSRV into resolution, it'd be really handy to start better supporting MSRV in rustup.

The problem I'd like to solve is: It's hard to ensure you're actually testing MSRV on CI. Currently, the best way seems to be to have a CI pipeline which hard-codes what the MSRV is, and either manually verify, or write custom automation, to verify that the CI pipeline's rust version is in sync with the crate's package.rust-version attribute. The fact that these can go out of sync is an annoying footgun.

For me, the goal here would be that CI can run cargo +msrv test and rustup will find the MSRV from the Cargo.toml file and use that as if it'd been specified on the command line.

So in this example:

[package]
rust-version = "1.54.0"
# ...

running cargo +msrv test would be the equivalent of running cargo +1.54.0 test.

A complication here is that workspaces may have different MSRV versions, and some crates in the workspace may not set an MSRV; if we take the serde repo as an example; as of commit d6de911855d1cc0ad13f87503a79d40dc4490442 it is a workspace with the following Cargo.toml files:

Probably the correct thing to do here is for rustup to look in the current directory for a Cargo.toml file, and use its rust-version if one's set (and error if you try to use +msrv without having one set), and for Cargo.toml to support setting a rust-version for a workspace using one of a few possible mechanisms:

  1. Delegating to a specific sub-crate - Cargo could allow you to say "serde is the main crate, inherit its rust-version"
  2. Selecting the minimum or maximum rust-version specified in a crate in the workspace
  3. Just explicitly specifying the MSRV if rustup happens to be running in that directory.

cc @epage who seems to be thinking about this at the moment.

kinnison commented 1 year ago

I am concerned that this mixes the concerns of Rustup and Cargo. To properly honour this, Rustup would need to understand how to parse the full workspace and to understand that field as Cargo would - this seems like it'd result in Rustup needing to use the cargo crate to achieve that; which is a huge huge new dependency for rustup.

I'm not saying this isn't possible, but I am wondering how sensible it would be to do it this way.

I'd also like to get @rbtcollins and @ehuss involved in this discussion as both are more immediately on the coal-face as it were.

epage commented 1 year ago

To expand on @kinnison's comment

For me, the goal here would be that CI can run cargo +msrv test and rustup will find the MSRV from the Cargo.toml file and use that as if it'd been specified on the command line.

Keep in mind the solution would also need to include installing the MSRV toolchain before having the proxies reference it.

rbtcollins commented 1 year ago

I think this should be a cargo feature.

Resolve the toolchain needed for msrv testing then invoke cargo +version .... Itself.

epage commented 1 year ago

Doing this in cargo has its own challenges