platformclasspath is incorrectly based on --tool_java_runtime_version

[fmeum]

Description of the bug:

The contents of the platformclasspath.jar seem to depend on the Java runtime specified via --tool_java_runtime_version, not the one specified via --java_runtime_version (as long as --java_runtime_version specifies a version >= 9). This is incorrect as the latter controls the runtime environment for regular Java targets built in the Java configuration.

As a result, Java cross-compilation for the specified Java runtime can succeed even though the target will fail at runtime.

Which category does this issue belong to?

Team-Java

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Invalid platformclasspath.jar contents:

1. In any Bazel project, run USE_BAZEL_VERSION=500967715d64874b384e9a4c43d96ed6531c0f77 bazel build @rules_java//toolchains:platformclasspath --java_runtime_version=remotejdk_20.
2. Observe that the generated jar does not contain java/util/HexFormat.class, which was added in Java 17.
3. Run USE_BAZEL_VERSION=500967715d64874b384e9a4c43d96ed6531c0f77 bazel build @rules_java//toolchains:platformclasspath --java_runtime_version=remotejdk_20 --tool_java_runtime_version=remotejdk_20.
4. Observe that the generated jar does contain java/util/HexFormat.class.

Cross-compilation only failing at runtime:

# BUILD.bazel
java_binary(
    name = "Main",
    srcs = ["Main.java"],
    main_class = "com.example.Main",
)
# Main.java
package com.example;

import java.util.HexFormat;
import java.util.Optional;
import java.util.Random;

public class Main {
  public static void main(String[] args) {
    # Java 11
    System.out.println(Optional.ofNullable("Hello World!").orElseThrow());
    # Java 17
    System.out.println(HexFormat.of().formatHex(new byte[] { 0x01, 0x02, 0x03 }));
  }
}

$ USE_BAZEL_VERSION=500967715d64874b384e9a4c43d96ed6531c0f77 bazel run :Main --java_language_version=8 --java_runtime_version=remotejdk_11 --tool_java_runtime_version=remotejdk_20
INFO: Invocation ID: 60d37c92-9efc-4562-bfa1-857a95462b7f
INFO: Analyzed target //:Main (49 packages loaded, 820 targets configured).
INFO: Found 1 target...
Target //:Main up-to-date:
  bazel-bin/Main
  bazel-bin/Main.jar
INFO: Elapsed time: 12.500s, Critical Path: 2.82s
INFO: 3 processes: 2 internal, 1 worker.
INFO: Build completed successfully, 3 total actions
INFO: Running command line: bazel-bin/Main
Hello World!
Exception in thread "main" java.lang.NoClassDefFoundError: java/util/HexFormat
	at com.example.Main.main(Main.java:10)
Caused by: java.lang.ClassNotFoundException: java.util.HexFormat
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
	... 1 more

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

dev (last_green)

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

[fmeum]

CC @hvadehra

[fmeum]

Looking into https://github.com/openjdk/jdk/blob/8dfde28b289cbb53173f0ab759156088bbaf74f1/src/jdk.compiler/share/classes/com/sun/tools/javac/file/Locations.java#L886, it seems that StandardLocations.PLATFORM_CLASS_PATH doesn't care about the value of --system.

[cushon]

Nice find. I can reproduce running DumpPlatformClassPath standalone:

$ external/remotejdk20_linux/bin/java \
-XX:+IgnoreUnrecognizedVMOptions '--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.platform=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED' \
DumpPlatformClassPath /tmp/platformclasspath.jar external/remotejdk11_linux
$ jar tf /tmp/platformclasspath.jar | grep HexFormat
java/util/HexFormat.class

It is not clear to me that the way we're relying on --system here ever worked. I think that path affects compilations with a Java <= 8 language level and a Java >= 9 target JDK, since platformclasspath.jar isn't used for compilations using newer language levels, and it is generated differently without relying on --system for compilations targeting older JVM versions.

