bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23.13k stars 4.05k forks source link

No error prone warnings reported on Bazel@HEAD #6077

Closed davido closed 5 years ago

davido commented 6 years ago

Building with all Error Prone warnings activated on Bazel@HEAD (8f0d73a648ab1abbbb38c1aee465661d5d403ac0) producing almost no warnings. This change seems to be related: 12dcd35ef7a26d690589b0fbefb1f20090cbfe15. When reverted, I see dozen of warnings: [1].

Reproducer: clone Gerrit and run:

  $ bazel build --java_toolchain //tools:error_prone_warnings_toolchain //java/...

[1] http://paste.openstack.org/show/729447

//CC @cushon

cushon commented 6 years ago

Internally javac is logging a large number of warnings because the class file version in the bootclasspath jar exceeds the highest version supported by the compiler, which we're filtering out in JavaBuilder:

warning: bazel-out/host/genfiles/external/bazel_tools/tools/jdk/platformclasspath8.jar(/java/lang/AutoCloseable.class): major version 54 is newer than 53, the highest major version supported by this compiler.
  It is recommended that the compiler be upgraded.

I believe those warnings are harmless: the --release machinery we're using to get a JDK 8 API uses v54 class files for the stub class files, but they're still the JDK 8 versions of those APIs. The skew comes from using a JDK 10 --host_javabase with a JDK 9 javac (due to the rollback in 12dcd35ef7a26d690589b0fbefb1f20090cbfe15).

javac has an internal limit to the number of errors and warnings it's willing to log. By default it's 100, and it can be configured using the -Xmaxerrs and -Xmaxwarns flags. Once it hits that limit it starts ignoring subsequent warnings, including the ones that would be produced by Error Prone.

Setting -Xmaxwarns to a large number works around the problem:

$ git diff -u tools/
diff --git a/tools/BUILD b/tools/BUILD
index 53f441a37b..9f5249a10d 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -1,4 +1,4 @@
-load("@bazel_tools//tools/jdk:default_java_toolchain.bzl", "default_java_toolchain")
+load("@bazel_tools//tools/jdk:default_java_toolchain.bzl", "default_java_toolchain", "DEFAULT_JAVACOPTS")

 py_binary(
     name = "merge_jars",
@@ -34,8 +34,11 @@ JDK9_JVM_OPTS = [
 # See https://github.com/bazelbuild/bazel/issues/3427 for more context
 default_java_toolchain(
     name = "error_prone_warnings_toolchain_bazel_0.16",
-    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath.jar"],
+    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath8.jar"],
     jvm_opts = JDK9_JVM_OPTS,
+    misc = DEFAULT_JAVACOPTS + [
+        "-Xmaxwarns 9999",
+    ],
     package_configuration = [
         ":error_prone",
     ],
@@ -44,7 +47,10 @@ default_java_toolchain(

 default_java_toolchain(
     name = "error_prone_warnings_toolchain",
-    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath.jar"],
+    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath8.jar"],
+    misc = DEFAULT_JAVACOPTS + [
+        "-Xmaxwarns 9999",
+    ],
     package_configuration = [
         ":error_prone",
     ],
$ bazel build --java_toolchain //tools:error_prone_warnings_toolchain //...
# lots of warnings are produced
cushon commented 6 years ago

@lberki the TL;DR is that 12dcd35ef7a26d690589b0fbefb1f20090cbfe15 causes javac compilation warnings to sometimes get dropped from build output, especially for large compilations.

I see a few options:

davido commented 6 years ago

roll 12dcd35 forward

@lberki May be I'm not seeing the whole picture here, but I don't understand why 30aac1c was reverted in 12dcd35, especially shortly before releasing 0.17. The commit message is also very short. Can you elaborate?

lberki commented 6 years ago

@davido , we are currently trying to figure out in what state to ship Bazel 0.17 . My initial reaction was that let's roll back the javac upgrade so that we have one less change to worry about, but then this bug happened and @cushon informed me about some things that could go wrong with that plan. I'm currently taking stock of the situation.

davido commented 6 years ago

@lberki Thanks for clarifying.

I have no doubt that you and @cushon will figure out what is the right course of action here. My feeling is though, that it's probably not the best choise to mix JDK 10 --host_javabase with a JDK 9 javac after the revert of 12dcd35.

Just to let you know, that I've tested Bazel@HEAD before the rollback in 12dcd35, and wasn't able to find any issue. We've even tried experimental JDK11 support with vanilla toolchain and all worked as expected.

lberki commented 6 years ago

Update: we decided to take a mulligan and retry the JDK/javac version bump in Bazel 0.18 after we add testing:

https://groups.google.com/forum/#!topic/bazel-sig-jvm/mGRr-asogn0

Sorry about the mess again :(

cushon commented 6 years ago

it's probably not the best choise to mix JDK 10 --host_javabase with a JDK 9 javac

It's not ideal, but we have validated that configuration fairly extensively against our code and I have relatively high confidence in it.

This issue in this bug is kind of a blind spot in that validation, since we don't surface warnings in the build log (Error Prone warnings are integrated into code review instead).

cushon commented 6 years ago

This should have been fixed by 7eb9ea150fb889a93908d96896db77d5658e5005; the --host_javabase and javac versions match again.