Ensure transitive excludes are updated#33925
Conversation
Sometimes a node's dependencies may be traversed with one set of transitive excludes. Then, later in the graph traversal, a new edge may be connected to that original node with less restrictive excludes. This changes the transitive excludes of the original node so that it may bring in additional transitive dependenices. We make sure to update the transitive excludes of a node's outgoing edges if it has already been traversed so that those outgoing edges properly propagate their new excludes
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The following builds have failed: |
|
The following builds have passed: |
Mark some nullable fields as nullable Add comments where necessary Adjust boolean logic to unconditionally run for loop, as its enclosing if statement will always be true Remove logger that we don't really ever use
When we get an edge from the edge cache make sure to update its transitive exlcudes. Since the edge is not attached at that point, also make sure to always recompute a node's transitive excludes when an edge is added to it. This can potentially open up optimizations in the future where we compute the node exclusions eagerly when the edge is added to it, and only if the transitive excludes change do we enqueue the node.
|
@bot-gradle test prf apt |
This comment has been minimized.
This comment has been minimized.
|
The following builds have passed: |
|
The following builds have passed: |
| // The excludes did change. Even if our direct filtered dependencies are the same, | ||
| // their transitive dependencies may be affected by the new excludes. So, we | ||
| // always update the outgoing edges to reflect the new resolution filter. | ||
|
|
||
| // If we return true here, we will short-circuit normal dependency traversal, | ||
| // which usually takes care of updating the transitive excludes. Do that here | ||
| // instead. |
There was a problem hiding this comment.
This all needs some updated wording now that we are no longer unconditionally doing this
ljacomet
left a comment
There was a problem hiding this comment.
Matches what we discussed and reasoned about.
Minor comment on code comments.
| assertResolvedFiles(resolvedJars) | ||
| } | ||
|
|
||
| def "node reentering the queue with a new incoming edge with fewer exclusions than all previous incoming edges properly propagates new transitive excludes"() { |
There was a problem hiding this comment.
💭 Without your comment on the PR, I think it would very be difficult to understand the purpose of this test.
Can you add some breadcrumbs here about how org:engine is first added to the graph excluding its transitive dep on org.slf4j. But then data:tools transitively re-includes org.slf4j as a transitive grandchild? I think that might really help future readers understand the point of this.
Also, the test name talks about "reentering the queue" and "edges" which is an "inner view" of dependency resolution, vs. what is present externally in the test code.
I think the logic of this test is fine, I just think it will be hard to understand the purpose of it years later if it fails from the test code itself.
There was a problem hiding this comment.
The thing is, I can't exactly tell you why sfl4j is involved here. All I can say is that if you remove it the test does not exhibit the broken behavior.
I could spend more hours debugging here and figure that out but I don't think that is worth it.
The test scenario here is derived from this issue #33924. It consists of a minified reproducer from a real-world build that used this graph in real life.
This is a pattern we consistently apply in the dependency management tests. We have a real world build that breaks something in the engine, then we add a derived reproducer from that build as a regression test.
it will be hard to understand the purpose of it years later
The purpose here is regression test. I've added an Issue link to this test case so we can trace it back to the original bug it is ensuring does not get reintroduced.
There was a problem hiding this comment.
I agree with your comment on the wording of the test name.
Though, if we wrote one to better reflect the "outside view" -- the fact that this is a regression test-- , it would be something like "graph from #33924 doesn't cause missing dependency", which I think is less useful than the inner view here.
| // The excludes did change. Even if our direct filtered dependencies are the same, | ||
| // their transitive dependencies may be affected by the new excludes. So, we | ||
| // always update the outgoing edges to reflect the new resolution filter. | ||
|
|
||
| // If we return true here, we will short-circuit normal dependency traversal, | ||
| // which usually takes care of updating the transitive excludes. Do that here | ||
| // instead. |
Add missing issue annotation to regression test Fix test name Fix javadoc wording Update confusing/out-of-date comment
After #33925, we follow up with some refactorings. Most importantly, we clean up visitOutgoingDependencies and excludesSameDependenciesAsPreviousTraversal, which were two very coupled methods. excludesSameDependenciesAsPreviousTraversal performed side-effects which anticipated what visitOutgoingDependencies would do with its return value. This sort of coupling is confusing to follow and maintain. We inline and refactor some logic to make the logic easier to follow and maintain.
|
Hi all, I'm wondering if it might also resolve the symptoms described in these issues:
which all seem to relate to dependency locking and resolution inconsistencies. We are observing similar behavior in our Gradle 8.10 multi-project setup when the dependency graph becomes sufficiently complex, and we are hopeful this might resolve |
|
@lan666as All of these issues seem very similar to the one we fixed here. Would you be able to test with 9.1.0 nightly to see if your bug still reproduces? |
After #33925, we follow up with some refactorings. Most importantly, we clean up visitOutgoingDependencies and excludesSameDependenciesAsPreviousTraversal, which were two very coupled methods. excludesSameDependenciesAsPreviousTraversal performed side-effects which anticipated what visitOutgoingDependencies would do with its return value. This sort of coupling is confusing to follow and maintain. We inline and refactor some logic to make the logic easier to follow and maintain.
Sometimes a node's dependencies may be traversed with one set of transitive excludes. Then, later in the graph traversal, a new edge may be connected to that original node with less restrictive excludes. This changes the transitive excludes of the original node so that it may bring in additional transitive dependenices.
We make sure to update the transitive excludes of a node's outgoing edges if it has already been traversed so that those outgoing edges properly propagate their new excludes
Fixes #33924
Reviewing cheatsheet
Before merging the PR, comments starting with