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

Default config should not include test classpath #505

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

blommish
Copy link

@blommish blommish commented Sep 6, 2024

Connected to

https://github.com/CycloneDX/cyclonedx-maven-plugin does not include the test scope as default config.

What has been done

  • To allow setting the list to emptylist, it makes most sense to let set methods overwrite previous values and not use addAll
  • Adding "runtimeClasspath", "compileClasspath", "annotationProcessor" as default configs.
    • These will also include compileOnly, runtimeOnly

For discussion:

Would the classpaths I've added be enough? Or should others also be added? Buildscript, plugin and sourceSet.
I'm not totally sure just having the 3 I've added is enough.
Or would it be better to just add the test-paths to ignore?

I'll wait to update the readme until I know if this could be used or not.

@blommish blommish requested a review from a team as a code owner September 6, 2024 15:35
@VinodAnandan
Copy link

Hi @blommish Thank you for your PR. Could you please address the DCO failure?
https://github.com/CycloneDX/cyclonedx-gradle-plugin/pull/505/checks?check_run_id=29790906669

Copy link
Member

@skhokhlov skhokhlov left a comment

Choose a reason for hiding this comment

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

Hi @blommish Thank you for your PR. There is problem here that this is quite a breaking change of the plugin behaviour. So I believe the best option would be to wait until the next major version release.

@@ -135,6 +134,7 @@ public CycloneDxTask() {
componentVersion.convention(getProject().getVersion().toString());

includeConfigs = getProject().getObjects().listProperty(String.class);
includeConfigs.addAll("runtimeClasspath", "compileClasspath", "annotationProcessor");
Copy link
Member

Choose a reason for hiding this comment

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

I think it should all configurations, but test ones. Otherwise it skips not only tests.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean.

I agree that it is an breaking change.
The issue was that the plugin contains configs that are not included in the maven plugin. #416 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that you cannot fully match maven plugin behavior just because it's a different build system. Using Gradle, you can custom configurations and use them during runtime. Using proposed changed to the default config, plugin will skip it which is incorrect.

 - Adding runtimeClasspath, compileClasspath and annotationProcessorPath to default includeConfigs

Signed-off-by: Johan Blomgren <[email protected]>
@blommish blommish force-pushed the default-config-including-test-scope branch from a8b7285 to 046f7af Compare September 11, 2024 11:52
@skhokhlov skhokhlov added this to the 2.0.0 milestone Oct 23, 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.

3 participants