Skip to content

Commit

Permalink
Add version to JavaRuntimeInfo.
Browse files Browse the repository at this point in the history
1. As suggested in bazelbuild#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 bazelbuild#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.)
  • Loading branch information
benjaminp committed Mar 21, 2023
1 parent f8a59d9 commit bfdf72a
Show file tree
Hide file tree
Showing 18 changed files with 99 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -129,7 +130,8 @@ public ConfiguredTarget create(RuleContext ruleContext)
hermeticInputs,
libModules,
defaultCDS,
hermeticStaticLibs);
hermeticStaticLibs,
ruleContext.attributes().get("version", INTEGER).toIntUnchecked());

TemplateVariableInfo templateVariableInfo =
new TemplateVariableInfo(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -62,7 +63,8 @@ public static JavaRuntimeInfo create(
hermeticInputs,
libModules,
defaultCDS,
hermeticStaticLibs);
hermeticStaticLibs,
version);
}

@Override
Expand Down Expand Up @@ -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,
Expand All @@ -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;
Expand All @@ -145,6 +149,7 @@ private JavaRuntimeInfo(
this.libModules = libModules;
this.defaultCDS = defaultCDS;
this.hermeticStaticLibs = hermeticStaticLibs;
this.version = version;
}

/** All input artifacts in the javabase. */
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -83,6 +84,10 @@ 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 major version of the Java runtime.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.add(attr("version", INTEGER))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,11 @@ public interface JavaRuntimeInfoApi extends StructApi {
doc = "Returns the JDK static libraries.",
structField = true)
Sequence<CcInfo> starlarkHermeticStaticLibs();

/** The Java version of the runtime. This may be 0 if the version is unknown. */
@StarlarkMethod(
name = "version",
doc = "The Java version of the runtime. This may be 0 if the version is unknown.",
structField = true)
int version();
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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):",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ 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/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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,13 @@

package com.google.devtools.build.lib.bazel.rules.java;

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 org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -46,4 +49,35 @@ 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 jvmFlags =
((TemplateExpansionAction) getGeneratingAction(getBinArtifact("test", ct)))
.getSubstitutions().stream()
.filter(s -> "%jvm_flags%".equals(s.getKey()))
.collect(onlyElement())
.getValue();
assertThat(jvmFlags).contains("-Djava.security.manager=allow");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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):",
Expand Down
2 changes: 1 addition & 1 deletion src/test/py/bazel/query_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def testSimpleQuery(self):
def testQueryFilesUsedByRepositoryRules(self):
self.ScratchFile('WORKSPACE')
self._AssertQueryOutputContains("kind('source file', deps(//external:*))",
'@bazel_tools//tools/jdk:jdk.BUILD')
'@bazel_tools//tools/genrule:genrule-setup.sh')

def testBuildFilesForExternalRepos_Simple(self):
self.ScratchFile('WORKSPACE', [
Expand Down
2 changes: 1 addition & 1 deletion tools/jdk/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 0 additions & 1 deletion tools/jdk/BUILD.tools
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ alias(

exports_files([
"BUILD.java_tools",
"jdk.BUILD",
])

alias(
Expand Down
4 changes: 3 additions & 1 deletion tools/jdk/jdk.BUILD → tools/jdk/jdk_build_file.bzl
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
load("@rules_java//java:defs.bzl", "java_runtime")
JDK_BUILD_TEMPLATE = """load("@rules_java//java:defs.bzl", "java_runtime")
package(default_visibility = ["//visibility:public"])
Expand Down Expand Up @@ -67,4 +67,6 @@ java_runtime(
":jdk-lib",
":jre",
],
version = {RUNTIME_VERSION},
)
"""
18 changes: 13 additions & 5 deletions tools/jdk/local_java_repository.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand All @@ -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")
3 changes: 2 additions & 1 deletion tools/jdk/remote_java_repository.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit bfdf72a

Please sign in to comment.