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.

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

Verify bootclasspath version is at most compilation runtime version #18343

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:config/compilation_mode",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import net.starlark.java.eval.NoneType;
import net.starlark.java.eval.Sequence;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkInt;
import net.starlark.java.eval.StarlarkThread;
import net.starlark.java.eval.StarlarkValue;
import net.starlark.java.syntax.Location;
Expand Down Expand Up @@ -74,22 +75,35 @@ private Provider() {
"The inputs to javac's --system flag, either a directory or a listing of files,"
+ " which must contain at least 'release', 'lib/modules', and"
+ " 'lib/jrt-fs.jar'"),
@Param(
name = "version",
positional = false,
named = true,
defaultValue = "0",
doc = "The major version of the runtime providing this bootclasspath."
),
},
selfCall = true,
useStarlarkThread = true)
public BootClassPathInfo bootClassPathInfo(
Sequence<?> bootClassPathList,
Sequence<?> auxiliaryList,
Object systemOrNone,
StarlarkInt version,
StarlarkThread thread)
throws EvalException {
NestedSet<Artifact> systemInputs = getSystemInputs(systemOrNone);
Optional<PathFragment> systemPath = getSystemPath(systemInputs);
int versionChecked = version.toInt("version");
if (versionChecked < 0) {
throw Starlark.errorf("version must be non-negative, got %d", versionChecked);
}
return new BootClassPathInfo(
getBootClassPath(bootClassPathList),
getAuxiliary(auxiliaryList),
systemInputs,
systemPath,
versionChecked,
thread.getCallerLocation());
}

Expand Down Expand Up @@ -150,18 +164,21 @@ private static Optional<PathFragment> getSystemPath(NestedSet<Artifact> systemIn
private final NestedSet<Artifact> auxiliary;
private final NestedSet<Artifact> systemInputs;
private final Optional<PathFragment> systemPath;
private final int version;

private BootClassPathInfo(
NestedSet<Artifact> bootclasspath,
NestedSet<Artifact> auxiliary,
NestedSet<Artifact> systemInputs,
Optional<PathFragment> systemPath,
int version,
Location creationLocation) {
super(creationLocation);
this.bootclasspath = bootclasspath;
this.auxiliary = auxiliary;
this.systemInputs = systemInputs;
this.systemPath = systemPath;
this.version = version;
}

@Override
Expand All @@ -175,6 +192,7 @@ public static BootClassPathInfo create(NestedSet<Artifact> bootclasspath) {
NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER),
NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER),
Optional.empty(),
0,
null);
}

Expand All @@ -184,6 +202,7 @@ public static BootClassPathInfo empty() {
NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER),
NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER),
Optional.empty(),
0,
null);
}

Expand All @@ -210,6 +229,11 @@ public NestedSet<Artifact> systemInputs() {
return systemInputs;
}

/** The major version of the runtime providing this bootclasspath. */
public int version() {
return version;
}

public boolean isEmpty() {
return bootclasspath.isEmpty();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.Runfiles;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget;
import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
Expand Down Expand Up @@ -160,6 +161,17 @@ public ConfiguredTarget create(RuleContext ruleContext)

JavaRuntimeInfo javaRuntime = JavaRuntimeInfo.from(ruleContext, "java_runtime");

if (javaRuntime.version() != 0 && javaRuntime.version() < bootclasspath.version()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wondered about using OptionalInt in the Java code here, but I don't have a strong opinion. If starlark is using 0, it might be clearer to be consistent

ruleContext.attributeError("java_runtime", String.format(
"The version of the Java compilation toolchain's java_runtime (%d) must be at least as"
+ " high as the version of the Java runtime that provides the bootclasspath (%d)."
+ " Either switch to a Java toolchain with a higher version of the Java runtime or"
+ " lower the version of --%sjava_runtime_version",
javaRuntime.version(), bootclasspath.version(),
ruleContext.getConfiguration().getOptions().get(CoreOptions.class).isExec
? "tool_" : ""));
}

JavaToolchainProvider provider =
JavaToolchainProvider.create(
ruleContext.getLabel(),
Expand Down
61 changes: 61 additions & 0 deletions src/test/shell/bazel/bazel_with_jdk_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -289,4 +289,65 @@ function test_bazel_compiles_with_localjdk() {
expect_not_log "exec external/remotejdk11_linux/bin/java"
}

function test_java_runtime_greater_than_java_toolchain_runtime_version() {
mkdir -p pkg
cat >pkg/BUILD <<'EOF'
java_binary(
name = "Main",
srcs = ["Main.java"],
main_class = "com.example.Main",
)
EOF

cat >pkg/Main.java <<'EOF'
package com.example;
public class Main {
public static void main(String[] args) {
System.out.println("Hello World!");
}
}
EOF

bazel build //pkg:Main \
--java_language_version=8 \
--java_runtime_version=remotejdk_20 \
&>"${TEST_log}" && fail "Expected build to fail"

expect_log "The version of the Java compilation toolchain's java_runtime (17) must be at least as high as the version of the Java runtime that provides the bootclasspath (20). Either switch to a Java toolchain with a higher version of the Java runtime or lower the version of --java_runtime_version"
}

function test_tool_java_runtime_greater_than_java_toolchain_runtime_version() {
mkdir -p pkg
cat >pkg/BUILD <<'EOF'
java_binary(
name = "Main",
srcs = ["Main.java"],
main_class = "com.example.Main",
)
genrule(
name = "gen",
outs = ["gen.txt"],
tools = [":Main"],
cmd = "$(location :Main) > $@",
)
EOF

cat >pkg/Main.java <<'EOF'
package com.example;
public class Main {
public static void main(String[] args) {
System.out.println("Hello World!");
}
}
EOF

bazel build //pkg:gen \
--tool_java_language_version=8 \
--tool_java_runtime_version=remotejdk_20 \
&>"${TEST_log}" && fail "Expected build to fail"

expect_log "The version of the Java compilation toolchain's java_runtime (17) must be at least as high as the version of the Java runtime that provides the bootclasspath (20). Either switch to a Java toolchain with a higher version of the Java runtime or lower the version of --tool_java_runtime_version"
}

run_suite "Tests detection of local JDK and that Bazel executes with a bundled JDK."
6 changes: 5 additions & 1 deletion tools/jdk/default_java_toolchain.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -241,9 +241,12 @@ def _bootclasspath_impl(ctx):
system = [f for f in ctx.files.target_javabase if f.basename in system_files]
if len(system) != len(system_files):
system = None
version = 0
if ctx.attr.target_javabase:
runtime_info = ctx.attr.target_javabase[java_common.JavaRuntimeInfo]
inputs.extend(ctx.files.target_javabase)
args.add(ctx.attr.target_javabase[java_common.JavaRuntimeInfo].java_home)
args.add(runtime_info.java_home)
version = runtime_info.version

ctx.actions.run(
executable = str(host_javabase.java_executable_exec_path),
Expand All @@ -257,6 +260,7 @@ def _bootclasspath_impl(ctx):
java_common.BootClassPathInfo(
bootclasspath = [bootclasspath],
system = system,
version = version,
),
OutputGroupInfo(jar = [bootclasspath]),
]
Expand Down
Loading