[release-0.13.x] fix(CI): Commit an MSRV compatible Cargo.lock
#492
Conversation
We want a later version than 1.0 since earlier versions have various bugs, and also dependency versions which are insufficient to rule out bugs in lower crates. Emprically, 1.30 requires a newer Rust than our MSRV. Therefore, request 1.29. Signed-off-by: Ian Jackson <[email protected]>
This will allow us to pin the dependencies in CI, avoiding uncontrolled changes from our dependencies causing our workflows to break. Signed-off-by: Ian Jackson <[email protected]>
This is not so trivial. In theory,
cargo +nightly update -Z minimal-versions generate-lockfile
ought to work.
However, there are rather many places in our dependency tree where
some crate A depends on crate B, version V, but it actualloy doesn't
build with version V, but requires something considerably newer.
There seemed to be particular problems in the parts of the dependency
graph near "pest". My atteempts at starting with the minimal versions
and upgrading crates as needed, were not successful.
However, I was able to converge reasonably quickly from the other end:
starting with the lockfile generated by 1.59, using recent versions,
and then repeatedly downgrading crates whose (actual, or declared)
MSRV was too high.
the following recipe generates a working lockfile:
cargo +1.59 generate-lockfile
cargo +nightly update -Z minimal-versions -p linux-raw-sys
cargo +nightly update -Z minimal-versions -p tokio
cargo +nightly update -Z minimal-versions -p rustix
cargo +nightly update -Z minimal-versions -p tempfile
cargo +nightly update -Z minimal-versions -p reqwest
cargo +nightly update -Z minimal-versions -p byteorder
cargo +nightly update -Z minimal-versions -p h2
cargo +nightly update -Z minimal-versions -p log
cargo +nightly update -Z minimal-versions -p tokio
cargo +nightly update -Z minimal-versions -p [email protected]
So that is what we commit here.
Signed-off-by: Ian Jackson <[email protected]>
|
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 |
| @@ -41,7 +41,7 @@ pathdiff = "0.2" | |||
| serde_derive = "1.0.8" | |||
| float-cmp = "0.9" | |||
| chrono = { version = "0.4", features = ["serde"] } | |||
| tokio = { version = "1", features = ["rt-multi-thread", "macros", "fs", "io-util", "time"]} | |||
| tokio = { version = "1.29", features = ["rt-multi-thread", "macros", "fs", "io-util", "time"]} | |||
There was a problem hiding this comment.
Is this really necessary? I wouldn't mind this change at all on master, but I am way more conservative with patchreleases.
I wouldn't argue too hard here, just want to make sure that you really think it is necessary here...
There was a problem hiding this comment.
TBH that was from my attempts at the other approach. I'm trying it without.
There was a problem hiding this comment.
I think this is going to work. Should I rebase this branch without that commit? I then need to rerun the generation script which, in practice, ends up with a Cargo.lock using tokio 1.28 rather than 1.29, so the tip will be different.
Alternatively I could push a revert of the tokio dep change, and leave the lockfile alone.
There was a problem hiding this comment.
As I expected my test passed. (https://github.com/ijackson/config-rs/actions/runs/6629032741, but that branch is ... rather odd.) So we should drop the change to tokio in Cargo.toml.
Should we (1) ship the Cargo.lock from this MR which was made with the tokio change using the recipe in the commit message (ie, git revert the tokio dependency commit), or (2) ship a Cargo.lock which was made without the tokio change, for somewhat greater coherency?
If 1, feel free to push that revert yourself. It should pass CI and we can merge this.
If 2, LMK and I'll prepare the branch and force push. (Or you can do it, e.g. to verify my work....)
There was a problem hiding this comment.
If I'm rebasing I'll fix the commit message typos @polarathene spotted. If we're doing the "revert" approach, I could still do that. Please let me know.
FTAOD I think either of the approaches above are fine and there is little to choose between them.
There was a problem hiding this comment.
Just to emphasize, if you don't revert this change (or use a min version of 1.27 instead), expect the fragile tokio-util pinning requirement.
Alternatively, raising warp will remove the 0.6 series of tokio-util, making it easier to pin tokio-util without a version ref.
0.7.8is compatible with Rust1.56.0and will resolve to that implicitly iftokiois <1.28. Otherwise it requires Rust1.60.- Or you can take the only pin some deps to their minimal versions approach instead that's been proposed. Even though that does not accurately represent using the latest compatible version possible for bugfixes / security patches that users on the MSRV likely would want and tests should be intended for ensuring is valid? 🤷♂️
This comment was marked as outdated.
This comment was marked as outdated.
Assuming the MSRV is intended as |
| [[package]] | ||
| name = "async-trait" | ||
| version = "0.1.74" |
There was a problem hiding this comment.
I just did cargo +1.55.0 check and it failed due to this dependency like cargo-msrv did. So it'd seem that the expected MSRV of this Cargo.lock is 1.56.1 after all?
There was a problem hiding this comment.
I'm a bit confused, where are you getting 1.55 from? I think the MSRV of the Cargo.lock I'm proposing is 1.56 (probably 1.56.0, although one should be using 1.56.1 anyway), and 1.56 is what's mentioned in the docs.
There was a problem hiding this comment.
Sorry I didn't see this reply earlier, 1.55 was from the cargo-msrv bisect I think, or it was just due to it settling on 1.56.1, so I tried 1.55.0 to check if it builds, but it failed as mentioned due to async-trait.
As you cleared up for me, tokio wasn't an issue at 1.33.0 because dev-dependencies were ignored unless running cargo check --tests, so the failure was only regarding a release build (_without dev-dependencies tokio isn't present in cargo tree / Cargo.lock either 👍 )
| [[package]] | ||
| name = "tokio" | ||
| version = "1.29.0" |
There was a problem hiding this comment.
I'd like to know how 1.33.0 is not compatible with an MSRV of 1.56.1?
EDIT: Because cargo +1.56.1 check --tests fails, MSRV for tokio 1.33.0 is 1.63.0.
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. Proper approach (rejected)In principle the correct solution to testing with the MSRV is as follows:
This is what I do in (for example) 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 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 Minor bodging (attempted, failed)The next most principled approach is to generate a lockfile with I tried this. But it got badly out of hand. Major bodgingSo instead, what I did was, by hook or by crook, generate a 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 |
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 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.
According to Tokio's README the MSRV of Tokio 1.30 onwards is 1.63. Empirically, 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. |
This comment was marked as outdated.
This comment was marked as outdated.
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 |
|
@matthiasbeyer I think this is just blocking on your decision about #492 (comment) |
|
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! 😆 Thanks for your contributions, all of you! I hope I can read through all of the above next week. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
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.
|
Reference: Commit message with mentioned recipeCommit: 5c4c72d This is not so trivial. In theory,
The following recipe generates a working lockfile: cargo +1.59 generate-lockfile
cargo +nightly update -Z minimal-versions -p linux-raw-sys
cargo +nightly update -Z minimal-versions -p rustix
cargo +nightly update -Z minimal-versions -p tempfile
cargo +nightly update -Z minimal-versions -p reqwest
cargo +nightly update -Z minimal-versions -p byteorder
cargo +nightly update -Z minimal-versions -p h2
cargo +nightly update -Z minimal-versions -p log
cargo +nightly update -Z minimal-versions -p tokio
cargo +nightly update -Z minimal-versions -p [email protected]
With a slight adjustment 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.0
Reference: Associated commit with messageEarlier commit with
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 Unfortunately from
I'm not sure if I agree with the need to run tests on the MSRV:
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 Many downstream aren't likely stuck on The main concern isn't so much upstreams breaking MSRV then:
Since running the examples on the target MSRV can require version pinning with 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.
Agree, this is not really feasible to resolve properly, not for MSRV of
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
This seems good 👍 I think given an MSRV as a baseline, This would not require implicit dependencies to build with UPDATE: Might be best served with: |
|
tl;dr: @matthiasbeyer Please would you merge this MR now, despite @polarathene's interventions, which I have tried to summarise and briefly analyse, below. Summary of @polarathene 's technical points, and my responsesI 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 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 I'm not sure whether these interventions are intended to be blocking. But I am requesting that you proceed as if they aren't. Personal, nontechnical, response to @polaratheneThanks 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:
If indeed that was your intent. |
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.0
I prefaced the two messages with a disclaimer that:
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 |
For reference, this already fails within 2 weeks since PR was opened: $ cargo +nightly update -Z minimal-versions -p [email protected]
error: package ID specification `[email protected]` did not match any packages
Did you mean one of these?
[email protected]
[email protected]
# Fragile:
$ cargo +nightly update -Z minimal-versions -p [email protected]Also: # 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.1
EDIT: Oh, not once was I corrected about the MSRV being So if anything, not only is my alternative lockfile generation simpler, it allows to continue supporting the original MSRV of |
polarathene
changed the title
Cargo.lock
|
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 It is not so easy to produce a working My approach to producing a working I am very open to an alternative method of generating a working If we can make a stable and reproducible way of generating a working The concern about dependabot could perhaps be resolved by committing the |
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
Yes, I have seen 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 am all good with that 👍
Documenting the generation is probably sufficient for now 👍 How would you like to proceed? I could commit EDIT: Just noticed #496 reading that now 😅 |
Right. Thanks for the thumbs-up there.
I suggest we merge #496 and then you consider rebasing #495 on top of it. If #495's Otherwise maybe it will be somehow possible to make a |
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 warnings
I am fine with this 👍 EDIT: Done, #495 has been updated and awaiting review: #495 (comment) |
|
Closing since #492 was merged. |
|
I mean #496 |
See individual commit messages for rationale, and how I made this lockfile.