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

Feat/new implementation cyclonedx bom #532

Merged

Conversation

gordonrousselle
Copy link
Contributor

@gordonrousselle gordonrousselle commented Oct 27, 2024

This is a proposal implementation for the next major version of the cyclonedx gradle plugin. The reason for this is twofold. Firstly, this implementation will support Gradle's configuration cache which will be enforced by default in the near future. Secondly, it aims to resolve some of the open issues reported against the current implementation.

Most of the existing features are still supported, but a couple of points are worth noting:

  • child projects, if exists, are now also included as component in the resulting BOM
  • component that represent gradle projects have purls without a type field (defaulted to JAR in previous version)
  • Version 1.0 of the CycloneDX BOM is not supported anymore

@gordonrousselle gordonrousselle force-pushed the feat/new-implementation-cyclonedxBom branch from 487c48f to e016f01 Compare October 28, 2024 11:51
@skhokhlov skhokhlov added this to the 2.0.0 milestone Oct 28, 2024
@skhokhlov
Copy link
Member

I think this PR should resolve #528, @gordonrousselle can you please add test for it?

@skhokhlov skhokhlov marked this pull request as draft November 1, 2024 22:15
…date sbom, missing nullables and remove duplicated code

Signed-off-by: Gordon <[email protected]>
@gordonrousselle gordonrousselle force-pushed the feat/new-implementation-cyclonedxBom branch from f235111 to be08c09 Compare November 11, 2024 19:39
…ackage:projectsAndScopes properties (adding back in later PR)

Signed-off-by: Gordon <[email protected]>
@skhokhlov skhokhlov marked this pull request as ready for review November 13, 2024 15:16
@skhokhlov skhokhlov force-pushed the feat/new-implementation-cyclonedxBom branch from a2c603f to 7a81820 Compare November 13, 2024 15:20
@jkowalleck jkowalleck requested a review from a team November 13, 2024 16:53
@skhokhlov
Copy link
Member

@stevespringett @DarthHater @nscuro @mr-zepol Do you have any concerns here? If not, I'll merge it tomorrow

@jeremylong
Copy link
Contributor

Somehow I missed this PR - I was about to submit a PR that migrates from Configuration.getResolvedConfiguration() to Configuration.getIncoming() because I have an Android project that is facing issues with variant selection. My PR is not as substantial - but does solve the issue with variant selection (in my case). However, when I use this implementation I still face:

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:cyclonedxBom'.
> Could not resolve all artifacts for configuration ...

The (trimmed) exception is:

...
Cause 22: org.gradle.internal.component.AmbiguousArtifactVariantsException: The consumer was configured to find a library for use during compile-time, preferably optimized for Android, as well as attribute 
...
 at org.cyclonedx.gradle.SbomGraphProvider.lambda$getArtifacts$6(SbomGraphProvider.java:131)

I'm looking into this more to see if I can provide a suggestion/workaround.

.filter(configuration -> shouldIncludeConfiguration(configuration)
&& !shouldSkipConfiguration(configuration)
&& configuration.isCanBeResolved())
.flatMap(config -> config.getIncoming().getArtifacts().getArtifacts().stream())
Copy link
Contributor

Choose a reason for hiding this comment

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

Add lenient artifactView to avoid errors when selecting variants in Android projects:

Suggested change
.flatMap(config -> config.getIncoming().getArtifacts().getArtifacts().stream())
.flatMap(config -> config.getIncoming().artifactView(view -> {view.lenient(true);}).getArtifacts().getArtifacts().stream())

Note that this suggestion will likely fail spotless and this should be run through ./gradlew :spotlessApply.

@skhokhlov
Copy link
Member

Hi @jeremylong, I'm very excited that such small change can fix Android issues. Can you please open a separate PR with this fix? Then we can properly test it and close related issues

@jeremylong
Copy link
Contributor

The "small" change - requires moving from Configuration.getResolvedConfiguration() to Configuration.getIncoming() And using a lenient artifactView. As such, this full PR would be needed - plus my small addition.

I might have a minor fix I can propose to the current implementation. Let me test something.

@jeremylong
Copy link
Contributor

It turns out even using a lenient configuration using the legacy getResolvedConfiguration() still fails for Android:

        final LenientConfiguration resolvedConfiguration =
                configuration.getResolvedConfiguration().getLenientConfiguration();
        final Set<ResolvedDependency> directModuleDependencies =
                resolvedConfiguration.getFirstLevelModuleDependencies();

As such, this PR - with the addition of the lenient artifact view appears to solve the android issues.

dependencyNode.inScopeConfiguration(projectName, configName);
graph.get(graphNode).add(dependencyNode);
queue.add(dependencyNode);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log any UnresolvedDependencyResult?

Suggested change
}
} else if (dep instanceof UnresolvedDependencyResult) {
UnresolvedDependencyResult unresolved = (UnresolvedDependencyResult) dep;
logger.debug(
"Unable to resolve artifact `{}` because {}",
unresolved.getAttempted().getDisplayName(),
unresolved.getFailure().toString());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. I have added it in.

@jeremylong
Copy link
Contributor

I could submit another PR that isn't as large, follows more of the code paths that were used previously, and solves the Android issues. But honestly, the code in this PR is really good and you should just go with this one.

@skhokhlov skhokhlov merged commit e24493f into CycloneDX:master Nov 22, 2024
7 checks passed
@jeremylong
Copy link
Contributor

I'll submit another PR to add the lenient artifactView.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants