fix(gradle-inspector): Handle dependency cycles properly

Do not cut the dependency tree on any already visited dependency in a configuration, but only for those that appear on the path from the root to a leaf. This fixes the discrepancy compared to the `Gradle` package manager that was identified at [1] but justified wrongly. [1]: #6882 (comment) Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
oss-review-toolkit · Sep 10, 2024 · ce8fa65 · ce8fa65
1 parent 2cd8fe4
commit ce8fa65
Show file tree

Hide file tree

Showing 3 changed files with 91 additions and 26 deletions.
diff --git a/...pector/src/funTest/assets/projects/synthetic/multi-kotlin-project-expected-output-cli.yml b/...pector/src/funTest/assets/projects/synthetic/multi-kotlin-project-expected-output-cli.yml
@@ -34,6 +34,9 @@ project:
       - id: "Maven:org.jetbrains.kotlin:kotlin-reflect:1.7.20"
         dependencies:
         - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+          dependencies:
+          - id: "Maven:org.jetbrains:annotations:13.0"
+          - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
       - id: "Maven:org.jetbrains.kotlin:kotlin-script-runtime:1.7.20"
       - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
         dependencies:
@@ -55,9 +58,23 @@ project:
           dependencies:
           - id: "Maven:org.jetbrains.kotlin:kotlin-script-runtime:1.7.20"
           - id: "Maven:org.jetbrains.kotlin:kotlin-scripting-common:1.7.20"
+            dependencies:
+            - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+              dependencies:
+              - id: "Maven:org.jetbrains:annotations:13.0"
+              - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
           - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+            dependencies:
+            - id: "Maven:org.jetbrains:annotations:13.0"
+            - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
         - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+          dependencies:
+          - id: "Maven:org.jetbrains:annotations:13.0"
+          - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
       - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+        dependencies:
+        - id: "Maven:org.jetbrains:annotations:13.0"
+        - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
   - name: "kotlinCompilerPluginClasspathTest"
     dependencies:
     - id: "Maven:org.jetbrains.kotlin:kotlin-scripting-compiler-embeddable:1.7.20"
@@ -74,9 +91,23 @@ project:
           dependencies:
           - id: "Maven:org.jetbrains.kotlin:kotlin-script-runtime:1.7.20"
           - id: "Maven:org.jetbrains.kotlin:kotlin-scripting-common:1.7.20"
+            dependencies:
+            - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+              dependencies:
+              - id: "Maven:org.jetbrains:annotations:13.0"
+              - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
           - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+            dependencies:
+            - id: "Maven:org.jetbrains:annotations:13.0"
+            - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
         - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+          dependencies:
+          - id: "Maven:org.jetbrains:annotations:13.0"
+          - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
       - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+        dependencies:
+        - id: "Maven:org.jetbrains:annotations:13.0"
+        - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
   - name: "kotlinKlibCommonizerClasspath"
     dependencies:
     - id: "Maven:org.jetbrains.kotlin:kotlin-klib-commonizer-embeddable:1.7.20"
@@ -89,8 +120,14 @@ project:
         - id: "Maven:org.jetbrains.kotlin:kotlin-reflect:1.7.20"
           dependencies:
           - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+            dependencies:
+            - id: "Maven:org.jetbrains:annotations:13.0"
+            - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
         - id: "Maven:org.jetbrains.kotlin:kotlin-script-runtime:1.7.20"
         - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+          dependencies:
+          - id: "Maven:org.jetbrains:annotations:13.0"
+          - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
       - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
         dependencies:
         - id: "Maven:org.jetbrains:annotations:13.0"
@@ -105,6 +142,9 @@ project:
         - id: "Maven:org.jetbrains:annotations:13.0"
         - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
     - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+      dependencies:
+      - id: "Maven:org.jetbrains:annotations:13.0"
+      - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
   - name: "testCompileClasspath"
     dependencies:
     - id: "Gradle:org.gradle.kotlin.dsl.samples.multiproject:core:1.0"
