java_proto_library results in spurious warning message

[lberki]

When compiling Java protos, the following error message is emitted:

warning: -parameters is not supported for target value 1.7. Use 1.8 or later.

This is because we force protos to be compiled with -source 7 -target 7 for some reason:

https://github.com/bazelbuild/bazel/blob/master/tools/jdk/BUILD.java_tools#L14

then we separately add -parameters, which is not compatible with target level 7:

https://github.com/bazelbuild/bazel/blob/master/tools/jdk/BUILD.java_tools#L53

Would be nice if we didn't spam the console. I don't see a very simple way of not putting -parameters on the protoc Java compile command lines, though: we don't have a catchall branch in compatible_javacopts and we can't disable a flag there, either.

[davido]

This was also reported in Gerrit issue tracker.

[davido]

It can be easily reproduced, e.g. in rules_closure repository:

  $ b10 build java/io/bazel/rules/closure/worker/...
SUBCOMMAND: # //java/io/bazel/rules/closure/worker:worker_protocol_proto [action 'Compiling Java headers java/io/bazel/rules/closure/worker/libworker_protocol_proto-speed-hjar.jar (1 source jar)', configuration: c153f7599a95154de41a48e7c8ebbc8e]
(cd /home/davido/.cache/bazel/_bazel_davido/2a0702ea37a9bc7e0f689212ecb3673c/execroot/io_bazel_rules_closure && \
  exec env - \
    LC_CTYPE=en_US.UTF-8 \
    PATH=/u01/app/oracle/product/11.2.0/xe/bin:/home/davido/pgm/pt:/home/davido/pgm/apache-maven-3.5.2/bin:/u01/app/oracle/product/11.2.0/xe/bin:/home/davido/pgm/pt:/home/davido/pgm/apache-maven-3.5.2/bin:/home/davido/bin:/usr/local/bin:/usr/bin:/bin:/usr/lib/mit/bin:/usr/lib/mit/sbin \
  external/remotejdk11_linux/bin/java -Xverify:none '--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED' '--add-opens=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED' '--patch-module=java.compiler=external/remote_java_tools_linux/java_tools/java_compiler.jar' '--patch-module=jdk.compiler=external/remote_java_tools_linux/java_tools/jdk_compiler.jar' '--add-opens=java.base/java.nio=ALL-UNNAMED' '--add-opens=java.base/java.lang=ALL-UNNAMED' -jar external/remote_java_tools_linux/java_tools/turbine_direct_binary_deploy.jar --output bazel-out/k8-fastbuild/bin/java/io/bazel/rules/closure/worker/libworker_protocol_proto-speed-hjar.jar --output_deps bazel-out/k8-fastbuild/bin/java/io/bazel/rules/closure/worker/libworker_protocol_proto-speed-hjar.jdeps --temp_dir bazel-out/k8-fastbuild/bin/java/io/bazel/rules/closure/worker/_javac/worker_protocol_proto/libworker_protocol_proto-speed-hjar_temp --bootclasspath bazel-out/k8-fastbuild/bin/external/bazel_tools/tools/jdk/platformclasspath.jar --source_jars bazel-out/k8-fastbuild/bin/java/io/bazel/rules/closure/worker/worker_protocol_proto-speed-src.jar --injecting_rule_kind java_proto_library --javacopts -source 8 -target 8 '-XDskipDuplicateBridges=true' -g -parameters -source 7 -target 7 -- --target_label //java/io/bazel/rules/closure/worker:worker_protocol_proto --classpath bazel-out/k8-fastbuild/bin/external/com_google_protobuf/libprotobuf_java-hjar.jar --noreduce_classpath)
SUBCOMMAND: # //java/io/bazel/rules/closure/worker:worker_protocol_proto [action 'Building java/io/bazel/rules/closure/worker/libworker_protocol_proto-speed.jar (1 source jar)', configuration: c153f7599a95154de41a48e7c8ebbc8e]

So the problem part seems to be this one:

  $  external/remotejdk11_linux/bin/java [...] --javacopts -source 8 -target 8 '-XDskipDuplicateBridges=true' -g -parameters -source 7 -target 7 --

I have sent this PR, but apparently this doesn't help, as even with custom built Bazel version with this PR included, I still see this warning.

@cushon @iirina @meisterT What am I missing?

[iirina]

@davido Your change also requires a java_tools release.

[davido]

@iirina Thanks for pointing this out. I had a strong impression, that what I was doing didn't have any effect. Could you point me to some documentation, how could I conduct java_tools release myself?

I was also trying to bump Java language level to Java 8 in protobuf project: [1].

[1] protocolbuffers/protobuf#6711

[iirina]

Of course, you can check out Manually trying out java_tools before the release. Please let me know if you have further questions or any feedback on trying out java_tools.

[iirina]

Closed by mistake :)

