rfc: duplicate dep handling#3230
rfc: duplicate dep handling#3230justus-camp-microsoft wants to merge 2 commits intomicrosoft:mainfrom
Conversation
| // where cargo gives them distinct symbol names, or crates that are | ||
| // only used at build time. Add entries here only when the duplicate | ||
| // cannot be reasonably unified. | ||
| let allowed: std::collections::BTreeSet<&str> = [ |
There was a problem hiding this comment.
Do these not cause reproducible build issues?
|
|
||
| let sh = XtaskShell::new()?; | ||
| let output = sh | ||
| .cmd("cargo") |
There was a problem hiding this comment.
Would it be easier to parse our Cargo.lock instead?
There was a problem hiding this comment.
Pull request overview
Adds a new cargo xtask fmt pass intended to detect duplicate crate versions (to improve reproducibility), and updates a few workspace dependencies/patches to eliminate known duplicates (notably base64/pbjson and nix).
Changes:
- Add a new
DuplicateDepsfmt pass that runscargo tree --duplicatesand fails when “problematic” duplicates are found. - Update workspace dependencies (
nix,rustyline) and[patch.crates-io]entries (pbjson*,gptman) to unify versions and remove duplicates. - Refresh
Cargo.lockaccordingly (including removal ofbase64 0.13.1).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
xtask/src/tasks/fmt/mod.rs |
Wires the new DuplicateDeps pass into the default fmt pass list. |
xtask/src/tasks/fmt/duplicate_deps.rs |
Implements the duplicate-dependency detection logic and a small parser unit test. |
Cargo.toml |
Bumps/selects dependencies and adds/updates patches to unify transitive versions. |
Cargo.lock |
Lockfile updates reflecting the dependency unification and bumps. |
| // Any duplicate non-proc-macro crate with the same package name | ||
| // can cause nondeterministic function ordering in LLVM's fat LTO | ||
| // pass, regardless of how different the versions are. | ||
| let versions_str: Vec<_> = versions.iter().map(|s| s.as_str()).collect(); | ||
| problems.push(format!("{name}: {}", versions_str.join(", "))); | ||
| } |
There was a problem hiding this comment.
The pass currently treats any duplicate non-proc-macro crate name as a problem (unless allowlisted), but the module docs and the error message indicate only duplicates with the same “effective major” (or name-colliding versions) are problematic. This mismatch will either produce false positives (failing on benign cross-major duplicates) or under-enforce the intended rule—please align the implementation with the documented criteria (e.g., group versions by effective-major and only flag duplicates within the same group, or update the docs/message to match the actual policy).
| // Crates that are known to have multiple versions and are acceptable. | ||
| // These are typically different major versions (e.g., bitflags 1 vs 2) | ||
| // where cargo gives them distinct symbol names, or crates that are | ||
| // only used at build time. Add entries here only when the duplicate | ||
| // cannot be reasonably unified. |
There was a problem hiding this comment.
The allowlist is keyed only by crate name, so it will also suppress duplicates within the same (effective) major for those crates (e.g., two different 2.x versions of bitflags), even though the surrounding comments justify these entries as cross-major coexistence. Consider making the allowlist version-aware (name + effective-major), or checking effective-major first and only skipping entries where the duplicates are known-safe (cross-major / build-only).
| //! Check for duplicate crate versions that can cause nondeterministic builds. | ||
| //! | ||
| //! When multiple versions of the same crate (by package name) are linked into | ||
| //! the final binary, LLVM's fat LTO pass may order them nondeterministically |
There was a problem hiding this comment.
Is there no codegen option to fix this instead? I'm surprised something like codegen_units=1 doesn't handle this already.
No description provided.