Skip to content

Circular dependency issue #11207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
mnonnenmacher wants to merge 3 commits into
base: main
Choose a base branch
from
+1,508 −1
Draft

Circular dependency issue #11207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
370fa49
test(yarn2): Demonstrate an issue with circular dependencies
mnonnenmacher Dec 9, 2025
1816af2
fix(model): Fix handling of cycles in transitive dependencies
mnonnenmacher Dec 10, 2025
88ddf12
test(model): Add a test to demonstrate issue with circular dependencies
mnonnenmacher Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion model/src/main/kotlin/utils/DependencyGraphBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,10 @@
processed: Set<D>
): DependencyReference? {
val transitiveDependencies = dependencyHandler.dependenciesFor(dependency).mapNotNullTo(mutableSetOf()) {
addDependencyToGraph(scopeName, it, transitive = true, processed)
if (it in processed) {
return@mapNotNullTo null
}
Comment on lines +380 to +382

Check warning

Code scanning / detekt

Reports code blocks that are not followed by an empty line Warning

Missing empty line after block.
Copy link
Member

@sschuberth sschuberth Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* A depth-first-search (DFS)-based implementation which breaks all cycles in O(V + E).
* Finding a minimal solution is NP-complete.
*/
internal fun breakCycles(edges: Collection<DependencyGraphEdge>): Set<DependencyGraphEdge> {

addDependencyToGraph(scopeName, it, transitive = true, processed + dependency)
}

val fragmentMapping = referenceMappings[index.fragment]
Expand Down
26 changes: 26 additions & 0 deletions model/src/test/kotlin/utils/DependencyGraphBuilderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,32 @@
graph.nodes shouldHaveSize 3
}

"deal with recursive cycles in dependencies" {
Copy link
Member

@sschuberth sschuberth Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep my older #10083 instead, which fixes an existing test in this regard?

val scope = "CyclicScope"
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

Unnecessary long whitespace
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

Newline expected before expression body

Check warning

Code scanning / detekt

Reports spaces around keywords Warning test

Missing spacing after "when"
id1 -> listOf(id2)
id2 -> listOf(id1)
else -> emptyList()
}

override fun linkageFor(dependency: Identifier) = PackageLinkage.STATIC

override fun createPackage(dependency: Identifier, issues: MutableCollection<Issue>) =
Package.EMPTY.copy(id = dependency)
}

DependencyGraphBuilder(handler)
.addDependency(scope, id1)
.addDependency(scope, id2)
.build()
}

"check for illegal references when building the graph" {
val depLang = createDependency("org.apache.commons", "commons-lang3", "3.11")
val depNoPkg = createDependency(NO_PACKAGE_NAMESPACE, "invalid", "1.2")
Expand Down
Loading
Loading