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

Record the rustc target platform into the SBOM #529

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Shnatsel
Copy link
Contributor

@Shnatsel Shnatsel commented Nov 1, 2023

Fixes #528

Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
@Shnatsel Shnatsel requested a review from a team as a code owner November 1, 2023 16:00
Copy link
Contributor

@lfrancke lfrancke left a comment

Choose a reason for hiding this comment

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

(didn't look at the code yet, just one question)

SingleTarget(target) => vec![Property::new("rustcTarget", &target)],
AllTargets => all_known_targets()
.into_iter()
.map(|target| Property::new("rustcTarget", &target))
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea in general. Have you checked if there are any plans/standards/best practices for what this property could be called? Other tools must have the same idea/concept in general (e.g. Go/C/C++)

Copy link
Contributor Author

@Shnatsel Shnatsel Nov 1, 2023

Choose a reason for hiding this comment

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

I have not really checked for other uses of properties (the free-form field), just skimmed the standardized fields of metadata.

This encodes Rust target triples specifically, e.g. x86_64-unknown-linux-gnu, so this is not going to be interoperable with anything else. Hence the use of properties instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. In that case, does it make sense to use a namespaced property instead?

https://github.com/CycloneDX/cyclonedx-property-taxonomy
https://github.com/CycloneDX/cyclonedx-property-taxonomy/blob/main/cdx.md

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does. Good point.

Copy link
Contributor

@lfrancke lfrancke left a comment

Choose a reason for hiding this comment

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

Tried, tested, works etc.
Happy to approve in principle but I think I'd like to talk about the name of the property a bit first.
This is the first PR (I believe) to introduce a custom property so it'll be a bit more involved. The next one will be easier.

cargo-cyclonedx/src/generator.rs Outdated Show resolved Hide resolved
SingleTarget(target) => vec![Property::new("rustcTarget", &target)],
AllTargets => all_known_targets()
.into_iter()
.map(|target| Property::new("rustcTarget", &target))
Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. In that case, does it make sense to use a namespaced property instead?

https://github.com/CycloneDX/cyclonedx-property-taxonomy
https://github.com/CycloneDX/cyclonedx-property-taxonomy/blob/main/cdx.md

Co-authored-by: Lars Francke <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
Signed-off-by: Sergey "Shnatsel" Davidoff <[email protected]>
@lfrancke
Copy link
Contributor

lfrancke commented Nov 6, 2023

I think this is basically good to go, we just need to wait for the result of CycloneDX/cyclonedx-property-taxonomy#75

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Nov 7, 2023

That property needs a fair bit of design work. We need to specify where it is legal for it to appear: on metadata only, or does it also appear on a component to specify what target is it for? When recording dependencies for all platforms, do we have a boolean flag to indicate it, or do we list all platforms considered? If the latter, does it make sense to make that property an array or a set, instead of specifying it multiple times?

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Nov 8, 2023

I've opened CycloneDX/cyclonedx-property-taxonomy#78 upstream. Once that's merged, this PR will need to be reworked to match that schema.

@Shnatsel Shnatsel marked this pull request as draft November 8, 2023 22:16
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.

Record the target platform in the SBOM
2 participants