Conversation
…ncies Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
There is no cycle in the originally defined
depCyc2 -> depFoo -> depCyc1
dependency chain. Doing so is actually not directly possible with the
recursively defined `PackageReference` class.
Solve that by using a primitive `DependencyHandler` that just uses
indexes to identify dependencies.
However, as the functionality to break cycles is not actually
implemented yet in the dependency graph builder, disable this test
temporarily until it is implemented with an upcoming change.
Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Stop processing transitive dependencies in `DependencyGraphBuilder` if the same dependency already appeared in the same branch. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
sschuberth
force-pushed
the
deps-comp-imps
branch
from
40f7f35 to
509af51
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11426 +/- ##
=========================================
Coverage 57.77% 57.78%
Complexity 1711 1711
=========================================
Files 347 347
Lines 12901 12904 +3
Branches 1236 1237 +1
=========================================
+ Hits 7453 7456 +3
+ Misses 5000 4999 -1
- Partials 448 449 +1
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:
|
| .build(checkReferences = false) | ||
|
|
||
| graph.nodes shouldHaveSize 6 | ||
| graph.edges shouldHaveSize 5 |
There was a problem hiding this comment.
It could be nice to verify the actual edges here to demonstrate how the cycles are broken.
There was a problem hiding this comment.
While debugging this with PNPM-specific changes on top, it looks to be as if indeed the cycle is broken at the "wrong" place. I need to further investigate this; moving back to draft.
| val depFoo = createDependency("org.foo", "foo", "1.2.0", dependencies = setOf(depCyc1)) | ||
| val depCyc2 = createDependency("org.cyclic", "cyclic", "77.7", dependencies = setOf(depFoo)) | ||
| // TODO: Enable this once the functionality to break cycles is fully implemented. | ||
| "deal with cycles in dependencies".config(enabled = false) { |
There was a problem hiding this comment.
"deal with cycles" is so unspecific, how about also changing it to "break cycles"?
| addDependencyToGraph(scopeName, it, transitive = true, processed) | ||
| if (it in processed) { | ||
| return@mapNotNullTo null | ||
| } |
There was a problem hiding this comment.
In #11207 I had this problem with this change:
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.areDependenciesEqual when it tries to compare the dependency subtrees when adding the nodes.
Is this somehow solved by the early returns you added in the first commit?
There was a problem hiding this comment.
Is this somehow solved by the early returns you added in the first commit?
Yes, at least that fixed the stack overflow I was seeing in my PNPM experiments.
2 participants
Please have a look at the individual commit messages for the details.