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

RFC: Minimum Supported Rust Version #2495

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
@newpavlov
Copy link

newpavlov commented Jul 4, 2018

This RFC adds an ability to specify Minimum Supported Rust Version (MSRV) in Cargo.toml using rust field:

[package]
name = "foo"
version = "0.1.0"
edition = "2018"
rust = "1.35"

Rendered

Internals discussion

@newpavlov newpavlov changed the title msrv RFC: Minimum Supported Rust Version Jul 4, 2018

@newpavlov newpavlov force-pushed the newpavlov:msrv branch from 02fb6cc to 4ec9682 Jul 4, 2018

@AllenChong

This comment was marked as resolved.

Copy link

AllenChong commented Jul 4, 2018

pull request

@Centril Centril added the T-cargo label Jul 4, 2018

If you are sure that your crate supports older Rust versions (e.g. by using CI
testing) you can change `rust` field accordingly. On `cargo publish` it will be
checked that crate indeed can be built with the specified version. (though this
check can be disabled with `--no-verify` option)

This comment has been minimized.

@Nemo157

Nemo157 Jul 5, 2018

Contributor

What is the intended meaning of "checked that crate indeed can be built with the specified version", use rustup to install the specified version and attempt to package with it?

It seems like this could be covered by the error listed above:

In case if you have rust="stable", but execute cargo publish with Nigthly toolcahin you will get an error.

if you have rust = "1.30" then you will only be allowed to publish when using Rust v1.30.

This comment has been minimized.

@newpavlov

newpavlov Jul 5, 2018

Author

I had in mind that cargo publish in addition to the current toolchain will build crate with MSRV toolchain with some additional checks.But you are right, if we extend behaviour described for "stable"/"nightly" (i.e. you can cargo publish only using MSRV toolchain), then describe checks will be redundant. I'll update RFC accordingly.

error. Although this check can be disabled with `--no-rust-check` option.

`rust` field should respect the following minimal requirements:
- value should be equal to "stable", "nigthly" or to a version in semver format

This comment has been minimized.

@shingtaklam1324

shingtaklam1324 Jul 5, 2018

Shouldn't this allow a full string, like the date of a nightly build? For some tools which basically is built for a specific nightly (clippy comes to mind) this can be a useful tool. As well as that, it would be useful as AFAIK 1.xx.0-nightly refers to all of the nightlys that have been built during the six weeks.


Edit: I see this is mentioned later on, but shouldn't it be a Rust version and not just any semver anyways?

Also there is a typo there with the spelling of nightly.

This comment has been minimized.

@newpavlov

newpavlov Jul 5, 2018

Author

I don't see much merit in using 1.xx.0-nightly versions instead of the simple "nightly". I think it's a reasonable assumption that nightly toolchain will stay fairly up-to-date for most of the developers, and for crates which experience frequent breakage it's better to have finer-grained control provided by nightly: 2018-01-01 approach.

Thank you for noticing the typo, I'll fix it!

`rust` field should respect the following minimal requirements:
- value should be equal to "stable", "nigthly" or to a version in semver format
("1.50" is a valid value and implies "1.50.0")
- version should not be bigger than the current stable toolchain

This comment has been minimized.

@shingtaklam1324

shingtaklam1324 Jul 5, 2018

Do you mean the current nightly toolchain? Because during the lifetime of Rust 1.xx stable, 1.xx+2 is the nightly version in use. This would then disallow the publishing to those crates to crates.io if it checks against the current stable.

This comment has been minimized.

@newpavlov

newpavlov Jul 5, 2018

Author

No, I meant stable. If we have stable Rust 1.35 and nightly 1.37, using rust="1.37" implying nightly toolchain will result in an ambiguity when stable Rust 1.37 will be published. So if crate depends on Nightly features, then it will have to use rust="nightly"/"nightly: ...".

rust-cases = [
{ cfg = "x86_64-pc-windows-gnu", version = "1.35" },
{ cfg = 'cfg(feature = "foo")', version = "1.33" },
]

This comment has been minimized.

@kennytm

kennytm Jul 5, 2018

Member

I prefer we reuse the existing [target.*] section for this:

rust = "1.30"

[target.x86_64-pc-windows-gnu]
rust = "1.35"

[target.'cfg(feature = "foo")']
rust = "1.33"

(Note that cargo should either fully support [target.'cfg(feature = "foo")'.dependencies] or outright reject it)

This comment has been minimized.

@newpavlov

newpavlov Jul 5, 2018

Author

Hm, reusing target is a good idea. I'll replace rust-case with your proposal.

As for [target.'cfg(feature = "foo")'.dependencies], while it's somewhat redundant to optional = true dependencies + foo = [".."], I think it could provide a better flexibility in specification of optional dependencies (e.g. add dependency only for given target if feature is enabled), so in my opinion it should be supported.

crate can be built. One way to achieve this is by using the following syntax:
- single version: rust = "nightly: 2018-01-01"
- enumeration: "nightly: 2018-01-01, 2018-01-15"
- (inclusive) range: "nightly: 2018-01-01..2018-01-15"

This comment has been minimized.

@kennytm

kennytm Jul 5, 2018

Member

Using .. as inclusive range in Rust is gonna be confusing. Could we incorporate the semver operators instead?

nightly: >=2018-01-01, <=2018-01-15

or

>=nightly-2018-01-01, <=nightly-2018-01-15

(every new nightly is considered a major version for sure.)

This comment has been minimized.

@newpavlov

newpavlov Jul 5, 2018

Author

I am not particularly attached to the proposed syntax (I think that exact versions ("nightly: 2018-01-01") will be used much more often than other variants), so probably using semver operators will be indeed a better choice.

Though, after some thought I've started to worry about inconsistency between how "stable" and "nightly" work. I think it will be better to make "nightly" work in the same fashion as "stable", i.e. cargo publish will select version of the currently used nightly toolchain. "nightly: *" in turn will be used to specify "all nightly Rust versions". cargo init should probably use the latter variant on nightly. What do you think?

This comment has been minimized.

@kennytm

kennytm Jul 5, 2018

Member

Sounds good to me, though it makes me think if it makes sense to mix both stable and nightly in the same line?

rust = "1.26, nightly: >=2017-01-01"

This comment has been minimized.

@newpavlov

newpavlov Jul 5, 2018

Author

Hm, any use-case examples? As I see it, if code enables nightly features it can't work on stable, and if it works on stable it should work on nightly. Well, excluding potential nightly regressions of course, which I think we can safely ignore.

UPD: Also there is a very old nightly versions possibility, but I think we can ignore such cases as well. Though from consistency point of view, such pattern could be allowed, but I am not sure if it's worth the additional complexity.

This comment has been minimized.

@kennytm

kennytm Jul 5, 2018

Member

@newpavlov num-traits uses a build script to determine whether to enable i128, which covers both stable and nightly. Though the said build script could emit an error itself meaning there's probably no real use case 🙃.

@crlf0710

This comment has been minimized.

Copy link

crlf0710 commented Jul 7, 2018

I think i like the key name be rustc better (or it will become problematic for alternative compilers like mrustc or something like that.)

I actually think the channels should not be mixed in. Since it's using semver, cargo init can just write "1" to represent all available rustc versions, right?

I also think that if a crate is nightly only. It can mark itself in an permanently unstable attribute, maybe something like nightly-rustc = ">=20180707" . And only nightly cargo can recognize and publish those crates, which is ok i think.

@newpavlov

This comment has been minimized.

Copy link
Author

newpavlov commented Jul 8, 2018

Personally I don't have a strong preference regarding the name, so it could be changed on implementation. But if we consider other compilers, then in my opinion rust fields makes more sense, as it can be interpreted as a version number of "rust specification", which currently provided in the form of the reference implementation. We could extend rust field to support experimental features of other compilers, e.g. in this form rust = "mrustc: ...".

Channels are mostly for convenience and providing sensible default option. Using "1.0" as a default value will result in too relaxed constraints on crate versions, thus making the feature significantly less useful in a long run. While the proposed channel handling will result in a too conservative constraint, at least we'll know for certain that crate can be built with a specified version.

As for nightly-rustc I don't see much benefit in introducing additional field, except making rust field pure-semver, which has arguable merit. Stable/nightly requirements are mutually exclusive, so there shouldn't be any problems in keeping them in one field. Also it will simplify explanation of this feature.

@newpavlov

This comment has been minimized.

Copy link
Author

newpavlov commented Jul 8, 2018

@aturon
Can this feature be implemented and introduced as part of the Rust 2018 release? (even in the form of "dumb fields") The proposed rust field is required, so it will be nice to make it optional only for 2015 edition.

@phaylon

This comment has been minimized.

Copy link

phaylon commented Jul 10, 2018

It might be nice if there were an env|cli|profile option to have cargo emit an error or warning if the used toolchain is newer than specified for cargo check and test. So you have the chance of catching it early if you want.

@kinggoesgaming

This comment has been minimized.

Copy link

kinggoesgaming commented Jul 10, 2018

