Fix a bug in Cargo's cyclic dep graph detection #9075
Conversation
|
r? @ehuss (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Over all, I think we end up copying the resolution graph in a lot of different places etch with small changes in representation. I big refactor ( for another day ) is to do an revue of all the places and try to organize it better. Definitely |
Cargo's cyclic dependency graph detection turns out to have had a bug for quite a long time as surfaced by rust-lang#9073. The bug in Cargo has to do with how dev-dependencies are handled. Cycles are "allowed" through dev-dependencies because the dev-dependency can depend on the original crate. Our cyclic graph detection, however, was too eagerly flagging a package as known to not have a cycle before we had processed everything about it. The fix here was basically to just simplify the graph traversal. Instead of traversing the raw `Resolve` this instead creates an alternate in-memory graph which has the actual edges we care about for cycle detection (e.g. every edge that wasn't induced via a dev-dependency). With this simplified graph we then apply the exact same algorithm, but this time it should be less buggy because we're not trying to do funky things about switching sets about what's visited halfway through a traversal. Closes rust-lang#9073
alexcrichton
force-pushed
the
fix-cycles
branch
from
576e0dd to
4b4dc0a
Compare
|
I'm not too too worried in this case, I've found that it's really difficult to have a "one graph rules them all" implementation and it typically ends up being more trouble than it's worth (in my experience at least). The main problem here was that a nonstandard traversal of a graph was being made and the nonstandard part led to a bug. This isn't really a performance sensitive part of the code either since it should only happen once-per-cargo-invocation in theory. |
|
👍 @bors r+ |
|
📌 Commit 4b4dc0a has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Cargo's cyclic dependency graph detection turns out to have had a bug
for quite a long time as surfaced by #9073. The bug in Cargo has to do
with how dev-dependencies are handled. Cycles are "allowed" through
dev-dependencies because the dev-dependency can depend on the original
crate. Our cyclic graph detection, however, was too eagerly flagging a
package as known to not have a cycle before we had processed everything
about it.
The fix here was basically to just simplify the graph traversal. Instead
of traversing the raw
Resolvethis instead creates an alternatein-memory graph which has the actual edges we care about for cycle
detection (e.g. every edge that wasn't induced via a dev-dependency).
With this simplified graph we then apply the exact same algorithm, but
this time it should be less buggy because we're not trying to do funky
things about switching sets about what's visited halfway through a
traversal.
Closes #9073