[davido]

Thanks @iirina ! It works now. There are two completely different issues:

• 1 Building and consuming custom java_tools version.

I applied my PR a on Bazel@HEAD, and created the zip on Linux:

  $ bazel build //src:java_tools_java11.zip
  [...]
  bazel-bin/src/java_tools_java11.zip

then I activated in gerrit tree the custom toolchain:

http_archive(
    name = "local_java_tools",
    urls = ["file:///home/davido/projects/bazel/bazel-bin/src/java_tools_java11.zip"]
)

and build gerrit with:

  $ bazel build \
    --host_java_toolchain=@local_java_tools//:toolchain \
    --java_toolchain=@local_java_tools//:toolchain \
  :release

Then I was still seeing two warnings (instead of dozen). The reason for that is that external/com_google_protobuf/libprotobuf_java.jar was still using Protobuf 3.10.0-rc1 with Java 7 compatibility mode, so that the second measure was needed:

• 2.1 Release new protobuf version wit my PR applied. This release is here: https://github.com/davido/protobuf/releases/tag/protobuf-3.10.0-rc10
• 2.2 Release custom version of rules_closure that consumes the custom version of protobuf above. This release is here: https://github.com/davido/rules_closure/releases/tag/v0.30

Patch Gerrit to use the above rules_closure:

