Skip to content
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.

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

Handling multiple ComponentArtifactIdentifier with same ComponentIdentifier currently causing "cannot merge duplicate" errors #69

Closed
sgjesse opened this issue Nov 7, 2023 · 5 comments · Fixed by #74

Comments

@sgjesse
Copy link
Contributor

sgjesse commented Nov 7, 2023

In the SpdxDocumentBuilder constructor ComponentArtifactIdentifier is mapped down to ComponentIdentifier, and if multiple ComponentArtifactIdentifier has the same ComponentIdentifier but not the same file the plugin fails with a "cannot merge duplicate..." error.

This is seen when integrating the spdx-gradle-plugin into the build of the D8/R8 compiler suite. It works fine except for one issue, where we get duplicate file references. There has already been some work in this area in the pull request Handle duplicate files references, but for the D8/R8 project that is not enough.

We have a setup where the plugin is passed both compileClasspath and runtimeClasspath. We also have implementation dependencies on other modules. And these things in combination leads to the following pairs of artifact files in the building of resolvedArtifacts:

    d8_r8/keepanno/build/classes/java/main
    d8_r8/keepanno/build/classes/kotlin/main

    d8_r8/resourceshrinker/build/classes/kotlin/main
    d8_r8/resourceshrinker/build/classes/java/main

    d8_r8/keepanno/build/classes/java/main
    d8_r8/keepanno/build/classes/kotlin/main

    d8_r8/keepanno/build/classes/java/main
    d8_r8/keepanno/build/libs/keepanno.jar

    d8_r8/resourceshrinker/build/classes/java/main
    d8_r8/resourceshrinker/build/libs/resourceshrinker.jar

The resourceshrinker project has both java and kotlin code. The keepanno project has only java code.

I am by no means a Gradle expert, but I can see why classes generated for both Java and Kotlin are present, but why one jar from libs is also there I am not sure. Does anybody have an idea of how should we handle this in the plugin? Is D8/R8 setup not supported/unexpected?

Right now we are using a custom version of the plugin with the following patch (I am not suggesting landing this as is - this is only for the plugin to work for our project, and I might as well just have all duplicates, but wanted to see failures if new patterns started to appear):

--- a/gradle.properties
+++ b/gradle.properties
@@ -1 +1 @@
-version=0.4.0
+version=0.4.0-r8-patch01
diff --git a/src/main/java/org/spdx/sbom/gradle/utils/SpdxDocumentBuilder.java b/src/main/java/org/spdx/sbom/gradle/utils/SpdxDocumentBuilder.java
index de3214a..af25bf2 100644
--- a/src/main/java/org/spdx/sbom/gradle/utils/SpdxDocumentBuilder.java
+++ b/src/main/java/org/spdx/sbom/gradle/utils/SpdxDocumentBuilder.java
@@ -168,9 +168,56 @@ public class SpdxDocumentBuilder {
                     (file1, file2) -> {
                       if (Objects.equals(file1, file2)) {
                         return file1;
-                      } else
+                      } else {
+                        // In the current R8 build setup we get the following file pairs
+                        // (as of 8c88aa02f7804f5a37ca6aea38cfc7f76c806360);
+                        //
+                        // d8_r8/keepanno/build/classes/java/main
+                        // d8_r8/keepanno/build/classes/kotlin/main
+                        //
+                        // d8_r8/resourceshrinker/build/classes/kotlin/main
+                        // d8_r8/resourceshrinker/build/classes/java/main
+                        //
+                        // d8_r8/keepanno/build/classes/java/main
+                        // d8_r8/keepanno/build/classes/kotlin/main
+                        //
+                        // d8_r8/keepanno/build/classes/java/main
+                        // d8_r8/keepanno/build/libs/keepanno.jar
+                        //
+                        // d8_r8/resourceshrinker/build/classes/java/main
+                        // d8_r8/resourceshrinker/build/libs/resourceshrinker.jar
+                        //
+                        // For now workaround the duplicated by preferring file to directory and
+                        // java directory to kotlin directory when both are directories.
+                        if (!file1.exists()) {
+                          return file2;
+                        }
+                        if (!file2.exists()) {
+                          return file1;
+                        }
+                        if (file1.isFile() && file2.isDirectory()) {
+                          return file1;
+                        }
+                        if (file1.isDirectory() && file2.isFile()) {
+                          return file2;
+                        }
+                        if (file1.isDirectory() && file2.isDirectory()) {
+                          if (file1.getName().equals(file2.getName())) {
+                            String parent1 = file1.getParent();
+                            String parent2 = file2.getParent();
+                            if (parent1.endsWith("classes/java") && parent2.endsWith(
+                                "classes/kotlin")) {
+                              return file1;
+                            }
+                            if (parent2.endsWith("classes/java") && parent1.endsWith(
+                                "classes/kotlin")) {
+                              return file2;
+                            }
+                          }
+                        }
                         throw new IllegalStateException(
                             "cannot merge duplicate " + file1 + " and " + file2);
+                      }
                     }));
     this.mavenArtifactRepositories = mavenArtifactRepositories;
     this.poms = poms;
@loosebazooka
Copy link
Collaborator

Oh compileClasspath does lower level things, like referencing classes files directly? I guess compileJava/compileKotlin doesn't force artifact archives to be built? Should the sbom be ignoring anything that's not a jar?

We actually only use resolvedArtifacts when processing external maven references anyway. Perhaps we should filter on stuff that's only !ProjectComponentIdentifier.

If you did something like:

        resolvedArtifacts.entrySet().stream()
+           .filter(e -> !(e.getKey() instanceof ProjectComponentIdentifier))
            .collect(

does that fix the problem in your build?

@sgjesse
Copy link
Contributor Author

sgjesse commented Nov 9, 2023

I tries that, but still get cannot merge duplicate .../d8_r8/keepanno/build/classes/java/main and .../d8_r8/keepanno/build/classes/kotlin/main

@loosebazooka
Copy link
Collaborator

Ah yeah, I had it wrong, it looks like this should work?

        resolvedArtifacts.entrySet().stream()
-           .filter(e -> !(e.getKey() instanceof ProjectComponentIdentifier))
+           .filter(e -> !(e.getKey().getComponentIdentifier() instanceof ProjectComponentIdentifier))
            .collect(

@sgjesse
Copy link
Contributor Author

sgjesse commented Nov 10, 2023

That works, thank you for finding the issue. Updated our patched version in https://r8-review.googlesource.com/c/r8/+/85841. Let me know when version 0.4.0 with this change lands, and I will update our project to use that instead.

@loosebazooka
Copy link
Collaborator

Sure np, I'll ping you when we release. Should be sometime next week (or today if we're lucky).

chachako pushed a commit to chachako/r8 that referenced this issue Nov 13, 2023
This is with patch suggested in
spdx/spdx-gradle-plugin#69 (comment)

Change-Id: If196acf43bf07608cf9925fe46f2a47176bed360
Bug: b/310179346
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants