Findbugs library should not be distributed to client apps

[lfradin]

The findbugs library is only required at build time. It should never be packaged in a distributed client application. Right now flyingsaucer depends on findbugs with a compile scope, which means every application using flyingsaucer will automatically inherit the dependency, and fingbugs will end in the packaged application (fat jar, war, ...).

The solution is to set its scope to provided, as recommended by various sources including https://stackoverflow.com/a/26868773.

This will avoid the burden for every application to exclude the findbugs dependency from flyingsaucer.

[asolntsev]

@lfradin I don't think we should remove this dependency.

This specific library is not Firebug itself, but is "jsr305.jar" that contains annotations like @Nullable, @Nonnullable etc. It's quite small, and it's useful for end users.

[lfradin]

@asolntsev, this is not removing the findbugs (jsr305) library. This is just making sure it is used by flyingsaucer, but not forced upon lient apps. Client apps should be responsible to decide which annotation library to use. This is best practice.

Also Findbugs (jsr305) has been superseded by spotbugs since 2017 (https://mvnrepository.com/artifact/com.github.spotbugs). See comment https://stackoverflow.com/a/67415527. And project link https://spotbugs.github.io/.

Now I agree that removing this dependency might break some application builds which used flyingsaucer, added the @Nonnull annotation to their own code, but forgot to add a direct dependency on jsr305 to their project. This should be clearly added to the flyingsaucer release notes.

[asolntsev]

@lfradin Look what I mean. JSR305 contains annotations like @Nonnullable and @ParametersAreNonnullByDefault. They help user's IDE to show warnings, thus avoiding potential errors:

If I exclude JSR305 dependency like this:

    implementation("org.xhtmlrenderer:flying-saucer-pdf:9.9.3") {
        exclude group:'com.google.code.findbugs'
    }

then IDE doesn't show such warnings anymore:

[lfradin]

Hi @asolntsev. Indeed, I am aware of this behavior. But you don't need to have the jsr305 to trigger the IDE feature. Indeed, if I exclude jsr305 from flyingsaucer, and add instead my preferred null check library (for example what seems to be the leader to become the successor to jsr305, jspecify, https://jspecify.dev/, by carefully setting it as provided of course), the IDE (Intellij IDEA for me), checks the null calls and will report the issues to the customers.

Note that Google Guava went through this process in the past in Guava 13 (https://github.com/google/guava/wiki/Release13#non-api-changes).

[asolntsev]

But you don't need to have the jsr305 to trigger the IDE feature. Indeed, if I exclude jsr305 from flyingsaucer, and add instead my preferred null check library, the IDE (Intellij IDEA for me), checks the null calls and will report the issues to the customers.

Sorry, but I don't observe such a behaviour. My IDEA does NOT show any warnings:

NB! JSpecify has less annotations compared to jsr305. For example, it's missing one of the most important annotations @CheckReturnValue.

[lfradin]

OK, @asolntsev, if you are not convinced that's OK. I'll close my pull request.

[asolntsev]

@lfradin No-no, I am totally open for dialog. And yes, I also feel that libraries like "jsr305" which are not needed for running production code should not get to production build. I just don't know how to achieve it without loosing IDE suport.

[lfradin]

Hello @asolntsev. Sorry if I misunderstood. I reopened the PR for the sake of discussion. I have no time to look right now, but will see a bit later tonight if I cannot find something else we can do for the IDE.

[asolntsev]

superseded by https://github.com/flyingsaucerproject/flyingsaucer/pull/407