Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
yesamer merged PR #6634: URL: https://github.com/apache/incubator-kie-drools/pull/6634 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
yesamer commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058269606
##
drools-base/src/main/java/org/drools/base/definitions/impl/KnowledgePackageImpl.java:
##
@@ -614,7 +614,7 @@ public void wireTypeDeclarations() {
Class typeClass = null;
try {
typeClass = typeDeclaration.getTypeClass();
-if (typeClass != null || !typeClass.isPrimitive()) {
+if (typeClass != null && !typeClass.isPrimitive()) {
Review Comment:
@gitgabrio Yes! I think that plugin is quite useful, we should evaluate to
enable it some modules
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
yesamer commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058269606
##
drools-base/src/main/java/org/drools/base/definitions/impl/KnowledgePackageImpl.java:
##
@@ -614,7 +614,7 @@ public void wireTypeDeclarations() {
Class typeClass = null;
try {
typeClass = typeDeclaration.getTypeClass();
-if (typeClass != null || !typeClass.isPrimitive()) {
+if (typeClass != null && !typeClass.isPrimitive()) {
Review Comment:
Yes! I think that plugin is quite useful, we should evaluate to enable it
some modules
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
gitgabrio commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058256654
##
drools-base/src/main/java/org/drools/base/definitions/impl/KnowledgePackageImpl.java:
##
@@ -614,7 +614,7 @@ public void wireTypeDeclarations() {
Class typeClass = null;
try {
typeClass = typeDeclaration.getTypeClass();
-if (typeClass != null || !typeClass.isPrimitive()) {
+if (typeClass != null && !typeClass.isPrimitive()) {
Review Comment:
Yes, clearly is a bug.. My doubt is that "in theory" it should throw a NPE
(the right-side should be executed only when typeClass is `null`, IINW), but it
no-one noticed that until now... anyway, good catch 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
yesamer commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058121582
##
build-parent/pom.xml:
##
@@ -1935,19 +1934,11 @@
com.github.spotbugs
spotbugs-maven-plugin
${version.com.github.spotbugs-maven-plugin}
-
-
- org.kie
- kie-build-tools
- ${project.version}
-
-
6
Max
true
${spotbugs.failOnViolation}
-spotbugs-excludes.xml
Review Comment:
@gitgabrio Yes, you can find that file here -->
https://github.com/kiegroup/droolsjbpm-build-bootstrap/tree/main/kie-build-tools/src/main/resources,
imported through kie-build-tools dependency
It contains a single rule.
I didn't find any other reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
yesamer commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058121582
##
build-parent/pom.xml:
##
@@ -1935,19 +1934,11 @@
com.github.spotbugs
spotbugs-maven-plugin
${version.com.github.spotbugs-maven-plugin}
-
-
- org.kie
- kie-build-tools
- ${project.version}
-
-
6
Max
true
${spotbugs.failOnViolation}
-spotbugs-excludes.xml
Review Comment:
@gitgabrio Yes, you can find that file here -->
https://github.com/lanceleverich/droolsjbpm-build-bootstrap/tree/master/kie-build-tools/src/main/resources,
imported through kie-build-tools dependency
It contains a single rule.
I didn't find any other reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
yesamer commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058125797
##
drools-base/src/main/java/org/drools/base/definitions/impl/KnowledgePackageImpl.java:
##
@@ -614,7 +614,7 @@ public void wireTypeDeclarations() {
Class typeClass = null;
try {
typeClass = typeDeclaration.getTypeClass();
-if (typeClass != null || !typeClass.isPrimitive()) {
+if (typeClass != null && !typeClass.isPrimitive()) {
Review Comment:
That is a bug spotted by spotbugs-maven-plugin
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
yesamer commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058121582
##
build-parent/pom.xml:
##
@@ -1935,19 +1934,11 @@
com.github.spotbugs
spotbugs-maven-plugin
${version.com.github.spotbugs-maven-plugin}
-
-
- org.kie
- kie-build-tools
- ${project.version}
-
-
6
Max
true
${spotbugs.failOnViolation}
-spotbugs-excludes.xml
Review Comment:
@gitgabrio Yes, you can find that file here -->
https://github.com/lanceleverich/droolsjbpm-build-bootstrap/tree/master/kie-build-tools/src/main/resources,
imported through kie-build-tools dependency
It contains a single rule.
I didn't find any other references.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
gitgabrio commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058018029
##
build-parent/pom.xml:
##
@@ -1935,19 +1934,11 @@
com.github.spotbugs
spotbugs-maven-plugin
${version.com.github.spotbugs-maven-plugin}
-
-
- org.kie
- kie-build-tools
- ${project.version}
-
-
6
Max
true
${spotbugs.failOnViolation}
-spotbugs-excludes.xml
Review Comment:
out of curiosity: do you know why that exclude was there and how it behaved ?
##
drools-base/pom.xml:
##
@@ -214,9 +214,6 @@
com.github.spotbugs
spotbugs-maven-plugin
-
Review Comment:
out of curiosity: do you know why that exclude was there and how it behaved ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
gitgabrio commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058027623
##
drools-drl/drools-drl-parser/pom.xml:
##
@@ -105,7 +105,6 @@
com.github.spotbugs
spotbugs-maven-plugin
-
${project.basedir}/src/main/spotbugs/spotbugs-exclude.xml
Review Comment:
Same as above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
gitgabrio commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058025519
##
drools-compiler/pom.xml:
##
@@ -185,7 +185,6 @@
com.github.spotbugs
spotbugs-maven-plugin
-
${project.basedir}/src/main/spotbugs/spotbugs-exclude.xml
Review Comment:
Same as above
##
drools-core/pom.xml:
##
@@ -225,9 +225,6 @@
com.github.spotbugs
spotbugs-maven-plugin
-
Review Comment:
Same as above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
gitgabrio commented on code in PR #6634: URL: https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058024337 ## drools-base/pom.xml: ## @@ -214,9 +214,6 @@ com.github.spotbugs spotbugs-maven-plugin - Review Comment: out of curiosity: why this removal ? Or, do you know why that exclude was there ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
gitgabrio commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058018029
##
build-parent/pom.xml:
##
@@ -1935,19 +1934,11 @@
com.github.spotbugs
spotbugs-maven-plugin
${version.com.github.spotbugs-maven-plugin}
-
-
- org.kie
- kie-build-tools
- ${project.version}
-
-
6
Max
true
${spotbugs.failOnViolation}
-spotbugs-excludes.xml
Review Comment:
out of curiosity: why this removal ? Or, do you know why that exclude was
there ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
gitgabrio commented on code in PR #6634:
URL:
https://github.com/apache/incubator-kie-drools/pull/6634#discussion_r3058015115
##
drools-base/src/main/java/org/drools/base/definitions/impl/KnowledgePackageImpl.java:
##
@@ -614,7 +614,7 @@ public void wireTypeDeclarations() {
Class typeClass = null;
try {
typeClass = typeDeclaration.getTypeClass();
-if (typeClass != null || !typeClass.isPrimitive()) {
+if (typeClass != null && !typeClass.isPrimitive()) {
Review Comment:
🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
[PR] Fix `spotbug-maven-plugin` usage [incubator-kie-drools]
yesamer opened a new pull request, #6634: URL: https://github.com/apache/incubator-kie-drools/pull/6634 Summary of Changes in This PR - Removed the kie-build-tools dependency from the SpotBugs configuration. It points to an old external module, and version 999-SNAPSHOT is no longer available, which breaks dependency resolution. - Removed references to missing SpotBugs exclude filter files. These references pointed to files that no longer exist and to the kie-build-tools JAR. - Upgraded the spotbugs-maven-plugin. - Fixed a potential NullPointerException detected by the `spotbugs-maven-plugin`. As a follow-up, we could agree to enable the `spotbugs-maven-plugin` in this project’s submodules (namely `drools-base`, `drools-compiler`, `drools-core`, and `drools-drl-parser` that already declare it) so that the build fails when SpotBugs detects any issues at compile time. It appears there was already an intention to enable this behavior in the past, since the following configuration is present in the Drools build-parent POM: ``` false ``` To enable this in specific submodules, we should: 1. In the target submodule’s pom.xml, set spotbugs.failOnViolation to true under . 2. Keep the spotbugs-maven-plugin execution bound to the verify phase, with both the spotbugs and check goals enabled: ``` com.github.spotbugs spotbugs-maven-plugin spotbugs verify spotbugs check ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
