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

more explicit build dependency handling #755

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

thillux
Copy link
Contributor

@thillux thillux commented Jul 18, 2024

This is a follow up on #736 which was merged and later reverted because of a infinite loop I accidentally produced there.

This PR differs in the following:

  • test for cyclic dependencies added
  • fixed dependency kind indexing under presence of cyclic dependencies
  • changed name of command line option for no build deps in SBOM, as dev deps are already excluded

@thillux thillux requested a review from a team as a code owner July 18, 2024 07:30
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.

I believe I have found a bug, please check the review comments.

cargo-cyclonedx/src/generator.rs Outdated Show resolved Hide resolved
cargo-cyclonedx/src/generator.rs Outdated Show resolved Hide resolved
@Shnatsel
Copy link
Contributor

I retract what I said about resolver v2 - we cannot fix that edge case because cargo metadata doesn't support resolver v2 either.

Please fix the test failures and I'll be happy to merge this.

Signed-off-by: Markus Theil <theil.markus@gmail.com>
Signed-off-by: Markus Theil <theil.markus@gmail.com>
Signed-off-by: Markus Theil <theil.markus@gmail.com>
Signed-off-by: Markus Theil <theil.markus@gmail.com>
@thillux
Copy link
Contributor Author

thillux commented Jul 22, 2024

@Shnatsel ready & thanks for your feedback regarding resolver v2.

@Shnatsel
Copy link
Contributor

Thank you, I will take a look in the next few days.

My revised feedback about resolver v2 is to ignore its existence. cargo metadata does not support it, so there is no way for us to support it either.

I am really sorry about sending you on this wild goose chase. I forgot that Cargo just does not expose enough data for us to handle this correctly.

@Shnatsel
Copy link
Contributor

I'm going to go ahead and merge this to expedite the process. What nits I have I'll address myself in a follow-up PR.

Thank you!

@Shnatsel Shnatsel merged commit 6b262d3 into CycloneDX:main Jul 31, 2024
13 checks passed
This was referenced Jul 31, 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.

2 participants