Add option to use JDK from remote repository instead of the embedded JDK as default host javabase. #6216

meisterT · 2018-09-24T14:12:31Z

After 09cf76e we should only download
that remote JDK if we actually need it.

We plan to hide the embedded JDK so that minimize it in order to shrink
Bazel's binary size. Users that don't need the JDK shouldn't pay the
price for it.
The minimizing step will reduce Bazel's binary size by about 40MB to
~130MB.

RELNOTES[INC]: Add --incompatible_use_remotejdk_as_host_javabase to default the host javabase to remote repository.

meisterT · 2018-09-24T14:13:21Z

cc @iirina @buchgr

cushon · 2018-09-25T00:58:50Z

src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java

@@ -602,6 +615,9 @@ public FragmentOptions getHost() {
    JavaOptions host = (JavaOptions) getDefault();

    host.javaBase = hostJavaBase;
+    if (useRemoteJdkAsHostJavaBase) {
+      host.javaBase = Label.parseAbsoluteUnchecked("@bazel_tools//tools/jdk:remotejdk");
+    }


Instead of unconditionally overriding the --host_javabase, could this supply a default to be used if --host_javabase is unset?

If there's an explicit --host_javabase flag I think we should try to honour that.

Thanks. I've updated the PR and added a test.

cushon · 2018-09-25T01:02:09Z

tools/jdk/BUILD

+        "//src/conditions:darwin_x86_64": "@remotejdk_macos//:jdk",
+        "//src/conditions:windows": "@remotejdk_win//:jdk",
+        "//src/conditions:linux_aarch64": "@remotejdk_linux_aarch64//:jdk",
+        "//conditions:default": "@remotejdk_linux//:jdk",


On non-linux platforms it might be better to not provide a default, and instead require an explicitly configured --host_javabase.

Why is that? I want to hide the embedded JDK without breaking anyone. This is basically maintaining the status quo where people don't have to specify a host_javabase.

Are you referring to this status quo?

bazel/src/BUILD

Line 210 in 1e68ace

"//conditions:default": [

That select is used when building Bazel itself, and binaries with embedded JDKs are provided for a fixed set of platforms.

This select will be used by Bazel installs on arbitrary platforms, and defaulting to linux on non-linux platforms is unhelpful. It's a regression compared to the current behaviour of the binaries without an embedded JDK, which I think default to using the local JDK as the server, host, and target javabases.

I see what you mean now - I've to do some research and will come back to you.

I was about to say the same :) I think it's fine to submit "are we on $OPERATING_SYSTEM to @bazel_tools somewhere. We already have //tools/osx:darwin, which should probably stay as an alias to whatever you come up with.

/cc @ilya-biryukov @mhlopko @katre because this is also interesting for C++ and platforms in general.

re: unknown platforms, I also prefer erroring out. It only happens with the incompatible flag set so it should give people heads-up, and I'd expect that those OSes are a niche use case which is served well enough by an appropriate --host_javabase flag.

As discussed offline: that already exists in @bazel_tools and I'm using it (I had to add linux as an option though).

cushon · 2018-09-25T07:06:12Z

src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java

@@ -601,6 +614,10 @@ public ImportDepsCheckingLevelConverter() {
  public FragmentOptions getHost() {
    JavaOptions host = (JavaOptions) getDefault();

+    if (useRemoteJdkAsHostJavaBase
+        && "@bazel_tools//tools/jdk:host_jdk".equals(hostJavaBase.toString())) {


Can the default be null? That would avoid the duplication here, and also make it possible to distinguish between 'unset' and 'explicitly set to the default value'.

lberki

In general, I am for this change. @meisterT : if you step up to do this work, please also consider the relative merits / demerits of explicitly opting into downloading the JDK and looking for a local one first.

lberki · 2018-09-25T12:06:01Z

src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java

@@ -597,11 +610,22 @@ public ImportDepsCheckingLevelConverter() {
              + "--java_header_compilation is enabled.")
  public boolean requireJavaToolchainHeaderCompilerDirect;

+  private Label getHostJavaBase() {
+    if (hostJavaBase == null) {
+      if (useRemoteJdkAsHostJavaBase) {


Suggestion: how about implementing this as a config_setting()? Then you need less logic in core (only the incompatible flag)

This is just temporary. When we switch the flag on, we can remove all the code here.

lberki · 2018-09-25T12:08:35Z

tools/jdk/BUILD

+        "//src/conditions:darwin_x86_64": "@remotejdk_macos//:jdk",
+        "//src/conditions:windows": "@remotejdk_win//:jdk",
+        "//src/conditions:linux_aarch64": "@remotejdk_linux_aarch64//:jdk",
+        "//conditions:default": "@remotejdk_linux//:jdk",


I was about to say the same :) I think it's fine to submit "are we on $OPERATING_SYSTEM to @bazel_tools somewhere. We already have //tools/osx:darwin, which should probably stay as an alias to whatever you come up with.

/cc @ilya-biryukov @mhlopko @katre because this is also interesting for C++ and platforms in general.

lberki · 2018-09-25T12:11:47Z

tools/jdk/BUILD

+        "//src/conditions:darwin_x86_64": "@remotejdk_macos//:jdk",
+        "//src/conditions:windows": "@remotejdk_win//:jdk",
+        "//src/conditions:linux_aarch64": "@remotejdk_linux_aarch64//:jdk",
+        "//conditions:default": "@remotejdk_linux//:jdk",


re: unknown platforms, I also prefer erroring out. It only happens with the incompatible flag set so it should give people heads-up, and I'd expect that those OSes are a niche use case which is served well enough by an appropriate --host_javabase flag.

meisterT · 2018-09-26T11:43:19Z

I think I've addressed all comments now.

CI is green and I've also tested with the _nojdk variant to make sure that we do the right thing.

cushon · 2018-09-26T16:57:53Z

src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java

@@ -597,11 +610,22 @@ public ImportDepsCheckingLevelConverter() {
              + "--java_header_compilation is enabled.")
  public boolean requireJavaToolchainHeaderCompilerDirect;

+  private Label getHostJavaBase() {
+    if (hostJavaBase == null) {
+      if (useRemoteJdkAsHostJavaBase && "1".equals(System.getProperty("embedded_jdk"))) {


Can you document this part? Why would we only use the remote JDK with binaries that have an embedded JDK?

Added a comment - we do want to make the switch as soft as possible.

cushon · 2018-09-27T16:31:33Z

src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java

+    if (hostJavaBase == null) {
+      if (useRemoteJdkAsHostJavaBase
+          // If there's no embedded JDK, we did use the local JDK as host JDK in the past.
+          // In order to make this flag flip as soft as possible, we will continue to do so.


To confirm: there are no known reasons to prefer using a local JDK in this case, and the motivation for this logic is to make this change a direct replacement for the current behaviour?

Any concerns with following up with another change (maybe behind another --incompatible_ flag) to remove this part? Being able to rely on the supported --host_javabase version for Bazel binaries without an embedded JDK seems like a Feature.

Yes, my motivation is to have a direct replacement for current behavior.

I guess we should figure out what the long term plan is before we the other flag, i.e. whether we want to default to the local JDK for everything or the remote JDK.

Okay. I'm not sure the additional complexity here is buying us much, but I understand wanting to reduce the risk of this change. How is this logic being tested? Is there coverage in bazel_java_test.sh for the no-JDK case?

I tested the logic manually. We could set up a test for it (next to bazel_java_test.sh) but that would require building another bazel and duplicating some bits of test setup. Do you think that's worth it, given this will go away again when we flip the flag?

The "1".equals(System.getProperty("embedded_jdk") part is going away after we flip the flag?

Is the removal of "1".equals(System.getProperty("embedded_jdk") going to happen behind a different --incompatible_changes_ flag?

It results in different behaviour for the binaries with and without embedded JDKs, which is visible to users. Maybe I'm just not understanding the plan.

Yes, the behavior is different for users with and without embedded JDK. It is already different today and I don't want to make it the same in this PR. This is basically maintaining current behavior.

What will the replacement for "1".equals(System.getProperty("embedded_jdk") be when that happens?

external/embedded_jdk links to the local JDK if there's no embedded JDK if I'm not mistaken. We'll use that then.

The whole check that we do in Java code now will go away once we flip/remove the flag since we'll do that in the BUILD file instead.

What will the replacement for "1".equals(System.getProperty("embedded_jdk") be when that happens?

external/embedded_jdk links to the local JDK if there's no embedded JDK if I'm not mistaken. We'll use that then.

I mean, how will that logic be expressed? You mentioned something about putting it in the BUILD file instead of current Java code?

Maybe it would be helpful to see the follow-up patch you have in mind, if I'm not following the explanation.

Thanks for pushing on it.
Turns out that we have to keep some logic about the embedded JDK if we don't want to switch the no JDK case. Example commit:
meisterT@5a2c55a

So we have two options:

keep this check and don't change how we handle the host JDK in the no JDK case

remove the check and also switch how we handle the host JDK in the no JDK case

Preferences?

Got it, thanks.

remove the check and also switch how we handle the host JDK in the no JDK case

I vote for this one, for two reasons:

I think it's where we want to end up eventually, for all of the reasons we aren't using a locally-installed JDK as the default host javabase for the embedded JDK binaries. It's mostly of question of whether to do it now or as a follow-up change with yet another --incompatible_changes_* flag.

The current nojdk --host_javabase behaviour isn't well-tested, and if remove this difference between the nojdk and embedded-JDK cases we have one fewer configuration to test. I'm uneasy about adding this logic without test coverage.

As discussed offline with @lberki, the plan is to try the "remove the check and also switch how we handle the host JDK in the no JDK case" option. Everything else here LGTM.

…JDK as default host javabase. After 09cf76e we should only download that remote JDK if we actually need it. We plan to hide the embedded JDK so that minimize it in order to shrink Bazel's binary size. Users that don't need the JDK shouldn't pay the price for it. The minimizing step will reduce Bazel's binary size by about 40MB to ~130MB. RELNOTES[INC]: Add --incompatible_use_remotejdk_as_host_javabase to default the host javabase to remote repository.

dslomov · 2018-11-05T11:45:04Z

Please follow the process for communicating this breaking change.

meisterT requested a review from lberki as a code owner September 24, 2018 14:12

googlebot added the cla: yes label Sep 24, 2018

meisterT requested a review from cushon September 24, 2018 14:12

cushon reviewed Sep 25, 2018

View reviewed changes

lberki reviewed Sep 25, 2018

View reviewed changes

meisterT force-pushed the remotejdk branch from e06c5d0 to 42fd1ae Compare September 25, 2018 13:22

iirina added the team-Rules-Java Issues for Java rules label Sep 25, 2018

meisterT force-pushed the remotejdk branch 2 times, most recently from 11b215c to 83f00ee Compare September 26, 2018 09:11

cushon reviewed Sep 26, 2018

View reviewed changes

cushon reviewed Sep 27, 2018

View reviewed changes

meisterT force-pushed the remotejdk branch 2 times, most recently from 06d94a2 to 4962301 Compare October 5, 2018 08:53

meisterT force-pushed the remotejdk branch from 4962301 to 95e99f4 Compare October 5, 2018 09:06

meisterT mentioned this pull request Oct 5, 2018

Bazel's embedded JDK is only used to run Bazel. #6313

Closed

bazel-io closed this in 1b75762 Oct 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to use JDK from remote repository instead of the embedded JDK as default host javabase. #6216

Add option to use JDK from remote repository instead of the embedded JDK as default host javabase. #6216

meisterT commented Sep 24, 2018

meisterT commented Sep 24, 2018

cushon Sep 25, 2018

meisterT Sep 25, 2018

cushon Sep 25, 2018

meisterT Sep 25, 2018

cushon Sep 25, 2018

meisterT Sep 25, 2018

lberki Sep 25, 2018

lberki Sep 25, 2018

meisterT Sep 26, 2018

cushon Sep 25, 2018

meisterT Sep 25, 2018

lberki left a comment

lberki Sep 25, 2018

meisterT Sep 26, 2018

lberki Sep 25, 2018

lberki Sep 25, 2018

meisterT commented Sep 26, 2018

cushon Sep 26, 2018

meisterT Sep 27, 2018

cushon Sep 27, 2018

meisterT Sep 27, 2018

cushon Sep 27, 2018

meisterT Oct 2, 2018

cushon Oct 2, 2018

meisterT Oct 2, 2018

cushon Oct 2, 2018

meisterT Oct 4, 2018

cushon Oct 4, 2018

cushon Oct 4, 2018

dslomov commented Nov 5, 2018

Add option to use JDK from remote repository instead of the embedded JDK as default host javabase. #6216

Add option to use JDK from remote repository instead of the embedded JDK as default host javabase. #6216

Conversation

meisterT commented Sep 24, 2018

meisterT commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lberki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meisterT commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dslomov commented Nov 5, 2018