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

`javac.jar` updated to JDK 10; will not run on JDK 8 as `--host_javabase` anymore #6035

Closed
lberki opened this Issue Aug 30, 2018 · 11 comments

Comments

Projects
None yet
7 participants
@lberki
Contributor

lberki commented Aug 30, 2018

Since @cushon updated the javac embedded into Bazel to the version in JDK 10, --host_javabase=<JDK8> won't work anymore.

Mitigating circumstances are that if we don't find a JDK, we still default to the embedded one, which does work with the embedded javac and that it's possible to revert to VanillaJavaBuilder to make Bazel work with --host_javabase=<JDK8> at the cost of some loss of functionality.

Before we release, we should try to ensure that we don't have to have another postmortem other than the one in the making for 0.16 .

@lberki

This comment has been minimized.

Contributor

lberki commented Aug 30, 2018

/cc @cushon @buchgr @iirina @philwo

Filing this as a P0 for the time being; feel free to downgrade as you see fit but let's not release before we have a plan.

@buchgr

This comment has been minimized.

Contributor

buchgr commented Aug 30, 2018

I may add that VanillaJavaBuilder on JDK8 does trigger at least one javac bug (the one with the imports) and that some users have already reported that to us. So falling back to VanillaJavaBuilder is not only a loss of functionality but also a breaking change for some.

@cushon

This comment has been minimized.

Contributor

cushon commented Aug 30, 2018

Sorry, this wasn't supposed to be a surprise. Related: 3619e90, c1705f0, 808ec9f.

Using --host_java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8 --java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8 is now required when using --host_javabase=<JDK8>.

@laurentlb

This comment has been minimized.

Contributor

laurentlb commented Aug 31, 2018

To clarify the situation, I have some questions. This is a breaking change, right? Do we have any idea how many many users this will break? Will the users get a meaningful error message?
If a project is broken by the change, all they have to do is pass extra flags?

@davido

This comment has been minimized.

Contributor

davido commented Sep 1, 2018

I think this is only theoretical problem. I'm not aware of anyone who is passing --host_javabase=<JDK8> option. So that I find this announcement in release notes of 0.17.1-rc1 confusing:

- Use VanillaJavaBuilder and disable header compilation in
    toolchain_hostjdk8. The default toolchain will soon drop
    compatibility with JDK 8. Using a JDK 8 host_javabase
    will only be supported when using 'VanillaJavaBuilder' (which
    does not support Error Prone, Strict Java Deps, or reduced classpaths) and with header
    compilation disabled.

IIUC, the fact that Bazel is using embedded JDK10 (or even switch to using JDK11 as embedded JDK in near future), doesn't matter to us, as long as Bazel@HEAD still producing Java 8 byte code per default.

The only huge breaking change for us (all Bazel users?) would be bumping the default toolchain to post Java 8 versions, e.g. with this diff applied on Bazel@HEAD:

diff --git a/tools/jdk/BUILD b/tools/jdk/BUILD
index a42f1656aa..e4abb72fd5 100644
--- a/tools/jdk/BUILD
+++ b/tools/jdk/BUILD
@@ -218,11 +218,10 @@ default_java_toolchain(
     target_version = "8",
 )
 
