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

Cargo's CI is not validating that the Cargo.lock is up-to-date correctly. #12082

Closed
ehuss opened this issue May 3, 2023 · 3 comments · Fixed by #12084
Closed

Cargo's CI is not validating that the Cargo.lock is up-to-date correctly. #12082

ehuss opened this issue May 3, 2023 · 3 comments · Fixed by #12084
Labels
A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc.

Comments

@ehuss
Copy link
Contributor

ehuss commented May 3, 2023

The check here does not properly check if Cargo.lock is up-to-date.

For example, current master has the following diff:

diff --git a/Cargo.lock b/Cargo.lock
index 8b8a0ed1c..0c1752488 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1403,7 +1403,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "4fc78f47095a0c15aea0e66103838f0748f4494bf7a9555dfe0f00425400396c"
 dependencies = [
  "bstr",
- "home 0.5.5 (registry+https://github.com/rust-lang/crates.io-index)",
+ "home 0.5.5",
  "once_cell",
  "thiserror",
 ]

AFAIK, the only way to check if it is up-to-date without running a build is to use cargo metadata --locked or cargo tree --locked. I'm not sure what the decisions were for making this a separate step instead of just adding --locked to the normal test command?

@ehuss ehuss added the A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. label May 3, 2023
@epage
Copy link
Contributor

epage commented May 3, 2023

I'm not sure what the decisions were for making this a separate step instead of just adding --locked to the normal test command?

The answer to that is in #11994

The downside with doing this in a tet job is that it means someone doesn't collect any feedback if this error occurs. This is also why distinct clippy jobs are nice so warnings about builds don't prevent seeing test errors or the other way around

I'd also add that I would be concerned about what is under test being subtle and people accidentally breaking it.

@weihanglo
Copy link
Member

We can switch to cargo metadata --locked for sure, though I don't quite understand why the diff is a proof of improper check.

@ehuss
Copy link
Contributor Author

ehuss commented May 4, 2023

Hm, something fishy is going on. cargo metadata --locked also doesn't fail. I tested it yesterday and it failed then, but it looks like I maybe had some other changes in my tree.

If you check out current master (6240788) and run cargo check, you should end up with a Cargo.lock that is modified with the above diff. I thought --locked is supposed to error if that happens. I thought it compared it textually. Looking closer, it looks like it re-serializes the resolve which re-normalizes it. The two sources are equivalent, which means things haven't actually changed.

I'll post a lock update and close this and chalk it up as a merge error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants