Make FindBugs effort and threshold configurable #84

oleg-nenashev · 2017-10-26T23:12:49Z

This change makes the default configuration of FindBugs much more paranoid about potential issues. #83 bumps the Parent POM to 3.x, so I feel it is a good time to introduce it.

On the Low level there are some Not-important issues, but the experience in jenkinsci/envinject-lib#13 shows that otherwise we may miss real NPE risks in Jenkins.

@reviewbybees @jglick @batmat

This change makes the default configuration of FindBugs much more paranoid. On the Low level there are many Not-important issues, but experience of jenkinsci/envinject-lib#13 shows that otherwise we may miss real NPE risks in Jenkins.

ghost · 2017-10-26T23:16:28Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

stephenc · 2017-10-26T23:26:02Z

I am worried that this is the wrong direction.

Could you provide some examples of real issues that are actually caught by this level in the current codebase.

jglick

Finding more bugs, OK. I guess we will find out if there are a ton of false positives.

oleg-nenashev · 2017-10-26T23:28:56Z

@stephenc see the example I have referenced in the PR description: jenkinsci/envinject-lib#13. There is a missing build nullness check, which is considered as a Low-severity issue by FindBugs

oleg-nenashev · 2017-10-26T23:29:56Z

I guess we will find out if there are a ton of false positives.

In the worst case there is a property which can be set by plugins when they get upgraded.

stephenc · 2017-10-26T23:52:59Z

i’d rather see the property updated on CI jobs and leave local builds faster by default. I fear this will just be another getActiveInstance() mess or else the majority setting the property back down again.

Could we do a poll of plugin developers to see their preferences.

While findbugs at its current level finds enough real bugs that I don’t want to turn it off, it’s close enough with false positives that I continue to want to turn it off.

The PR you cite as an example even contains “incorrect” suppression of findbugs warnings, presumably to get the build to pass... which feels like a warning sign to me.

Just my €0.02

KostyaSha · 2017-10-27T00:20:18Z

I'm using max by default everywhere. The only inconvenient thing that i should annotate method instance of single call that i want suppress. The second is that requireNonNull, Objects.isNull() doesn't work with findbugs inspection. Waiting for spotbugs...

jtnord · 2017-10-27T10:53:42Z

Any change for findbugs should IMO also come hand in hand with spotbogs 3.1 and replace JSR305 given it is dead and no more... (the spotbugs annotations are no longer deprecated!)

oleg-nenashev · 2017-10-27T10:56:45Z

Any change for findbugs should IMO also come hand in hand with spotbogs 3.1 and replace JSR305 given it is dead and no more... (the spotbugs annotations are no longer deprecated!)

It is in my longer-term TODO list. This change will likely require Plugin POM 4.x, but it's something I want to do for better integration with https://github.com/jenkinsci/pom (will likely require JEP as well).

jglick · 2017-10-30T17:20:28Z

leave local builds faster by default

When using -DskipTests FindBugs is already skipped, so that is not an issue.

oleg-nenashev · 2017-10-31T10:18:14Z

Yes, I do not see an issue with build duration. Anyway, FindBugs contributes less that a couple of JTH test cases. Regarding the impact on plugins, it's proposed for Plugin Pom 3.0 for a reason.

The PR you cite as an example even contains “incorrect” suppression of findbugs warnings, presumably to get the build to pass... which feels like a warning sign to me.

The only suppressions in my PR is @SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "Deprecated code"). Yes, FindBugs starts failing the build due to that, but I would argue that it's a correct behavior from Java standpoint. Serialization over remoting just does not require that, because in all sane cases the Serializable class is classloaded from the master.

Could we do a poll of plugin developers to see their preferences.

I will raise a mailing list thread for that

batmat · 2017-11-01T14:50:27Z

pom.xml

@@ -65,6 +65,10 @@
    <!-- Whether the build should fail if findbugs finds any error. -->
    <!-- It is strongly encouraged to leave it as true. Use false with care only in transient situations. -->
    <findbugs.failOnError>true</findbugs.failOnError>
+    <!-- Defines a default FindBugs threshold. -->
+    <!-- In Plugin POM 2.x the default value was Medium, but it was not enough to discover some null handling cases -->
+    <findbugs.threshold>Low</findbugs.threshold>


I'm OK with the idea, a bit more unsure about the ratio of false-positives this could detect.
Currently, I like the fact I'm not bothered that often, and when I do it's almost always a good catch. I wouldn't like to see dozens of errors with only 1 actual in the middle.

jglick · 2017-11-27T19:49:31Z

As an aside:

Serialization over remoting just does not require that, because in all sane cases the Serializable class is classloaded from the master.

Unless the class is defined in remoting.jar itself, in which case this change does not apply anyway.

I seem to recall users of IBM J9 agents complaining that this JVM inferred a different default serialVersionUID than the OpenJDK-based master (or vice-versa), causing problems for remote callables missing an explicit ID. I countered that the Java specification defines the expected default value so it would be a JVM bug in such a case. Not sure if I am remembering the problem correctly.

batmat · 2017-11-30T10:07:50Z

@jglick it's foggy for me too, because from ~10 years ago, but I can relate, and do remember the same experience with serialVersionUid defaults on J9.

jglick · 2017-11-30T20:10:34Z

