Don't update lockfiles from previous Cargo versions if --locked is passed
#4684
Conversation
src/cargo/ops/lockfile.rs
Outdated
| // Old lockfiles have unused `[root]` section, | ||
| // just ignore it if we are in the `--frozen` mode. | ||
| if !strict && orig.starts_with("[root]") { | ||
| orig = orig.replacen("[root]", "[[package]]", 1); |
There was a problem hiding this comment.
This isn't quite right because IIRC the [root] was the lexicographically last package (if you have a bunch of path dependencies it's the last one alphabetically). Essentially I've seen cargo build on nightly move around this section.
That being said I'm sort of ok just comparing TOML here, I don't think we need to compare literal encodings per se, but rather if Cargo generates one encoding and the encoding on the filesystem is the same, it seems fine to avoid rewriting?
There was a problem hiding this comment.
Ah, an excellent catch!
I initially though of comparing TOMLs, but then decided that it might be too slow (because of the hyper-optimized has_crlf). But we certainly can do anything inside this if, as it's a cold path.
src/cargo/ops/lockfile.rs
Outdated
| @@ -97,6 +91,23 @@ pub fn write_pkg_lockfile(ws: &Workspace, resolve: &Resolve) -> CargoResult<()> | |||
| }) | |||
| } | |||
|
|
|||
| fn are_equal_lockfiles(mut orig: String, current: &str, strict: bool) -> bool { | |||
There was a problem hiding this comment.
I think strict here has an inverse meaning of its name, if strict is true that means that we can update the lock file, but I'd expect strict: true to mean that updates are disallowed? Perhaps the Config could be passed in here to be inspected locally where necessary?
There was a problem hiding this comment.
Yeah, naming is confusing indeed, I've flipped between strict and relaxed several times. In itself, I think strict is right though: strict comparison means that we do byte-for-byte comparison, and !strict means that we deem lockfiles equal even if there are minor differences. The tricky bit is that we need to do approximate comparison if lockfile update is not allowed :)
I'll just rename this flag to lock_update_allowed to avoid inventing confusing abstractions :) Re passing config, I doubt we ever need anything except lock_update_allowed boolean, and in theory keeping this flag a simple bool allows us to write unit-tests for this function :)
|
Looks great to me, thanks! r=me with one small nit |
|
It would be great if we could backport this to beta, and if possible add a general regression testing that Cargo supports lockfiles generated by 1.0 cargo (or some version, anyway) and doesn't attempt modification with --locked or equivalents. |
+1. What's the procedure? We wait until this PR is r+'ed and tested, and then I cherry-pick the commit onto 1.21 branch?
We do have such test!
This test is added in this pull request. It starts with a lockfile post 1.0 though, because we already add |
|
📌 Commit 63d83e8 has been approved by |
Don't update lockfiles from previous Cargo versions if `--locked` is passed Recently, we've stopped outputting `[root]` section to Cargo.lock files. The problem with this is that somebody might want to have a Cargo.lock file with a `[root]` section for compatibility with older Cargos, but at the same time use newer Cargo to build the crate. In this case, newer Cargo would remove `[root]` from the lockfile. Such updating of the lockfiles to the latest versions is a reasonable behavior, but it might be useful to be able to override it with `--locked` flag. Context: #4563 (comment) r? @alexcrichton
To contributing.md? That would be excellent! FYI, in intellij-rust we have |
|
@matklad oh I figured I'd put it next to the rust-lang/rust backport instructions -- https://github.com/rust-lang-nursery/rust-forge/blob/master/beta-backporting.md#backporting-in-rust-langcargo I don't mind it going anywhere though! |
|
Backport PR to cargo: #4687 |
|
☀️ Test successful - status-appveyor, status-travis |
|
Backport PR to rust: rust-lang/rust#45658 |
A previous patch in rust-lang#4684 attempted to fix this, but didn't work for the case where the [root] crate wasn't the first crate in the sorted package array.
A previous patch in rust-lang#4684 attempted to fix this, but didn't work for the case where the [root] crate wasn't the first crate in the sorted package array.
Do not update semantically equivalent lockfiles with --frozen/--locked. A previous patch in #4684 attempted to fix this, but didn't work for the case where the [root] crate wasn't the first crate in the sorted package array. cc @matklad -- fixes a problem noted in #4684 (comment) which appears to have gone unfixed r? @alexcrichton Beta backport will be posted as soon as this is reviewed. Merges cleanly.
Recently, we've stopped outputting
[root]section to Cargo.lock files. The problem with this is that somebody might want to have a Cargo.lock file with a[root]section for compatibility with older Cargos, but at the same time use newer Cargo to build the crate. In this case, newer Cargo would remove[root]from the lockfile. Such updating of the lockfiles to the latest versions is a reasonable behavior, but it might be useful to be able to override it with--lockedflag.Context: #4563 (comment)
r? @alexcrichton