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

SbomExtractor improvements #95

Merged
merged 6 commits into from
Dec 12, 2024
Merged

Conversation

lhns
Copy link
Contributor

@lhns lhns commented Dec 6, 2024

This PR adds the following attributes to sboms:

It also fixes an error caused by modules that are reported multiple times (cross builds, complex dependsOn structure and such)

Copy link
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

looks good, a few optional comments to consider, and you'll have to update the scripted test expectations to match the new behavior.

src/main/scala/com/github/sbt/sbom/BomExtractor.scala Outdated Show resolved Hide resolved
src/main/scala/com/github/sbt/sbom/BomExtractor.scala Outdated Show resolved Hide resolved
src/main/scala/com/github/sbt/sbom/BomExtractor.scala Outdated Show resolved Hide resolved
src/main/scala/com/github/sbt/sbom/BomExtractor.scala Outdated Show resolved Hide resolved
@lhns lhns force-pushed the feature/bom-extractor-updates branch from 900959f to b1dad9c Compare December 9, 2024 11:26
Copy link
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

code is looking good, just needs the default value for 'include bom hashes' in the plugin and updated test expectations in the scripted tests

@lhns
Copy link
Contributor Author

lhns commented Dec 9, 2024

oh no why are the hashes different in ci...

@lhns
Copy link
Contributor Author

lhns commented Dec 9, 2024

ohh they are selected based on the java version in org.cyclonedx.util.BomUtils:

        try {
            digests.add(DigestUtils.getSha3_256Digest());
        } catch (Exception | NoSuchMethodError e) { /* Not available in Java 8 and only available in later versions of DigestUtils */ }

Copy link
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

One final tweak, then looks great to me!

includeBomTimestamp := false,
includeBomToolVersion := true,
includeBomHashes := true,
enableBomSha3Hashes := true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should default this to false, to be more reproducible by default...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought about this but java 8 is mostly end of life anyway and the sha3 family will be pretty important in the future and should probably be enabled by default sooner than later. The gradle cyclonedx plugin also does not filter out hashes.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Great improvement!

@raboof raboof merged commit 062e821 into sbt:main Dec 12, 2024
10 checks passed
@lhns lhns mentioned this pull request Dec 12, 2024
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 this pull request may close these issues.

include component hashes
2 participants