Fix recompiles of stale dependencies and cyclic graphs #1242
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Originally discovered through rust-lang#1236, this commit fixes a bug in Cargo where crates may not be recompiled when they need to (leading to obscure errors from the compiler). The scenario in question looks like: * Assume a dependency graph of `A -> B -> C` and `A -> C` * Build all packages * Modify C * Rebuild, but hit Ctrl+C while B is building * Modify A * Rebuild again Previously, Cargo only considered the freshness of a package to be the freshness of the package itself (checking source files, for example). To handle transitive recompilations, Cargo propagates a dirty bit throughout the dependency graph automatically (instead if calculating it as such). In the above example, however, we have a problem where as part of the last rebuild Cargo thinks `B` and `C` are fresh! The artifact for `C` was just recompiled, but `B`'s source code is untainted, so Cargo does not think that it needs to recompile `B`. This is wrong, however, because one of `B`'s dependencies was rebuilt, so it needs to be rebuilt. To fix this problem, the fingerprint (a short hash) for all packages is now transitively propagated (the fingerprint changes when an upstream package changes). This should ensure that even when Ctrl+C is hit (or the situation explained in rust-lang#1236) that Cargo will still consider packages whose source code is untainted as candidates for recompilation. The implementation is somewhat tricky due to the actual fingerprint for a path dependency not being known until *after* the crate is compiled (the fingerprint is the mtime of the dep-info file). Closes rust-lang#1236
|
r? @wycats (rust_highfive has picked a reviewer for you, use r? to override) |
| method: Method) | ||
| -> CargoResult<CargoResult<Box<Context>>> { | ||
| // Dependency graphs are required to be a DAG. Non-transitive |
There was a problem hiding this comment.
This comment makes less sense after the move.
There was a problem hiding this comment.
Good point, updated.
alexcrichton
force-pushed
the
issue-1236
branch
2 times, most recently
from
d583a38
to
7e990ad
Compare
|
@bors r=wycats 7e990ad |
|
⌛ Testing commit 7e990ad with merge a7b8429... |
|
💔 Test failed - cargo-mac-64 |
Re-jigger some code in the resolver and refactor a little bit to get the ordering of checks right to forbid cyclic dependencies from ever reaching the backend. Closes rust-lang#834
alexcrichton
force-pushed
the
issue-1236
branch
from
7e990ad
to
06f37a0
Compare
|
☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-32, cargo-win-64 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Set out to solve #1236, but ended up solving #834 as well!