@@ -123,6 +163,9 @@ project:
         - id: "Maven:org.jetbrains:annotations:13.0"
         - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
     - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+      dependencies:
+      - id: "Maven:org.jetbrains:annotations:13.0"
+      - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
 packages:
 - id: "Maven:net.java.dev.jna:jna:5.6.0"
   purl: "pkg:maven/net.java.dev.jna/jna@5.6.0"

diff --git a/...ector/src/funTest/assets/projects/synthetic/multi-kotlin-project-expected-output-core.yml b/...ector/src/funTest/assets/projects/synthetic/multi-kotlin-project-expected-output-core.yml
@@ -32,6 +32,9 @@ project:
       - id: "Maven:org.jetbrains.kotlin:kotlin-reflect:1.7.20"
         dependencies:
         - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+          dependencies:
+          - id: "Maven:org.jetbrains:annotations:13.0"
+          - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
       - id: "Maven:org.jetbrains.kotlin:kotlin-script-runtime:1.7.20"
       - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
         dependencies:
@@ -53,9 +56,23 @@ project:
           dependencies:
           - id: "Maven:org.jetbrains.kotlin:kotlin-script-runtime:1.7.20"
           - id: "Maven:org.jetbrains.kotlin:kotlin-scripting-common:1.7.20"
+            dependencies:
+            - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+              dependencies:
+              - id: "Maven:org.jetbrains:annotations:13.0"
+              - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
           - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+            dependencies:
+            - id: "Maven:org.jetbrains:annotations:13.0"
+            - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
         - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+          dependencies:
+          - id: "Maven:org.jetbrains:annotations:13.0"
+          - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
       - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+        dependencies:
+        - id: "Maven:org.jetbrains:annotations:13.0"
+        - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
   - name: "kotlinCompilerPluginClasspathTest"
     dependencies:
     - id: "Maven:org.jetbrains.kotlin:kotlin-scripting-compiler-embeddable:1.7.20"
@@ -72,9 +89,23 @@ project:
           dependencies:
           - id: "Maven:org.jetbrains.kotlin:kotlin-script-runtime:1.7.20"
           - id: "Maven:org.jetbrains.kotlin:kotlin-scripting-common:1.7.20"
+            dependencies:
+            - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+              dependencies:
+              - id: "Maven:org.jetbrains:annotations:13.0"
+              - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
           - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+            dependencies:
+            - id: "Maven:org.jetbrains:annotations:13.0"
+            - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
         - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+          dependencies:
+          - id: "Maven:org.jetbrains:annotations:13.0"
+          - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
       - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+        dependencies:
+        - id: "Maven:org.jetbrains:annotations:13.0"
+        - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
   - name: "kotlinKlibCommonizerClasspath"
     dependencies:
     - id: "Maven:org.jetbrains.kotlin:kotlin-klib-commonizer-embeddable:1.7.20"
@@ -87,8 +118,14 @@ project:
         - id: "Maven:org.jetbrains.kotlin:kotlin-reflect:1.7.20"
           dependencies:
           - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+            dependencies:
+            - id: "Maven:org.jetbrains:annotations:13.0"
+            - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
         - id: "Maven:org.jetbrains.kotlin:kotlin-script-runtime:1.7.20"
         - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
+          dependencies:
+          - id: "Maven:org.jetbrains:annotations:13.0"
+          - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib-common:1.7.20"
       - id: "Maven:org.jetbrains.kotlin:kotlin-stdlib:1.7.20"
         dependencies:
         - id: "Maven:org.jetbrains:annotations:13.0"

diff --git a/plugins/package-managers/gradle-plugin/src/main/kotlin/OrtModelBuilder.kt b/plugins/package-managers/gradle-plugin/src/main/kotlin/OrtModelBuilder.kt
@@ -27,7 +27,6 @@ import org.apache.maven.model.building.ModelBuildingResult
 
 import org.gradle.api.Project
 import org.gradle.api.artifacts.Configuration