diff --git a/WORKSPACE b/WORKSPACE
index 4aeef5c652..c51bea2efd 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -32,9 +32,9 @@ http_archive(
 
 http_archive(
     name = "io_bazel_rules_closure",
-    sha256 = "39b7bec43e6178d065875987b18623d476acd54f355d7711ce9dce4a3eec0795",
-    strip_prefix = "rules_closure-0.25",
-    urls = ["https://github.com/davido/rules_closure/archive/v0.25.tar.gz"],
+    sha256 = "03140a50a6dccc3f5f000f7f9c60ae2a4c44c4f97f5f5cd76ce4506ac60b8d8f",
+    strip_prefix = "rules_closure-0.30",
+    urls = ["https://github.com/davido/rules_closure/archive/v0.30.tar.gz"],
 )
 
 # File is specific to Polymer and copied from the Closure Github -- should be
@@ -69,6 +69,11 @@ closure_repositories(
     omit_rules_cc = True,
 )

Now I can build gerrit without those annoying warnings:

  $ bazel build \
    --host_java_toolchain=@local_java_tools//:toolchain \
    --java_toolchain=@local_java_tools//:toolchain \
  :release

Cool, what?

[davido]

OK, now that I have reproduced the problem it is clear what is going on. As pointed out by @lberki the two Javac options could be mutually exclusive:

• -target [release] (Generate class files for specific VM version)
• -parameters (Generate metadata for reflection on method parameters)

This was always like that, but JDK 11 and later were changed to be more strict about the incompatibility of the aforementioned options and perform the check and issue the warning. So that while all is fine on JDK 8:

  $ cat A.java
  class A {}
  $ javac -version
  javac 1.8.0_222
  $ javac -source 7 -target 7 A.java
  <no warning>

we get this warning on JDK 11:

  $ cat A.java
  class A {}
  $ 
  $ /usr/lib64/jvm/java-11-openjdk-11/bin/javac -source 7 -target 7 -parameters A.java
  warning: -parameters is not supported for target value 1.7. Use 1.8 or later.

BTW the same holds true for JDK 13:

  $ ~/pgm/jdk-13/bin/javac -source 7 -target 7 -parameters A.java
  warning: -parameters is not supported for target value 7. Use 8 or later.

and upcoming JDK 14:

  $ ~/pgm/jdk-14/bin/javac -source 7 -target 7 -parameters A.java
  warning: -parameters is not supported for target value 7. Use 8 or later.

Moreover, now, that we fully understand the problem, it is clear that it is entirely unrelated to java_proto_library so that this issue has much broader scope: "java_library: Setting Java source level 7 and lower results in spurious warning message".

Why we are seeing this spam only on java_proto_library rules? Because this rule is passing source 7 target 7, but this is just a condense. Any java_library with target < 8 would regress after Remote JDK upgrade from 9 to 11.

Reproducer:

A.java:

class A {}

WORKSPACE:

 name("foo")

BUILD:

java_library(
    name = "a",
    srcs = ["A.java"],
    javacopts = [
        "-source 7",
        "-target 7",
    ],
)

With Bazel@HEAD I am getting:

  $ bazel@HEAD build :a
  warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
Target //:a up-to-date:
  bazel-bin/liba.jar

Note, that if I change the language level to 6, I still get the same:

  $ bazel@HEAD build :a
  warning: [options] source value 6 is obsolete and will be removed in a future release
  warning: [options] target value 1.6 is obsolete and will be removed in a future release
  warning: -parameters is not supported for target value 1.6. Use 1.8 or later.
  Target //:a up-to-date:
  bazel-bin/liba.jar

The spam problem is the regression of the upgrade of the remote JDK to JDK 11.

Given that Protobuf rules are targeting language level 7 and given that so many projects depend in Protobuf, and considering the fact, that my attempt to drop Java 7 compatibility in Protobuf project was rejected, this regression affects almost everyone in the wild.

For example in gerrit build toolchain, we are seeing this warning dozen times:

• During the build of protobuf itself (transitive dependency of rules_closure and gerrit)
• During the build of rules_closure (transitive dependency of gerrit)
• During the build of gerrit itself (gerrit has number of java_proto_library rules)

Here is the proper fix.

[cushon]

Why does this problem need to be solved in Bazel, e.g. by normalizing javacopts?
Can't the warning be avoided by not configuring builds to use -source <= 7 -target <= 7 -parameters?

[davido]

Can't the warning be avoided by not configuring builds to use -source <= 7 -target <= 7 -parameters?

I don't see how this can be avoided by build configuration.

Java toolchain definition in Bazel is unconditionally adding -parameters option. Moreover it is also unconditionally adding -source 7 -target 7 options for protobuf Java toolchain, see the code pointers in issue description.

So that the downstream projects not putting the incompatible options, but Bazel does it.

In protobuf project, this rule is used (see https://github.com/protocolbuffers/protobuf/blob/master/BUILD#L658-L659)

java_library(
    name = "protobuf_java",
    srcs = glob([
        "java/core/src/main/java/com/google/protobuf/*.java",
    ]) + [
        ":gen_well_known_protos_java",
    ],
    javacopts = select({
        "//:jdk9": ["--add-modules=jdk.unsupported"],
        "//conditions:default": [
            "-source 7",
            "-target 7",
        ],
    }),
    visibility = ["//visibility:public"],
)

to produce byte code major version 51. Behind Protobuf's project back, Bazel is unconditionally adding -parameters.

In Gerrit project, java_proto_library rule is used (see: https://github.com/GerritCodeReview/gerrit/blob/master/proto/BUILD#L15-L24):

proto_library(
    name = "entities_proto",
    srcs = ["entities.proto"],
)

java_proto_library(
    name = "entities_java_proto",
    visibility = ["//visibility:public"],
    deps = [":entities_proto"],
)

How should Protobuf and Gerrit (and many other) projects configure their java_librarys and java_proto_librarys rules differently to prevent Bazel putting incompatible options on Javac command line?

[cushon]

it is also unconditionally adding -source 7 -target 7

#9450 would have addressed that part, right?

[davido]

#9450 would have addressed that part, right?

Correct, however, even with #9450 merged and released, this simple java_library rule in downstream project would still poison the Javac command line:

  java_library(
      name = "a",
      srcs = ["A.java"],
      javacopts = [
          "-source 7",
          "-target 7",
      ,
  )

And that's exactly the problem of Protobuf project (and my attempt to change that was rejected upstream, see protocolbuffers/protobuf#6711).

Just to make it clear: Gerrit Code Review project does not target Java 7, and as build tool chain maintainer here I totally don't care about Protobuf compatibility with Android or whatever other parties. What I do care about is that build result in Gerrit is spammed (dozen or even hundred of times?) and that the developers are complaining and that this regression with Remote JDK 11 upgrade is not fixed for months.

[cushon]

I see bazel's job here as to collect javac options from multiple sources (e.g. command line --javacopts, java_toolchain.javacopts, java_library.javacopts, ...) and pass them through to javac.

Changing the configuration for the build to avoid passing combinations of flags javac warns about seems like a better solution than trying to do more javac flag parsing in bazel.

I guess you could define a java_toolchain that omits -parameters to ensure it's never used with -source 7 -target 7. It'd be nice if javac allowed -parameters to be disabled with a separate flag (-no-parameters or something) which could be used to override the global options in a particular target; that might be a worthwhile javac feature request.

[davido]

It'd be nice if javac allowed -parameters to be disabled with a separate flag (-no-parameters or something) which could be used to override the global options in a particular target; that might be a worthwhile javac feature request.

Interesting suggestion. However, even if this request would be filed to JDK project, accepted, implemented and released this would take years to be available in the wild. At that point nobody would care about -target 7 and -parameters incompatibility problem. OTOH, the warnings are real (in current and future Protobuf releases, as my attempt to drop Java 7 compatibility mode in Protobuf was rejected) and we see them today and continue to see in the next years.

Based on your suggestion, what if we would add "virtual" support for -no-parameters option in bazel, by adding NoParametersDetectorNormailzer in JavacOptions. This class would drop the -parameters-option in case -no-parameters was passed, and of course it would always filter out virtual -no-parameters-options, as this is a fake option.

That way we would add this addition to the protobuf project to silent that annoying warning:

java_library(
    name = "protobuf_java",
    srcs = glob([
        "java/core/src/main/java/com/google/protobuf/*.java",
    ]) + [
        ":gen_well_known_protos_java",
    ],
    javacopts = select({
        "//:jdk9": ["--add-modules=jdk.unsupported"],
        "//conditions:default": [
            "-source 7",
            "-target 7",
            "-no-parameters",
        ],
    }),
    visibility = ["//visibility:public"],
)

Alternative approach would be to patch Protobuf during the fetch and just remove the -source 7 -target 7 part. The problem with this approach is that Gerrit is not fetching Protobuf itself, but transitive dependency rules_closure does it for Gerrit project. So that either we patch it in rules_closure, or, if this approach is rejected, then we could add omit_protobuf option to closure repositories loading machinery and load and patch Protobuf in Gerrit.

[davido]

Alternative approach would be to patch Protobuf during the fetch and just remove the -source 7 -target 7 part. The problem with this approach is that Gerrit is not fetching Protobuf itself, but transitive dependency rules_closure does it for Gerrit project. So that either we patch it in rules_closure, or, if this approach is rejected, then we could add omit_protobuf option to closure repositories loading machinery and load and patch Protobuf in Gerrit.

Done in: [1], that is going probably to be rejected.

[1] bazelbuild/rules_closure#432

[cushon]

However, even if this request would be filed to JDK project, accepted, implemented and released this would take years to be available in the wild.

It's not too late for JDK 14, which is only 6 months out.

I'm not sure it should be a priority for Bazel to work around perceived toolchain bugs; fixing them upstream seems strictly better.

If you want to avoid patching dependencies, I think the other approach I mentioned of removing -parameters from the java_toolchain you use would work and be less invasive.

[iirina]

I think @davido's proposed PR is reasonable in this case. It doesn't do any harm, it's simple and was handed over to us :) I agree the Bazel team shouldn't invest much time around tuning the toolchain for very specific use-cases, but at the same time we'd like to make the life of Bazel users as easy as possible. One could use default_java_toolchain and override the misc option, but I suspect more users being annoyed by these messages and we can't ask everyone to add a custom toolchain for this sole reason.

[cushon]

It doesn't do any harm

It regresses support for using -parameters with the Java 7 language level, which is a thing we're actually relying on internally.

The entire JavacOptions class is technical debt we should try to pay off by upstreaming javac CLI improvements, and not add to unless it's essential.

[davido]

It regresses support for using -parameters with the Java 7 language level, which is a thing we're actually relying on internally.

How can you use -parameters with the Java 7 language level internally at Google, if Javac doesn't support -parameters and -target 7?

Have you patched your JDK to also produce parameter names even with Java 7 language level or have you patched your JDK to ignore -parameters options when using Java 7 language level, in which case my CL approach is just fine?

In both cases, you could upstream your JDK patches, right? So that the open source community would benefit from it as well?

[cushon]

It doesn't even require patching javac, just using a version that predates JDK-8190452.

[davido]

FWIW, Bazel project is heavily affected by the problem as well. To verify my CL at Bazel@HEAD, I am building java_tools and seeing this:

  $ cd bazel
  $ bazel build //src:java_tools_java11.zip
  [...]
   INFO: From Building src/main/protobuf/libworker_protocol_proto-speed.jar (1 source jar) [for host]:
warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
INFO: From Building src/main/protobuf/libdesugar_deps_proto-speed.jar (1 source jar) [for host]:
warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
INFO: From Building src/main/protobuf/libworker_protocol_proto-speed.jar (1 source jar):
warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
INFO: From Building src/main/protobuf/libjava_compilation_proto-speed.jar (1 source jar):
warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
INFO: From Building src/main/protobuf/libdeps_proto-speed.jar (1 source jar):
warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
INFO: From Building src/main/protobuf/libexecution_statistics_proto-speed.jar (1 source jar) [for host]:
warning: -parameters is not supported for target value 1.7. Use 1.8 or later.
Target //src:java_tools_java11.zip up-to-date:
  bazel-bin/src/java_tools_java11.zip
INFO: Elapsed time: 168.729s, Critical Path: 46.43s
INFO: 557 processes: 466 linux-sandbox, 91 worker.
INFO: Build completed successfully, 628 total actions

So, the whole Bazel Team (50 devs?) around the globe are going to see that spam dozen or hundred times a day for years to come, given that my both approaches to fix it in Protobuf repository and in Bazel repository were rejected?

REALLY?

[iirina]

It regresses support for using -parameters with the Java 7 language level, which is a thing we're actually relying on internally.

I wasn't aware we're relying on it internally. This changes the story a bit :)

At this point I see two options:

• rebuild javac11 used by bazel to accept the -parameters -source 7 -target 7 combination.
• add a new toolchain that doesn't use -parameters in the default java_tools and an incompatible flag that makes it the default in Bazel

[davido]

rebuild javac11 used by bazel to accept the -parameters -source 7 -target 7 combination.

This wouldn't solve the problem for toolchain_vanilla, where we would like to build with vanilla JDK 13,14, ... , and given that toolchain_vanilla inherits its parameters from the default java toolchain.

[iirina]

We can add a new config_setting in protobuf that points to the default toolchain in bazel

config_setting(
    name = "default_bazel_toolchain",
    values = {
        "java_toolchain": "@bazel_tools//tools/jdk:remote_toolchain",
    },
)

and use it the same way they use //:jdk9 to omit -source 7 -target 7. This will affect bazel protobuf users that use javac 11 and target java 7. Java 7 is no longer supported by Oracle since 2015 and Bazel doesn't officially support Java 7, so I think the proposed change is reasonable.

The second part of the problem affects @bazel_tools//tools/jdk:toolchain_vanilla which one can use to build with javac 7 or newer javac versions not yet supported by bazel (javac 13, 14, etc.). For this use case I propose the following:

1. add a new vanilla toolchain target for javac 7 @bazel_tools//tools/jdk:toolchain_vanilla_jdk7
2. keep the old @bazel_tools//tools/jdk:toolchain_vanilla only for newer Java releases
3. add a new config_setting in protobuf to match @bazel_tools//tools/jdk:toolchain_vanilla

Between 1. and 2. has to be an incompatible flag somewhere for java 7 users to migrate to the new jdk7 vanilla toolchain.

Right now we can go ahead with the first part of the plan (default_bazel_toolchain config_setting) which fixes the regression introduced by upgrading to javac 11.

cc @hlopko

[davido]

This will affect bazel protobuf users that use javac 11 and target java 7.

Isn't this is very similar to what my PR was trying to achieve and was already rejected? What makes your think that yet another attempt to do the same: drop support to target Java level 7 in Protobuf project would be accepted? With my and your suggestions, building:

  $ bazel build :protobuf_java

in protobuf project wouldn't produce Java language Level 7 (byte code major version 51) any more.

and use it the same way they use //:jdk9 to omit -source 7 -target 7

It was me who introduced jdk9 config setting in Protobuf (to build Gerrit against newer JDK version), and it was a oversight from my side, to omit "-source 7 and target 7" options for JDK9.

So in fact it is broken now in Protobuf: with JDK9 that doesn't exist any more, Java language level 8 would be generated (byte code major version 52), but with default remote JDK (JDK 11) Java language level 7 (byte code major version 51) would be generated.

Suggestion: instead of adding new config sections for toolchain_vanilla and default_bazel_toolchain in Bazel and Protobuf projects and complicate things even further, how about to talk to Protobuf folks (internally?) and kindly ask them to re-consider and approve my PR?

1. add a new vanilla toolchain target for javac 7 @bazel_tools//tools/jdk:toolchain_vanilla_jdk7
2. keep the old @bazel_tools//tools/jdk:toolchain_vanilla only for newer Java releases
3. add a new config_setting in protobuf to match @bazel_tools//tools/jdk:toolchain_vanilla

I don't think these backwards compatibility efforts are really needed.

[davido]

FTR: This is fixed now in gerrit project.

java_proto_library was fixed with my PR, that was merged in Bazel in the end. To activate new java_tools release, this CL was applied in gerrit.

protobuf_java rules was fixed with patch on fetch approach with my PR, that was merged in rules_closure repository and in this CL in gerrit the rules_closure version was bumped.