I think there is a more direct way to implement that logic using the JRT filesystem instead of going through the filemanager to get the classes: bazelbuild/rules_java#131

[fmeum]

since platformclasspath.jar isn't used for compilations using newer language levels

I observed that as well, but it looked like it was still being passed on the command line (in addition to --system) and causing action invalidation if it changed. I would need to confirm this again, but if true, would that also be something we could change?

[cushon]

I observed that as well, but it looked like it was still being passed on the command line (in addition to --system) and causing action invalidation if it changed. I would need to confirm this again, but if true, would that also be something we could change?

Yes, they're both currently passed as inputs corresponding to javac's -bootclasspath and --system flags. The compiler consults the bootclasspath if -source 8 -target 8 or earlier is configured, and --system for -source 9 -target 9. So it's slightly annoying to try to figure out which one to pass, because the -source/-target flags could be coming from the toolchain, or per-target javacopts, or command line --javacopts=, etc.

I'm not sure how to figure out which one to pass without doing javac option parsing in the rule logic. It'd be nice to avoid that complexity, but I guess it's something we could consider if avoiding the input would be a big improvement.

All of this will some day go away when we stop supporting JDK 8, but that will be a while.

WDYT, do you have any better ideas?

[cushon]

I think there is a more direct way to implement that logic using the JRT filesystem instead of going through the filemanager to get the classes: bazelbuild/rules_java#131

That got merged as bazelbuild/rules_java@325a9e1

[fmeum]

WDYT, do you have any better ideas?

Thanks for the explanation, I don't. But once this particular bug is fixed, I agree that it shouldn't really matter anymore to have the bootstrap classpath as an input.

All of this will some day go away when we stop supporting JDK 8, but that will #19036 (comment).

We need this functionality as well (transitively, because the tools we provide need to be compatible with Java 8), so I won't complain.

That got merged as bazelbuild/rules_java@325a9e1

Thanks for the quick fix! Happy to add some tests when this is picked up by Bazel.

Currently, it is possible to compile and run a java_test with --java_runtime_version=remotejdk_20 --java_language_version=8, which will result in it being compiled against whatever --tool_java_runtime_version indicates and tested against JDK 20.
With this fix merged, that would only continue to work if all class files comprising JDK 20 are still compatible with the compilation runtime, which is currently JDK 17. I wonder whether updating Bazel to a rules_java version containing your fix would also require bumping the compilation runtime to JDK 21.

However, I checked that at least HexFormat.class specifies the Java 9 classfile version, which I assume is because of bazelbuild/rules_java@325a9e1#diff-07001d4a213f7a49333fbe0e76684ab007668b8312a01e58118d55f51cbc0dfaR207. Is that modification safe to apply in general, even if the standard library starts to use new classfile features that are relevant for APIs, not just implementations?

[cushon]

However, I checked that at least HexFormat.class specifies the Java 9 classfile version, which I assume is because of bazelbuild/rules_java@325a9e1#diff-07001d4a213f7a49333fbe0e76684ab007668b8312a01e58118d55f51cbc0dfaR207. Is that modification safe to apply in general, even if the standard library starts to use new classfile features that are relevant for APIs, not just implementations?

That workaround is not guaranteed to be safe in general. I dug up some of the history, inside Google we have at least one tool that relies on ct.jar for newer language levels and doesn't support the equivalent of --system, and that also lags in support for the latest language levels. So that workaround allows it not crash when targeting e.g. Java 21.

For Bazel we could try removing the workaround and see if there are still any affected use-cases. The newer class file versions will only show up in ct.jar when targeting newer language levels, and if nothing in the default toolchain is affected it might be fine to drop that workaround.

But otherwise, you're right that it will likely cause problems eventually as the JDK APIs depend on newer language features, and if tools relying on ct.jar have class file parsers that complain if they see features in bytecode that aren't supposed to be there at the current major version.

[fmeum]

This was fixed by a recent rules_java update in Bazel.