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

Add support for maven optionality, fixes #314 #356

Merged
merged 1 commit into from
May 16, 2023

Conversation

knrc
Copy link
Contributor

@knrc knrc commented May 4, 2023

No description provided.

@hboutemy
Copy link
Contributor

hboutemy commented May 6, 2023

yes, using Maven POM's <dependency><groupId/><artifactId><version/><optional>true</optional></dependency> as the source for CycloneDX component scope="optional" makes sense

IMHO it makes even more sense than the current (#65) use of maven-dependency-analyzer = the bytecode analyzer used by dependency:analyze to detect unused declared dependencies (with known shortcomings)

I think it would be better to have Maven dependency optional value used by default, and let the use of dependency-analyzer just as a compatibility option if someone really prefers

notice that we need to find better name for the plugin property: useMavenOptionality is awful... :)

@hboutemy
Copy link
Contributor

hboutemy commented May 6, 2023

trying to find namings.
We have 2 algorithms to detect CycloneDX component optional scope:

  1. current Use maven dependency analyzer to set scope automatically #65 use of maven-dependency-analyzer bytecode analysis "unused but declared": let's call it "detect unused"?
  2. Maven pom.xml 's dependency optional boolean: POM dependency optional

I'd say that option 2 should be default
and activating option 1 could be named detectUnusedForOptionalScope set to true?

@knrc
Copy link
Contributor Author

knrc commented May 7, 2023

yes, using Maven POM's <dependency><groupId/><artifactId><version/><optional>true</optional></dependency> as the source for CycloneDX component scope="optional" makes sense

It fits with the maven idea, but more importantly it is what users would reasonably expect it to be.

IMHO it makes even more sense than the current (#65) use of maven-dependency-analyzer = the bytecode analyzer used by dependency:analyze to detect unused declared dependencies (with known shortcomings)

Agreed, it's flaky and I now know it doesn't work for any artifacts that have been relocated. More importantly it doesn't match the maven resolution behaviour.

I think it would be better to have Maven dependency optional value used by default, and let the use of dependency-analyzer just as a compatibility option if someone really prefers

I would be very happy if we chose that, do we need to make that change in a major version update?

notice that we need to find better name for the plugin property: useMavenOptionality is awful... :)

Yes, I know :)

@knrc
Copy link
Contributor Author

knrc commented May 7, 2023

trying to find namings. We have 2 algorithms to detect CycloneDX component optional scope:

  1. current Use maven dependency analyzer to set scope automatically #65 use of maven-dependency-analyzer bytecode analysis "unused but declared": let's call it "detect unused"?
  2. Maven pom.xml 's dependency optional boolean: POM dependency optional

I'd say that option 2 should be default and activating option 1 could be named detectUnusedForOptionalScope set to true?

Sounds good to me, I'll rework the PR (likely tomorrow) and push it up again.

@hboutemy hboutemy merged commit f23deec into CycloneDX:master May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants