Fix Issue #1536

[Preston4tw]

Remove "provided" scope from com.google.auto.value:auto-value.
Scope provided seems to cause build system consumers to expect that
auto-value is present on the classpath somewhere at runtime, while
nothing is providing it.

I tested this locally with gradle and this seems to be the smallest
change that fixes #1536. Gradle properly handles the auto-value
dependency without requiring it to be shaded.

[cpovirk]

It might make sense to have a compile/runtime dependency only on auto-value-annotations and then to add auto-value (which contains the processor) to annotationProcessorPaths instead. See https://github.com/google/auto/blob/master/value/userguide/index.md#with-maven

[Preston4tw]

Ok, if I understood you correctly, this next commit is in line with what you proposed, and still seems to work in testing error prone against a gradle project through the net.ltgt.errorprone gradle plugin. It's worth noting that I saw this comment, #1398 (comment) , and I've been testing using -XepPatchChecks:... successfully without needing to include diffutils-1.3.0.jar in the shaded jar as well.

[cpovirk]

Thanks. I was proposing to put auto-value under annotationProcessorPaths, rather than keeping it in dependencies as provided. But I suspect that either way is an improvement over the current setup.

[Preston4tw]

I tried this diff (and then removing com.google.auto.value:auto-value as a dependency in the various pom.xml files), but this results in some test failures I don't quite understand:

diff --git a/pom.xml b/pom.xml
index 5108624da..c819933d0 100644
--- a/pom.xml
+++ b/pom.xml
@@ -131,6 +131,13 @@
           <testExcludes>
             <exclude>**/testdata/**</exclude>
           </testExcludes>
+          <annotationProcessorPaths>
+            <path>
+              <groupId>com.google.auto.value</groupId>
+              <artifactId>auto-value</artifactId>
+              <version>${autovalue.version}</version>
+            </path>
+          </annotationProcessorPaths>
         </configuration>
       </plugin>
       <plugin>

Results :

Failed tests: 
  ErrorProneJavacPluginTest.applyFixes:151 
expected to be true
  ErrorProneJavacPluginTest.applyToPatchFile:210 
expected to be true
  ErrorProneJavacPluginTest.compilesWithFix:383 expected to contain: [TestCompilesWithFix]
but was            : plug-in not found: ErrorProne
  ErrorProneJavacPluginTest.explicitBadPolicyGiven:259 expected java.lang.RuntimeException to be thrown, but nothing was thrown
  ErrorProneJavacPluginTest.hello:110 expected to contain: [CollectionIncompatibleType]
but was            : plug-in not found: ErrorProne
  ErrorProneJavacPluginTest.noPolicyGiven:236 expected java.lang.RuntimeException to be thrown, but nothing was thrown
  ErrorProneJavacPluginTest.stopOnErrorPolicy:313 value of    : iterable.size()
expected    : 2
but was     : 1
iterable was: [plug-in not found: ErrorProne]

Tests run: 3612, Failures: 7, Errors: 0, Skipped: 49

[cpovirk]

Sorry about that. I hit a similar problem with another project, so I think I know what's up.

As soon as you add the first entry to annotationProcessorPaths, Maven/javac stops looking for other annotation processors (and plugins, like Error Prone) in your regular classpath.

This is probably the more sensible behavior. However, if a project currently depends on finding annotation processors or plugins in the regular classpath, it produces a breakage.

The fix is to change the setup for Error Prone itself to list it in annotationProcessorPaths, as in the instructions.

However, I have no idea what additional challenges that might present, so I completely understand if you want to stick with provided. Thanks for trying annotationProcessorPaths out.

(You can see where I unknowingly introduced and unknowingly fixed the problem in another project.)

[Preston4tw]

My understanding so far is that adding the annotationProcessorPaths section to pom.xml adds a -processorpath arg to javac. Without -processorpath specified, annotators are searched for in the classpath.

The test failures are because the tests were relying on being able to find the locally compiled ErrorProne in the classpath, ex. https://github.com/google/error-prone/blob/master/core/src/test/java/com/google/errorprone/ErrorProneJavacPluginTest.java#L302.

As I understand it, the fix here would be to add the target/ directory to processorpath or modify the tests somehow to do that themselves. We can't just add ErrorProne to the top pom.xml as an annotationProcessor because that goes through the normal maven external resolution process (if you set the project pom versions to 2.3.6-SNAPSHOT maven quickly fails on resolution for this reason). I can't figure out how to add target/ to processorpath and I'm also unsure how the tests could be modified to do that.

If anyone has any ideas on how to accomplish the above I'd be happy to try them and modify the PR accordingly if any of the ideas pan out, otherwise I think the PR is good as-is.

As a side note I noticed I had committed the diffutils change and I can either leave that in the PR as-is or remove that if you have any comments on that one way or the other. My testing against a gradle project using JDK 11 and using many -XepPatchChecks: arguments succeed without this dependency being included in the shaded jar.

[tbroyer]

I can't understand why changing how the tests are compiled (looking up annotation processors from annotationProcessorPaths rather than the classpath) would change how they behave at runtime. There must be something else that breaks the tests.
I'll have a look if I find time this weekend.

[tbroyer]

Ok, understood: same change needs to be done with auto-service: use auto-service-annotations as dependency and move auto-service to annotationProcessorPaths.
The META-INF/services/com.sun.source.util.Plugin file no longer is generated, so indeed JavaC can no longer find the plugin when using -Xplugin:ErrorProne.
As @cpovirk said, as soon as you start using -processorpath (annotationProcessorPaths in Maven), you need to move all processors to it (and ideally out of the classpath); but the problem wasn't ErrorProne itself (which is not used when building ErrorProne), but auto-service.

BTW, should auto-service-annotations and auto-value-annotations be shaded into the with-dependencies JAR? They're not required, but if we add them as transitive dependencies to those consuming ErrorProne as a Maven dependency, then maybe we should add them to the with-dependencies JAR too, for consistency.

[Preston4tw]

Thanks for taking a look and explaining the issue @tbroyer. I've tested moving auto-value and auto-service into annotationProcessorPaths and it works in my local testing and passes the unit tests. I've also modified the shade config to include the auto-value and auto-service annotations.

[Preston4tw]

Anything left to do on this PR before merge?