Implement support for rust-version field in project metadata

[djc]

Needs a bunch more work, but I'd like some early feedback! Remaining work:

• Bikeshedding name (picked rust-version here over msrv or min-rust-version)
• Failing test for local dependency with unsatisfied MSRV
• Requirement cannot be smaller than 1.27
• Requirement cannot be smaller than initial release of edition used
• Check for run, verify and publish subcommands
• More tests (would love feedback on this)
• Handle pre-release identifiers
• Disallow semver range operators
• Feature gate it
• Add documentation for unstable feature

Minimal version of @moxian's earlier take in #7801.

cc rust-lang/rust#65262

[rust-highfive]

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

[ehuss]

I don't have strong opinions about the name. rust-version sounds reasonable to me.

As for dependency support, I think this check will need to be done elsewhere. I'm not sure if there is a perfect spot. Maybe immediately after the call to build_unit_dependencies it can walk the unit graph and check each package. Or it could walk the unit graph in Context::compile (similar to the check_collisions check). I think earlier might be better?

Other things to add to the todo list is:

• Feature-gate it.
• Add documentation (unstable.md).

I'm a bit confused why this is changing the Summary. Shouldn't it just be stored in the Package?

The RFC mentioned "semver format without range operators". Do you happen to remember why?

Unfortunately just checking a versionreq doesn't work well with pre-release identifiers like -nightly. rust-version="1.40" doesn't match those (the nightly/beta tests are failing).

I can't think of a lot more tests to add. The only thing I can think of is to add some registry packages to exercise that. It looks like you added the field to the cargo_test_support::registry::Package test structure, so I assume you intend to follow-through with that? In Package::publish it will need to add the field to Cargo.toml. Let me know if you have any questions about that.

[est31]

Requirement cannot be smaller than 1.27

Why is this the case?

[djc]

From the RFC:

Version can not be smaller than 1.27 (version in which package.rust field became a warning instead of an error).

(Not clear how much sense this makes if we end up picking a different version.)

[djc]

@ehuss reverted the changes to Summary and RegistryPackage in favor of exposing this value through the Package, as you suggested. The current version handles the local dependency case, at least. Updated the task list.

I added the feature-gating, too, but I'm currently unsure how to then enable this feature for the tests. Any help there appreciated.

[ehuss]

In the tests, you need to call .masquerade_as_nightly_cargo() on each p.cargo(…) invocation. This makes cargo believe it is on the nightly channel.

For the manifest, you need to add cargo-features = ["rust-version"] at the top of every Cargo.toml. This is the nightly-only opt-in mechanism for new fields.

[bors]

☔ The latest upstream changes (presumably #8073) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #8068) made this pull request unmergeable. Please resolve the merge conflicts.

[djc]

@ehuss this seems like a decent MVP to merge. Please have another look.

[ehuss]

Hm, the caret here might be confusing for some people, particularly those not very familiar with Cargo or semver requirements. Perhaps if the message could say something like "... it requires rustc 1.2345.0 or newer, ...".

[djc]

Fixed. This required a bit of surgery, with the result that the manifest propagates the value as a string, which gets checked for correctness (including parsing as VersionReq) when the manifest gets ingested, but is parsed again when checking in cargo_compile, where I unwrap() the error (since this is checked earlier). Is that okay?

[ehuss]

Yea, I think so. If it's a problem later, it can be changed.

[bors]

☔ The latest upstream changes (presumably #8167) made this pull request unmergeable. Please resolve the merge conflicts.

[oxalica]

Don't we need check for update sub-command?
I think we should avoid bumping dependencies to versions that break current rust-version.

[djc]

That was explicitly scoped out of RFC 2495.

[ehuss]

Ping @djc: do you think you'll be able to address the comments and rebase on master?

[ehuss]

I think for this, we don't need a specific -Z flag, so I think rust_version can be removed from CliUnstable. Just when the user passes the --ignore-rust-version flag, it should call fail_if_stable_opt in command_prelude.rs to ensure the user also passed in -Z unstable-options. Search around for some examples of using that method.

For nightly/unstable features, there are two independent opt-in mechanisms. The cargo-features = […] in Cargo.toml is for allowing unstable syntax in Cargo.toml. This is checked by the features.required(Feature::rust_version()) which you have correctly added. The command-line -Z flags are only needed for CLI extensions (things that don't appear in Cargo.toml). How those are used depend on the situation:

• If there is just a new flag, like --config, then you call config.unstable_flags.fail_if_stable_opt(...) to ensure the user also passed in -Z unstable-options to enable it.
• If there is new behavior that otherwise doesn't have any new CLI options, then we add a new -Z flag to the CliUnstable struct, and then check config.cli_unstable().my_feature_name. Unstable -Z flags are already rejected on non-nightly channels.

There is a bit of documentation on this at https://doc.crates.io/contrib/process/unstable.html and in the features module.

---

As for centralizing the argument, there are a set of arguments that are essentially the same for every "build-like" command. That is:

• bench
• build
• check
• doc
• fix
• run
• rustc
• rustdoc
• test

So we don't have to fuss with ensuring that these commands always include the right options, I was thinking it would be nice just to have an arg_build_common() method in command_prelude.rs that adds those options. It should call:

• arg_features
• arg_jobs
• arg_target_triple
• arg_target_dir
• arg_manifest_path
• arg_message_format
• arg_ignore_rust_version

It's up to you if you want to do that. Otherwise, just make sure the list above is covered by arg_ignore_rust_version. I think adding a common method would help when we add more flags in the future that need to be included in all of these commands.

[djc]

I have not grouped the build common options in this PR, I might do a follow-up to improve the situation there.

[djc]

From the RFC:

Version can not be smaller than 1.27 (version in which package.rust field became a warning instead of an error).

I don't know if/how to implement this. I think Cargo will now still error if the unstable field gets used. I could restrict it for now to not allow it to be smaller than 1.51, if that makes sense, but that version should eventually probably be adjusted to the version that stabilizes this field.

[ehuss]

Thanks!

As for the 1.27 floor, I think the intent is: let's say it gets stabilized in version 1.60. A project whose MSRV is 1.35 can specify 1.35 in the manifest to make it clear what their MSRV is. Even though older versions will ignore it, I think it makes sense to specify the actual version instead of the version when it first starts getting honored.

However, thinking about that more, I think if this gets merged now, that means that versions starting with 1.51 will never be able to use this setting, because Cargo will complain about an unstable feature. I'm concerned that this will make it very difficult to use for a long time (until a project's MSRV reaches whenever it is stabilized, which could be a year or more).

Since this feature is fundamentally about improving MSRV support, I think that may be a bad way to start it off. This key is not critical (it doesn't actually change anything). I'm thinking instead of calling unstable_features.require(...) in toml/mod.rs it could instead do something like this:

if features.require(Feature::rust_version()).is_err() {
    warnings.push(
        "`rustc-version` is not supposed on this version of Cargo and will be ignored"
            .to_string(),
    );
}

Maybe include a check to nightly_features_allowed() and add a hint about how to enable it on nightly (similar to how require() works).

I think that should be safe to do, it will just print a warning just like any other unknown key.

[djc]

Resolved your final comments and changed the error to warn (including tests for both nightly and stable).

[ehuss]

Looks good, thanks! 🎉

@bors r+

[bors]

📌 Commit c221fec has been approved by ehuss

[bors]

⌛ Testing commit c221fec with merge 8e075c9...

[djc]

Thanks for the reviews!

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 8e075c9 to master...