spdx / spdx-gradle-plugin

Apache License 2.0
15 stars 10 forks source link

Resolved external artifacts reconciliation throwing IllegalStateException #115

Open vajain-1982 opened 7 months ago

vajain-1982 commented 7 months ago

Hi, External artifiacts reconcile throws illegalStateException https://github.com/spdx/spdx-gradle-plugin/blob/main/src/main/java/org/spdx/sbom/gradle/utils/SpdxDocumentBuilder.java#L174 This could happen if same componentIdentfier resolves to two different files ( possible due to transitive dependencies of different jars). In this case task simple fails as it couldn't make a decision which one is right. Shouldn't this be handled gracefully or given an option to user to pick one ( first | second) in case such condition occurs. Observed this issue for with netty jars which are platform dependent ,where component identifier is same.

cannot merge duplicate /Users/.gradle/caches/modules-2/files-2.1/io.netty/netty-transport-native-epoll/4.1.105.Final/888ba2b17e360367e01a380fbbd048266cb92305/netty-transport-native-epoll-4.1.105.Final-linux-riscv64.jar and /Users/.gradle/caches/modules-2/files-2.1/io.netty/netty-transport-native-epoll/4.1.105.Final/2958234697ebe6a61d2f43257fe1ed6d19a46b57/netty-transport-native-epoll-4.1.105.Final-linux-aarch_64.jar

loosebazooka commented 7 months ago

Yeah I think we never found a good example of when that happens. Can you include a minimal sample project where this error occurs, that'll help us address it better?

There might be some underlying variant information that we can use. Component Id might be insufficient.

vajain-1982 commented 7 months ago

I don't have good sample example for this . jars for different arch are coming from my organizations custom repo ( can't share details) , any other detail like resolved artifact object structure through debug can help to fix this ?

loosebazooka commented 7 months ago

I think we need to rewrite a lot of the code to be "variant-aware" but that is hard because I haven't found a good example where multiple variants of the same componentId are included in a build, variants are picked and the rest are ignored. Without that I can't exactly design for it.

vajain-1982 commented 7 months ago

Understood , will check if I can build such case with sample public repo . But for the mean time can we handle the error gracefully or provide a way/config to handle the conflict due to duplicate componentIdentifier. Currently any such case fails the complete task.

loosebazooka commented 7 months ago

Is your build using multiple configurations in the sbom configuration? This is the only way I can think of to bring in multiple variants into a project and create this clash?

vajain-1982 commented 7 months ago

my repo has a dependency on an organization level auth library which has library dependency on netty with different arch and as I also have included the netty it downloads the other arch lib too , Since the component identifier is same , plugin fails. I agree with you that it's hard to handle all such cases and probably requires a lot of work , but I am kind of not in favour of failing for such cases instead plugin should run with warning , recommending user to fix this to achieve better /verbose sbom generation. Let me know your thought on this.

loosebazooka commented 7 months ago

Oh yeah the error message could be better. I don't think we can act on it though. The fix is likely excluding the unused netty dependency somehow. But variant selection should already do that which is why this is very confusing to me

vajain-1982 commented 7 months ago

I am thinking of more like a warning instead of error. This would inform the user about issue but still generate sbom with best effort approach.

loosebazooka commented 7 months ago

Fundamentally this points to an issue with the user's build. Unless I'm mistaken, two variants of the same component cannot be part of a single configuration. We can provide a better error message, but other than that, I don't see any changes.

kamildoleglo commented 7 months ago

Same here. In our case it's Netty DNS resolver:

cannot merge duplicate /Users/kdoleglo/.gradle/caches/modules-2/files-2.1/io.netty/netty-resolver-dns-native-macos/4.1.107.Final/93db846854c5dbcf3f7690049da954b9a2decc2e/netty-resolver-dns-native-macos-4.1.107.Final-osx-aarch_64.jar and /Users/kdoleglo/.gradle/caches/modules-2/files-2.1/io.netty/netty-resolver-dns-native-macos/4.1.107.Final/ca071cb93bd52f4193a980158b97e3d068d22d18/netty-resolver-dns-native-macos-4.1.107.Final-osx-x86_64.jar

loosebazooka commented 7 months ago

@kamildoleglo can you point me to a sample project. I'd love to solve this, but I need an example to work with

kamildoleglo commented 7 months ago

I'll try to make one. While I agree with you that probably

two variants of the same component cannot be part of a single configuration

I use several configurations and I want all of them documented in one file:

configurations.set(listOf(project.configurations.runtimeClasspath.name, <some more configurations here>))

so that may trigger this behaviour

EDIT: Actually no, I only left one configuration there and it still fails with the same message

loosebazooka commented 7 months ago

yeah multiple configurations will definitely trigger it. But I'm more curious about the single configuration case.

kamildoleglo commented 7 months ago

Reproduced here: https://github.com/kamildoleglo/spdx-repro The template was generated automatically by Gradle, I just added some dependencies

loosebazooka commented 7 months ago

@vlsi fyi what we were talking about last week

loosebazooka commented 7 months ago

Ah so this is actually not variants, this is just multiple classifiers of the same object, which could(?) end up being easier to handle.

vlsi commented 7 months ago

Dependencies with classifiers should better be avoided, especially with Gradle (e.g. https://github.com/gradle/gradle/issues/16665#issuecomment-809613071). The reasoning is that "classifiers" have no metadata. Classifiers have little to no meaning, and they are hard to process dependency-wise.

kamildoleglo commented 7 months ago

I agree with that, however this is a valid Gradle configuration and script. IMHO SPDX generation for valid Gradle scripts shouldn't fail in any case. Emitting a warning or providing a way / requiring to manually fix the resulting output is fine as well

loosebazooka commented 7 months ago

@goneall what's the maven plugin do here to represent multiple artifacts with a classifier? Is there an spdx field for this?

For instance the case of a project depending on both:

the spdx entry is something like

name = `io.netty:netty-resolver-dns-native-macos`
version = `4.1.107.Final`

my best guess is just append to version so it's like 4.1.107.Final:osx-aarch_64 ?

goneall commented 7 months ago

what's the maven plugin do here to represent multiple artifacts with a classifier?

I'm not completely sure - it's quite possible there is an issue in the Maven plugin related to generating multiple artifacts as well. If we had a Maven POM file example, I could try it out and see how it behaves.

Is there an spdx field for this?

I don't believe there is. We could add a filed such as "target architecture" for a package, but I don't think that would cover all the uses of the classifier.

my best guess is just append to version so it's like 4.1.107.Final:osx-aarch_64 ?

This seems like a good approach since the qualifier could be considered a refinement of the version (even though it doesn't strictly follow the SemVer syntax/semantics).

loosebazooka commented 7 months ago

I don't believe there is. We could add a filed such as "target architecture" for a package, but I don't think that would cover all the uses of the classifier.

Yeah I don't think target architecture would work. We can't really know what the classifier's meaning is, and adding logic feels like we'll probably get it wrong.

Lemme see if I can recreate this problem in maven.

loosebazooka commented 7 months ago

@goneall, filed https://github.com/spdx/spdx-maven-plugin/issues/164