Add version to JavaRuntimeInfo.

benjaminp · copybara-github · commit 7556e1107b66 · 2023-03-23T08:00:29.000-07:00
1. As suggested in #6354 (comment), add a JDK version attribute to the java_runtime rule. This will allow changing behavior based on the Java runtime's version. 2. As an application of the above, pass -Djava.security.manager=allow to Java tests on JDK 17+. This makes the Bazel Java test runner to work on JDK 19. Fixes #14502. 3. To make the above generally useful, the remote_java_repository and local_java_repository must set the new version attribute in the java_runtime rules that they create. This means the old static jdk.BUILD file no longer suffices. Move the contents of jdk.BUILD into a Starlark file, so the version can be interpolated in by JDK repository rules and macros. (This isn't the nicest, but local_java_repository is not a repository rule, so the template cannot be in a non-Starlark file.) Closes #17775. PiperOrigin-RevId: 518860040 Change-Id: I8223b6407dd09528a4e5a6bf12354e5fc68278c6
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
@@ -57,6 +57,7 @@
 import com.google.devtools.build.lib.rules.java.JavaConfiguration.OneVersionEnforcementLevel;
 import com.google.devtools.build.lib.rules.java.JavaHelper;
 import com.google.devtools.build.lib.rules.java.JavaRuleOutputJarsProvider;
+import com.google.devtools.build.lib.rules.java.JavaRuntimeInfo;
 import com.google.devtools.build.lib.rules.java.JavaSemantics;
 import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider;
 import com.google.devtools.build.lib.rules.java.JavaTargetAttributes;
