HBASE-24065 Enable SpotBugs in PreCommit #1364
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Yetus's SpotBugs module depends on `maven_add_install`. In `Jenkinsfile_GitHub`, I'm pretty strict on my module definitions, with ``` GENERAL_CHECK_PLUGINS = 'all,-compile,-javac,-javadoc,-jira,-shadedjars,-unit' JDK_SPECIFIC_PLUGINS = 'compile,github,htmlout,javac,javadoc,maven,mvninstall,shadedjars,unit' ``` The general check requests all but omits `compile` and `maven`, which I think means the `spotbugs` check gets dropped. Before HBASE-23767, we had just a single yetus that did all, so spotbugs would have been run.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ndimiduk
force-pushed
the
24065-enable-spotbugs-precommit
branch
from
4a9f96a
to
46000cc
Compare
|
🎊 +1 overall
This message was automatically generated. |
saintstack
reviewed
Mar 26, 2020
| @@ -46,6 +46,7 @@ pipeline { | |||
| TESTS_FILTER = 'cc,checkstyle,javac,javadoc,pylint,shellcheck,whitespace,perlcritic,ruby-lint,rubocop,mvnsite' | |||
| EXCLUDE_TESTS_URL = "${JENKINS_URL}/job/HBase-Find-Flaky-Tests/job/${CHANGE_TARGET}/lastSuccessfulBuild/artifact/excludes" | |||
|
|
|||
| //// | |||
There was a problem hiding this comment.
Trying to distinguish between comments that describe the following section of code vs. comments that describe the following line. Suggestions?
| PLUGINS = "${JDK_SPECIFIC_PLUGINS}" | ||
| // | ||
| // run SpotBugs only once, and run it on JDK8. | ||
| PLUGINS = "${JDK_SPECIFIC_PLUGINS},spotbugs" |
There was a problem hiding this comment.
It used to be -findbugs with hyphen. No need of it when doing spotbugs?
There was a problem hiding this comment.
-fingbugs explicitly disabled findbugs. I'm trying to explicitly enable spotbugs. The two appear to be driven by the same yetus "check", so I'm attempting to ensure that module is enabled. Because we don't ship findbugs binaries in our Dockerfile anymore, I think that "check" will do the right thing, as long as it's enabled (see earlier comments re: that check also requiring the maven check as a dependency).
There was a problem hiding this comment.
The -fingbugs is from the release note of yetus, as the official way to enable spotbugs.
https://issues.apache.org/jira/browse/YETUS-749
Of course I haven't checked the code of yetus so maybe specify spotbugs also works.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bharathv
reviewed
Mar 26, 2020
There was a problem hiding this comment.
Something looks off here. Is it just me or has the spotbugs disappeared from the build report?
The way I read https://issues.apache.org/jira/browse/YETUS-749 is --> -findbugs with SPOTBUGS_HOME set enables spotbugs. no? I thought this is what HBASE-23077 did. After HBASE-23077 I do see spot bugs warnings in the github PRs, is there something additional that we are trying to generate?
That's what I'm trying to enable here. I think the functionality was dropped in HBASE-23077 or HBASE-23767.
Ah thanks, I hadn't read the Yetus side of the feature change. I believe that when I'm currently thinking there's a plugin dependency for spotbugs that I'm missing because of the explicit omission of |
|
Oh, unless spotbugs only runs when a java file is modified... function spotbugs_filefilter
{
declare filename=$1
if [[ ${BUILDTOOL} == maven
|| ${BUILDTOOL} == ant ]]; then
if [[ ${filename} =~ \.java$
|| ${filename} =~ (^|/)${SPOTBUGS_MODE}-exclude.xml$ ]]; then
add_test "${SPOTBUGS_MODE}"
fi
fi
} |
ndimiduk
force-pushed
the
24065-enable-spotbugs-precommit
branch
from
46000cc
to
5e33ccf
Compare
|
that worked! |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
requested changes
Mar 27, 2020
| PLUGINS = "${JDK_SPECIFIC_PLUGINS}" | ||
| // | ||
| // run SpotBugs only once, and run it on JDK8. | ||
| PLUGINS = "${JDK_SPECIFIC_PLUGINS},spotbugs" |
There was a problem hiding this comment.
The -fingbugs is from the release note of yetus, as the official way to enable spotbugs.
https://issues.apache.org/jira/browse/YETUS-749
Of course I haven't checked the code of yetus so maybe specify spotbugs also works.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bharathv
reviewed
Mar 27, 2020
There was a problem hiding this comment.
what's the problem for the current pre commit spotbugs? It is run in the general check section, you want to move it to the jdk8 specific section? why?
@Apache9 My last question was exactly this.
The way I read https://issues.apache.org/jira/browse/YETUS-749 is --> -findbugs with SPOTBUGS_HOME set enables spotbugs. no? I thought this is what HBASE-23077 did. After HBASE-23077 I do see spot bugs warnings in the github PRs, is there something additional that we are trying to generate?
I think the patch refactors to use a more sensible query option "spotbugs" rather than un-intuitive -findbugs, but I see your point that it can be run as a part of general checks itself . @ndimiduk can probably can answer that part.
|
Should we do the cosmetic change of -findbugs -> spotbugs? '-findbugs' is misleading for the readers if they don't know '-' actually disables the configs :-) |
|
I think the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Yetus's SpotBugs module depends on
maven_add_install. InJenkinsfile_GitHub, I'm pretty strict on my module definitions, withThe general check requests all but omits
compileandmaven, whichI think means the
spotbugscheck gets dropped. Before HBASE-23767,we had just a single yetus that did all, so spotbugs would have been
run.