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

Document Rust MSRV of 1.71 requirement for release 0.7.1 #26

Closed
polarathene opened this issue Oct 26, 2023 · 8 comments
Closed

Document Rust MSRV of 1.71 requirement for release 0.7.1 #26

polarathene opened this issue Oct 26, 2023 · 8 comments

Comments

@polarathene
Copy link

Bug description

a81c05c introduced usage of hash_one(), which CI is failing on due to this being an unstable feature (_CI is running with Rust 1.70, hash_one() stablized in 1.71). Caught by a crate (rust-ini) releasing with an update to their dependency of ordered-multimap.

  • Would you like to work on a fix? n

To Reproduce

Try build with earlier rust 1.70 (June 2023).

Expected behavior

Perhaps set your MSRV to 1.71 in Cargo.toml? That would better communicate incompatibility if you're unable to support older rust toolchains less than 6 months old.

Screenshots

image

Environment

  • ordered-multimap version: 0.7.1 (indirectly via rust-ini 0.20)
  • Rust 1.70 (June 2023)
@sgodwincs
Copy link
Owner

Apologies, this was an oversight of mine. My lint during CI was failing so I fixed it but didn't realize it required MSRV 1.71. I'll set the MSRV in Cargo.toml as you suggest.

@polarathene
Copy link
Author

I'll set the MSRV in Cargo.toml as you suggest.

You might want to do something like was done for this crate: fitzgen/bumpalo#206

  • They published a new version with rust-version added and yanked the previous version.
  • Cargo then would use the latest version compatible with the active toolchain based on rust-version info.
    • If the active toolchain satisfies the rust-version declared, it'll use the latest release from semver.
    • Otherwise cargo will rollback until a release with rust-version declared is compatible.
    • Or when rust-version is no longer declared in earlier releases, this is assumed compatible (which may be incorrect and fails).

Yanking may affect some projects negatively until they update to new releases?:

It seems like that's related to the Cargo.lock and that it's possible for them to still install based on Cargo.lock despite yanked versions with --locked, and then resolved by updating Cargo.lock with new release.

cargo-msrv

According to cargo-msrv, 0.7.0 is compatible with Rust MSRV of 1.64.0 (same as ordered-multimap 0.6.0), while 0.7.1 bumps that to the mentioned 1.71.1 (not 1.71.0).

cargo-msrv output (Click to view)
Check for toolchain '1.62.1-x86_64-unknown-linux-gnu' failed with:
┌──────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ error: package `hashbrown v0.14.2` cannot be built because it requires rustc 1.63.0 or newer, while the  │
│ currently active rustc version is 1.62.1                                                                 │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────┘

Check for toolchain '1.63.0-x86_64-unknown-linux-gnu' failed with:
┌──────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Checking dlv-list v0.5.2                                                                                 │
│ error[E0658]: use of unstable library feature 'nonzero_ops'                                              │
│   --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/dlv-list-0.5.2/src/lib.rs:78:12          │
│    |                                                                                                     │
│ 78 |     self.0.checked_add(rhs).map(Self)                                                               │
│    |            ^^^^^^^^^^^                                                                              │
│    |                                                                                                     │
│    = note: see issue #84186 <https://github.com/rust-lang/rust/issues/84186> for more information        │
│                                                                                                          │
│ For more information about this error, try `rustc --explain E0658`.                                      │
│ error: could not compile `dlv-list` due to previous error                                                │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────┘
Check for toolchain '1.64.0-x86_64-unknown-linux-gnu' succeeded
   Finished The MSRV is: 1.64.0   █████████████████████████████████████████████████████████████████ 00:00:03

This applies to dlv-list 0.5.0 as well, it's MSRV due to the above requires Rust 1.64.0.


Next steps

ordered-multimap

So possibly consider this approach?:

  • Yank 0.7.1
  • Publish 0.7.2 with rust-version = "1.64.0" (optional, may warrant yanking 0.7.0? Otherwise toolchains lower than 1.64.0 would fallback to 0.7.0 when a downstream has a semver of ordered-multimap = "0.7")
  • Publish 0.7.3 with rust-version = "1.71.1" (any toolchain and 1.71.1 would fallback to 0.7.2 release instead or fail on 0.7.0)

dlv-list

Since you maintain dlv-list, you might want to add rust-version = "1.64.0" there too.

  • This is less important, as cargo will resolve ordered-multimap by rust-version which would already mandate the 1.64.0 MSRV, so yanking earlier dlv-list releases wouldn't matter as much.
  • It would be relevant for the current iteration of cargo +nightly update -Z msrv-policy, which doesn't resolve crate dependencies in the same way, and appears to attempt resolving each crate independently AFAIK (even if a parent crate in the dependency chain has declared rust-version, it's not trusted?).

rust-version in practice

Click to view

Beyond that, a cargo check with the rust-version toolchain for those crates could be useful CI check prior to publishing? It'll be more reliable at knowing when to raise your rust-version but from what I understand it's mostly a guide, as it only works reliably when upstream dependencies maintain a correct rust-version too.

One example is with ordered-multimap = "0.4" that rust-ini 0.18.0 uses.

  • If you run cargo +1.56.1 check, it would install ordered-multimap 0.4.0 not ordered-multimap 0.4.3 that semver would resolve to.
  • Even though you don't use rust-version in your crate, the dependency on hashbrown at the time did with rust-version = "1.56.1".
  • Cargo notices that it can't install ordered-multimap releases 0.4.1 to 0.4.3 because those bumped the hashbrown version which has a higher rust-version when resolving, thus cargo is smart enough to know 0.4.0 is compatible for Rust 1.56.1 👍

I think this is a good practice as it demonstrates that (provided the MSRV rust-version is accurate) cargo will better resolve semver for downstream crates that lack rust-version.

  • If rust-ini 0.20.0 had declared rust-version = "1.64.0" with their release depending on ordered-multimap = "0.7", while accurate at the time, this issue is an example of upstream dependencies not using rust-version where it would become inaccurate, until the upstream crate resolves it (and so forth).
  • With broader adoption of rust-version I think this will become a smoother experience. As you can see above with cargo-msrv for the hashbrown crate failure, the rust-version also gets communicated with what the compatible version is.

@polarathene
Copy link
Author

My lint during CI was failing so I fixed it but didn't realize it required MSRV 1.71

I'm guessing the lint was due to running a newer version of rust toolchain than the actual MSRV for the crate?

So if you had established MSRV was 1.64.0 like shown above, the CI wouldn't have complained about it?

  • Only once it was necessary to raise the MSRV would the lint show up as it's a better supported way from 1.71.1 onwards?
  • If so, you could potentially revert that and keep your CI happy (unless testing against newer toolchains / nightly too?).

My understanding is that MSRV bumps aren't considered breaking changes and can be encountered in patch releases as well like happened here. rust-version can at least communicate that concern better for resolving, but MSRV bumps are sometimes deferred until it's necessary for the crate to leverage or support updating upstream dependencies requirements.

EDIT: Oh it looks like your CI only tests on nightly? It's your call either way. I'm still fairly new to all this myself, and have noticed some projects supporting MSRV of 6-12 months old. They just hold back on upgrading dependencies when the MSRV is no longer satisfied until they can raise their MSRV, but adopting rust-version seems to help 👍

@polarathene
Copy link
Author

Heads-up, I've just found out that I misunderstood the ordered-multimap = "0.4" resolution with the 1.56.1 rust toolchain.

It wasn't respecting the rust-version, but the toolchain is too old that Cargo.toml of newer releases for crates like ahash were not considered valid, so it just ignored their existence. Rust 1.60.0 was required where that problem doesn't exist I'm told.

So my suggestions here should probably be disregarded. rust-version is still relevant for the resolver with -Z msrv-policy AFAIK, but otherwise point releases that bump the MSRV will still be resolved, even with rust-version.

@Xuanwo
Copy link

Xuanwo commented Nov 20, 2023

I'll set the MSRV in Cargo.toml as you suggest.

Thanks for the responnse. But I think we should not bump the MSRV in a 0.7.x version: it's a breaking change.

I will suggest:

  • Yank v0.7.1 (it's broken).
  • Release v0.7.2 with rust-version= "1.64.0".
  • Releaes v0.8.0 with any version you want.

@polarathene
Copy link
Author

polarathene commented Nov 21, 2023

it's a breaking change.

TL;DR: No it's not, MSRV bumps don't equate to semver breaking changes.


MSRV bumps aren't considered semver breaking changes from what I've read.

  • Reason IIRC is due to rippling the MSRV bump downstream, which shouldn't force major/minor version bump of the dep chain throughout crates, which in turn introduces more duplicate crates being resolved despite no real change through the chain.
  • As opposed to resolving the dep version appropriately for the MSRV with -Z msrv-policy / Cargo.lock pin where it matters. Which doesn't need to ripple version bumps throughout downstream crates because of someone else deciding to bump MSRV.

Adopting rust-version will allow you to better resolve implicit deps that are compatible with your toolchain version.

  • You'll find this happens across quite a few popular crates already.
  • Although it's not always maintained correctly, such as with tokio-util introducing a change that required raising their declared rust-version, but was missed. -Z msrv-policy will not be reliable when that happens
  • MSRV can also be bumped down as seen with zerocopy, where downstreams like ahash shouldn't have to publish new versions in response (the linked issue was a user complaining about MSRV going up due to zerocopy addition, but not long after zerocopy lowered the MSRV).
  • hashbrown lowered from 1.64 MSRV to 1.63 in the v0.14.x series, but with the caveat that requires version pinning or opt-out of a default feature. This is an example of MSRV depending on context, and another scenario where -Z msrv-policy may not always resolve correctly.

You can see this PR that details how config-rs maintains a 1.56.1 MSRV for it's 0.13.x series.

  • Attempting to install it without the required lockfile generation and pinning would not actually work for that MSRV target, any downstream would need to do similar. A Cargo.lock.msrv is used with CI to run tests that verify the crate builds with the MSRV and passes tests.
  • It would be silly for config-rs to bump to 0.14 because of any upstream crates requiring a new MSRV beyond the one config-rs has. The crate can build for the MSRV, but it doesn't need to support that out of the box without the resolver / lockfile effort as after releasing the crate, upstream crates can release within semver and that may not be compatible, yet completely out of control of config-rs.
  • At the same time if upstream crates are only raising MSRV, but not introducing real breaking changes, it would be bad if they just bump their major (or minor if major is zero). While it's true that any improvements, fixes or security patches would be blocked by the -Z msrv-policy when targeting the MSRV, that is not the case for most users that are comfortable with much newer Rust versions, they should not be deprived of that, nor does config-rs need to raise the MSRV unnecessarily if it were to follow the semver bump, both demographics can be supported and the resolver can minimize duplicates.

@Xuanwo
Copy link

Xuanwo commented Nov 21, 2023

Thanks for @polarathene's explanation! While I still believed it would be best to include higher rust-version in a minor version bump (from 0.7 -> 0.8), I've softened my stance from strongly suggest to weak suggest, leaving the final decision up to the maintainer.

@sgodwincs
Copy link
Owner

sgodwincs commented Apr 1, 2024

Sorry for the long delay, this should be fixed now.

I've yanked 0.7.1, released 0.7.3 (messed up 0.7.2 mistakenly) with MSRV 1.64.0.

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

No branches or pull requests

3 participants