crate_universe adds registry+https://github.com/rust-lang/crates.io-index to Cargo.lock

[dae]

I recently updated from an old crate_universe to the one in the latest rules_rust release. Most things seem to be working well, but I've run into an issue with Cargo.lock being changed.

Each time I run REPIN=workspace bazel build, the Cargo.lock file is updated to add explicit references to the default registry, eg:

@@ -118,7 +118,7 @@ checksum = "7d6083ccb191711e9c2b80b22ee24a8381a18524444914c746d4239e21d1afaf"
 dependencies = [
  "askama_escape",
  "humansize",
- "nom 6.1.2",
+ "nom 6.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "num-traits",
  "percent-encoding",
  "proc-macro2",

After that, I can build with bazel successfully (though it splices twice - once with REPIN defined, and then the next time I run bazel without REPIN as well).

The problem is that the standard cargo does not include the explicit registry, so if I do some editing in Rust Analyzer, or use something like 'cargo add', the Cargo.lock file is updated to remove the registry entries, leaving a dirty file in git. And if I then run 'bazel build' again, it tries to splice again, and then fails reporting that the lock needs repinning.

Is there some way to get the two to agree on one particular format so they don't end up competing with each other?

Semi-related to https://github.com/bazelbuild/rules_rust/issues/1522

[illicitonion]

I started debugging this:

We overwrite the lockfile here: https://github.com/bazelbuild/rules_rust/blob/2bb077b3b7585e87bf6f5bfcce77f9470d1d5210/crate_universe/src/cli/generate.rs#L130-L132

AFAICT the bug here is that cargo-lock and cargo have different heuristics for when to include a registry. I think cargo proper does so if the crate exists in multiple registries, whereas cargo-lock seems to do so if there are multiple versions of the crate in use in your dep tree.

I'll keep digging a bit...

[dae]

Out of curiosity, is the updating of Cargo.lock just there to support the REPIN=1 / REPIN=cratename use cases, or is it necessary for other reasons as well? As someone not familiar with the inner workings of it, I must admit I found the behaviour a bit surprising - I expected the Cargo.toml/.lock files to be treated as the source of truth, with crate_universe using them as-is, or presenting an error that the lock file was out of date.

[illicitonion]

Yeah, it's only for when REPIN happens - I've got a draft PR put together which fixes the issue, will write some tests and get it up in the next couple of days!

[dae]

Just to confirm, does that mean that dependency updating when REPIN is set is just to avoid a separate manual call to 'cargo update'? If so, I can't speak for other people, but I'd personally prefer to manage Cargo.lock updates with the standard cargo tooling, with the Bazel dependencies derived from the Cargo.lock source of truth, like cargo-raze did. If that were practical, it would mean that updating the Bazel lockfile from the Cargo lockfile was an idempotent operation, and one could commit only the Cargo.lock file to the repo without fear of the Bazel deps falling out of sync. Would that make sense, or is there more going on that I'm not aware of?

[illicitonion]

Just to confirm, does that mean that dependency updating when REPIN is set is just to avoid a separate manual call to 'cargo update'?

Yes - particularly, this matters in cases where e.g. there isn't exactly one Cargo.lock file corresponding with exactly one Cargo.toml workspace (e.g. if folks are using annotations to add (extra) dependencies, where there may not even be any Cargo.toml file), or where you can't assume that folks have a cargo toolchain installed locally at all.

If so, I can't speak for other people, but I'd personally prefer to manage Cargo.lock updates with the standard cargo tooling, with the Bazel dependencies derived from the Cargo.lock source of truth, like cargo-raze did.

That's a supported workflow - if you REPIN=workspace rather than REPIN=1 it will do a super conservative update, and should just update the cargo bazel lockfile, not the Cargo.lock. (It will do so via a call to cargo update, but that should be a no-op). This isn't necessarily super discoverable, though, at the moment :) See also https://github.com/bazelbuild/rules_rust/issues/1522#issuecomment-1219470021

[dae]

Thanks Daniel, I appreciate you taking the time to elaborate, and for the fix in #1539. I'll stick with manually setting workspace for now, and have suggested it become the default on #1522.

[UebelAndre]

continuing https://github.com/bazelbuild/rules_rust/pull/1539#issuecomment-1238460872 here to not distract from the PR:

I opened https://github.com/rustsec/rustsec/issues/687 and want to see if we can instead do some minimal parsing of an existing lockfile to figure out how to render it. I think this would keep the functionality simpler and reduce some plumbing. Hopefully the maintainers there will respond. If folks don't fell that's appropriate definitely let me know 😄

[UebelAndre]

@dae would you be able to provide a repro for this issue? I can try to break it down further to try and respond to https://github.com/rustsec/rustsec/issues/687#issuecomment-1241465446

[dae]

He's a small repo that demonstrates the issue: https://github.com/ankitects/rules_rust_registry_repro

[UebelAndre]

Don't know if folks here saw https://github.com/rustsec/rustsec/issues/687#issuecomment-1275018145 but I feel the proper solution to this issue is to instead have cargo-lock serialized correctly. I just recently updated all dependencies in crate_universe (https://github.com/bazelbuild/rules_rust/pull/1591) to make sure pulling in new changes would be easy. Would someone be willing to submit a fix there so crate_universe can just bump a pin?

[illicitonion]

I feel the proper solution to this issue is to instead have cargo-lock serialized correctly

I'm not sure I agree it's more proper - cargo-lock's aim as a project isn't necessarily to be byte-for-byte compatible with outputting Cargo's idiosyncrasies, and if it's not a guarantee they're excited to sign up for, we may run into more issues in the future.

Given we actually are getting the raw output from cargo itself, it seems strange to round-trip that through a deserialize and reserialize - I'm not sure what value that's providing over just using cargo's output.

Regardless:

Would someone be willing to submit a fix there so crate_universe can just bump a pin?

I'm happy to try to put something together, but realistically it will be some weeks until I can find the time to do so.

[UebelAndre]

I was finally able to get around to writing up a patch for this:

• https://github.com/rustsec/rustsec/pull/767