bazelbuild / stardoc

Stardoc: Starlark Documentation Generator
Apache License 2.0
103 stars 40 forks source link

failed: error executing Turbine command with Bazel@HEAD #195

Closed sgowroji closed 5 months ago

sgowroji commented 7 months ago

https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/3475#018befa0-9798-4ad2-811f-1bce58807fc4

Platform :

Logs:

 ERROR: C:/b/5sycpaii/external/rules_jvm_external~5.2/private/tools/java/com/github/bazelbuild/rules_jvm_external/jar/BUILD:3:12 Building external/rules_jvm_external~5.2/private/tools/java/com/github/bazelbuild/rules_jvm_external/jar/AddJarManifestEntry.jar (1 source file) [for tool] failed: (Exit 1): turbine_direct_graal.exe failed: error executing Turbine command (from target @@rules_jvm_external~5.2//private/tools/java/com/github/bazelbuild/rules_jvm_external:rules_jvm_external)

Steps:

git clone https://github.com/bazelbuild/stardoc.git
git reset bf441c86274770ea281d21e22292ecdaa6451eaa --hard
export USE_BAZEL_VERSION=88cfacc4ff68d616fee981be55daaeaa760f5b3e
bazel build  incompatible_disable_starlark_host_transitions --enable_bzlmod  //...

CC Greenteam @mai93

fmeum commented 7 months ago

I think that this is due to an inherent incompatibility of manual --release with a Graal native image of turbine: it just doesn't ship with ct.sym information.

@cushon Is this supported at Google? I'm not sure how we could make it work. Try to fall back to non-Graal turbine based on javacopts or just ignore --release with it. Neither sounds great.

cushon commented 7 months ago

It isn't currently supported at Google. (We don't use --release much in part because it can't be combined with --add-exports=, and doesn't allow access to some APIs on Java 8 and earlier due to JDK-8206937).

It would be nice for Bazel to support it.

Either of the options you mentioned sound better than reporting an error.

I think that if ct.sym was available, all of the logic that reads it would work fine with the native image. Another option might be to unconditionally make ct.sym available as an input to turbine, and then set the path as a system property that it could checked here before trying to read ${java.home}/lib/ct.sym. One way to do that might be to add another field to BootClassPathInfo to save the location of ct.sym, and plumb it through to turbine.

fmeum commented 7 months ago

That's a very helpful suggestion, I will try to implement this.

I think that the file needs a new field on java_runtime though: Don't we want the ct.sym of the Java toolchain's java_runtime, not the one of the runtime providing the bootclasspath? I could mimic lib_modules.

cushon commented 7 months ago

I was imaging looking for a file with the right basename, similar to: https://github.com/bazelbuild/rules_java/blob/336963de4ef103bb307b0b4ad482e99e26d881aa/toolchains/default_java_toolchain.bzl#L251-L252

But that's a good point that ideally we want the ct.sym from the toolchains' runtime, not the target JDK, for consistency with the non-native-image case, and to support target JDKs that predate support for --release. Adding an attribute to java_runtime sounds good.

mai93 commented 7 months ago

A number of projects are failing with similar error with bazel@head(rules_webtesting, rules_jvm_external example), will it require a fix in the different repositories?

fmeum commented 7 months ago

No, this will be fixed centrally by updating the default rules_java used by Bazel.

It does require some back and forth between changes to rules_java and the Bazel repo though. I am working on the fix, but if you would prefer the pipeline to be green in the meantime, you can roll back https://github.com/bazelbuild/bazel/commit/8cab157f1d901f71a394ef5a300677f5d8d412e0.

cushon commented 7 months ago

For posterity, the error message is:

java.lang.NullPointerException: attempted to use --release, but JAVA_HOME is not set
--
  | at java.base@20.0.2/java.util.Objects.requireNonNull(Objects.java:259)
  | at com.google.turbine.binder.CtSymClassBinder.bind(CtSymClassBinder.java:52)
  | at com.google.turbine.main.Main.bootclasspath(Main.java:309)
  | at com.google.turbine.main.Main.compile(Main.java:142)
  | at com.google.turbine.main.Main.compile(Main.java:133)
  | at com.google.turbine.main.Main.main(Main.java:89)
cpsauer commented 5 months ago

(Just hit this one.)

cpsauer commented 5 months ago

Is this now fixed in Bazel HEAD, just waiting for another rolling release?

sgowroji commented 5 months ago

CI looks Green, Hence we can close this issue.

cpsauer commented 5 months ago

Sweet! Thanks for confirming and for all your work.