-# Default to the Java 8 language level.
-# TODO(cushon): consider if/when we should increment this?
+# Default to the Java 10 language level.
 alias(
     name = "toolchain",
-    actual = "toolchain_java8",
+    actual = "toolchain_java10",
 )
 
 [

Bazel would produce (without passing any options) Java 10 byte code per default:

  $ bazel build java/com/google/gerrit/common:server
   Target //java/com/google/gerrit/common:server up-to-date:
  bazel-bin/java/com/google/gerrit/common/libserver.jar
INFO: Elapsed time: 15.365s, Critical Path: 10.01s
  $ javap -v -cp bazel-bin/java/com/google/gerrit/common/libserver.jar com.google.gerrit.common.data.WebLinkInfoCommon | grep -F "major version"
  major version: 54

IMO such a Bazel change should be properly announced so that Bazel users can adjust their build toolchains, IDE integrations and CIs to still produce Java 8 byte code if required (on stable branches Java 8 compatibility should be maintained for years, and, obviously, we would still like to only have one Bazel version installed on our CI-Docker image and build all current and recent Gerrit versions with latest and greatest Bazel release).

So that with the diff above (that switched Bazel default language level to Java 10), I still don't need to pass --host_javabase option to produce Java 8 byte code and can just say:

  $ bazel build --host_java_toolchain=@bazel_tools//tools/jdk:toolchain_java8 --java_toolchain=@bazel_tools//tools/jdk:toolchain_java8 java/com/google/gerrit/common:server
 Target //java/com/google/gerrit/common:server up-to-date:
  bazel-bin/java/com/google/gerrit/common/libserver.jar
INFO: Elapsed time: 15.365s, Critical Path: 10.01s
  $ javap -v -cp bazel-bin/java/com/google/gerrit/common/libserver.jar com.google.gerrit.common.data.WebLinkInfoCommon | grep -F "major version"
  major version: 52

My environment:

  $ $JAVA_HOME/bin/java -fullversion
  openjdk full version "1.8.0_171-b11"
@cushon

This comment has been minimized.

Contributor

cushon commented Sep 1, 2018

This is a breaking change, right?

Yes, explicitly configuring host_javabase=<jdk8> but using the default java_toolchain no longer works.

Do we have any idea how many many users this will break?

I haven't seen a lot of use-cases that aren't met by the default toolchain and where host_javabase=<jdk8> is a hard requirement. But in general I don't think we have good data on the usage of particular Java toolchain features, so it's hard to say.

Will the users get a meaningful error message?

Not particularly. The default toolchain includes JVM flags that don't exist on JDK 8, so compilation actions fail before JavaBuilder is even executed:

bazel build --host_javabase=:jdk8 :all
...
---8<---8<--- Start of log, file at /usr/local/google/home/cushon/.cache/bazel/_bazel_cushon/8c38d57ae237ae214c53581f830f9e07/bazel-workers/worker-6-Javac.log ---8<---8<---
Unrecognized VM option 'CompactStrings'
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
---8<---8<--- End of log ---8<---8<---

If a project is broken by the change, all they have to do is pass extra flags?

For the most part, yes: --host_java_toolchain=@bazel_tools//tools/jdk:to olchain_hostjdk8 --java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8.

That configuration provides a working host_javabase=<jdk8> compatible toolchain, but it isn't always a drop-in replacement for the default toolchain as @buchgr noted.

I tried to improve the RELNOTES entry from 3619e90. The draft release notes for 0.17 includes this:

"""
The default java_toolchain no longer supports using JDK 8 as a --host_javabase. To use JDK 8 as a --host_javabase, also set --host_java_toolchain=@bazel_tools//tools/jdk:to olchain_hostjdk8 --java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8. Note that toolchain_hostjdk8 uses VanillaJavaBuilder (which does not support Error Prone, Strict Java Deps, or reduced classpaths) and disables header compilation.
"""

IIUC, the fact that Bazel is using embedded JDK10 (or even switch to using JDK11 as embedded JDK in near future), doesn't matter to us, as long as Bazel@HEAD still producing Java 8 byte code per default.

Yep, that's the goal.

The only huge breaking change for us (all Bazel users?) would be bumping the default toolchain to post Java 8 versions

Agreed, there are no plans to do that, or at least not for a long time.

If/when it happens I'd expect it to be announced well in advance, and for it to be easy to keep using the Java 8 language level by passing some additional configuration.

@ittaiz

This comment has been minimized.

Member

ittaiz commented Sep 2, 2018

@cushon

This comment has been minimized.

Contributor

cushon commented Sep 2, 2018

@ittaiz the issue described in the original comment affects 0.17rc1. Can you report the problem you're experiencing separately and include a way to reproduce it?

@davido

This comment has been minimized.

Contributor

davido commented Sep 2, 2018

I tried to improve the RELNOTES entry from 3619e90. The draft release notes for 0.17 includes this:

"""
The default java_toolchain no longer supports using JDK 8 as a --host_javabase. To use JDK 8 as a --host_javabase, also set --host_java_toolchain=@bazel_tools//tools/jdk:to olchain_hostjdk8 --java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8. Note that toolchain_java8 uses VanillaJavaBuilder (which does not support Error Prone, Strict Java Deps, or reduced classpaths) and disables header compilation.
"""

I find the improved release notes version still misleading, especially the "Note that toolchain_java8 uses VanillaJavaBuilder (which does not support Error Prone, Strict Java Deps, or reduced classpaths) and disables header compilation." - part, because on most recent Bazel@HEAD (c8e6796) passing toolchain_java8, without --host_javabase JDK8 still supports EP:

  $ bazel build --javacopt="-Xep:ReferenceEquality:ERROR" --host_java_toolchain=@bazel_tools//tools/jdk:toolchain_java8 --java_toolchain=@bazel_tools//tools/jdk:toolchain_java8 java/com/google/gerrit/server:server
[...]
/com/google/gerrit/server/cache/proto/Cache.java:17129: error: [ReferenceEquality] Comparison using reference equality instead of value equality
      return this == DEFAULT_INSTANCE
                  ^
    (see https://errorprone.info/bugpattern/ReferenceEquality)
  Did you mean 'return this.equals(DEFAULT_INSTANCE)'?
/com/google/gerrit/server/cache/proto/Cache.java:17260: error: [ReferenceEquality] Comparison using reference equality instead of value equality
        if (other == com.google.gerrit.server.cache.proto.Cache.AllExternalIdsProto.getDefaultInstance()) return this;
                  ^
    (see https://errorprone.info/bugpattern/ReferenceEquality)
  Did you mean 'if (Objects.equals(other, com.google.gerrit.server.cache.proto.Cache.AllExternalIdsProto.getDefaultInstance())) return this;' or 'if (other.equals(com.google.gerrit.server.cache.proto.Cache.AllExternalIdsProto.getDefaultInstance())) return this;'?
Target //java/com/google/gerrit/server:server failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 34.739s, Critical Path: 22.15s
INFO: 123 processes: 76 remote cache hit, 24 linux-sandbox, 23 worker.
FAILED: Build did NOT complete successfully
@cushon

This comment has been minimized.

Contributor

cushon commented Sep 3, 2018

I find the improved release notes version still misleading, especially the "Note that toolchain_java8...

Thanks, that was a typo: I meant toolchain_hostjdk8.

@davido

This comment has been minimized.

Contributor

davido commented Sep 4, 2018

Thanks, that was a typo: I meant toolchain_hostjdk8.

Indeed, that would be much more clearer.

@laurentlb laurentlb assigned lberki and cushon and unassigned laurentlb Sep 4, 2018

@bazel-io bazel-io closed this in 7eb9ea1 Sep 7, 2018

laurentlb added a commit that referenced this issue Sep 7, 2018

Automated rollback of 808ec9f and 4c9149d and some manual merging.
Fixes #6035.

This change rolls back the version of the JDK embedded into Bazel to 9.

*** Reason for rollback ***

Bazel 0.17 still needs to support JDK 8 and we need to roll out nits deprecation behind a flag of some sort.

*** Original change description ***

Clean up Java toolchain configuration

now that JDK 8 host_javabases are no longer supported.

PiperOrigin-RevId: 211953405

laurentlb added a commit that referenced this issue Sep 7, 2018

Automated rollback of 808ec9f and 4c9149d and some manual merging.
Fixes #6035.

This change rolls back the version of the JDK embedded into Bazel to 9.

*** Reason for rollback ***

Bazel 0.17 still needs to support JDK 8 and we need to roll out nits deprecation behind a flag of some sort.

*** Original change description ***

Clean up Java toolchain configuration

now that JDK 8 host_javabases are no longer supported.

PiperOrigin-RevId: 211953405

laurentlb added a commit that referenced this issue Sep 10, 2018

Automated rollback of 808ec9f and 4c9149d and some manual merging.
Fixes #6035.

This change rolls back the version of the JDK embedded into Bazel to 9.

*** Reason for rollback ***

Bazel 0.17 still needs to support JDK 8 and we need to roll out nits deprecation behind a flag of some sort.

*** Original change description ***

Clean up Java toolchain configuration

now that JDK 8 host_javabases are no longer supported.

PiperOrigin-RevId: 211953405

laurentlb added a commit that referenced this issue Sep 10, 2018

Automated rollback of 808ec9f and 4c9149d and some manual merging.
Fixes #6035.

This change rolls back the version of the JDK embedded into Bazel to 9.

*** Reason for rollback ***

Bazel 0.17 still needs to support JDK 8 and we need to roll out nits deprecation behind a flag of some sort.

*** Original change description ***

Clean up Java toolchain configuration

now that JDK 8 host_javabases are no longer supported.

PiperOrigin-RevId: 211953405

laurentlb added a commit that referenced this issue Sep 12, 2018

Automated rollback of 808ec9f and 4c9149d and some manual merging.
Fixes #6035.

This change rolls back the version of the JDK embedded into Bazel to 9.

*** Reason for rollback ***

Bazel 0.17 still needs to support JDK 8 and we need to roll out nits deprecation behind a flag of some sort.

*** Original change description ***

Clean up Java toolchain configuration

now that JDK 8 host_javabases are no longer supported.

PiperOrigin-RevId: 211953405

laurentlb added a commit that referenced this issue Sep 12, 2018

Automated rollback of 808ec9f and 4c9149d and some manual merging.
Fixes #6035.

This change rolls back the version of the JDK embedded into Bazel to 9.

*** Reason for rollback ***

Bazel 0.17 still needs to support JDK 8 and we need to roll out nits deprecation behind a flag of some sort.

*** Original change description ***

Clean up Java toolchain configuration

now that JDK 8 host_javabases are no longer supported.

PiperOrigin-RevId: 211953405
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment