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

feat: Allow override of isSupportingMultipleReports property #908

Conversation

Alex-Vol-Amz
Copy link

In environments where Spotbugs is loaded via classpath instead of a repository access the mechanism for discovering the version fails to produce the desired value for the isSupportingMultipleReports property. Instead of relying on the automatically observed version number this change makes an optional settable flag that can serve the same purpose trusting the consumer to make the right choice.

The intended usage is to enable multiple reports when using the Spotbugs versions > 4.5.0 even when the tool is loaded via alternate dependency mechanisms.

In our build environment the dependency libraries are provided by an external system that is not using standard Gradle dependency declarations and therefore the logic attempting to resolve the dependency to extract the version fails.

Alex-Vol-Amz and others added 2 commits June 24, 2023 16:45
In environments where Spotbugs is loaded via classpath instead of a
repository access the mechanism for discovering the version fails to
produce the desired value for the isSupportingMultipleReports property.
Instead of relying on the automatically observed version number this
change makes an optional settable flag that can serve the same purpose
trusting the consumer to make the right choice.
@Alex-Vol-Amz
Copy link
Author

I considered if it would be appropriate to add this to the README document. It is a very specialized use case forced on us by the way Spotbugs is loaded as a module dependency and not using the standard group:name:version vector declarations.

I can add the new exposed value in the SpotbugsTask documentation in the README if desired. It would be an exceptional case but worth having as it might be a bit confusing to others.

@hazendaz hazendaz requested a review from KengoTODA July 4, 2023 20:35
@KengoTODA
Copy link
Member

First, thanks for your contribution! And sorry for my laziness.

I feel this change which adds a new public API is too complicated to resolve the issue you faced, how do you feel that if we pick some of following alternatives:

  1. Drop support for SpotBugs 4.4.x and older, then treat isSupportingMultipleReports as always true
  2. Use a system property to set isSupportingMultipleReports explicitly just like com.github.spotbugs.snom.worker (which means adding a "hidden" configuration that is usually needless for most of users)

BTW, just for my curiosity, could you share how you do 'loading spotbugs via classpath'?

In environments where Spotbugs is loaded via classpath instead of a repository access

It might be possible to cover your usage by changing the logic to find the artifact, but still not so sure.

@KengoTODA KengoTODA self-assigned this Jul 24, 2023
@Alex-Vol-Amz
Copy link
Author

I am fine with option 1. We already use Spotbugs 4.5 in our environment and do not expect not to have that be the case.

Option 2 is fine as well and will solve the problem in a way not affecting most users, I can make that work by adding the property, but this being Gradle I think I would use gradle properties instead of system properties.

I did not use the right term for the way we load Spotbugs, we embed Gradle in a build system that manages dependencies outside of Gradle, a Gradle plugin we created bridges the gap by providing an extension that we can call and returns as result the ModuleDependency objects we need to add in a dependencies configuration. That makes the version of Spotbugs not visible and the code that attempts to check for it fails producing the No spotbugs found in the {} configuration message. The dependencies themselves resolve the locally cached artifacts and do not provide the standard group/name/version metadata with the ModuleDependency. To make it more complicated we often remap versions to a simplified version string of the form 4.5.x or even 4.x if we treat the module as stable between major releases.

I do not think it is worth the extra effort to discover the local cached artifact and try to infer a version from it.

@github-actions
Copy link

🎉 This issue has been resolved in version 5.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Alex-Vol-Amz
Copy link
Author

Thanks @KengoTODA I appreciate the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants