typetools / checker-framework

Pluggable type-checking for Java
http://checkerframework.org/
Other
1.01k stars 352 forks source link

Improve dependency on com.google.errorprone:javac #3213

Open joachim-durchholz-six opened 4 years ago

joachim-durchholz-six commented 4 years ago

https://checkerframework.org/manual/#maven does not explain what the com.google.errorprone:javac dependency is needed for.

If it is a Java compiler, it seems odd to be a runtime dependency. If it is a dependency of the Checker Framework itself, which runs as an annotation processor, it should be a dependency in the section. Even better, org.checkerframework:checker (as supplied in should itself declare its dependency on javac.

Another, minor point: The section declares true. It shouldn't do that, it's the responsibility of the respective release engineer to decide whether forking is a good idea or not - unless Checker Framework explicitly requires a fork, in which case the configuration snippet should explain why.

mernst commented 4 years ago

https://checkerframework.org/manual/#maven does not explain what the com.google.errorprone:javac dependency is needed for.

You can scan down to the next use of "errorprone" to see how it is used. This is explained in detail at https://checkerframework.org/manual/#java8-install . I could add a cross-reference if that would be helpful, but I'm reluctant to suggest that every user should put that explanation in all their pom.xml files.

If it is a Java compiler, it seems odd to be a runtime dependency. If it is a dependency of the Checker Framework itself, which runs as an annotation processor, it should be a dependency in the section.

You are right: this dependency declaration is odd, because com.google.errorprone:javac is only a compile-time dependency. Maven needs to download the artifact from Maven Central, so that the artifact can be mentioned in the JVM arguments. Declaring the runtime dependency achieves this effect. We are not Maven experts, and we couldn't figure out a better way to achieve the effect. Can you give a suggestion about how to improve this Maven configuration?

Another, minor point: The section declares true. It shouldn't do that, it's the responsibility of the respective release engineer to decide whether forking is a good idea or not - unless Checker Framework explicitly requires a fork, in which case the configuration snippet should explain why.

It is necessary. Without <fork>true</fork>, maven-compiler-plugin ignores the supplied JVM arguments (that is, the -J arguments and -Xbootclasspath). I added a note to this effect in f957aae2fe002f82593f5d5a42a175fa13ceef6f. Thanks for the suggestion.

joachim-durchholz-six commented 4 years ago

I meant to say that the reason for using errorprone should be documented (or referenced) on the page. (BTW I consider it a good idea to explain things in POMs. Colleagues need to understand them to be able to maintain them. Oh, and "colleagues" includes my future self two years from now, that guy is so bad at remembering old technical details that I have to help him out.)

For compile-time-only code, I know of three approaches:

Unfortunately, I am just a Maven-use expert, not a Maven-plugin one, so running this by a true Maven plugin expert would probably be smart.