Run ARQ during task serialization #72

eygraber · 2023-11-08T22:00:42Z

No description provided.

loosebazooka · 2023-11-09T17:26:12Z

alright one final rebase I guess 🎉 thanks for splitting everything up

- Solves the issue with configuration variants and mismatched attributes - Reflects the current best approach that Gradle suggests - They're hopefully going to provide better APIs in the future

eygraber · 2023-11-09T17:29:32Z

Done!

loosebazooka · 2023-11-09T19:13:14Z

src/main/java/org/spdx/sbom/gradle/SpdxSbomPlugin.java

                    t.getRootComponents().add(rootComponent);
                  }
+
+                  Provider<List<ResolvedArtifactResult>> resolvedArtifactsProvider =
+                      t.getRootComponents()


I think accessing the getRootComponents() here feels a bit weird to me. Can we create an intermediate that can be shared so we can still set t.getRootComponents()... and then reuse here?

loosebazooka · 2023-11-09T19:17:32Z

src/main/java/org/spdx/sbom/gradle/SpdxSbomPlugin.java

-        Collection<ResolvedArtifactResult> resolvedArtifactResults) {
-      return resolvedArtifactResults.stream()
-          // ignore gradle API components as they cannot be serialized
-          .filter(x -> !(x.getId().getComponentIdentifier() instanceof OpaqueComponentIdentifier))


is there a reason we don't need this filter anymore?

Updated, must've lost it in the refactor

src/main/java/org/spdx/sbom/gradle/maven/DependencyResolver.java

loosebazooka · 2023-11-10T14:16:49Z

LGTM, except some very minor things. Sorry it took me a little to read and understand what was going on.

loosebazooka

I believe a separation of the arq result of poms and archives needs to happen.

src/main/java/org/spdx/sbom/gradle/maven/DependencyResolver.java

loosebazooka · 2023-11-13T22:00:48Z

Hey yeah sorry I'm doing some of my own experiments to understand this.

loosebazooka · 2023-11-14T19:56:47Z

src/main/java/org/spdx/sbom/gradle/SpdxSbomPlugin.java

                  for (var configurationName : configurationNames) {
-                    Provider<Set<ResolvedArtifactResult>> artifacts =


Instead of inferring the artifact from the pom resolution, why not just keep this section in?

I believe that's what causing the attribute mismatch errors

Correct me if I'm wrong, but I thought that was the pom resolver stuff, while the artifact resolver was actually fine? I'm suggesting we still keep the code in here for dealing with POMs, and for the actual artifacts themselves keep the other section?

I'll try again and see what happens

Although I'd think that it would be simpler to derive it from the POM vs resolving the artifact.

Okay just for posterity, conversation continued here: https://gradle-community.slack.com/archives/CAHSN3LDN/p1699854619838439

Based on that conversation I'll investigate some alternatives.

Instead of inferring the artifact from the pom resolution, why not just keep this section in?

I tested this and it has issues resolving variants.

Can you provide an example where this issue arises? It'd be nice to have a test to prevent regression and provide a sort of documentation via code on this specific case?

I think it comes up with Kotlin Multiplatform dependencies. I'll try to get a failing test on main

src/main/java/org/spdx/sbom/gradle/SpdxSbomPlugin.java

eygraber · 2023-11-24T08:30:05Z

src/main/java/org/spdx/sbom/gradle/SpdxSbomPlugin.java

+                            .artifactView(
+                                viewConfiguration ->
+                                    viewConfiguration.attributes(
+                                        attributes ->
+                                            attributes.attribute(
+                                                Attribute.of("artifactType", String.class),
+                                                "android-aar-or-jar")))


This turned out to be the solution for resolving normal artifacts. I was inspired by gradle/gradle#20779.

Attribute.of("artifactType", String.class) comes from here

"android-aar-or-jar" comes from here

loosebazooka

Sorry it took a while, I was on vacation.

loosebazooka · 2023-11-29T14:28:18Z

src/main/java/org/spdx/sbom/gradle/maven/DependencyResolver.java

+ * @see org.gradle.api.artifacts.dsl.DependencyHandler
+ * @see org.gradle.api.artifacts.result.ResolvedArtifactResult
+ */
+public class DependencyResolver {


I think this DependencyResolver is now only used for POMs, so can we rename it a bit? DependecyPomMapper? or something along those lines?

It might even make sense to just move this whole thing into PomResolver

loosebazooka · 2023-11-29T14:29:16Z

src/main/java/org/spdx/sbom/gradle/maven/DependencyResolver.java

+        .filter(ResolvedArtifactResult.class::isInstance)
+        .map(ResolvedArtifactResult.class::cast)
+        // ignore gradle API components as they cannot be serialized
+        .filter(x -> !(x.getId().getComponentIdentifier() instanceof OpaqueComponentIdentifier))


I'm not actually sure this filter is useful here? Since we apply it above on the artifact transformer?

True I'll remove it

loosebazooka · 2023-11-29T14:32:56Z

src/main/java/org/spdx/sbom/gradle/SpdxSbomPlugin.java

+                            .artifactView(
+                                viewConfiguration ->
+                                    viewConfiguration.attributes(
+                                        attributes ->
+                                            attributes.attribute(
+                                                Attribute.of("artifactType", String.class),
+                                                artifactType)))


Would this now provide an incomplete sbom on a project with a war dependency or something (in the weird war overlay case)? or if someone was importing any other non jar/aar type from a repository (a zip or rar for instance) that pulls in resources and stuff?

Yes, technically. We can try covering as many cases as possible, and add support for plugins that provide artifact types (like Android), but I don't know if there's a way to get everything. Maybe there's an attribute other than artifactType to use. I'll look into it.

Maybe it's possible to map the POM info into this and get a unique set of the packaging from the POM and use that to set up the attributes. Not sure if that'll work, but it's an idea 😅

I think we can wait on that. This PR has had a lot of work done on it, I'd like to get it in soonish

Can we just apply this filter in the android usecase? It appears this only happens when referencing a subproject from within an android project -- because android subprojects output all these crazy variants?

if android project { apply artifactView } else { just do what we were doing before }

I'm just not super fond of applying the filter and potentially missing some cases (war, zip, rar, etc)

I think just applying the filter in the Android use case would work for now until there's another type of plugin that causes the same issue.

I'm experimenting with making filters based on the packaging in the poms.

I was able to get it working by using a separate artifactView for each packaging found in the POMs, but it required some provider magic redirections (i.e. declare a property for the set of packagings at the top, flatMapping it to the artifactViews, and populating the packagings property later on in the file mapping from the pomInfo property).

It seems to be working pretty well. I tested on a regular Kotlin JVM project, a Kotlin JVM project that depends on a KMP project, and a KMP project, and it all worked fine. For the KMP project, using the jvm configuration resulted in an sbom referencing jar files, and using the js configuration resulted in an sbom referencing klib files (which was pretty cool).

Android was a trouble maker again, but that was because there are conflicts where requesting jar artifacts returns the classes.jar from the aar file. The workaround was to detect if there was an Android plugin and replace aar and jar with android-aar-or-jar in the list of packagings.

It seems to be working with the configuration cache, and from what I can tell the resolution is happening during the configuration phase, so it should be fine. Let me know what you want to do with that.

loosebazooka · 2023-11-29T14:33:43Z

src/main/java/org/spdx/sbom/gradle/SpdxSbomPlugin.java

                  for (var configurationName : configurationNames) {
-                    Provider<Set<ResolvedArtifactResult>> artifacts =


Can you provide an example where this issue arises? It'd be nice to have a test to prevent regression and provide a sort of documentation via code on this specific case?

src/main/java/org/spdx/sbom/gradle/maven/PomResolver.java

eygraber · 2023-11-29T21:35:22Z

Addressed the PR comments. Working on getting a failing test case to demonstrate the attribute/variant issue

eygraber · 2023-11-29T23:46:46Z

Pushed a test case that fails on main but not on this branch. It is technically failing on this branch because there's no Android SDK defined. Options are:

Add a local.properties file to the test project that points to a local Android SDK
Don't expect build success, but look for the Android SDK missing error, since that means that the dependency resolution succeeded

Downside to 1 is that it will require that anyone running tests has the Android SDK at that specific location. Downside to 2 is that it is brittle.

loosebazooka · 2023-11-30T03:35:38Z

The test case was super helpful for me to understand the problem. We can apply a conditional run on the testcase using @EnabledIfEnvironmentVariable or something from junit5. Then we just add an android sdk to CI and always set the ENV to catch this at least in CI. But shouldn't break standard dev flows.

eygraber · 2023-11-30T05:52:58Z

Adding the setup-android action requires using at least JDK 17 to build. Bumping that in the setup-java action should be fine (and updating Gradle to 8.5 should allow using JDK 21), as it's just used for building, and the target compat is set to 11.

loosebazooka · 2023-11-30T13:47:41Z

I think the problem with depending on the pom is that it's not an accurate representation of a variant. A variant can have any packaging, but the pom only shows what the "main" variant's packaging is.

eygraber · 2023-11-30T16:00:31Z

True, so then I guess the best approach would be to special case Android, and deal with any other variant issues similarly when they come up.

This reverts commit 5ce33b4.

eygraber · 2023-11-30T23:35:08Z

OK I made those changes, and surprise! there's another issue 😅

A Kotlin multiplatform (KMP) build that includes an android target would cause issues because the Android plugin is present, so we use the artifactView for aar-or-jar, but if you generate an sbom for the js target (for example, would apply to any non JVM target) then the klib dependencies aren't found (since the attribute filter is only looking for aar and jar).

So we can change the condition to only use the artifactView if the android plugin is present, and the KMP plugin is not present, but that will cause the variant errors when generating an sbom for the android target.

This might not be an issue in practice, since the user would probably be generating the sbom for a leaf module (e.g. an app), and with KMP those tend to only target one platform. What are your thoughts on that?

loosebazooka · 2023-12-01T20:39:34Z

So I'm not familiar enough with kotlin multiplatform to know user behavior with it. I think we get a version of this PR in now with what we kind of landed on earlier. And then we can deal with KMP in a followup.

loosebazooka · 2023-12-04T15:30:15Z

@eygraber so this is ready to merge?

eygraber · 2023-12-04T16:12:37Z

Yes, it should be!

eygraber mentioned this pull request Nov 8, 2023

Several Gradle improvements #27

Closed

eygraber changed the title ~~Arq serialization~~ Run ARQ during task serialization Nov 8, 2023

eygraber mentioned this pull request Nov 9, 2023

Inject project dependencies #71

Merged

Run ARQ during task serialization

2163fa1

- Solves the issue with configuration variants and mismatched attributes - Reflects the current best approach that Gradle suggests - They're hopefully going to provide better APIs in the future

eygraber force-pushed the arq-serialization branch from d4c8f84 to 2163fa1 Compare November 9, 2023 17:28

loosebazooka mentioned this pull request Nov 10, 2023

Ignore project dependencies from artifact file resolution #74

Merged

loosebazooka reviewed Nov 10, 2023

View reviewed changes

loosebazooka requested changes Nov 10, 2023

View reviewed changes

src/main/java/org/spdx/sbom/gradle/maven/DependencyResolver.java Outdated Show resolved Hide resolved

eygraber added 5 commits November 12, 2023 19:24

Create an intermediate list property for root components

37453be

Use method references for casting to ResolvedArtifactResult

2609d3d

Added filter that got left out in refactor

510e3da

Document DependencyResolver

54e8803

Formatting

5c7a665

eygraber requested a review from loosebazooka November 13, 2023 20:58

loosebazooka reviewed Nov 14, 2023

View reviewed changes

eygraber force-pushed the arq-serialization branch 5 times, most recently from a963c25 to 629b042 Compare November 24, 2023 08:25

eygraber commented Nov 24, 2023

View reviewed changes

src/main/java/org/spdx/sbom/gradle/SpdxSbomPlugin.java Outdated Show resolved Hide resolved

eygraber commented Nov 24, 2023

View reviewed changes

eygraber requested a review from loosebazooka November 24, 2023 08:32

eygraber added 2 commits November 24, 2023 03:46

Resolve artifacts using an ArtifactView

1f32579

Set the ArtifactView type based on the type of project

7a9627f

eygraber force-pushed the arq-serialization branch from 629b042 to 7a9627f Compare November 24, 2023 08:51

loosebazooka reviewed Nov 29, 2023

View reviewed changes

Address PR comments

3b4d352

eygraber force-pushed the arq-serialization branch 3 times, most recently from 63b88bd to 2222683 Compare November 30, 2023 05:52

eygraber added 5 commits November 30, 2023 18:09

Add test for builds that use variant dependencies

5439405

Use packaging from POMs to drive artifactViews

5ce33b4

Add an action to setup an Android SDK for tests

785aed1

Revert "Use packaging from POMs to drive artifactViews"

e872aee

This reverts commit 5ce33b4.

Only use an artifactView for Android projects

2810048

eygraber force-pushed the arq-serialization branch from 2222683 to 2810048 Compare November 30, 2023 23:16

loosebazooka approved these changes Dec 1, 2023

View reviewed changes

loosebazooka merged commit e89f622 into spdx:main Dec 5, 2023
1 check passed

eygraber deleted the arq-serialization branch December 6, 2023 00:24

		for (var configurationName : configurationNames) {
		Provider<Set<ResolvedArtifactResult>> artifacts =

Run ARQ during task serialization #72

Run ARQ during task serialization #72

Conversation

eygraber commented Nov 8, 2023 • edited Loading

loosebazooka commented Nov 9, 2023 • edited Loading

eygraber commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loosebazooka commented Nov 10, 2023

loosebazooka left a comment

Choose a reason for hiding this comment

loosebazooka commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loosebazooka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eygraber commented Nov 29, 2023

eygraber commented Nov 29, 2023

loosebazooka commented Nov 30, 2023 • edited Loading

eygraber commented Nov 30, 2023

loosebazooka commented Nov 30, 2023

eygraber commented Nov 30, 2023

eygraber commented Nov 30, 2023

loosebazooka commented Dec 1, 2023 • edited Loading

loosebazooka commented Dec 4, 2023

eygraber commented Dec 4, 2023

eygraber commented Nov 8, 2023 •

edited

Loading

loosebazooka commented Nov 9, 2023 •

edited

Loading

loosebazooka commented Nov 30, 2023 •

edited

Loading

loosebazooka commented Dec 1, 2023 •

edited

Loading