-import org.gradle.api.artifacts.ModuleVersionIdentifier
 import org.gradle.api.artifacts.component.ComponentIdentifier
 import org.gradle.api.artifacts.component.ModuleComponentIdentifier
 import org.gradle.api.artifacts.component.ProjectComponentIdentifier
@@ -52,9 +51,6 @@ internal class OrtModelBuilder : ToolingModelBuilder {
 
     private val platformCategories = setOf("platform", "enforced-platform")
 
-    private val visitedDependencies = mutableSetOf<ModuleComponentIdentifier>()
-    private val visitedProjects = mutableSetOf<ModuleVersionIdentifier>()
-
     private val logger = Logging.getLogger(OrtModelBuilder::class.java)
     private val errors = mutableListOf<String>()
     private val warnings = mutableListOf<String>()
@@ -85,11 +81,7 @@ internal class OrtModelBuilder : ToolingModelBuilder {
 
             // Omit configurations without dependencies.
             root.dependencies.takeUnless { it.isEmpty() }?.let { dep ->
-                // Reset visited dependencies and projects per configuration.
-                visitedDependencies.clear()
-                visitedProjects.clear()
-
-                OrtConfigurationImpl(name = config.name, dependencies = dep.toOrtDependencies(poms))
+                OrtConfigurationImpl(name = config.name, dependencies = dep.toOrtDependencies(poms, emptySet()))
             }
         }
 
@@ -146,7 +138,8 @@ internal class OrtModelBuilder : ToolingModelBuilder {
     }
 
     private fun Collection<DependencyResult>.toOrtDependencies(
-        poms: Map<String, ModelBuildingResult>
+        poms: Map<String, ModelBuildingResult>,
+        visited: Set<ComponentIdentifier>
     ): List<OrtDependency> =
         if (GradleVersion.current() < GradleVersion.version("5.1")) {
             this
@@ -163,8 +156,12 @@ internal class OrtModelBuilder : ToolingModelBuilder {
                     if (isBom) return@mapNotNull null
 
                     val selectedComponent = dep.selected
+                    val id = selectedComponent.id
 
-                    when (val id = selectedComponent.id) {
+                    // Cut the graph on cyclic dependencies.
+                    if (id in visited) return@mapNotNull null
+
+                    when (id) {
                         is ModuleComponentIdentifier -> {
                             val pomFile = if (selectedComponent is ResolvedComponentResultInternal) {
                                 val repositoryId = runCatching {
@@ -199,19 +196,13 @@ internal class OrtModelBuilder : ToolingModelBuilder {
                             }
 
                             val modelBuildingResult = poms[id.toString()]
-                            if (modelBuildingResult == null && id !in visitedDependencies) {
+                            if (modelBuildingResult == null) {
                                 val message = "No POM found for $id."
                                 logger.warn(message)
                                 warnings += message
                             }
 
-                            val dependencies = if (id in visitedDependencies) {
-                                // Cut the graph on cyclic dependencies.
-                                emptyList()
-                            } else {
-                                visitedDependencies += id
-                                selectedComponent.dependencies.toOrtDependencies(poms)
-                            }
+                            val dependencies = selectedComponent.dependencies.toOrtDependencies(poms, visited + id)
 
                             OrtDependencyImpl(
                                 groupId = id.group,
@@ -238,13 +229,7 @@ internal class OrtModelBuilder : ToolingModelBuilder {
 
                         is ProjectComponentIdentifier -> {
                             val moduleId = selectedComponent.moduleVersion ?: return@mapNotNull null
-                            val dependencies = if (moduleId in visitedProjects) {
-                                // Cut the graph on cyclic dependencies.
-                                emptyList()
-                            } else {
-                                visitedProjects += moduleId
-                                selectedComponent.dependencies.toOrtDependencies(poms)
-                            }
+                            val dependencies = selectedComponent.dependencies.toOrtDependencies(poms, visited + id)
 
                             OrtDependencyImpl(
                                 groupId = moduleId.group,