rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.5k stars 2.37k forks source link

Different git shas for the same git repo in Cargo.lock silently ignored #14230

Open liskin opened 1 month ago

liskin commented 1 month ago

Problem

Given multiple dependencies fetched from the same git repo, e.g.:

[dependencies]
tracing = { git = "https://github.com/tokio-rs/tracing", branch = "v0.1.x", version = "0.1.40" }
tracing-subscriber = { git = "https://github.com/tokio-rs/tracing", branch = "v0.1.x", version = "0.3.18" }

and Cargo.lock referring to multiple different git refs, e.g.:

[[package]]
name = "tracing"
version = "0.1.40"
source = "git+https://github.com/tokio-rs/tracing?branch=v0.1.x#8b7a1dde69797b33ecfa20da71e72eb5e61f0b25"

[[package]]
name = "tracing-core"
version = "0.1.32"
source = "git+https://github.com/tokio-rs/tracing?branch=v0.1.x#6d00d7d9f72dc6797138a1062bc33073afbad5a1"

cargo tree (and even cargo build) will silently pick one of those git refs, ignoring the rest. All crates from the git repo will be built from the same revision, despite Cargo.lock specifying otherwise. No error or warning is emitted, and Cargo.lock isn't updated to reflect the actual revisions that end up being used.

The behaviour I expect is an error if --locked is passed to cargo.

In the absence of --locked, I'm not sure if I want to see an error or if Cargo.lock should be updated. Certainly it shouldn't just silently pick one though.

Alternatively, cargo could actually support having several different revisions for a single git repo. After all, if the crates in question were published into the registry, there'd be no problem of them having different versions.

Steps

  1. cargo init, cargo build, git add ., git commit
  2. cargo add --git https://github.com/tokio-rs/tracing --branch v0.1.x tracing tracing-subscriber, git commit
  3. cargo update --precise 8b7a1dde69797b33ecfa20da71e72eb5e61f0b25 tracing, git commit
  4. git checkout -p @^ (select only the tracing-core hunk)
  5. cargo tree — all tracing* crates are at 6d00d7d9, but Cargo.lock still specifies 8b7a1dde… for most of them

(the above steps executed today are captured in the following git repo for future reference: https://github.com/liskin/2024-07-10-cargo-lock-git-bug)

Possible Solution(s)

No response

Notes

It could be argued that cargo update will never produce such inconsistencies in Cargo.lock. Indeed, it was a "race condition" between two git pull requests: one person adding another dep from the repo, another bumping the revision of the existing deps. After merging the two independent PRs, we ended up with inconsistent revisions in Cargo.lock and were baffled why we can't use code that should now be available after the revision bump.

This was not an isolated incident. I went to check the rest of our codebase and found another instance of multiple different revs for a single git repo in Cargo.lock. So I'd argue this is not super rare, and should be handled by cargo somehow.

Version

cargo 1.79.0 (ffa9cf99a 2024-06-03)
release: 1.79.0
commit-hash: ffa9cf99a594e59032757403d4c780b46dc2c43a
commit-date: 2024-06-03
host: x86_64-unknown-linux-gnu
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Debian n/a (trixie) [64-bit]
weihanglo commented 1 month ago

Thanks for the report and the reproducible example!

We can look at this issue from several different angles.

It could be argued that cargo update will never produce such inconsistencies in Cargo.lock.

Cargo only distinguishes Git dependencies by the provided rev, branch, or tag value. If two Git dependencies are on the same branch or tag, they are considered coming from the same Git source. That being said, Cargo already supports depending on different revision today, via the rev field.

Hence, the argument is correct. If not it is definitely a bug.

In your case, it is kinda like Git merge algorithm contributing to this bug. A merge-conflict friendly lockfile is undoubtedly a thing we try to achieve. https://github.com/rust-lang/cargo/issues/13300 is a recent issue that manual edits cause weird lockfile behavior. There is also an idea that we could use a custom merge driver https://github.com/rust-lang/cargo/issues/1818.

In the absence of --locked, I'm not sure if I want to see an error or if Cargo.lock should be updated. Certainly it shouldn't just silently pick one though.

Yeah I don't think Cargo should just error out in this case. A previously working lockfile shouldn't become invalid in future version of Cargo, unless the lockfile version has been bumped (don't worry if you've never heard of it; that version should be opaque to normal end users). Even though, Cargo should try upgrading the old version to the new one. This kind of fix is not trivial. However, I think the first step of this could be a warning, letting user know that Cargo is randomly picking a revision because the lockile was manually edtied to a inconsistent state. After that we can consider a lockfile version bump https://github.com/rust-lang/cargo/issues/12120.