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.

Sign up for GitHub

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

Bazel: Bump java source and target language level to java 8 #6711

Closed
wants to merge 1 commit into from
Closed

Bazel: Bump java source and target language level to java 8 #6711

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,8 +654,8 @@ java_library(
javacopts = select({
"//:jdk9": ["--add-modules=jdk.unsupported"],
"//conditions:default": [
"-source 7",
"-target 7",
"-source 8",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that we are going to drop Java 7 support?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we would like to drop Java 7 support soon.

Copy link
Contributor Author

@davido davido Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is: Java 11 doesn't support building with language level 7. Moreover, Bazel switched to using JDK 11 internally, and the console is spammed with spurious warning messages:

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

in many issue trackers: [1], [2].

But I see now, that Maven build doing something similar in java/pom.xml so that this PR is incomplete:

        <plugin>
          <artifactId>maven-compiler-plugin</artifactId>
          <version>3.6.1</version>
          <configuration>
            <source>1.7</source>
            <target>1.7</target>
          </configuration>
        </plugin>

But personally, I don't care about Maven. Would it be an option to remove Java 7 support for Bazel driven build, so to say merge this PR as is, but let Maven world untouched? That way all bazel friends in the wild wouldn't see these annoying warnings, for each and every java_proto_library rules in their own and transitive dependencies BUILD files.

For example in Gerrit Code Review, I am seeing these warnings dozen times:

  • During the build of protobuf itself (would be fixed in this PR)
  • During the build of rules_closure (transitive dependency of gerrit)
  • During the build of gerrit itself, should be fixed with this CL in Bazel itself

[1] bazelbuild/bazel#8772
[2] https://bugs.chromium.org/p/gerrit/issues/detail?id=11102

Copy link
Contributor

@rafi-kamal rafi-kamal Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. We don't want existing JDK 7 users to be stuck in an old version of Protobuf. But since you are saying Java 11 doesn't support building language level 7 and Bazel switched to using JDK 11, does this mean there are currently no Bazel users who are using JDK 7?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize, my bad. Java 11, 13 and even 14 version still support java language level 7 (byte code major version 51).

This issue is entirely a Bazel bug (incompatibility of two Java compiler options) and should be be fixed in Bazel instead. Sorry for the noise.

Having said that, you should consider to discontinue Java 7 language level support in future Protobuf releases.

The most recent JDK 13 and upcoming JDK 14 release already issue this warning. JDK 13:

  $ cat A.java 
class A {}
  $ ~/pgm/jdk-13/bin/javac -source 7 -target 7 -parameters A.java
warning: [options] source value 7 is obsolete and will be removed in a future release
warning: [options] target value 7 is obsolete and will be removed in a future release
warning: -parameters is not supported for target value 7. Use 8 or later.

JDK 14:

  $ ~/pgm/jdk-14/bin/javac -source 7 -target 7 -parameters A.java
warning: [options] source value 7 is obsolete and will be removed in a future release
warning: [options] target value 7 is obsolete and will be removed in a future release
warning: -parameters is not supported for target value 7. Use 8 or later.

"-target 8",
],
}),
visibility = ["//visibility:public"],
Expand Down Expand Up @@ -755,8 +755,8 @@ java_library(
javacopts = select({
"//:jdk9": ["--add-modules=jdk.unsupported"],
"//conditions:default": [
"-source 7",
"-target 7",
"-source 8",
"-target 8",
],
}),
visibility = ["//visibility:public"],
Expand All @@ -768,8 +768,8 @@ java_library(
"java/util/src/main/java/com/google/protobuf/util/*.java",
]),
javacopts = [
"-source 7",
"-target 7",
"-source 8",
"-target 8",
],
visibility = ["//visibility:public"],
deps = [
Expand Down