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.

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.

@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

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.

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

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...

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!)

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).

leave local builds faster by default

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

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

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.

@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.

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

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

@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.

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.

@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