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

Introduce a toolchain for sbom generation and manipulation. #157

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheGrizzlyDev
Copy link
Contributor

@TheGrizzlyDev TheGrizzlyDev commented Sep 23, 2024

This toolchain must support arbitrary opaque internal formats and allow for a last stage translation into whatever final SBOM format the user wants to use (eg: SPDX, CycloneDX, ...). The default toolchain is only for demonstration purposes at the moment and only allows creating SPDX SBOMs with no additional manipulations.

This PR should not change any underlying behaviour, but rather only make the SBOM behaviour modifiable by users.

This toolchain must support arbitrary opaque internal formats and allow
for a last stage translation into whatever final SBOM format the user
wants to use (eg: SPDB, CycloneDX, ...). The default toolchain is only
for demonstration purposes at the moment and only allows creating SPDX
SBOMs with no additional manipulations.
@sluongng
Copy link

@mzeren-vmw @aiuto as mentioned in the meeting yesterday, I think having a toolchain defined would be a good way to separate "interface" and "implementations", especially in the bzlmod setups.

So I am in favor of getting this PR reviewed and merged.

@@ -18,6 +18,8 @@ package(
default_visibility = ["//visibility:public"],
)

load(":toolchains.bzl", "sbom_tools_toolchain")

Choose a reason for hiding this comment

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

Nit: I think there is a buildifier lint rule that mandate load statements must be on top of BUILD files?

@@ -31,3 +33,17 @@ exports_files(
]),
visibility = ["//doc_build:__pkg__"],
)


toolchain_type(name = "sbom_tools")

Choose a reason for hiding this comment

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

Nit: I would prefer toolchain stuffs to live in a separate dir.

That way the target label is more expressive @rules_gathering//:sbom_tools -> @rules_gathering//tools/sbom:toolchain_type. This follows the convention of @bazel_tools//tools/cpp:toolchain_type.

Comment on lines +6 to +10
SbomToolsInfo = provider("Toolchain used to generate and maniputate SBOMs", fields = [
"create_intermediate_sbom",
"merge_intermediate_sboms",
"convert_intermediate_sbom_to_format"
])

Choose a reason for hiding this comment

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

I would prefer to keep the provider in a separate file from the default implementation.
Some folks have already mentioned that they want to use their implementations written in Go/Java.

sbom_tools_toolchain = rule(
_sbom_tools_toolchain,
attrs = {
"create_intermediate_sbom_binary": attr.label(

Choose a reason for hiding this comment

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

Nit: personal taste but I would prefer to have a tight coupling between binary and toolchain rule. So I recommend making this an internal attribute with "_" prefix

Suggested change
"create_intermediate_sbom_binary": attr.label(
"_create_intermediate_sbom_binary": attr.label(

This would communicate to downstream implementations that if you want to use a different binary, you would need to write your own toolchain instead of reusing this one. This should free us from having to maintain flags' backward compatibility for the binary.

@@ -16,3 +16,5 @@ bazel_dep(name = "bazel_skylib", version = "1.7.1", dev_dependency = True)
bazel_dep(name = "rules_pkg", version = "1.0.1", dev_dependency = True)
bazel_dep(name = "rules_python", version = "0.35.0", dev_dependency = True)
bazel_dep(name = "stardoc", version = "0.6.2", dev_dependency = True)

register_toolchains("@rules_license//rules_gathering:example_sbom_tools_toolchain")

Choose a reason for hiding this comment

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

Registering a default toolchain is a great move.

Would be nice to have a small doc explaining to users that the toolchain should be registered automatically in BzlMod, and that it can be overridden if folks don't like the default option.

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.

2 participants