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

Generate output per target, not per package #441

Closed
wants to merge 13 commits into from

Conversation

lfrancke
Copy link
Contributor

@lfrancke lfrancke commented Jul 28, 2023

This changes the CLI interface of the tool. It removes output-pattern and output-prefix as the name of the file is now generated from the name and type of the target.

This is because a single package can have multiple targes, some of which can be applications, one can be a library.
The old implementation would all tag the same (application if any application target exists) and the name would be the one of the package but each target can have a different name, so the resulting SBOM cannot be mapped to the artifact that is generated.

@lfrancke lfrancke requested a review from a team as a code owner July 28, 2023 15:45
This will currently break everything that is not output-pattern package

Signed-off-by: Lars Francke <git@lars-francke.de>
Signed-off-by: Lars Francke <git@lars-francke.de>
@lfrancke lfrancke marked this pull request as draft July 28, 2023 15:52
lfrancke and others added 7 commits July 28, 2023 18:02
Signed-off-by: Lars Francke <git@lars-francke.de>
…tput file

Signed-off-by: Lars Francke <git@lars-francke.de>
Signed-off-by: Lars Francke <lars.francke@gmail.com>
Signed-off-by: Lars Francke <lars.francke@gmail.com>
Signed-off-by: Lars Francke <lars.francke@gmail.com>
…members will now also be added as dependencies

Signed-off-by: Lars Francke <lars.francke@gmail.com>
Signed-off-by: Lars Francke <lars.francke@gmail.com>
# Conflicts:
#	cargo-cyclonedx/src/generator.rs
Signed-off-by: Lars Francke <lars.francke@gmail.com>
Signed-off-by: Lars Francke <lars.francke@gmail.com>
Signed-off-by: Lars Francke <lars.francke@gmail.com>
@lfrancke
Copy link
Contributor Author

I will try to add tests for this next week

@lfrancke lfrancke marked this pull request as draft August 26, 2023 06:00
@Shnatsel
Copy link
Contributor

I think this PR is a good start, but I'm not convinced that this is the final interface we want to go for.

I would prefer to make the option to write BOMs for everything opt-in, and preserve the existing flags for output selection because it may be necessary to write the BOM to a specific file in automated pipelines.

By default only one file should be written for a single target, with a sane default (binary preferred to library, failure in case multiple binaries exist) and CLI flags to control what exactly the BOM should be emitted for (--bin, --lib, --example - same as Cargo).

@lfrancke
Copy link
Contributor Author

I'm fine with that and I'm not tied to my PR in any way. Feel free to use it in your implementation or discard all of it and start from scratch.

I'll need to think a bit about what we'd require as we'll be using this in an automated pipeline and the pipeline itself doesn't necesarily know what the target is going to be.

We're using a Docker Builder image which just builds a Rust project in a directory but only the thing using the image will then know which artifact and BOM are important.

@Shnatsel
Copy link
Contributor

It occurred to me that there is more nuance to the possible requirements - for example, generating BOMs for all binaries but not the library, if a library is present along with the binaries. This will require a rather lot of flags to properly control, so I like the direction of this PR as an intermediate step towards figuring out all those flags. I would prefer for users with requirements for those flags to show up and discuss it with them, rather than try to design the flags in a vacuum.

@lfrancke
Copy link
Contributor Author

Yeah, our use-case is definitely that we need SBOMs for all binaries but not the libraries.

Copy link
Contributor

@Shnatsel Shnatsel left a comment

Choose a reason for hiding this comment

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

Let's get this merged and shipped, and possibly refine it later.

This clearly generates more correct SBOMs, the behavior is reasonable, and the code looks good by and large.

FYI I am not a fan of multiple unrelated changes being rolled up into a single PR (license error => debug, default behavior changed from workspace-only to all dependencies, and separate SBOMs) but I do not disagree enough with any of the changes to block this.

@Shnatsel
Copy link
Contributor

Thank you for your contribution! I do believe that even if this is not the final state we want to settle on, this is a good starting point and a step in the right direction.

@Shnatsel Shnatsel mentioned this pull request Oct 23, 2023
4 tasks
@Shnatsel
Copy link
Contributor

This PR fixes #444, is that correct?

@Shnatsel Shnatsel mentioned this pull request Oct 23, 2023
@lfrancke
Copy link
Contributor Author

No..I believe that is not fixed in this PR. I couldn't figure out how to do it using the cargo API

@Shnatsel
Copy link
Contributor

So there are separate SBOM files but they all include the same list of components. Is that correct?

@lfrancke
Copy link
Contributor Author

Yes. It's still broken just in a different way :)
I only found the other issue afterwards.

Not sure if we should merge this. It'll work with top level dependencies but not "all"

I think it's strictly better than what we have today but not great.

@Shnatsel
Copy link
Contributor

I see. Best hold it off until that works, then.

@lfrancke
Copy link
Contributor Author

Agreed. I tried but didn't get it working using the current code.

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.

None yet

2 participants