Circular dependency issue #11207
Circular dependency issue #11207
Conversation
Add a Yarn2 test project to demonstrate an issue with the handling of circular dependencies. The project has two dependencies, `mlly:1.8.0` and `pkg-types:1.3.1`, which both depend on each other. Signed-off-by: Martin Nonnenmacher <[email protected]>
Stop processing transitive dependencies in `DependencyGraphBuilder` if the same dependency already appeared in the same branch. Signed-off-by: Martin Nonnenmacher <[email protected]>
Add a test to demonstrate that the default implementation of `DependencyHandler.areDependenciesEqual` runs into a stack overflow if there are cycles in the dependencies. Signed-off-by: Martin Nonnenmacher <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (it in processed) { | ||
| return@mapNotNullTo null | ||
| } |
Check warning
Code scanning / detekt
Reports code blocks that are not followed by an empty line Warning
There was a problem hiding this comment.
If we do this, do we then still need this breakCycles() logic:
ort/model/src/main/kotlin/utils/DependencyGraphBuilder.kt
Lines 493 to 497 in 60316b7
| val id1 = Identifier("NPM::mlly:1.8.0") | ||
| val id2 = Identifier("NPM::pkg-type:1.3.1") | ||
|
|
||
| val handler = object : DependencyHandler<Identifier> { |
Check warning
Code scanning / detekt
Reports multiple space usages Warning test
| val handler = object : DependencyHandler<Identifier> { | ||
| override fun identifierFor(dependency: Identifier) = dependency | ||
|
|
||
| override fun dependenciesFor(dependency: Identifier) = when(dependency) { |
Check warning
Code scanning / detekt
Format signature to be single when possible, multiple lines otherwise. Warning test
| val handler = object : DependencyHandler<Identifier> { | ||
| override fun identifierFor(dependency: Identifier) = dependency | ||
|
|
||
| override fun dependenciesFor(dependency: Identifier) = when(dependency) { |
Check warning
Code scanning / detekt
Reports spaces around keywords Warning test
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11207 +/- ##
============================================
- Coverage 57.47% 57.46% -0.02%
Complexity 1704 1704
============================================
Files 346 346
Lines 12855 12857 +2
Branches 1222 1223 +1
============================================
- Hits 7389 7388 -1
- Misses 4992 4995 +3
Partials 474 474
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
As a side note, IMO breaking cycles should be an inherent feature of the dependency graph builder, so that not all package managers need to implement such logic separately. Also see #10083 in that regard. |
| graph.nodes shouldHaveSize 3 | ||
| } | ||
|
|
||
| "deal with recursive cycles in dependencies" { |
There was a problem hiding this comment.
Can we keep my older #10083 instead, which fixes an existing test in this regard?
This PR demonstrates an issue with the handling of circular dependencies in the
DependencyGraphBuilder, found while working on fixing theYarn2dependency resolution.The first commit creates a minimal test project for Yarn2 with two dependencies that depend on each other. This leads to a stack overflow in
DependencyGraphBuilder.insertIntoGraphwhen it tries to add transitive dependencies. The root cause is that theYarn2DependencyHandler.dependenciesForimplementation is based on the output ofyarn infowhich contains the cycle:{"value":"mlly@npm:1.8.0","children":{"Version":"1.8.0","Manifest":{"License":"MIT","Homepage":null},"Dependencies":[{"descriptor":"pkg-types@npm:^1.3.1","locator":"pkg-types@npm:1.3.1"}, ...]}} {"value":"pkg-types@npm:1.3.1","children":{"Version":"1.3.1","Manifest":{"License":"MIT","Homepage":null},"Dependencies":[{"descriptor":"mlly@npm:^1.7.4","locator":"mlly@npm:1.8.0"}, ...]}}This dependency cycle was found when trying to analyze this project:
https://github.com/traefik/traefik/tree/master/webui
The second commit tries to fix that issue by breaking cycles in transitive dependencies. With that fix, the first stack overflow does not happena anymore, but then there is a stack overflow in the default implementation of
DependencyHandler.areDependenciesEqualwhen it tries to compare the dependency subtrees when adding the nodes.The third commit adds a minimal reproducer for the Yarn2 scenario from the first commit.
@oheger-bosch I am not sure what would be the best approach to fix this issue and if the fix from the second commit is even correct. I was hoping that maybe you have an idea how to handle such a scenario. One possible fix for the second issue could be to add a custom implementation of
areDependenciesEqualtoYarn2DependencyHandlerthat can handle the cycles, but I think it would be better if such scenarios would be correctly handled by the default implementations.