This should also help with crates.io have a badge for it too

versions from `0.1.0` to `0.1.10`, in versions from `0.1.0` to `0.1.5` `rust`
field in the `Cargo.toml` sent to crates.io equals to "1.30" and for others to
"1.40". Now if you'll build your project with Rust 1.33 `cargo` will select
`foo v0.1.5`, and `foo v0.1.10` if you'll build your project with Rust 1.30 or

This comment has been minimized.

@Ortham

Ortham Jul 11, 2018

Shouldn't this be "foo v0.1.10 if you build your project with Rust 1.40 or later" (1.30 -> 1.40)?

This comment has been minimized.

@newpavlov

newpavlov Jul 11, 2018

Author

Yes, you are right. Thank you for noticing the typo!

On `cargo publish` cargo will take currently used Rust compiler version and
will insert it before uploading the crate. In other words localy your `Cargo.toml`
willl still have `rust="stable"`, but version sent to crates.io will have
`rust="1.30"` if you've used Rust 1.30. If "nightly: \*" is used, then `cargo`

This comment has been minimized.

@teiesti

teiesti Jul 11, 2018

I don't like this kind of automatic conversion from stable to e.g. 1.30. I think it is counter-intuitive.

Instead, I would like to propose a slightly more complicated approach with a huge effect on the question raised above (namely "Is a MSRV change a breaking change for a crate which then requires a major version bump?"). Let me give an example:

  1. A user might declare the MSRV to be 1.30. This means that the crate which is published to crates.io can be compiled by every compiler between 1.30 (inclusive) and 2 (exclusive), just as semantic versioning suggests. If he uploads the next (patch or minor) version of the same crate, the MSRV must be equal to or smaller than 1.30. Everything else would be considered a breaking change. If he uploads the next major version, of course he's free to change the MSRV.

  2. A user might also declare the MSRV to be stable. When published to crates.io, stable is converted to stable(1.30) with 1.30 being the current stable release. This means that the published artifact will work with a compiler between version 1.30 (inclusive) and 2 (exclusive). So far no difference! Now, times passes and a new stable compiler (1.31) was published. The crate artifact which was already published still works with a compiler between 1.30 (inclusive) and 2 (exclusive). But if the crate author opts to publish a new crate version, the MSRV for the new artifact is automatically raised to stable(1.31) which means that it guarantees to work with a compiler version between 1.31 (inclusive) and 2 (exclusive). This MSRV change wound not be considered a breaking change since it was already declared before.

Therefore a crate author has to options:

  1. Fix the MSRV to a specific version, e.g. 1.30.
  2. Fix the MSRV to stable which means that a future crate version might require a future compiler version (which is at least stable).

Anyway, changing the MSRV is considered to be a breaking change which requires a major version bump. (Someone might relax this requirement as I did in the example above and allow a change from e.g. stable to 1.32 or 1.31 to 1.30, since that is no compatibility problem).

I think, one could take this idea even further and allow labels like stable - 0.4 which means that, a future crate works with the current version minus 4 minor versions. I think this is even a reasonable alternative to LTS releases as proposed in #2483. (Or we could then say that lts is stable minus one year or another reasonable time considered to be a long term...)

This comment has been minimized.

@teiesti

teiesti Jul 11, 2018

By the way, this idea does not work for nightly because the nightly compiler does not guarantee backward compatibility.

This comment has been minimized.

@newpavlov

newpavlov Jul 11, 2018

Author

Probably my wording was not clear enough, but I've meant almost exactly the same behaviour as was described by you. Automatic conversion from stable to 1.30 happens if user executes cargo publish with 1.30 toolchain, in his local Cargo.toml rust field will be left unchanged, equal to stable. After that user makes some changes to the crate and updates toolchain to stable 1.33. Now on cargo publish for the next crate version stable will be converted to 1.33, which will mean >=1.33.0, <2.0.0.

For nightlies I currently propose to use nightly: * as a default option for crates which require nightly compiler, which will mean "any Nightly version".

This comment has been minimized.

@newpavlov

newpavlov Jul 11, 2018

Author

As for LTS releases, they are not only about MSRV, they will also serve as a synchronization point for crate authors and package authors (think Debian). LTS also means that crucial updates (e.g. security or soundness fixes) will be backported to those versions. So I don't think that stable - 0.4 will be able to cover all LTS use-cases.

This comment has been minimized.

@teiesti

teiesti Jul 11, 2018

Probably my wording was not clear enough, but I've meant almost exactly the same behaviour as was described by you.