@@ -548,6 +549,9 @@ public Iterable<String> getJvmFlags(
         if (testClass == null) {
           ruleContext.ruleError("cannot determine test class");
         } else {
+          if (JavaRuntimeInfo.from(ruleContext).version() >= 17) {
+            jvmFlags.add("-Djava.security.manager=allow");
+          }
           // Always run junit tests with -ea (enable assertion)
           jvmFlags.add("-ea");
           // "suite" is a misnomer.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/jdk.WORKSPACE.tmpl b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/jdk.WORKSPACE.tmpl
@@ -1,14 +1,15 @@
 # External dependencies for the java_* rules.
 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
 load("@bazel_tools//tools/build_defs/repo:utils.bzl", "maybe")
+load("@bazel_tools//tools/jdk:jdk_build_file.bzl", "JDK_BUILD_TEMPLATE")
 load("@bazel_tools//tools/jdk:local_java_repository.bzl", "local_java_repository")
 load("@bazel_tools//tools/jdk:remote_java_repository.bzl", "remote_java_repository")
 
 maybe(
     local_java_repository,
     name = "local_jdk",
     java_home = DEFAULT_SYSTEM_JAVABASE,
-    build_file = "@bazel_tools//tools/jdk:jdk.BUILD",
+    build_file_content = JDK_BUILD_TEMPLATE,
 )
 
 # OpenJDK distributions that should only be downloaded on demand (e.g. when
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntime.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntime.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.rules.java;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.devtools.build.lib.packages.Type.INTEGER;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -129,7 +130,8 @@ public ConfiguredTarget create(RuleContext ruleContext)
             hermeticInputs,
             libModules,
             defaultCDS,
-            hermeticStaticLibs);
+            hermeticStaticLibs,
+            ruleContext.attributes().get("version", INTEGER).toIntUnchecked());
 
     TemplateVariableInfo templateVariableInfo =
         new TemplateVariableInfo(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntimeInfo.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntimeInfo.java
@@ -52,7 +52,8 @@ public static JavaRuntimeInfo create(
       NestedSet<Artifact> hermeticInputs,
       @Nullable Artifact libModules,
       @Nullable Artifact defaultCDS,
-      ImmutableList<CcInfo> hermeticStaticLibs) {
+      ImmutableList<CcInfo> hermeticStaticLibs,
+      int version) {
     return new JavaRuntimeInfo(
         javaBaseInputs,
         javaHome,
@@ -62,7 +63,8 @@ public static JavaRuntimeInfo create(
         hermeticInputs,
         libModules,
         defaultCDS,
-        hermeticStaticLibs);
+        hermeticStaticLibs,
+        version);
   }
 
   @Override
@@ -125,6 +127,7 @@ private static JavaRuntimeInfo from(RuleContext ruleContext, ToolchainInfo toolc
   @Nullable private final Artifact libModules;
   @Nullable private final Artifact defaultCDS;
   private final ImmutableList<CcInfo> hermeticStaticLibs;
+  private final int version;
 
   private JavaRuntimeInfo(
       NestedSet<Artifact> javaBaseInputs,
@@ -135,7 +138,8 @@ private JavaRuntimeInfo(
       NestedSet<Artifact> hermeticInputs,
       @Nullable Artifact libModules,
       @Nullable Artifact defaultCDS,
-      ImmutableList<CcInfo> hermeticStaticLibs) {
+      ImmutableList<CcInfo> hermeticStaticLibs,
+      int version) {
     this.javaBaseInputs = javaBaseInputs;
     this.javaHome = javaHome;
     this.javaBinaryExecPath = javaBinaryExecPath;
@@ -145,6 +149,7 @@ private JavaRuntimeInfo(
     this.libModules = libModules;
     this.defaultCDS = defaultCDS;
     this.hermeticStaticLibs = hermeticStaticLibs;
+    this.version = version;
   }
 
   /** All input artifacts in the javabase. */
@@ -232,6 +237,11 @@ public Depset starlarkJavaBaseInputs() {
     return Depset.of(Artifact.class, javaBaseInputs());
   }
 
+  @Override
+  public int version() {
+    return version;
+  }
+
   @Override
   public com.google.devtools.build.lib.packages.Provider getProvider() {
     return PROVIDER;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntimeRule.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaRuntimeRule.java
@@ -18,6 +18,7 @@
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 import static com.google.devtools.build.lib.packages.BuildType.LICENSE;
+import static com.google.devtools.build.lib.packages.Type.INTEGER;
 import static com.google.devtools.build.lib.packages.Type.STRING;
 
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
@@ -83,6 +84,11 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("java_home", STRING))
         .add(attr("output_licenses", LICENSE))
+        /* <!-- #BLAZE_RULE(java_runtime).ATTRIBUTE(version) -->
+        The feature version of the Java runtime. I.e., the integer returned by
+        <code>Runtime.version().feature()</code>.
+        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+        .add(attr("version", INTEGER))
         .build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/java/JavaRuntimeInfoApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/java/JavaRuntimeInfoApi.java
@@ -104,4 +104,11 @@ public interface JavaRuntimeInfoApi extends StructApi {
       doc = "Returns the JDK static libraries.",
       structField = true)
   Sequence<CcInfo> starlarkHermeticStaticLibs();
+
+  /** The Java feature version of the runtime. This is 0 if the version is unknown. */
+  @StarlarkMethod(
+      name = "version",
+      doc = "The Java feature version of the runtime. This is 0 if the version is unknown.",
+      structField = true)
+  int version();
 }
diff --git a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl
@@ -54,6 +54,8 @@ def _bazel_java_binary_impl(ctx):
     )
 
     if ctx.attr.use_testrunner:
+        if semantics.find_java_runtime_toolchain(ctx).version >= 17:
+            jvm_flags.append("-Djava.security.manager=allow")
         test_class = ctx.attr.test_class if hasattr(ctx.attr, "test_class") else ""
         if test_class == "":
             test_class = helper.primary_class(ctx)
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
@@ -580,6 +580,7 @@ public void setupMockToolsRepository(MockToolsConfig config) throws IOException
         "",
         "def http_jar(**kwargs):",
         "  pass");
+    config.create("embedded_tools/tools/jdk/jdk_build_file.bzl", "JDK_BUILD_TEMPLATE = ''");
     config.create(
         "embedded_tools/tools/jdk/local_java_repository.bzl",
         "def local_java_repository(**kwargs):",
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/java/BUILD b/src/test/java/com/google/devtools/build/lib/bazel/rules/java/BUILD
@@ -17,11 +17,14 @@ java_library(
     name = "JavaTests_lib",
     srcs = glob(["*.java"]),
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/analysis:actions/template_expansion_action",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/bazel/rules/java:bazel_java_semantics",
         "//src/main/java/com/google/devtools/build/lib/packages",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
+        "//src/test/java/com/google/devtools/build/lib/testutil:TestConstants",
         "//third_party:guava",
         "//third_party:junit4",
         "//third_party:truth",
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/java/JavaConfiguredTargetsTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/java/JavaConfiguredTargetsTest.java
@@ -14,11 +14,18 @@
 
 package com.google.devtools.build.lib.bazel.rules.java;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.MoreCollectors.onlyElement;
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.TestConstants.TOOLS_REPOSITORY;
 
 import com.google.common.base.Joiner;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.actions.TemplateExpansionAction;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.util.OS;
+import java.util.Arrays;
+import java.util.Objects;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -46,4 +53,44 @@ public void testResourceStripPrefix() throws Exception {
         Joiner.on(" ").join(getGeneratingSpawnActionArgs(getBinArtifact("bin.jar", target)));
     assertThat(resourceJarArgs).contains("--resources a/path/to/strip/bar.props:bar.props");
   }
+
+  @Test
+  public void javaTestSetsSecurityManagerPropertyOnVersion17() throws Exception {
+    scratch.file(
+        "a/BUILD",
+        "java_runtime(",
+        "    name = 'jvm',",
+        "    java = 'java_home/bin/java',",
+        "    version = 17,",
+        ")",
+        "toolchain(",
+        "    name = 'java_runtime_toolchain',",
+        "    toolchain = ':jvm',",
+        "    toolchain_type = '" + TOOLS_REPOSITORY + "//tools/jdk:runtime_toolchain_type',",
+        ")",
+        "java_test(",
+        "    name = 'test',",
+        "    srcs = ['FooTest.java'],",
+        "    test_class = 'FooTest',",
+        ")");
+    useConfiguration("--extra_toolchains=//a:java_runtime_toolchain");
+    var ct = getConfiguredTarget("//a:test");
+    var executable = getExecutable(ct);
+    if (OS.getCurrent() == OS.WINDOWS) {
+      var jvmFlags =
+          getGeneratingSpawnActionArgs(executable).stream()
+              .filter(a -> a.startsWith("jvm_flags="))
+              .flatMap(a -> Arrays.stream(a.substring("jvm_flags=".length()).split("\t")))
+              .collect(toImmutableList());
+      assertThat(jvmFlags).contains("-Djava.security.manager=allow");
+    } else {
+      var jvmFlags =
+          ((TemplateExpansionAction) getGeneratingAction(executable))
+              .getSubstitutions().stream()
+                  .filter(s -> Objects.equals(s.getKey(), "%jvm_flags%"))
+                  .collect(onlyElement())
+                  .getValue();
+      assertThat(jvmFlags).contains("-Djava.security.manager=allow");
+    }
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/java/JavaRuntimeInfoTest.java b/src/test/java/com/google/devtools/build/lib/rules/java/JavaRuntimeInfoTest.java
@@ -38,7 +38,8 @@ public void equalityIsObjectIdentity() {
             NestedSetBuilder.emptySet(Order.STABLE_ORDER),
             null,
             null,
-            ImmutableList.of());
+            ImmutableList.of(),
+            17);
     JavaRuntimeInfo b =
         JavaRuntimeInfo.create(
             NestedSetBuilder.emptySet(Order.STABLE_ORDER),
@@ -49,7 +50,8 @@ public void equalityIsObjectIdentity() {
             NestedSetBuilder.emptySet(Order.STABLE_ORDER),
             null,
             null,
-            ImmutableList.of());
+            ImmutableList.of(),
+            17);
 
     new EqualsTester().addEqualityGroup(a).addEqualityGroup(b).testEquals();
   }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoaderTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoaderTest.java
@@ -106,6 +106,8 @@ private static void mockEmbeddedTools(Path embeddedBinaries) throws IOException
         "  if name not in native.existing_rules():",
         "    repo_rule(name = name, **kwargs)");
     FileSystemUtils.writeIsoLatin1(tools.getRelative("tools/jdk/BUILD"));
+    FileSystemUtils.writeIsoLatin1(
+        tools.getRelative("tools/jdk/jdk_build_file.bzl"), "JDK_BUILD_TEMPLATE = ''");
     FileSystemUtils.writeIsoLatin1(
         tools.getRelative("tools/jdk/local_java_repository.bzl"),
         "def local_java_repository(**kwargs):",
diff --git a/src/test/py/bazel/query_test.py b/src/test/py/bazel/query_test.py
@@ -40,8 +40,10 @@ def testSimpleQuery(self):
 
   def testQueryFilesUsedByRepositoryRules(self):
     self.ScratchFile('WORKSPACE')
-    self._AssertQueryOutputContains("kind('source file', deps(//external:*))",
-                                    '@bazel_tools//tools/jdk:jdk.BUILD')
+    self._AssertQueryOutputContains(
+        "kind('source file', deps(//external:*))",
+        '@bazel_tools//tools/genrule:genrule-setup.sh',
+    )
 
   def testBuildFilesForExternalRepos_Simple(self):
     self.ScratchFile('WORKSPACE', [
diff --git a/tools/jdk/BUILD b/tools/jdk/BUILD
@@ -16,7 +16,7 @@ filegroup(
         "default_java_toolchain.bzl",
         "fail_rule.bzl",
         "java_toolchain_alias.bzl",
-        "jdk.BUILD",
+        "jdk_build_file.bzl",
         "local_java_repository.bzl",
         "nosystemjdk/README",
         "proguard_whitelister.py",
diff --git a/tools/jdk/BUILD.tools b/tools/jdk/BUILD.tools
@@ -200,7 +200,6 @@ alias(
 
 exports_files([
     "BUILD.java_tools",
-    "jdk.BUILD",
 ])
 
 alias(
diff --git a/tools/jdk/jdk_build_file.bzl b/tools/jdk/jdk_build_file.bzl
@@ -1,4 +1,20 @@
-load("@rules_java//java:defs.bzl", "java_runtime")
+# Copyright 2023 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""A templated BUILD file for Java repositories."""
+
+JDK_BUILD_TEMPLATE = """load("@rules_java//java:defs.bzl", "java_runtime")
 
 package(default_visibility = ["//visibility:public"])
 
@@ -67,4 +83,6 @@ java_runtime(
         ":jdk-lib",
         ":jre",
     ],
+    version = {RUNTIME_VERSION},
 )
+"""
diff --git a/tools/jdk/local_java_repository.bzl b/tools/jdk/local_java_repository.bzl
@@ -146,11 +146,17 @@ def _local_java_repository_impl(repository_ctx):
     version = repository_ctx.attr.version if repository_ctx.attr.version != "" else _detect_java_version(repository_ctx, java_bin)
 
     # Prepare BUILD file using "local_java_runtime" macro
-    build_file = ""
-    if repository_ctx.attr.build_file != None:
+    if repository_ctx.attr.build_file_content and repository_ctx.attr.build_file:
+        fail("build_file and build_file_content are exclusive")
+    if repository_ctx.attr.build_file_content:
+        build_file = repository_ctx.attr.build_file_content
+    elif repository_ctx.attr.build_file:
         build_file = repository_ctx.read(repository_ctx.path(repository_ctx.attr.build_file))
+    else:
+        build_file = ""
+    build_file = build_file.format(RUNTIME_VERSION = version if version.isdigit() else "0")
 
-    runtime_name = '"jdk"' if repository_ctx.attr.build_file else None
+    runtime_name = '"jdk"' if build_file else None
     local_java_runtime_macro = """
 local_java_runtime(
     name = "%s",
@@ -201,10 +207,11 @@ _local_java_repository_rule = repository_rule(
         "java_home": attr.string(),
         "version": attr.string(),
         "build_file": attr.label(),
+        "build_file_content": attr.string(),
     },
 )
 
-def local_java_repository(name, java_home, version = "", build_file = None):
+def local_java_repository(name, java_home, version = "", build_file = None, build_file_content = None):
     """Registers a runtime toolchain for local JDK and creates an unregistered compile toolchain.
 
     Toolchain resolution is constrained with --java_runtime_version flag
@@ -220,7 +227,8 @@ def local_java_repository(name, java_home, version = "", build_file = None):
       name: A unique name for this rule.
       java_home: Location of the JDK imported.
       build_file: optionally BUILD file template
+      build_file_content: optional BUILD file template as a string
       version: optionally java version
     """
-    _local_java_repository_rule(name = name, java_home = java_home, version = version, build_file = build_file)
+    _local_java_repository_rule(name = name, java_home = java_home, version = version, build_file = build_file, build_file_content = build_file_content)
     native.register_toolchains("@" + name + "//:runtime_toolchain_definition")
diff --git a/tools/jdk/remote_java_repository.bzl b/tools/jdk/remote_java_repository.bzl
@@ -18,6 +18,7 @@ Rule remote_java_repository imports and registers JDK with the toolchain resolut
 """
 
 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
+load("@bazel_tools//tools/jdk:jdk_build_file.bzl", "JDK_BUILD_TEMPLATE")
 
 def _toolchain_config_impl(ctx):
     ctx.file("WORKSPACE", "workspace(name = \"{name}\")\n".format(name = ctx.name))
@@ -47,7 +48,7 @@ def remote_java_repository(name, version, target_compatible_with = None, prefix
     """
     http_archive(
         name = name,
-        build_file = "@bazel_tools//tools/jdk:jdk.BUILD",
+        build_file_content = JDK_BUILD_TEMPLATE.format(RUNTIME_VERSION = version),
         **kwargs
     )
     _toolchain_config(

Original file line number	Diff line number	Diff line change
`@@ -54,6 +54,8 @@ def _bazel_java_binary_impl(ctx):`
`54`	`54`	`)`
`55`	`55`
`56`	`56`	`if ctx.attr.use_testrunner:`
	`57`	`+ if semantics.find_java_runtime_toolchain(ctx).version >= 17:`
	`58`	`+ jvm_flags.append("-Djava.security.manager=allow")`
`57`	`59`	`test_class = ctx.attr.test_class if hasattr(ctx.attr, "test_class") else ""`
`58`	`60`	`if test_class == "":`
`59`	`61`	`test_class = helper.primary_class(ctx)`