java_test: Classpath collision between external dependency on auto-value (1.4-rc1) with internally shipped auto-value (1.2) in TestRunner_deploy.jar

[davido]

/CC @dborowitz @hanwen

Not sure, if it's auto-value or Bazel issue. Reporting it here, because it works as expected with Buck, but failing with Bazel:

https://gerrit-ci.gerritforge.com/view/Gerrit/job/Gerrit-master-bazel/203/console

After upgrading Bazel to 0.4.0 and auto-value to com.google.auto.value:auto-value:1.4-rc1, our unit test sarted to fail:

  $ bazel test --test_output errors --test_summary detailed --flaky_test_attempts 3 --test_verbose_timeout_warnings //gerrit-common:auto_value_tests
INFO: Found 1 test target...
ERROR: /home/davido/projects/gerrit2/gerrit-common/BUILD:71:1: Java compilation in rule '//gerrit-common:auto_value_tests' failed: Worker process sent response with exit code: 1.
gerrit-common/src/test/java/com/google/gerrit/common/AutoValueTest.java:25: error: @AutoValue processor threw an exception: java.lang.NullPointerException
  abstract static class Auto {
                  ^
  	at com.google.auto.value.processor.AutoValueProcessor.getFieldOfClasses(AutoValueProcessor.java:566)
  	at com.google.auto.value.processor.AutoValueProcessor.allMethodExcludedAnnotations(AutoValueProcessor.java:818)
  	at com.google.auto.value.processor.AutoValueProcessor.defineVarsForType(AutoValueProcessor.java:763)
  	at com.google.auto.value.processor.AutoValueProcessor.processType(AutoValueProcessor.java:488)
  	at com.google.auto.value.processor.AutoValueProcessor.process(AutoValueProcessor.java:195)
  	at com.sun.tools.javac.processing.JavacProcessingEnvironment.callProcessor(JavacProcessingEnvironment.java:803)
  	at com.sun.tools.javac.processing.JavacProcessingEnvironment.discoverAndRunProcs(JavacProcessingEnvironment.java:715)
  	at com.sun.tools.javac.processing.JavacProcessingEnvironment.access$2000(JavacProcessingEnvironment.java:93)
  	at com.sun.tools.javac.processing.JavacProcessingEnvironment$Round.run(JavacProcessingEnvironment.java:1023)
  	at com.sun.tools.javac.processing.JavacProcessingEnvironment.doProcessing(JavacProcessingEnvironment.java:1130)
  	at com.sun.tools.javac.main.JavaCompiler.processAnnotations(JavaCompiler.java:1141)
  	at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:832)
  	at com.sun.tools.javac.main.Main.compile(Main.java:253)
  	at com.google.devtools.build.buildjar.javac.BlazeJavacMain.compile(BlazeJavacMain.java:140)
  	at com.google.devtools.build.buildjar.javac.BlazeJavacMain.compile(BlazeJavacMain.java:132)
  	at com.google.devtools.build.buildjar.AbstractJavaBuilder$1.invokeJavac(AbstractJavaBuilder.java:66)
  	at com.google.devtools.build.buildjar.ReducedClasspathJavaLibraryBuilder.compileSources(ReducedClasspathJavaLibraryBuilder.java:81)
  	at com.google.devtools.build.buildjar.AbstractJavaBuilder.compileJavaLibrary(AbstractJavaBuilder.java:69)
  	at com.google.devtools.build.buildjar.AbstractJavaBuilder.run(AbstractJavaBuilder.java:108)
  	at com.google.devtools.build.buildjar.BazelJavaBuilder.processRequest(BazelJavaBuilder.java:92)
  	at com.google.devtools.build.buildjar.BazelJavaBuilder.runPersistentWorker(BazelJavaBuilder.java:70)
  	at com.google.devtools.build.buildjar.BazelJavaBuilder.main(BazelJavaBuilder.java:47)
gerrit-common/src/test/java/com/google/gerrit/common/AutoValueTest.java:27: error: cannot find symbol
      return new AutoValue_AutoValueTest_Auto(val);
                 ^
  symbol:   class AutoValue_AutoValueTest_Auto
  location: class Auto
2 errors
Target //gerrit-common:auto_value_tests failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.754s, Critical Path: 0.27s