@jenkinsci/code-reviewers might want to weigh in here.

KostyaSha · 2017-11-30T20:18:20Z

on what level Objects.isNull() works for findbugs?

…-paranoidMode

…e it configurable

oleg-nenashev · 2017-12-03T21:10:09Z

@reviewbybees @jenkinsci/code-reviewers I have reverted the defaults change for now, but I made the parameter configurable at least. So it unblocks my use-cases without changing the default behavior. PTAL.

oleg-nenashev · 2017-12-03T21:11:23Z

on what level Objects.isNull() works for findbugs?

IIRC it does not even with the Max effort. Feel free to test it though.
Keep fingers crossed that SpotBugs fixes it.

jetersen

🚀

stephenc · 2017-12-03T21:12:15Z

pom.xml

+      -->
+    <findbugs.threshold>Medium</findbugs.threshold>
+    <!-- Defines a FindBugs effor. Use "Max" to maximize the scan depth -->
+    <findbugs.effort>default</findbugs.effort>


The only reason I am
Ok with these properties - rather than plugins just shock horror configuring the standard Maven way - is because it allows CLI override which enables easier exploration of the potential impact of changing these levels

oleg-nenashev · 2017-12-03T21:15:05Z

@stephenc

The only reason I am
Ok with these properties - rather than plugins just shock horror configuring the standard Maven way - is because it allows CLI override which enables easier exploration of the potential impact of changing these levels

Yes, it is one of the reasons.
Probably we need to just introduce the "paranoid" profile in Plugin POM, which sets all possible check flags to the most restrictive behavior. Not for the checkstyle, I'd guess

stale

Enable the max effort and low threshold by default.

4692ed8

This change makes the default configuration of FindBugs much more paranoid. On the Low level there are many Not-important issues, but experience of jenkinsci/envinject-lib#13 shows that otherwise we may miss real NPE risks in Jenkins.

oleg-nenashev requested review from jglick and batmat October 26, 2017 23:12

jglick previously approved these changes Oct 26, 2017

View reviewed changes

batmat reviewed Nov 1, 2017

View reviewed changes

jglick requested a review from stephenc November 30, 2017 20:10

oleg-nenashev added 2 commits December 3, 2017 23:59

Merge commit 'fd85a004047d89f9d0fc911398ab7e49680d43ba' into findbugs…

8fc0144

…-paranoidMode

Revert the default FindBugs settings to the POM 2.x behavior, but mak…

c85491f

…e it configurable

oleg-nenashev changed the title ~~Enable the max effort and low threshold by default~~ Make FindBugs effort and threshold configurable Dec 3, 2017

stephenc approved these changes Dec 3, 2017

View reviewed changes

jetersen approved these changes Dec 3, 2017

View reviewed changes

stephenc reviewed Dec 3, 2017

View reviewed changes

Fix typo in the comment

ea31d9c

jglick self-requested a review December 6, 2017 16:37

jglick approved these changes Dec 7, 2017

View reviewed changes

oleg-nenashev merged commit 03b7103 into jenkinsci:master Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make FindBugs effort and threshold configurable #84

Make FindBugs effort and threshold configurable #84

oleg-nenashev commented Oct 26, 2017

ghost commented Oct 26, 2017

stephenc commented Oct 26, 2017

jglick left a comment

oleg-nenashev commented Oct 26, 2017

oleg-nenashev commented Oct 26, 2017

stephenc commented Oct 26, 2017

KostyaSha commented Oct 27, 2017 •

edited

Loading

jtnord commented Oct 27, 2017

oleg-nenashev commented Oct 27, 2017

jglick commented Oct 30, 2017

oleg-nenashev commented Oct 31, 2017

batmat Nov 1, 2017

jglick commented Nov 27, 2017

batmat commented Nov 30, 2017

jglick commented Nov 30, 2017

KostyaSha commented Nov 30, 2017

oleg-nenashev commented Dec 3, 2017

oleg-nenashev commented Dec 3, 2017

jetersen left a comment

stephenc Dec 3, 2017

oleg-nenashev commented Dec 3, 2017

Make FindBugs effort and threshold configurable #84

Make FindBugs effort and threshold configurable #84

Conversation

oleg-nenashev commented Oct 26, 2017

ghost commented Oct 26, 2017

stephenc commented Oct 26, 2017

jglick left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Oct 26, 2017

oleg-nenashev commented Oct 26, 2017

stephenc commented Oct 26, 2017

KostyaSha commented Oct 27, 2017 • edited Loading

jtnord commented Oct 27, 2017

oleg-nenashev commented Oct 27, 2017

jglick commented Oct 30, 2017

oleg-nenashev commented Oct 31, 2017

batmat Nov 1, 2017

Choose a reason for hiding this comment

jglick commented Nov 27, 2017

batmat commented Nov 30, 2017

jglick commented Nov 30, 2017

KostyaSha commented Nov 30, 2017

oleg-nenashev commented Dec 3, 2017

oleg-nenashev commented Dec 3, 2017

jetersen left a comment

Choose a reason for hiding this comment

stephenc Dec 3, 2017

Choose a reason for hiding this comment

oleg-nenashev commented Dec 3, 2017

KostyaSha commented Oct 27, 2017 •

edited

Loading