Fix fingerprint for canceled build script. #6782
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
Thanks for writing all this up, the new docs look great!
The fix here also looks good to me too, so r=me with just a few nits here and there.
For solving the other issues you've opened (man you've got good eyes), I wonder if we could solve those by passing &Context to all of the child work tasks? That way each work item would have at least read-only access to the Context (as it's shared in parallel), and I think that using crossbeam should allow us to share lifetimes across the threads?
| //! first build) is also considered dirty. | ||
| //! - Dirty propagation is done in the `JobQueue`. When a Unit is dirty, the | ||
| //! `JobQueue` automatically treats anything that depends on it as dirty. | ||
| //! Anything that relies on this is probably a bug. The fingerprint should |
There was a problem hiding this comment.
This is definitely something we slowly realized over time, and I think at this point we should probably remove this logic in JobQueue perhaps? (fine to leave to a separate PR).
It may uncover a bug or two either in the test suite or in the wild, but it's all stuff we'd have to fix eventually anyway.
There was a problem hiding this comment.
That might be a worthwhile experiment, at least to see where things break in the testsuite.
| //! `RUST_LOG=cargo::core::compiler::fingerprint=trace cargo build` can be | ||
| //! used to display this log information. | ||
| //! - A "dep-info" file which contains a list of source filenames for the | ||
| //! target. This is produced by `rustc`'s `--emit=dep-info` flag. Cargo uses |
There was a problem hiding this comment.
I think technically we actually rewrite the output of the compiler, right? IIRC we have a binary-encoding (ish) which puts the cwd at the top and then all the paths are rewritten as relative to that (or something like that)
There was a problem hiding this comment.
Correct, I have tried to clarify it.
That sounds like it would be difficult. At this point, I think I'm going to leave things as-is for now. The issues I filed are somewhat difficult to hit, and any solution to fix them will require a lot of code changes. If at some point in the future that someone wants to do a major overhaul of fingerprinting, that would be a good opportunity to make sure all these edge cases are covered. I don't think these edge cases justify that much code churn right now. While working on this, it's been interesting to look at how other projects deal with it. Ninja for example has some of the same bugs with mtime handling. One thought is to add filesize (and maybe other metadata) to the fingerprint to reduce the chances of mtime errors. It wouldn't guard against modifications that do not change the size, but might help. I do see complaints about tools that rely heavily on hashing being too slow. |
|
@bors r=alexcrichton |
|
📌 Commit a09e295 has been approved by |
bors
added
S-waiting-on-bors
…chton Fix fingerprint for canceled build script. Fixes #6770. See that issue for a description. The fix is to include the compilation of the build script itself in the fingerprint of the `RunCustomBuild` Unit. This also includes some documentation for how fingerprints work. Also includes a test for docker-style caching of dependencies where timestamps get truncated to the nearest second.
|
☀️ Test successful - checks-travis, status-appveyor |
Ah yep, definitely sounds too difficult for this PR
Seems like not a bad idea! |
Update cargo Update cargo 22 commits in 0e35bd8af0ec72d3225c4819b330b94628f0e9d0..63231f438a2b5b84ccf319a5de22343ee0316323 2019-03-13 06:52:51 +0000 to 2019-03-27 12:26:45 +0000 - Code cleanup (rust-lang/cargo#6787) - Add cargo:rustc-link-arg to pass custom linker arguments (rust-lang/cargo#6298) - Testsuite: remove some unnecessary is_nightly checks. (rust-lang/cargo#6786) - cargo metadata: Don't show `null` deps. (rust-lang/cargo#6534) - Some fingerprint cleanup. (rust-lang/cargo#6785) - Fix fingerprint for canceled build script. (rust-lang/cargo#6782) - Canonicalize default target if it ends with `.json` (rust-lang/cargo#6778) - Fix setting `panic=unwind` compiling lib a extra time. (rust-lang/cargo#6781) - Always nicely show errors from crates.io if possible (rust-lang/cargo#6771) - Testsuite: Make `cwd()` relative to project root. (rust-lang/cargo#6768) - Allow `cargo fix` if gitignore matches root working dir. (rust-lang/cargo#6767) - Remove redundant imports (rust-lang/cargo#6763) - Handle backcompat hazard with `toml` crate (rust-lang/cargo#6761) - Fix spurious error in dirty_both_lib_and_test. (rust-lang/cargo#6756) - Update toml requirement from 0.4.2 to 0.5.0 (rust-lang/cargo#6760) - Reuse std::env::consts::EXE_SUFFIX (rust-lang/cargo#6758) - Proptest 0.9.1 (rust-lang/cargo#6753) - Don't need extern crate in 2018 (rust-lang/cargo#6752) - Release a jobserver token while locking a file (rust-lang/cargo#6748) - Minor doc fix for publish command synopsis (rust-lang/cargo#6749) - Stricter package change detection. (rust-lang/cargo#6740) - Fix resolving yanked crates when using a local registry. (rust-lang/cargo#6742)
Fixes #6770. See that issue for a description.
The fix is to include the compilation of the build script itself in the fingerprint of the
RunCustomBuildUnit.This also includes some documentation for how fingerprints work.
Also includes a test for docker-style caching of dependencies where timestamps get truncated to the nearest second.