Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No error prone warnings reported on Bazel@HEAD #6077

Closed
davido opened this issue Sep 4, 2018 · 8 comments
Closed

No error prone warnings reported on Bazel@HEAD #6077

davido opened this issue Sep 4, 2018 · 8 comments
Assignees
Labels
team-Rules-Java Issues for Java rules

Comments

@davido
Copy link
Contributor

davido commented Sep 4, 2018

Building with all Error Prone warnings activated on Bazel@HEAD (8f0d73a) producing almost no warnings. This change seems to be related: 12dcd35. 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
Copy link
Contributor

cushon commented Sep 5, 2018

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 12dcd35).

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 cushon added the team-Rules-Java Issues for Java rules label Sep 5, 2018
@cushon
Copy link
Contributor

cushon commented Sep 5, 2018

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

I see a few options:

  • fix this after 0.17
  • try the -Xmaxwarns <large number> work-around, which should be harmless aside from potentially resulting in build log spam if a compilation produces a huge number of 'real' warnings
  • roll 12dcd35 forward

@davido
Copy link
Contributor Author

davido commented Sep 5, 2018

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
Copy link
Contributor

lberki commented Sep 5, 2018

@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
Copy link
Contributor Author

davido commented Sep 5, 2018

@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
Copy link
Contributor

lberki commented Sep 5, 2018

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
Copy link
Contributor

cushon commented Sep 5, 2018

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
Copy link
Contributor

cushon commented Sep 7, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-Java Issues for Java rules
Projects
None yet
Development

No branches or pull requests

3 participants