Fix fingerprint calculation for patched deps. #6493
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
|
I'd like to propose backporting this to beta, just to avoid annoyance when working in the rustc repo (every time you rebase, you rebuild the compiler, and then forced to build it again!). |
|
SGTM! Working in the rustc repo is painful enough, as is 😄 |
|
looks like it was not clear how this worked even in the PR https://github.com/rust-lang/cargo/pull/4125/files#r121797031 . Is there an easy way to ensure this is still fast, it looks ok but it would be nice to double check? Otherwise looks good to me, and I would be ok with a backport. |
|
@bors: r+ Nice find! A backport also sounds good to me! |
|
📌 Commit ed98cab has been approved by |
Fix fingerprint calculation for patched deps. If you have A→B→C where B and C are in a registry, and you `[patch]` C, the fingerprint calculation wasn't working correctly when C changes. The following sequence illustrates the problem: 1. Do a build from scratch. 2. Touch a file in C. 3. Build again. Everything rebuilds as expected. 4. Build again. You would expect this to be all fresh, but it rebuilds A. The problem is the hash-busting doesn't propagate up to parents from dependencies. Normal targets normally aren't a problem because they have a `LocalFingerprint::MtimeBased` style local value which always recomputes the hash. However, registry dependencies have a `Precalculated` style local value which never recomputes the hash. The solution here is to always recompute the hash. This shouldn't be too expensive, and is only done when writing the fingerprint, which should only happen when the target is dirty. I'm not entirely certain why the caching logic was added in #4125. Fixes rust-lang/rust#57142
|
☀️ Test successful - status-appveyor, status-travis |
|
@Eh2406 I did some timing tests of time spent computing hashes. Building cargo it spends about 700µs, and a noop build spends about 300µs. There was no difference with or without this PR for normal builds. This should only cost a little extra for the |
|
Thank you for indulging my paranoia! That sounds more than acceptable. |
Update cargo 24 commits in 0d1f1bbeabd5b43a7f3ecfa16540af8e76d5efb4..34320d212dca8cd27d06ce93c16c6151f46fcf2e 2018-12-19 14:45:14 +0000 to 2019-01-03 19:12:38 +0000 - Display environment variables for rustc commands (rust-lang/cargo#6492) - Fix a very minor race condition in `cargo fix`. (rust-lang/cargo#6515) - Add a high-level overview of how `fix` works. (rust-lang/cargo#6516) - Add dependency `registry` to `cargo metadata`. (rust-lang/cargo#6500) - Fix fingerprint calculation for patched deps. (rust-lang/cargo#6493) - serialize version directly (rust-lang/cargo#6512) - use DYLD_FALLBACK_LIBRARY_PATH for dylib_path_envvar on macOS (rust-lang/cargo#6355) - Fix error message when resolving dependencies (rust-lang/cargo#6510) - use PathBuf in cargo metadata (rust-lang/cargo#6511) - Fixed link to testsuite in CONTRIBUTING.md (rust-lang/cargo#6506) - Update display of contents of Cargo.toml (rust-lang/cargo#6501) - Update display of contents of Cargo.toml (rust-lang/cargo#6502) - Fixup cargo install's help message (rust-lang/cargo#6495) - testsuite: Require failing commands to check output. (rust-lang/cargo#6497) - Delete unnecessary 'return' (rust-lang/cargo#6496) - Fix new unused patch warning. (rust-lang/cargo#6494) - Some minor documentation changes. (rust-lang/cargo#6481) - Add `links` to `cargo metadata`. (rust-lang/cargo#6480) - Salvaged semver work (rust-lang/cargo#6476) - Warn on unused patches. (rust-lang/cargo#6470) - don't write a an incorrect rustc version to the fingerprint file (rust-lang/cargo#6473) - Rewrite `login` and registry cleanups. (rust-lang/cargo#6466) - [issue#6461] Fix cargo commands list (rust-lang/cargo#6462) - Restrict registry names to same style as package names. (rust-lang/cargo#6469)
|
@ehuss I see you updated the cargo submodule in rust-lang/rust#57315. That puts this change on the train tracks for the next release train. But at what point would rustbuild be fixed with this? The dogfooding isn't clear to me. What version of cargo does rustbuild use? |
|
Rustbuild uses the beta compiler. The exact version is recorded in stage0.txt. So to fix this, the beta compiler needs to be updated. That's why there is #6514. Then the beta rustc needed to be updated (rust-lang/rust#57292). Fortunately the rollup finished with about 10 minutes to spare before the next beta was released (beta build starts at 3:20 am UTC AFAIK). Now that the beta is released, stage0 can be bumped (rust-lang/rust#57336). Once that lands, then it should be fixed. 😄 |
If you have A→B→C where B and C are in a registry, and you
[patch]C, the fingerprint calculation wasn't working correctly when C changes. The following sequence illustrates the problem:The problem is the hash-busting doesn't propagate up to parents from dependencies. Normal targets normally aren't a problem because they have a
LocalFingerprint::MtimeBasedstyle local value which always recomputes the hash. However, registry dependencies have aPrecalculatedstyle local value which never recomputes the hash.The solution here is to always recompute the hash. This shouldn't be too expensive, and is only done when writing the fingerprint, which should only happen when the target is dirty. I'm not entirely certain why the caching logic was added in #4125.
Fixes rust-lang/rust#57142