To be honest, I am still not sure that we are talking about the same thing: I think, the crucial difference is that in the first case (rust = 1.30) the published MSRV on crates.io is 1.30, while in the second case (rust = stable), the published MSRV is stable(1.30). To be even clearer: 1.30 is not at all equal to stable(1.30). While they behave equal when it comes to the crate's current version, they are different for the next (patch or minor) version: In the first case the MSRV will be 1.30, while in the second case it might be 1.30 or stable(1.31) or even 1.31. Therefore, the crate user can determine from the difference between 1.30 and stable(1.30) how the crate author answers the question "Is an MSRV change a breaking change?". In the first case, the answer is yes, in the second case it is no!

I would like to see a whole passage dedicated to that matter which leaves no room for interpretation... ;-)

I would also like to see a passage dedicated to "version arithmetic" which covers e.g.:

  • Under which circumstances is a MSRV downgrade/upgrade a breaking change?
    • 1.30 can be 1.29, 1.28.1, ... in the next crate version.
    • stable(1.30) can be 1.29, 1.28.1, ... or stable(1.30.1), stable(1.31), ... or 1.30, 1.30.1, 1.31, ... in the next crate version (as long as the rustc is already stabilized).
  • What if crate A depends on crate B and crate C. What are the requirements for the MSRV of crate A given the MSRV of B and C?
    • Something like MSRV(A) <= min(MSRV(A), MSRV(B)). We need a (partial?) order here!
  • (How) is it possible to calculate with MSRVs? Examples:
    • stable(1.30.1) - 0.0.1 = stable(1.30)
    • stable(1.30) - 0.3 = stable(1.27)
    • stable(1.30.1) - 0.0.2 = stable(1.30.0) Does this even work?
    • stable(1.30) - 1 = undefined

As for LTS releases, they are not only about MSRV, they will also serve as a synchronization point for crate authors and package authors (think Debian). LTS also means that crucial updates (e.g. security or soundness fixes) will be backported to those versions.

You are right. But I highly doubt the utility of LTS releases for other cases than language version requirements:

  • Take the argument about patches that are backported to LTS. If such a package becomes necessary, Debian (or anyone else) will need to adopt the fixed LTS version. In that case, they can also adopt the current stable version, since Rust is guaranteed to be backward-compatible (in version 1.x). LTS releases are even counterproductive in this use case, since they encourage not upgrading to the current release and therefore increase the number of old compilers out there.
  • Take the argument about synchronization points. We already have them, they are called "editions". (We could also publish guideline that give suggestion when a good times has come to update and how long crates should support old compiler versions. This could be done in an RFC which depends on things like stable(1.30) - 0.2.)

This comment has been minimized.

@teiesti

teiesti Jul 11, 2018

Okay, I've re-read your RFC and I think now that You've mentioned some of my points. But perhaps, they are still useful to clarify things... ;-)

Btw Thanks for Your effort!

This comment has been minimized.

@newpavlov

newpavlov Jul 12, 2018

Author

Hm, I don't quite get stable(x) functionality then. The only difference which I can see is that stable(1.30) will hint that rust field was automatically inserted by cargo, otherwise behavior looks similar to me. I'll try to describe current proposal a bit better in a separate PR to my repo. I'll link it later and I will be happy to hear your comments on which parts you think will need additional clarification.

how the crate author answers the question "Is an MSRV change a breaking change?"

As I see it, if public API does not change, then MSRV change will never (well, except the initial migration to using rust field) be a breaking change, be it upgrade or downgrade. Dependency versions resolution algorithm will handle a selection of appropriate versions for current toolchain automatically.

What if crate A depends on crate B and crate C. What are the requirements for the MSRV of crate A given the MSRV of B and C?

Don't forget that dependency can be defined as 0.x, so you have not one version, but a set. Thus formula will be MSRV(A) >= max(min( [MSRV(A1), ..MSRV(An)] ), min( [MSRV(B1), ..MSRV(Bn)] )). For stable versions order should be quite simple and you even don't have to calculate it explicitly. Running cargo publish with your MSRV toolchain will automatically check if non-empty solution for dependency versions exists.

But with "nightly versions" extension things become more complex. You'll have to check that your MSRV condition contains only such nightly versions which can be covered by all of your dependencies.

(How) is it possible to calculate with MSRVs?

I am not sure this feature will pulls its weight. Plus it can be added later in a backwards compatible way by a separate proposal.

This comment has been minimized.

@teiesti

teiesti Jul 14, 2018

