fix(fingerpring): do not touch intermediate artifacts #7050
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
Now that the `mtime` of intermediate artifacts is not updated there's no need for this test anymore (it now fails because without the `mtime`s it cannot perform the intended GC operation).
|
Looks good! Let me know when you think it is ready! |
|
@Eh2406 Last commit is passing so ready. |
|
(forget to |
|
@bors r+ |
|
📌 Commit 009876a has been approved by |
bors
added
S-waiting-on-bors
…ate-artifacts-mtime, r=Eh2406 fix(fingerpring): do not touch intermediate artifacts Fixes #6972. The newly introduced [test](9aa7a4d) is [failing](https://travis-ci.com/rust-lang/cargo/jobs/209849725#L2569-L2580) as discussed in #6972 (comment) (replicating the issue). Removing the `touching of intermediate artifacts` as suggested fixes the issue, but makes the old test `simple_deps_cleaner_does_not_rebuild` fail. The `simple_deps_cleaner_does_not_rebuild` test is not needed anymore so it's removed. r? @Eh2406
|
☀️ Test successful - checks-travis, status-appveyor |
schomatis
deleted the
fix/fingerprint/dont-update-intermediate-artifacts-mtime
branch
|
Thanks @Eh2406 for the guidance throughout this issue! And also thanks to @ehuss for the great document about fingerprints which was what allowed me the insight to delve in this issue in the first place, I think is documentation like that one that empowers the community to take action in a project. |
|
Thanks for the help! We agree and are trying to improve the internal documentation. As someone with a fresh perspective what did you have trouble understanding / where can we improve the docs more? |
|
Well, if I have to name one off the top of my head the thing I struggled the most while navigating through the code was that I didn't have a clear high level perspective of where were the important bits of the system I had to follow to reason about how were the fingerprints used. I think something like that would belong in the architecture documentation or a sub-document of it (the current information there is useful but I feel it falls short to match the huge system on interconnected components Cargo encompasses). But now that you mention you're interested, if I see a concrete opportunity to improve documentation on a certain topic I'll submit an issue about it. |
Update cargo 17 commits in 807429e1b6da4e2ec52488ef2f59e77068c31e1f..4c1fa54d10f58d69ac9ff55be68e1b1c25ecb816 2019-06-11 14:06:10 +0000 to 2019-06-24 11:24:18 +0000 - Fix typo in comment (rust-lang/cargo#7066) - travis: enforce formatting of subcrates as well (rust-lang/cargo#7063) - _cargo: Make function style consistent (rust-lang/cargo#7060) - Update some fix comments. (rust-lang/cargo#7061) - Stabilize default-run (rust-lang/cargo#7056) - Fix typo in comment (rust-lang/cargo#7054) - fix(fingerpring): do not touch intermediate artifacts (rust-lang/cargo#7050) - Resolver test/debug cleanup (rust-lang/cargo#7045) - Rename to_url → into_url (rust-lang/cargo#7048) - Remove needless lifetimes (rust-lang/cargo#7047) - Revert test directory cleaning change. (rust-lang/cargo#7042) - cargo book /reference/manifest: fix typo (rust-lang/cargo#7041) - Extract resolver tests to their own crate (rust-lang/cargo#7011) - ci: Do not install addons on rustfmt build jobs (rust-lang/cargo#7038) - Support absolute paths in dep-info files (rust-lang/cargo#7030) - ci: Run cargo fmt on all workspaces (rust-lang/cargo#7033) - Deprecated ONCE_INIT in favor of Once::new() (rust-lang/cargo#7031)
Fixes #6972.
The newly introduced test is failing as discussed in #6972 (comment) (replicating the issue). Removing the
touching of intermediate artifactsas suggested fixes the issue, but makes the old testsimple_deps_cleaner_does_not_rebuildfail. Thesimple_deps_cleaner_does_not_rebuildtest is not needed anymore so it's removed.r? @Eh2406