Executed 0 out of 1 test: 1 fails to build.

Interesting enough, on the same code base, Buck build just works:

$ buck test //gerrit-common:auto_value_tests
Shutting down nailgun server...
Using watchman.
[-] PROCESSING BUCK FILES...FINISHED 0.1s [100%] 🐳  New buck daemon
[-] DOWNLOADING... (0.00 B/S AVG, TOTAL: 0.00 B, 0 Artifacts)
[-] BUILDING...FINISHED 1.7s [100%] (24/24 JOBS, 24 UPDATED, 2 [8.3%] CACHE MISS)
[-] TESTING...FINISHED 0.4s (1 PASS/0 FAIL)
RESULTS FOR //gerrit-common:auto_value_tests
PASS    <100ms  1 Passed   0 Skipped   0 Failed   com.google.gerrit.common.AutoValueTest
TESTS PASSED

Bazel vesion:

$ bazel version
Build label: 0.4.0

[davido]

So, verified just now: after downgrading auto-value to 1.3-rc1, it works again on Bazel 0.4.0:

$ grep auto-value WORKSPACE 
  artifact = 'com.google.auto.value:auto-value:1.3-rc1',

$ bazel test --test_output errors --test_summary detailed --flaky_test_attempts 3 --test_verbose_timeout_warnings //gerrit-common:auto_value_tests
INFO: Found 1 test target...
Target //gerrit-common:auto_value_tests up-to-date:
  bazel-bin/gerrit-common/auto_value_tests.jar
  bazel-bin/gerrit-common/auto_value_tests
INFO: Elapsed time: 4.380s, Critical Path: 1.23s

Executed 1 out of 1 test: 1 test passes.

[davido]

Well, this diff in auto-value project would fix this breakage:

diff --cc value/src/main/java/com/google/auto/value/processor/AutoValueProcessor.java
index f945bf0,f945bf0..6d5dcb7
--- a/value/src/main/java/com/google/auto/value/processor/AutoValueProcessor.java
+++ b/value/src/main/java/com/google/auto/value/processor/AutoValueProcessor.java
@@@ -562,8 -562,8 +562,12 @@@ public class AutoValueProcessor extend
        Class<? extends Annotation> annotation,
        String fieldName,
        Elements elementUtils) {
--    TypeMirror annotationMirror =
--        elementUtils.getTypeElement(annotation.getCanonicalName()).asType();
++    TypeElement typeMirrorElement =
++        elementUtils.getTypeElement(annotation.getCanonicalName());
++    if (typeMirrorElement == null) {
++      return ImmutableSet.of();
++    }
++    TypeMirror annotationMirror = typeMirrorElement.asType();

      for (AnnotationMirror annot : element.getAnnotationMirrors()) {
        if (!annot.getAnnotationType().equals(annotationMirror)) {

However this is the wrong fix. I think I understand now what happens. First thing to notice, is that only one single java_tests rule: //gerrit-common:auto_value_tests is affected but not other java_libary rules in Gerrit project that use auto-value library.

What is the difference between java_library (works as expected) and java_tests (is broken after upgrade of auto-value library to 1.4-rc1)?

The major difference is that implicitly TestRunner_deploy.jar is prepended (this appears to be the culprit) to the classpath: [1]

--target_label
//gerrit-common:auto_value_tests
--classpath
external/bazel_tools/tools/jdk/TestRunner_deploy.jar:[...]:bazel-out/local-fastbuild/genfiles/external/auto_value/jar/_ijar/jar/external/auto_value/jar/auto-value-1.4-rc1-ijar.jar

But, the external/bazel_tools/tools/jdk/TestRunner_deploy.jar is ueber JAR with (now) outdated version 1.2 of auto-value library in addition to the auto-value-1.4-rc1-ijar.jar. So that we have class path collision. The missing class is AutoValue.CopyAnnotation, that was introduced in this commit: [2]:

$ jar -tf bazel-out/local-fastbuild/genfiles/external/auto_value/jar/_ijar/jar/external/auto_value/jar/auto-value-1.4-rc1-ijar.jar | grep CopyAnnotations
com/google/auto/value/AutoValue$CopyAnnotations.class

$ jar -tf /home/davido/.cache/bazel/_bazel_davido/27a001f4182820ef315d8d2d4f1edafe/execroot/gerrit/external/bazel_tools/tools/jdk/TestRunner_deploy.jar | grep AutoValue
com/google/testing/junit/runner/util/AutoValue_TestIntegration$1.class
com/google/testing/junit/runner/util/AutoValue_TestIntegration$Builder.class
com/google/testing/junit/runner/util/AutoValue_TestIntegration.class
com/google/auto/value/AutoValue.class
com/google/auto/value/AutoValue$Builder.class
com/google/auto/value/AutoValue.java
com/google/auto/value/processor/AutoValueTemplateVars.java
com/google/auto/value/processor/AutoValueProcessor$1.class
com/google/auto/value/processor/AutoValueProcessor$ObjectMethodToOverride.class
com/google/auto/value/processor/AutoValueBuilderProcessor.class
com/google/auto/value/processor/AutoValueBuilderProcessor.java
com/google/auto/value/processor/AutoValueProcessor.class
com/google/auto/value/processor/AutoValueProcessor$Property.class
com/google/auto/value/processor/AutoValueProcessor.java
com/google/auto/value/processor/AutoValueTemplateVars.class
com/google/auto/value/extension/AutoValueExtension.class
com/google/auto/value/extension/AutoValueExtension$Context.class
com/google/auto/value/extension/AutoValueExtension.java

Note that the com/google/auto/value/AutoValue$CopyAnnotations is not included, and this is causing the NPE in this invocation:

private ImmutableSetMultimap<ExecutableElement, String> allMethodExcludedAnnotations(
      Iterable<ExecutableElement> methods) {
    ImmutableSetMultimap.Builder<ExecutableElement, String> result = ImmutableSetMultimap.builder();
    for (ExecutableElement method : methods) {
      result.putAll(
          method,
          getFieldOfClasses(
              method, AutoValue.CopyAnnotations.class, "exclude", processingEnv.getElementUtils()));
    }
    return result.build();
  }

[1] http://paste.openstack.org/show/588066
[2] google/auto@6dca85e

[davido]

Fixed in: [1].

[1] https://bazel-review.googlesource.com/7212

[aehlig]

Assigning to @philwo as he is reviewing the proposed patch anyway.

[cgrushko]

Ugh, that sucks...
A few notes -

1. The PR doesn't fix the problem, of course. It might actually break others, whose AutoValue is of an older version. Nevertheless, I'll approve it, because Bazel has itself to gain from a bumped version (specifically, the CopyAnnotation which I added, allows for AutoValue-ed classes to be exposed to Skylark).
2. @kush-c is looking into loading the runner classes in a separate class-loader, which might solve the problem. In the meanwhile, we can (a) stop depending on AutoValue in the TestRunner or (b) try to move the TestRunner_deploy.jar to the end of the classpath. I have a vague feeling that Java pick the first class it finds. (b) could solve all such collisions, if it works.

I assigned to @kush-c to have a look.

[kush-c]

@lberki,

1. Were we not trying to remove all external dependencies from the TestRunner? Is Autovalue one of the dependency we wish to remove from the TestRunner?
2. Regarding the separate ClassLoader to load the Test Target's classes, that's part of a separate project, which we're still experimenting with, and I wasn't planning to modify the master branch anytime soon. However if we do decide to use a separate classloader to solve this problem, I could add it. (I have something similar to what Buck does here: https://github.com/facebook/buck/blob/master/src/com/facebook/buck/cli/bootstrapper/ClassLoaderBootstrapper.java)

@cgrushko,
3. Even if moving the TestRunner_deploy.jar to the end of the classpath works, will it not be just an inversion of the current problem?

[cgrushko]

Yep, moving the TestRunner to the end might cause the TestRunner to fail. Disregard the idea :)

[lberki]

/cc @iirina

[lberki]

I'm removing the dependency on AutoValue from the test runner.

[davido]

Thanks for the quick fix.

[tbroyer]

AutoValue has, by design, zero runtime dependency, so couldn't it have simply been marked as neverlink=1? AFAICT that'd have excluded it from TestRunner_deploy.jar and would have fixed the issue too.
(same for AutoService BTW)