I was thinking about the theoretical foundations of my proposal and I came up with this article. It is some kind of pre-work for my proposal in this thread but I think its worth sharing here. (Anywhere, I'll try to write another article surrounding these ideas soon.)

An internals thread for a discussion about the article can be found here.

fixes, option addition, wording improvements
- fixed typo and RFC link
- improved wording a bit
- added `--check-msrv-toolchain` option ( @phaylon )
convention for post-1.0 crates to bump minor version on MSRV change to allow
publishing backports which fix serious issues using patch version)

## Extension: nightly versions

This comment has been minimized.

@fintelia

fintelia Jul 12, 2018

I like the idea of flagging supported compiler versions, but I don't quite understand how a crate would actually be able to use this in practice if the list of supported versions has to go in the published Cargo.toml file itself.

Typically when publishing a nightly only crate, I know it works for the current nightly and that it will continue to work for all future nightlies until there is a breaking change. With this design, it seems like I'd need to publish a new version of the crate every day with an updated Cargo.toml saying that the new nightly also works or else my users would be unable to upgrade their compiler.

This comment has been minimized.

@newpavlov

newpavlov Jul 12, 2018

Author

As I see it, the main use-case will be to provide exact nightly version, on which crate is developed and tested. So e.g. if you depend on rocket x.y.z which is developed and published with nightly-2019-02-01 toolchain, then your crate will have to be developed with the same toolchain. This will result in a soft synchronization of nightly versions used across nightly crates ecosystem. (e.g. crates will update nightlies ever 2-3 weeks or so, not every night)

If your crate does not experience breakage too often, then you can use default nightly: * option.

This comment has been minimized.

@fintelia

fintelia Jul 12, 2018

Using the field to indicate a known good nightly version seems reasonable.

However, I'd be a concerned about it factoring into version resolution then ("stage 3" of the RFC). The entire ecosystem of nightly crates would have to update in lockstep or else become mutually incompatible. Further, trying out new nightlies would require waiting for all your dependencies to be updated. Basically, you'd no longer be running on the nightly channel but instead a sort of "monthly" channel which lacked both the stability of stable and the quick bug fixes of nightly.

This comment has been minimized.

@newpavlov

newpavlov Jul 12, 2018

Author

I think most of the nightly crates will just use "nightly: *", some may use ranges (though properly testing it will not be easy), finer grained nightly selection will be used only be a handful of crates with rare intersections with each other. Plus do not forget that you always can disable MSRV constraint in dependency versions resolution with --no-msrv-check flag.

This comment has been minimized.

@fintelia

fintelia Jul 12, 2018

I'm sorry, I'm still not fully understanding. Lets use a concrete example. I maintain the rahashmap crate which depends on some rather unstable features and so breaks every couple weeks. The last time this happened, I fixed the issue and promptly published a new version. This new version supports nightly-2018-07-10, nightly-2018-07-11, and all future nightlies until another breaking change happens. If this RFC & extension were implemented, what would be the correct value for MSRV?

This comment has been minimized.

@newpavlov

newpavlov Jul 12, 2018

Author

The problem with "all future nightlies until another breaking change happens" is that this constraint can be properly expressed at the moment of crate publishing, as you don't know when breaking change will happen. The closest thing which we can get is to use rust="nightly: >= 2018-07-10" (it's mentioned in the extension). On the next breaking change you'll publish rust="nightly: >= 2018-08-10". So assuming that user keeps all his dependencies updated, with new toolchain he'll use newer crate version, and on old toolchain the older crate version will be used. But nightly: * will not be much worse and will require significantly less effort from you.

But this approach feels quite fragile. To reliably solve this problem we will need an additional channel for notifying users "this crate broke on the following nightly versions" which can't be done via Cargo.toml.

@vi

This comment has been minimized.

Copy link

vi commented Jul 12, 2018

Shall it support usual version syntax syntax like =1.35, ^1.35, 1.35.0?

@kinggoesgaming

This comment has been minimized.

Copy link

kinggoesgaming commented Jul 13, 2018

Shall it support usual version syntax syntax like =1.35, ^1.35, 1.35.0?

@vi

Even if it was supported, I don't think it would something that would be useful, as it is only marking the absolute minimum version of Rust needed. The support for that syntax makes more sense for crates.

@dekellum

This comment has been minimized.

Copy link

dekellum commented Jul 13, 2018

If you accept the fact that rust has in the past and will in the future make non-backward compatible changes, then a maximum supported version (or in other words a complete dependency range with minimum and maximum) is useful for crate lib authors. See also rust-lang/rust#52320 for implications regarding how this is a two way street of effects.

@fintelia

This comment has been minimized.

Copy link

fintelia commented Jul 13, 2018

@dekellum Take a look at the inline review comments about nightly versioning above. Specifying a maximum supported version in the Cargo.toml of a crate generally isn't helpful: you typically don't know when the next relevant breaking change will happen. And once you've published a crate, if there is later a breaking change, you can't go back and change it to say what the maximum supported version is.

@dekellum

This comment has been minimized.

Copy link

dekellum commented Jul 13, 2018

@fintelia, I respectively still think a maximum supported version is useful and applicable. I/you can publish a patch release (e.g. 1.2.1 for a 1.2.0 crate) with a new maximum version bound, once you determine that a stable or nighly rust has a breaking change that effects the lib.

@newpavlov

This comment has been minimized.

Copy link
Author

newpavlov commented Sep 12, 2018

If rust = would be put by cargo new automatically then it maybe better be warning instead?

You can disable checks with --no-msrv-check, so I think it's worth to keep stricter defaults (for new, publish and build sub-commands) as described right now.

Allow rustc-minimum-version field in Cargo.toml's [package] section.

I am personally strongly against such long names. I think your variant is overly verbose and does not make much difference, while Cargo.toml becomes more noisy.

Don't use it for dependency resolution.

Support only exactly 1.x.x format. Only one version. No semver ranges. Ignore nightlies

The blocker here not an implementation difficulty, but Rust team green light on this idea. One of the cargo developers mentioned that it will not be too hard to add such constraint, so it does not make sense to water down this feature like this. Plus I think that rust = "stable" variant is a must-have if we don't want >90% of newly published crates to omit specifying MSRV.

@kornelski

This comment has been minimized.

Copy link
Contributor

kornelski commented Sep 12, 2018

OK, I don't feel strongly about the name, so rust/rustc + -min/-msrv or whatever is fine too.

The advantage of not using this property for dependency resolution is ability to remove it in the future without breaking anything. This reduces risk of introducing the feature. So if the libs team is not sure if they want this feature, I'm assuming a no-risk version is easier to try and release to stable ASAP, and parts of this feature that are less clear can be added later.

I'd totally like 90% of crates to have MSRV specified, with good defaults and automation, but if details on how to do it are blocking this feature, this means 0% of crates having MSRV until all of it is worked out. So I'd like to get MVP out of the door to get >0% of crates with MSRV while we work on getting it up to 90%.

@fintelia

This comment has been minimized.

Copy link

fintelia commented Oct 21, 2018

What is the status of this RFC?

@newpavlov

This comment has been minimized.

Copy link
Author

newpavlov commented Oct 22, 2018

@fintelia
We are awaiting decision from Rust team. I guess they are too busy with 2018 edition to make decision on this RFC.

@aturon
Is my understanding correct?

@briansmith

This comment has been minimized.

Copy link

briansmith commented Nov 25, 2018

I think this RFC is way too complicated.

  • We don't need to change how cargo publish works; that can be done in the future.
  • We don't need to automatically translate stable to the version that the crate was published with. We can just require it to be a valid version number literal. Future iterations can add more complexity, maybe.
  • We don't need the minimum supported rust version to affect dependency resolution. This would be an anti-feature. If I fix a critical security bug in my crate and publish a new version, and that new version requires a new version of Rust, then people shouldn't keep using the older version because of the Rust version dependency.

The ONLY thing we need here is this: When building the crate, check to see if the MSRV is higher than the version of Rust being used to build the crate. If it is, stop building and report a readable error message stating that "Rust version X is required to build this." (Wordsmith away.) That's it!

@stbuehler

This comment has been minimized.

Copy link

stbuehler commented Nov 25, 2018

@briansmith You're not the first to describe a "minimal-minimal-rfc"

* We don't need the minimum supported rust version to affect dependency resolution. This would be an anti-feature. If I fix a critical security bug in my crate and publish a new version, and that new version requires a new version of Rust, then people shouldn't keep using the older version because of the Rust version dependency.

You might not need it, but I think it is sad when people using stable linux distributions can only develop rust code when they are using rustup with the latest compiler. I tried using rust from debian testing for some time, and even that just wasn't doable.

If you have crate versions affected by critical (security) bugs, you should yank them imho. I'd hope cargo update would tell you about those.

