Closed ijackson closed 7 months ago
ijackson
commented
8 months ago I put the recipe for generate the lockfile in the commit message, rather than in a script to run from CI, because, sadly, it will rot. As newer dependencies get published, the list of things to downgrade will grow (and possibly things will get more complicated).
The -Z minimal-versions generate-lockfile approach wouldn't have suffered from that problem.
polarathene
commented
8 months ago Commit: https://github.com/mehcode/config-rs/pull/492/commits/f1445bc16d099392207db8c9725cb9469a2dcafb Emprically, 1.30 requires a newer Rust than our MSRV. Therefore, request 1.29.
Typo: Empirically
Commit: https://github.com/mehcode/config-rs/pull/492/commits/5c4c72d96c1d69f45bc91142e9764902baf645db My atteempts at starting with the minimal versions and upgrading crates as needed, were not successful.
Typo: attempts
Do I understand this correctly, instead of installing the semver version in Cargo.toml as the minimum supported dependency, preferring the newest in semver, you're generating a lock with the minimal version?
You note though that this is a bit tricky to manage this way, and the main goal is to ensure the Cargo.lock respects a supported MSRV?
The PR and commits lack reference to what the target MSRV is. I'm assuming it's the one referenced in the README for the 0.13.x branch, which is 1.56.1. Using the cargo-msrv tool to verify:
$ cargo-msrv
Fetching index
Determining the Minimum Supported Rust Version (MSRV) for toolchain x86_64-unknown-linux-gnu
Using check command cargo check
Check for toolchain '1.52.1-x86_64-unknown-linux-gnu' failed with:
┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Updating crates.io index │
│ Downloading crates ... │
│ error: failed to download `syn v2.0.38` │
│ │
│ Caused by: │
│ unable to get packages from source │
│ │
│ Caused by: │
│ failed to parse manifest at `/home/xiaobu/.cargo/registry/src/github.com-1ecc6299db9ec823/syn-2.0.38/Cargo.toml` │
│ │
│ Caused by: │
│ feature `edition2021` is required │
│ │
│ this Cargo does not support nightly features, but if you │
│ switch to nightly channel you can add │
│ `cargo-features = ["edition2021"]` to enable this feature │
└───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
Check for toolchain '1.63.0-x86_64-unknown-linux-gnu' succeeded
Check for toolchain '1.57.0-x86_64-unknown-linux-gnu' succeeded
Check for toolchain '1.54.0-x86_64-unknown-linux-gnu' failed with:
┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ error: failed to download `async-trait v0.1.74` │
│ │
│ Caused by: │
│ unable to get packages from source │
│ │
│ Caused by: │
│ failed to parse manifest at `/home/xiaobu/.cargo/registry/src/github.com-1ecc6299db9ec823/async-trait-0.1.74/Cargo.toml` │
│ │
│ Caused by: │
│ feature `edition2021` is required │
│ │
│ this Cargo does not support nightly features, but if you │
│ switch to nightly channel you can add │
│ `cargo-features = ["edition2021"]` to enable this feature │
└───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
Check for toolchain '1.55.0-x86_64-unknown-linux-gnu' failed with:
┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ error: failed to download `async-trait v0.1.74` │
│ │
│ Caused by: │
│ unable to get packages from source │
│ │
│ Caused by: │
│ failed to parse manifest at `/home/xiaobu/.cargo/registry/src/github.com-1ecc6299db9ec823/async-trait-0.1.74/Cargo.toml` │
│ │
│ Caused by: │
│ feature `edition2021` is required │
│ │
│ The package requires the Cargo feature called `edition2021`, but that feature is not stabilized in this version of Cargo (1.55.0 (32da73ab1 │
│ 2021-08-23)). │
│ Consider trying a newer version of Cargo (this may require the nightly release). │
│ See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#edition-2021 for more information about the status of this feature. │
└───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
Check for toolchain '1.56.1-x86_64-unknown-linux-gnu' succeeded
Finished The MSRV is: 1.56.1 ██████████████████████████████████████████████████████████████████████████████████████████████████████████ 00:02:03It also confirms that 1.56.1 is the MSRV.
I am not sure what the issue was with the tokio version? With an MSRV of 1.56.1 it is compatible with tokio v1.33.0? 🤔
$ cargo +1.56.1 tree --package tokio
tokio v1.33.0
├── bytes v1.5.0
├── libc v0.2.149
├── mio v0.8.9
│ └── libc v0.2.149
├── num_cpus v1.16.0
│ └── libc v0.2.149
├── pin-project-lite v0.2.13
├── socket2 v0.5.5
│ └── libc v0.2.149
└── tokio-macros v2.1.0 (proc-macro)
├── proc-macro2 v1.0.69
│ └── unicode-ident v1.0.12
├── quote v1.0.33
│ └── proc-macro2 v1.0.69 (*)
└── syn v2.0.38
├── proc-macro2 v1.0.69 (*)
├── quote v1.0.33 (*)
└── unicode-ident v1.0.12
# No issues with this MSRV to build:
$ cargo +1.56.1 check
Finished dev [unoptimized + debuginfo] target(s) in 0.03s
$ cargo +1.56.1 build
Finished dev [unoptimized + debuginfo] target(s) in 0.03s
# With `rust-version = "1.56.1" in Cargo.toml:
$ cargo-msrv verify
Fetching index
Verifying the Minimum Supported Rust Version (MSRV) for toolchain x86_64-unknown-linux-gnu
Using check command cargo check
Finished Satisfied MSRV check: 1.56.1 ███████████████████████████████████████████████████████████████████████████████████████████████████ 00:00:00I put the recipe for generate the lockfile in the commit message, rather than in a script to run from CI, because, sadly, it will rot. As newer dependencies get published, the list of things to downgrade will grow (and possibly things will get more complicated).
Assuming the MSRV is intended as 1.56.1, it would seem more reliable to use cargo-msrv (as I have suggested earlier) to determine this than the approach you've noted will be a maintenance burden?
ijackson
commented
8 months ago Do I understand this correctly, instead of installing the semver version in
Cargo.tomlas the minimum supported dependency, preferring the newest in semver, you're generating a lock with the minimal version?You note though that this is a bit tricky to manage this way, and the main goal is to ensure the
Cargo.lockrespects a supported MSRV?
I'm not quite sure I follow the question but I will try to answer it anyway :-).
The problem I am trying to solve with this MR is that CI is failing with the declared MSRV of 1.56. We want to test our MSRV in CI as otherwise it's almost certain we'll accidentally violate it.
In principle the correct solution to testing with the MSRV is as follows:
Cargo.tomls.cargo +nightly update -Z minimal-versions to generate a lockfile using the lowest version permitted by our Cargo.tomlThis is what I do in (for example) derive-adhoc.
However, 1 must be done transitively: if any of our dependencies have too-relaxed versions, then we need to get the dependency updated; and we would then need to update our dependency edge to declare a requirement on the version which is fixed. And this applies all the way to the bottom of the stack.
This might involve patch releases of multiple crates throughout our dependency stack. My investigations (or random flailing, depending how you look at it) led me to suspect that this proper approach wouldn't yield success, at least not for the 0.13.x release branch of config-rs.
It's easier to do this kind of cleanup to all of our upstreams if we'd be working on their mainlines. But, the job is a big one and might involve starting at the bottom. Realistically, a good place to start would be to try to get pest to build with -Z minimal-versions.
The next most principled approach is to generate a lockfile with -Z minimal-versions and then update individual crates (deep in our stack) that, in practice, are broken. (For direct dependencies, we could easily, and should, just change our Cargo.toml.)
I tried this. But it got badly out of hand.
So instead, what I did was, by hook or by crook, generate a Cargo.lock that can be used to test that we do build with our MSRV. That will prevent us from adding MSRV breaking to config-rs.
Downstreams with a comparable MSRV might have to redo some of this work. My particular prompt is the Arti project, where we have an MSRV of 1.65, and we have already adopted the "minor bodging" approach described above. In practice that seems to be working for us.
But, at least in a release branch, I don't think we will often want to update our Cargo.lock in CI.
ijackson
commented
8 months ago @polarathene
Assuming the MSRV is intended as
1.56.1, it would seem more reliable to usecargo-msrv(as I have suggested earlier) to determine this than the approach you've noted will be a maintenance burden?
Sorry for not replying to that suggestion earlier. I had read it, but it didn't seem directly on-point. Let me explain why. (I have read enough of the cargo-msrv docs to feel I know what it does.)
Firstly, I don't think we are trying to determine our MSRV. We are trying to assure that we do not break our intended MSRV. The basic principle is that we should advance our MSRV deliberately, and ideally avoid doing that on a release branch.
To do that, we do not need to build and test with many different toolchains. Nor do we need to inspect the rust-version value in any of our (transitive) dependencies. (Those are the things cargo-msrv can do that aren't easy without it.)
We just need to test with our intended MSRV. If that passes, then the Rust projects' compatibility guarantees mean that we should build with later versions too (and, in practice, that always is the case).
And, whether we use cargo-msrv or just test with our MSRV in CI, we still need a suitable lockfile. Since cargo dosn't do MSRV-aware resolution, this is not straightforward.
I am not sure what the issue was with the tokio version? With an MSRV of 1.56.1 it is compatible with tokio v1.33.0? 🤔
According to Tokio's README the MSRV of Tokio 1.30 onwards is 1.63.
Empirically, cargo +1.59 clippy --locked --offline --all-targets --all-features -- -D warnings failed with Tokio 1.30.
Note that in order to properly check our MSRV, the CI doesn't just build the package. It runs the tests too. I think this is correct.
polarathene
commented
8 months ago Thanks for taking the time to give a detailed reply! ❤️
Building the crate is successful for 1.56.1, which is the documented MSRV.
It seems that when you throw tests into the mix that is not the case, and I could not seem to find much information at a quick glance with some search queries if MSRV is meant to also cover dev-dependencies, or only the release crate that would be pulled in as a dependency to other projects. I'd assume that's what MSRV is generally intended for though?
I have seen some projects document MSRV for running, and MSRV for building/testing, so perhaps that distinction is worthwhile? cargo-msrv seems to lack the ability to take the dev-dependencies context into account with cargo check --tests :(
$ cargo +1.56.1 check --tests
error: package `rustix v0.38.9` cannot be built because it requires rustc 1.63 or newer, while the currently active rustc version is 1.56.1I understand now I think, thanks! 👍
ijackson
commented
8 months ago It seems that when you throw tests into the mix that is not the case, and I could not seem to find much information at a quick glance with some search queries if MSRV is meant to also cover dev-dependencies, or only the release crate that would be pulled in as a dependency to other projects. I'd assume that's what MSRV is generally intended for though?
Downstreams don't run the tests so they might be OK with it just building. But we want to detect semantic as well as syntactic problems in our CI. That will help assure that downstream users get not only a build that completes, but also working programs.
Declaring the MSRV with rust-version in the Cargo.toml is probably a good idea, but IMO (i) probably not on the stable release branch, where we should avoid unecessary changes (and especially, build system changes) and (ii) even on the main branch, not until we have checked that the documented MSRV isn't unecessarily high (since adding the rust-version will break things that might have previously been working).
ijackson
commented
8 months ago @matthiasbeyer I think this is just blocking on your decision about https://github.com/mehcode/config-rs/pull/492#discussion_r1370437830
matthiasbeyer
commented
8 months ago No time to read this all now, because I am traveling until Sunday. Just wanted to say that I don't mind typos in commit messages or doc comments! So these should not block merges! I don't think anyone intended to block a merge here because of commits, I just wanted this to be noted so everyone knows! :laughing:
Thanks for your contributions, all of you! I hope I can read through all of the above next week.
polarathene
commented
8 months ago I don't think anyone intended to block a merge here because of commits
I'm not blocking on anything, this isn't an area that I am confident in so leave the discussion between you two 👍
I only mentioned the typos since once you merge, rewriting history isn't usually viable if the typos were a concern.
Thanks for your contributions, all of you! I hope I can read through all of the above next week.
You're welcome, I still plan to get back to my contributions here ASAP 😅
EDIT: Accidentally marked this as off-topic, meant to be "outdated" to reduce noise in the discussion, but I can't seem to change that or unhide the comment 🤷♂️
polarathene
commented
8 months ago Been writing this up for a while, fairly tedious and I don't expect anyone else to read through it all, but it serves as documentation on issues being tackled/discussed by the PR.
@ pings for going forward.Cargo.lock exists, which adjusts how the bots engage with PRs which may not be desirable.json5 (pest) with -Z minimal-versions.Realistically, a good place to start would be to try to get
pestto build with-Z minimal-versions.
This actually seems to be due to json5 (parent dependency) pulling in pest allowing a semver with minimum versions that are incompatible with the MSRV of config-rs?
The json5 crate does not appear to be actively maintained anymore, so trying to address that may not be successful?
Perhaps the issue does bubble further down into the EDIT: pest crates..pest_meta min version was the problem
pest deps chain resolve workspace pest deps via path with 2.0 min bound (_which is problematic for -Z minimal versions on pest_meta_).json5 (other than perhaps raising the min bound even higher, but json5 could otherwise support as low of an MSRV as Rust 1.31, aka edition = "2018").-Z msrv-policy vs -Z minimal-versions:
cargo +nightly update -Z msrv-policy (paired with rust-version = "1.56.0" in Cargo.toml and json5 = "0.4"), you'll find it resolves pest to 2.5.7 and serde to 1.0.190.cargo +nightly update -Z minimal-versions will resolve to pest 2.0.0 and serde 1.0.0 (unrelated to config-rs, but a minimal Cargo.toml for isolating resolver scope to json5). Not as useful.You could manage minimal versions for only direct dependencies (only those directly in our Cargo.toml) with cargo +nightly update -Z direct-minimal-versions?
+nightly toolchain it resolves the semver of implicit dependencies for that toolchain, which is problematic since -Z only works with nightly (EDIT: Partially a valid concern, only because of the later mentioned Rust < 1.60.0 resolver behaviour issue).json5 => pestpest 2.5.7 is the last valid release resolved (ignoring the 2.6.x yanked series) due to this Cargo.toml line:
pest 2.7.0 to use the dep: prefix syntax for the [features] table that was introduced in Rust 1.60.0.dep: feature (namespaced dependencies) usage prevents optional dependencies becoming implicit features.Minimal versions:
pest 2.0.2 and serde 1.0.69 for json5 0.4.0 to build successfully on Rust 1.56.1.ucd-trie to it's minimum with cargo +nightly update -Z minimal-versions --package ucd-trie (0.1.1, anything above 0.1.2 introduces MSRV blockers due to edition = "2018" / edition = "2021")
1.38.0 (no lower apparently due to change in Cargo.lock v2 compatibility, introduced in Rust 1.38 and generated by default from Rust 1.41).cargo-msrv would require version pinning in Cargo.toml instead (to generate the Cargo.lock on a toolchain prior to 1.41) which will then support building down to Rust 1.31 (due to the edition = 2018 syntax used by json5 0.4.0).-Z minimal-versions applies all the way down the dependency chain, affecting it's reliability:
pest_derive 1.0.2, this is actually due to the minimal version of pest_meta 2.0.0 (_dependency of pest_derive 2.x via pest_generator_) bringing in the older 1.x releases of pest_derive and pest.pest_meta would correctly use the 2.x releases of those deps instead, thus you'd need to express that boundary in Cargo.toml or Cargo.lock.-Z direct-minimal-versions works a bit better, but as implicit dependencies resolve semver on nightly toolchain:
pest 2.5.7, and resolves to pest 2.7.5 which is not compatible on the MSRV of Rust 1.56.1.pest release (2.7.5) requires Rust 1.61.0.# `-Z minimal-versions` compatibility for json5:
json5 = "0.4.0"
# json5 relies on these dep versions at a minimum:
pest = "2.0.2"
serde = "1.0.69"
# 2.0.0 min version brings in 1.x pest versions, avoid that:
pest_meta = "2.0.3"
# Pinning this dep additionally supports earlier than Rust 1.56 (down to 1.31):
# 0.1.3 requires 1.31 (edition 2018)
# 0.1.6 requires 1.56 (edition 2021)
#ucd-trie = "=0.1.2"rust-ini => ordered-multimap => hashbrown => ahashMeanwhile ahash 0.7.6 also adopted the dep: syntax feature too. ahash 0.7.6 got yanked, so 0.7.7 is the earliest and current release with that change.
ordered-multimap 0.4.3 depends on hashbrown 0.12ordered-multimap 0.4.2 depends on hashbrown 0.11 and so does ordered-multimap 0.4.1.ordered-multimap 0.4.0 depends on hashbrown 0.9.hashbrown 0.11.0 depends on ahash = "0.7" and so does hashbrown 0.12.x, thus neither of these are compatible on Rust 1.56.1, resolving ordered-multimap down to the 0.4.0 release.hashbrown 0.9.1 depends on ahash = "4.4", which has valid releases that are not yanked (0.4.8) and do not have the dep: usage in Cargo.toml 🎉 And that is how the resolving happened 😮💨
hashbrown 0.12.3 is the last release that declared a rust-version of 1.56.0 before bumping it, but the 0.12.x series never raised it in response to ahash (not that it would help, as -Z msrv-policy would rollback to an earlier 0.12.3 that claims compatibility, but the newer ahash 0.7 dependency releases breaking that).ahash (introducing MSRV bumps with the point releases) that's not really avoidable and affects earlier releases just the same. hashbrown has no control over its upstreams affecting semver in that way.0.11.x / 0.12.x release of hashbrown:
ahash crate had not yanked earlier versions (without the Cargo.toml syntax change that Rust 1.56.1 cannot understand, but the yank was necessary AFAIK).rust-version becomes useful.Latest rust-ini 0.20.0 bumps to ordered-multimap 0.7 which currently requires Rust 1.71.1 with no use of rust-version by either crate to guard against that. Meanwhile rust-ini 0.19.0 bumped to ordered-multimap 0.6 which has an MSRV of 1.64 (due to dlv-list 0.5).
I initially investigated with cargo-minimal-versions and learned about -Z msrv-policy as a better way of resolving a Cargo.lock that is compatible with the MSRV, but not foolproof (even with broader adoption, see the hashbrown 0.14.2 example at the link).
Thankfully pest has been declaring rust-version since their 2.2.0 release with 1.56.0, and bumping that appropriately to 1.60.0 for the 2.7.0 release.
-Z msrv-policy.pest due to incompatible min bounds (-Z minimal-versions) or target MSRV being too old for implicit deps semver (-Z direct-minimal-versions) as both were noted earlier.Unfortunately, since rust-version is not widely adopted yet, -Z msrv-policy can fail to resolve Cargo.lock in a compatible manner (as noted earlier with ahash).
Due to the resolver bug with crates that adopted 1.60.0 weak (?) or namespaced (dep:) feature syntax:
config-rs will resolve for cargo +1.56.1 check just fine without the nightly resolver options. -Z direct-minimal-versions may still be useful to test.config-rs that raise the MSRV from 1.56.1 where the resolver issue is no longer present blocking semver resolution for those 1.60.0 syntax feature crates.Dev dependencies fail to resolve correctly for the intended MSRV of 1.56.1 however
Cargo.lock, or sometimes one that is tailored to the MSRV referred to as Cargo.lock.msrv.polarathene
commented
8 months ago I put the recipe for generate the lockfile in the commit message, rather than in a script to run from CI, because, sadly, it will rot. As newer dependencies get published, the list of things to downgrade will grow (and possibly things will get more complicated).
The
-Z minimal-versions generate-lockfileapproach wouldn't have suffered from that problem.
tokio-util@0.7.9 change is too fragile, it has to match a version that is resolved at the time.h2 to a release before it bumped to tokio-util 0.7, but then that conflicts with hyper semver.. (EDIT: Unless tokio is below 1.28, which then resolves to compatible tokio-util 0.7.8)With a slight adjustment to Cargo.toml (which still respects the MSRV constraints), we can then minimize this down to:
# Minimal versions in Cargo.toml only, resolve the rest by rust-version
cargo +nightly update -Z direct-minimal-versions -Z msrv-policy
# Pins required after `-Z msrv-policy` due to lack of implicit deps using rust-version:
# nom, warp => multipart, futures-util (many parent deps):
cargo update --package memchr --precise 2.5.0
# rust-ini:
cargo update --package ordered-multimap --precise 0.4.0Is this really necessary? I wouldn't mind this change at all on master, but I am way more conservative with patch releases.
It's a dev dependency, so I really don't see any issue with that especially when it's within the MSRV.
It should be a minimum version of 1.27, but could be up to 1.29.1 (last supported tokio release for MSRV of 1.56). That would require some extra explicit pinning, thus preferring tokio ="1.27" is better. Next paragraph with bullet points explains why.
Unfortunately from tokio 1.28 the resolver will then allow an incompatibility due to h2 => tokio-util.
h2 0.3.12 would keep tokio-util on 0.6.x. While h2 could support up to 0.3.20.. the 0.7.x series of tokio-util introduced a change in 0.7.9 that is not compatible with rust-version = "1.56" - but that was not bumped upon package release.tokio-util 0.7.8 would work too, but parents like warp => hyper can resolve to a release of hyper that raises the min semver of h2 to 0.3.17, but prior to warp 0.3.3 there is a direct dependency on tokio-util 0.6.x which would make cargo update for this transitive dependency a bit of a hassle over time (requires cargo update --package to reference the resolved tokio-util version to adjust via --precise).warp can be bumped up to 0.3.5 which makes it easier to pin tokio-util 0.7.8:
tokio = "1.27" in Cargo.toml paired with cargo +nightly update -Z direct-minimal-versions -Z msrv-policy.-Z msrv-policy and adding the tokio-util 0.7.8 pin, that works well too and likely results in a better dependency tree.The problem I am trying to solve with this MR is that CI is failing with the declared MSRV of
1.56. We want to test our MSRV in CI as otherwise it's almost certain we'll accidentally violate it.
I'm not sure if I agree with the need to run tests on the MSRV:
Cargo.lock ensures the main dependencies are compatible? If not, would it be wrong to take the approach of testing with dev-dependencies unsupported on MSRV until proven otherwise?examples compile, but I'm a bit conflicted on the importance of the examples being constrained to the MSRV too 🤷♂️ (supporting a minimum version of rust to compile a crate is one thing, enforcing it on the examples and tests another)Presently, the main dependencies build fine without a lockfile, but this is only because newer releases of some crates are ignored as the MSRV considers the Cargo.toml invalid. At some point an implicit dependency bump could break that and require maintaining a Cargo.lock that the downstream needs to manage to maintain the MSRV.
Many downstream aren't likely stuck on 1.56.1, so while that possibility could occur at some point, the impact on downstream users is unclear.
The main concern isn't so much upstreams breaking MSRV then:
Cargo.lock or alternatively raising MSRV.Cargo.toml or introduce any changes into it's own source that would raise the MSRV accidentally.
Cargo.lock helps verify the crate can build on the MSRV constraint with a known set of compatible dependencies.dev-dependencies are used for both examples and tests, but aren't part of the published crate for downstreams to use. MSRV could break there:Cargo.lock.Since running the examples on the target MSRV can require version pinning with Cargo.lock, the user would need to look at how we manage that (if we document it), or figure it out for themselves.
As opposed to keeping examples and tests independent of that MSRV constraint, and just focusing on correctness which is far less of a maintenance burden?
I've managed to satisfy the MSRV with reduced effort, but it appears quite a few implicit deps made changes in recent months that would have been missed if this was tackled before then, so I don't consider it worthwhile as opposed to an MSRV bump.
Proper approach (rejected)
Agree, this is not really feasible to resolve properly, not for MSRV of 1.56.0. It's more at risk of breaking again too if upstreams aren't careful with their updates.
Minor bodging (attempted, failed) Major bodging
Not too different vs "Proper approach", but easier to implement, however maintenance burden may be a bit concerning (could be a signal for bumping MSRV).
Not sure what the difference between the two bodging descriptions is?
The concern with downstreams appears to be the same as with MSRV bumps that commonly ripple up the chain, bumping MSRVs or requiring those with older MSRV requirements to enjoy maintaining a Cargo.lock (as version pinning from Cargo.toml is strongly discouraged from what I've seen).
In principle the correct solution to testing with the MSRV is as follows:
This seems good 👍
I think given an MSRV as a baseline, Cargo.toml can then express the minimum versions semver as what satisfies the MSRV, no earlier since that's already a hard requirement? (at least edition sets a hard boundary on MSRV, and the rust-version field can too although opt-out is supported)
This would not require implicit dependencies to build with -Z minimal-versions - especially when (EDIT: Only valid with rust-version info can be leveraged by the resolver-Z msrv-policy).
UPDATE: Might be best served with: cargo +nightly update -Z direct-minimal-versions -Z msrv-policy
ijackson
commented
7 months ago tl;dr:
@matthiasbeyer Please would you merge this MR now, despite @polarathene's interventions, which I have tried to summarise and briefly analyse, below.
I have read these messages. It's not so easy to see what they're trying to achieve, but I think I have grasped the main points.
The bulk of the messages consists of detailed analysis of the bugs in our dependencies, which are causing a plain -Z minimal-versions to fail. There are some observations about the maintenance status of our dependencies. I don't think this is actionable; at least, not on the config-rs stable release branch. It might be actionable as MRs against some of our dependencies, or as a programme of improvement for our mainline.
Also, @polarathene doubts whether it is a good idea to run the tests in our MSRV CI check. I think running the tests in CI, even in the MSRV check, is important. But I also think this is not something we should be changing on the stable branch at this stage. Committing a Cargo.lock is a simple intervention which alllows us to continue to run those tests; which has been recognised by Rust upstream as a good approach; and which has no impact on our users. A proposal to not run the tests during MSRV checks should be made on the mainline, in its own MR, and shouldn't form part of fixing the CI on the release branch.
I'm not sure whether these interventions are intended to be blocking. But I am requesting that you proceed as if they aren't.
Thanks for your enthusiasm, but I am finding your contributions here very frustrating.
It is quite unclear whether you intend to block this MR to fix the CI on the stable branch? Because that is the effect your messages are having. Even your "tl;dr" is 11 paragraphs and needs a "tl;dr"!
In practical terms, the maintainers are naturally going to be reluctant to proceed unless they can see whether there is consensus on my proposal, and they feel they understand the underlying issues. But that requires reading and comprehend everything you've written. But the rate at which you are producing text seems to exceed the rate at which the maintainers are able to digest it.
The maintainers must also figure out whether they think you are objecting, and, if so, whether your objections are sound. But it is very unclear from your messges whether you intend to object. That important framing is lacking.
There are probably improvements that could be made to the approach I have taken here. But I don't think your messages offer a viable alternative. We need the CI fixed on the stable branch. Further improvements could be made in followup MRs. Therefore, with some reluctance, I am inviting @matthiasbeyer to disregard your comments.
Your messages would have been much less problematic if they had started with:
tl;dr: Please merge this MR
Summary
I have done some investigation of the problems with our dependencies and our MSRV. The full report of my findings is below. You don't need to read it, but I wanted to write it down for posterity and in case we might want to try to improve things further.
I've put it in this MR since I wasn't sure I wanted to file a separate ticket and I'm not sure ithe information is actionable.
If indeed that was your intent.
polarathene
commented
7 months ago There are probably improvements that could be made to the approach I have taken here. But I don't think your messages offer a viable alternative. We need the CI fixed on the stable branch. Further improvements could be made in followup MRs. I am inviting @matthiasbeyer to disregard your comments.
-Z minimal-versions.tokio-util@0.7.9.While my 2nd message opens early with:
cargo +nightly update -Z direct-minimal-versions -Z msrv-policy
cargo update --package memchr --precise 2.5.0
cargo update --package ordered-multimap --precise 0.4.0cargo update instead of 10.
tokio-util (requires bumping warp to warp = "=0.3.5", which should be acceptable, otherwise tokio = "1.27" min version inCargo.toml`).Your messages would have been much less problematic if they had started with:
I prefaced the two messages with a disclaimer that:
The maintainers must also figure out whether they think you are objecting, and, if so, whether your objections are sound. But it is very unclear from your messages whether you intend to object. That important framing is lacking.
cargo check --tests or similar need to run with the MSRV, I am not blocking on that, it is just an opinion.Cargo.lock being worthwhile, I raised a concern with how dependabot may unintentionally adjust when Cargo.lock is committed and behave as if config-rs is an app, rather than a library. I expressed that I'd like to look into that first.@matthiasbeyer Please would you merge this MR now, despite @polarathene's interventions, which I have tried to summarise and briefly analyse, below.
While I apologize for the misunderstanding / confusion my messages caused you, I find this a tad disrespectful to request (I was notified around 2am).
Since your applying pressure, I am responding early with the above (before I was ready for my follow-up comment later on today). Likewise with the following suggestion (verified to work).
Here are the minimal changes that will work:
git clone https://github.com/mehcode/config-rs -b release-0.13.x --depth 1 .
# Requires `rust-version = "1.56.0" in Cargo.toml, and `warp = "=0.3.5"`:
cargo +nightly update -Z msrv-policy
# 3 pins for MSRV 1.56 compatibility (_due to lack of support for `-Z msrv-policy`_):
cargo update --package memchr --precise 2.5.0
cargo update --package ordered-multimap --precise 0.4.0
cargo update --package tokio-util --precise 0.7.8
# These both pass successfully:
cargo +1.56.1 check --tests
cargo +1.56.1 clippy --locked --offline --all-targets --all-features -- -D warningsIgnore the earlier -Z direct-minimal-versions suggestion as Cargo.toml presently misrepresents many direct deps supported min version for the MSRV. I have resolved what those are if they'd be of interest.
polarathene
commented
7 months ago
Your documented approach (via commit message) is 9 deps being updated with
-Z minimal-versions.One of which is relying on a version ref (fragile),
tokio-util@0.7.9.
For reference, this already fails within 2 weeks since PR was opened:
$ cargo +nightly update -Z minimal-versions -p tokio-util@0.7.9
error: package ID specification `tokio-util@0.7.9` did not match any packages
Did you mean one of these?
tokio-util@0.6.10
tokio-util@0.7.10
# Fragile:
$ cargo +nightly update -Z minimal-versions -p tokio-util@0.7.10Also:
# Still broken:
$ cargo +1.56.1 check --tests
error: package `scoped-tls v1.0.1` cannot be built because it requires rustc 1.59 or newer, while the currently active rustc version is 1.56.1
$ cargo +1.56.1 clippy --locked --offline --all-targets --all-features -- -D warnings
error: package `chrono v0.4.31` cannot be built because it requires rustc 1.57.0 or newer, while the currently active rustc version is 1.56.1cargo +nightly update -Z minimal-versions -p chrono will fail without using --precise or updating the min version in Cargo.toml since the actual min compatible version for MSRV tests to pass is 0.4.23, and 0.4.26 is the last release supporting MSRV of 1.56.
EDIT: Oh, not once was I corrected about the MSRV being 1.59. The README on the branch and prior discussion in this PR I asked about the MSRV multiple times and came to the conclusion of it being 1.56.1 since that's what cargo-msrv deduced and what the README documents.
So if anything, not only is my alternative lockfile generation simpler, it allows to continue supporting the original MSRV of 1.56.1 instead of the pattern of bumping the MSRV for CI to pass.
ijackson
commented
7 months ago I'm going to try to limit myself to comments which will hopefully allow us to move forward.
@polarathene Thanks for the alternative MR #495.
Thanks also for the investigations. I agree that it's likely that the actual MSRV is 1.56 rather than 1.59. I didn't set out, with this MR, to lower the MSRV of the 0.13.x branch. But that seems like it would be a good thing to do.
I do think that we should continue to run tests and examples with our MSRV in CI. I think we must do that with a fixed Cargo.lock, since otherwise our CI is likely to (once again) break due to changes in our dependency stack.
It is not so easy to produce a working Cargo.lock. I note that #495 does not produce a Cargo.lock capable of running the tests and examples. I used the runes in https://github.com/mehcode/config-rs/pull/495#issuecomment-1793634940, with that branch, and it didn't work for me. (And, CI on that MR is failing, but that's partly due to that branch not having a Cargo.lock at all.)
My approach to producing a working Cargo.lock was indeed not very principled. My own commit message was not intended as a recipe for future use. It was intended as an explanation of what I had done, for traceability and documentation.
I am very open to an alternative method of generating a working Cargo.lock. In whatever way we produce it, the method that was used generating it ought to be written down somewhere. (And as I say I think it needs to be committed to the tree and used for our MSRV CI.)
If we can make a stable and reproducible way of generating a working Cargo.lock that would be ideal; we could even commit the generation script. That's what we do in the Arti project. But my attempts to do that for config-rs were not successful.
The concern about dependabot could perhaps be resolved by committing the Cargo.lock under a different name. In derive-adhoc I have a Cargo.lock.minimal. We would have to change the CI to cp Cargo.lock.minimal Cargo.lock in the MSRV tests.
polarathene
commented
7 months ago It is not so easy to produce a working
Cargo.lock. I note that #495 does not produce aCargo.lockcapable of running the tests and examples.I used the runes in #495 (comment), with that branch, and it didn't work for me.
How did you test? I had verified with:
cargo +1.56.1 check --tests
cargo +1.56.1 clippy --locked --offline --all-targets --all-features -- -D warningsIt's possible that something may have changed upstream since I last tested which may have introduced another resolver issue, I will verify it again soon.
If there is a different command that fails, please let me know and I'll sort that out. If I'm unable to reproduce the failure I would be very interested in knowing what is different with our environments (I used the official Rust Docker image, cloned the release branch and generated Cargo.lock with commands I shared on the updated Cargo.toml).
The concern about dependabot could perhaps be resolved by committing the Cargo.lock under a different name.
Yes, I have seen Cargo.lock.msrv is quite common.
I haven't had time yet to continue looking into that concern, I should be able today. I've not configured dependabot before but assume there's some explicit option to alleviate the implicit detection concern 👍
I do think that we should continue to run tests and examples with our MSRV in CI. I think we must do that with a fixed Cargo.lock, since otherwise our CI is likely to (once again) break due to changes in our dependency stack.
I am all good with that 👍
If we can make a stable and reproducible way of generating a working Cargo.lock that would be ideal; we could even commit the generation script.
Documenting the generation is probably sufficient for now 👍
How would you like to proceed?
I could commit Cargo.lock similar to what you've done here on my PR if you're ok with that approach? It may help with verifying the lock works on your end and the CI should then pass.
EDIT: Just noticed https://github.com/mehcode/config-rs/pull/496 reading that now 😅
ijackson
commented
7 months ago How would you like to proceed?
EDIT: Just noticed #496 reading that now 😅
Right. Thanks for the thumbs-up there.
I could commit
Cargo.locksimilar to what you've done here on my PR if you're ok with that approach? It may help with verifying the lock works on your end and the CI should then pass.
I suggest we merge #496 and then you consider rebasing #495 on top of it. If #495's Cargo.toml is compatible its CI will pass, then.
Otherwise maybe it will be somehow possible to make a Cargo.lock that does work with #495. AFAICT from reading #495, the process of making a working Cargo.lock may be easier with those dependency updates. Like I say in #496, if we can come up with a better way of generating a working Cargo.lock then I'm all in favour. I just think it should be committed. If we merge #496, committed as Cargo.lock.msrv.
polarathene
commented
7 months ago How did you test? I had verified with:
I didn't get a response to this, I have verified again.
Here are the exact steps:
# Reproduction environment:
docker run --rm -it --workdir /tmp/reproduction rust bash
# Prepare:
# Clone PR branch: https://github.com/mehcode/config-rs/pull/495
git clone https://github.com/mehcode/config-rs -b 'chore/0.13.x-min-versions' --depth 1 .
rustup toolchain install nightly 1.56.1
rustup component add --toolchain 1.56.1 clippy
# Create lockfile and pin deps with bad msrv-policy support:
cargo +nightly update -Z msrv-policy
cargo update --package memchr --precise 2.5.0
cargo update --package ordered-multimap --precise 0.4.0
cargo update --package tokio-util --precise 0.7.8
# Verify tests pass:
cargo +1.56.1 check --tests
cargo +1.56.1 clippy --locked --offline --all-targets --all-features -- -D warningsI suggest we merge #496 and then you consider rebasing #495 on top of it. If #495's
Cargo.tomlis compatible its CI will pass, then.
I am fine with this 👍
EDIT: Done, #495 has been updated and awaiting review: https://github.com/mehcode/config-rs/pull/495#issuecomment-1798007587
ijackson
commented
7 months ago Closing since #492 was merged.
ijackson
commented
7 months ago I mean #496
See individual commit messages for rationale, and how I made this lockfile.