I really think it should affect dependency resolution in the future (a "minimal-minimal-rfc" obviously doesn't need it).

@tarcieri

This comment has been minimized.

Copy link

tarcieri commented Nov 25, 2018

We don't need the minimum supported rust version to affect dependency resolution. This would be an anti-feature. If I fix a critical security bug in my crate and publish a new version, and that new version requires a new version of Rust, then people shouldn't keep using the older version because of the Rust version dependency.

I'd agree the default behavior of cargo's dependency resolver should not be to select older packages based on the minimum required Rust version. That said, I'm somewhat curious under what circumstances a security fix would require a new version of Rust (short of a vulnerability in rustc/std). If you're only fixing the latest release and don't want to support older releases which contain a vulnerability, why not yank them?

Back to the question of dependency resolution, as an example from another language ecosystem, the minimum Ruby version has been part of RubyGem's "spec" files almost from the beginning, and is part of Bundler's version resolution mechanism (where Bundler is the dependency resolver tool. In Rust, cargo provides the features of both RubyGems and Bundler).

By default Bundler prefers the latest version of each gem, irrespective of the minimum required Ruby version, and will fail if it selects a package that needs a newer Ruby version. However, it also supports an option in a Gemfile (the equivalent of Cargo.toml) to only consider packages which are compatible with a specified Ruby version.

I personally liked this behavior as it allowed for rapidly adopting new language features in an unobtrusive way, in my particular case for internal company packages in a company where Ruby versions varied.

However, these projects were also audited by bundle audit (via Brakeman) to ensure none of their dependencies contained security vulnerabilities. GitHub now performs these same checks on Ruby projects automatically and exposes them through the web UI. So there was a sort of "compensating control" to ensure that if people were using older Ruby versions, and were using this particular Bundler feature, they at least would be aware they had vulnerable dependencies.

I would like to think cargo-audit could act in a similar capacity, but as its author, I will admit I don't think it's there yet, particularly in terms of adoption (let alone GitHub integration)

@briansmith

This comment has been minimized.

Copy link

briansmith commented Nov 26, 2018

. That said, I'm somewhat curious under what circumstances a security fix would require a new version of Rust (short of a vulnerability in rustc/std).

Let's say in one commit I introduce a dependency on a feature in the newest stable Rust. Then I fix a security bug in the next commit. Or, when coding the security fix, I make use of a feature of the newest stable Rust (because why not?).

If you're only fixing the latest release and don't want to support older releases which contain a vulnerability, why not yank them?

Literally nobody thinks about whether or not older releases contain a vulnerability or not (at least I don't). I suppose I could always yank every older version when I publish a new version so there's never ever one non-yanked version; I don't see any downsides of doing that.

@tarcieri

This comment has been minimized.

Copy link

tarcieri commented Nov 26, 2018

I suppose I could always yank every older version when I publish a new version so there's never ever one non-yanked version; I don't see any downsides of doing that.

I am considering actually doing that in a particular case, as it were (although it's for a crate whose previous release builds were always broken).

@newpavlov

This comment has been minimized.

Copy link
Author

newpavlov commented Nov 26, 2018

@briansmith

We don't need to change how cargo publish works; that can be done in the future.

As was stated in the RFC, first stage will be to have a "dumb" optional field which can be used with third-party tools. In other words this RFC attempts to describe a future which I believe we should pursue.

We don't need to automatically translate stable to the version that the crate was published with. We can just require it to be a valid version number literal. Future iterations can add more complexity, maybe.

Again, translating "stable" to the version is a second stage. For the first stage we could accept only valid version numbers. I believe allowing "stable" field value is a must have if we want to make MSRV field mandatory (in future edition), so those who don't bother with tracking MSRV will have a lazy "just take my toolchain version" option. I believe that making the field optional will reduce usefulness of the feature significantly.

We don't need the minimum supported rust version to affect dependency resolution. This would be an anti-feature. If I fix a critical security bug <..>

As others have said if your crate has a security bug you should yank it.

Those who care about supporting older version of Rust compiler need such feature. Otherwise we will have to bump major crate version when Rust gets a new feature which we would like to use, or keep codebase in a worse state than it could be.

When building the crate, check to see if the MSRV is higher than the version of Rust being used to build the crate.

This approach will make a lot of crates unusable for older toolchains. I think we should rely on tools like cargo outdated to check if upstream crates have updates which require newer MSRV compared to the current crate MSRV or toolchain version.

@kornelski

This comment has been minimized.

Copy link
Contributor

kornelski commented Nov 26, 2018

If MSRV was added without affecting dependency resolution, would it be a breaking change if it started affecting dependency resolution later?

@newpavlov

This comment has been minimized.

Copy link
Author

newpavlov commented Nov 26, 2018

It's not a bigger breaking change than publishing crate with a bigger MSRV today. Plus you can always disable MSRV constraints with --no-msrv-check flag.

@stbuehler

This comment has been minimized.

Copy link

stbuehler commented Nov 26, 2018

If MSRV was added without affecting dependency resolution, would it be a breaking change if it started affecting dependency resolution later?

No. As long as it doesn't affect dependency resolution your builds will simply fail if you don't have the required compiler version. If it gets added you can compile more than before.

@briansmith

This comment has been minimized.

Copy link

briansmith commented Nov 27, 2018

Here's an even more conservative proposal, which could serve as a stepping stone:

If any (non-fatal) error is encountered, and the crate advertised a MSRV, and the version of rustc is older than that, DO NOT EMIT any message for that error. Instead, emit an error message that the MSRV requirement was not met. If no errors are encountered, then do nothing.

In particular, this will ensure that the initial implementations of the MSRV mechanism would not cause any crate to fail to build on its own; it would merely replace the confusing error messages with a clear error message.

To be clear, failing to meet the MSRV should eventually become an error on its own. This conservative suggestion is intended to show a very gradual way forward.

@newpavlov

This comment has been minimized.

Copy link
Author

newpavlov commented Nov 27, 2018

failing to meet the MSRV should eventually become an error on its own

It's the main difference between your approach and one described in this RFC. You consider that breaking builds for older toolchain versions on a new patch update of some upstream dependency which internally uses new shiny lang/std features is a "good" thing. IIUC supporting only the latest stable version is a policy which you have adopted in ring and nothing wrong with that, but others (including me) take supporting older toolchain versions quite seriously and do not want break other people's build without VERY good reason, but still would like to use new features when they can improve code without making it a breaking change.

With the functionality proposed in this RFC you can simply use "stable" as your MSRV and yank older patch versions of your crates. This effectively will enforce latest stable as a requirement for using your crates. Yes, you will not use most of the functionality in this RFC, but others will.

@briansmith

This comment has been minimized.

Copy link

briansmith commented Nov 27, 2018

@newpavlov Regardless of which way is better, the fact is that no progress is has been made on this feature for a very long time, despite lots of people willing to help with it. Thus, my idea is to greatly simplify the feature to the minimum thing that could possibly help and which has zero risk (since nothing will break unless it was already broken) so that things can move forward. With a specification as complex as what you described, it would take at least a year to get into Rust stable, in my estimate. I'm trying to shorten the time until we get something useful.

@newpavlov

This comment has been minimized.

Copy link
Author

newpavlov commented Nov 27, 2018

This is why this RFC proposes implementation stages. On the first stage MSRV field will not influence dependency resolution (it could or could not support "stable" value, I don't think that adding it will be too difficult) and cargo can emit warning that advertised MSRV in a given crate is bigger than version of the currently used toolchain, or it can be done by a third-party tool. It will be essentially the same functionality as in your previous message, but without silencing errors. (though it may be better to do silencing, I don't have a strong opinion here)

And I would like to cite this message by @Eh2406:

Not speaking to any broader discussion of whether we should do this, but as the person most likely to have to implement it, I do not foresee this being hard to implement in cargo’s resolver. One way is we can just filter the result from querying the index to crates with msrv < our msrv. The tricky part would be, as usual, the error messages, but even that is doable.

So the main blocker for MSRV is not implementation difficulty, but a principal decision from Rust team.

@briansmith

This comment has been minimized.

Copy link

briansmith commented Nov 29, 2018

So the main blocker for MSRV is not implementation difficulty, but a principal decision from Rust team.

Right, so let's agree on the easiest stuff to agree on now, and defer the harder-to-agree-on stuff for later.

Anyway, I think we should collect some motivation for doing this. There are multiple motivations:

  1. It is confusing for users of a crate to get compilation errors because the crate has a MSRV higher than what the user is currently using. This is important but not critical.

  2. Rust sometimes has bugs or changes in semantics (either language semantics or library semantics). If my crate is used in an older version of Rust then, if it compiles, the user may get wrong or even dangerous results. For example, consider rust-lang/rust#18086. If you use extern "fastcall" then you must use a particular version of Rust (which one? I'm not sure). Otherwise the code might compile but it will be dangerously wrong. Similarly, one must use Rust 1.22 or later to avoid CVE-2018-1000657, one must use Rust 1.29.1 or later to avoid CVE-2018-1000810, etc. This is a critical reason to implement at least the very minimal version of this feature.

@kornelski

This comment has been minimized.

Copy link
Contributor

kornelski commented Dec 21, 2018

In the meantime, edition = "2018" acts as msrv = "1.31".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment