rules_scala support for JDK 21 #1521
Comments
|
Seems this exception is starting with JDK 18 (before it was only a warning): |
|
Trying to localize this, it seems to occur when trying to compile any Scala code (tried |
|
Reproduction: https://github.com/gergelyfabian/bazel-scala-example/tree/java_21 The following changes are necessary in .bazelrc, to ensure this is reproducible: |
gergelyfabian
changed the title
|
Even when running on Java 17, when first using rules_scala (and it gets built), there is a warning: |
|
Related to #1425. |
|
The workaround from #1425 works. just need to add |
|
The workaround fixes |
|
If scalafmt is enabled, it will also fail: |
|
Note: JDK 21 does not work with Scala 2.12.17, after upgrading to Scala 2.12.18 it works (with the above limitations): |
|
I could fix scalafmt by changing phase_scalafmt (is there anything exposed so that I wouldn't have to do that?): ctx.actions.run(
arguments = ["--jvm_flag=-Dfile.encoding=UTF-8", "--jvm_flag=-Djava.security.manager=allow", _format_args(ctx, src, file)],
executable = ctx.executable._fmt,
outputs = [file],
inputs = [ctx.file.config, src],
execution_requirements = {"supports-workers": "1"},
mnemonic = "ScalaFmt",
) |
|
For coverage, changed phase_coverage to: ctx.actions.run(
mnemonic = "JacocoInstrumenter",
inputs = [input_jar],
outputs = [output_jar],
executable = ctx.attr._code_coverage_instrumentation_worker.files_to_run,
execution_requirements = {"supports-workers": "1"},
arguments = ["--jvm_flag=-Djava.security.manager=allow", args],
) |
|
Uploaded my temporary fixes here: https://github.com/bazelbuild/rules_scala/compare/master...gergelyfabian:rules_scala:jdk_21?expand=1 |
|
One more thing that will need to get fixed I guess, when running coverage: |
|
Found the following related issue: bazelbuild/rules_java#141 It seems may need to retry this after upgrading to Bazel 7. |
|
The reason I didn't fix this when I reported #1425 is that it's a little more complicated than just adding Adding this flag will break compatibility with Java 11 and below. This is because on versions older than 12, This will cause a crash on any pre-12 JVM, if you set this flag. I believe the way to fix this is to conditionally pass the flag in the JVM arguments. We need to only pass it on if the JDK version is At the time I raised the issue, it was unclear to me how to accomplish this easily. Since then, this commit in Bazel has added the Java version to the There's an example of how to do this in the linked commit. I think we need to do the same in rules_scala. Regarding compatibility with older Bazel versions, I think the easy solution is to break compatibility with Bazel versions older than ones that have this commit (I believe it was backported to the 6.x line). Other people can probably weigh in on whether this is okay. |
|
As looking at bazelbuild/rules_java#141, I think I'll try to make some changes for rules_scala to make it compatible (at least for my project) with Bazel 7.0 rc2, and then recheck JDK 21 compatibility on Bazel 7.0 rc2 using the instructions from there. |
|
Here is the PR with fixes for Bazel 7.0.0rc2: #1524 |
|
Upgrading to Bazel 7.0 does not fix the basic issues with JDK 21, but obviously it's better that we can use an official remotejdk_21. Instructions I used to set up JDK after upgrading to Bazel 7.0rc2: |
|
Regarding SecurityManager... Could we rewrite code that uses SecurityManager in rules_scala to something else (I'm not an expert here) or our only option is reenabling it? On a different note, after my initial investigations I'm not sure enabling SecurityManager will be all we need to change. |
|
I believe the JDK team are aware that trapping For now, I think our only option is to reenable the SM, or to make do without exit traps. I think enabling the SM is probably the better option. |
You are right, but for the basic functionality of compiling and testing Scala code on Java 21, enabling the SM seems to be sufficient. We are building with JDK 21 internally on Bazel 6 (we manually define a Java toolchain rather than using the remotejdk ones, in order to be able to upgrade Java independently of Bazel) and stock rules_scala, and the only thing we needed to fix was setting the SM flag. I believe someone on our team looked into what was needed to get Jacoco working as well, I'll ask if they can weigh in. We don't use scalafmt or the junit rule, so can't speak to those. |
|
I'm also getting this error when compiling proto files: And I can't seem to find a solution :( |
|
git diff on what works for me: diff --git a/scala/private/phases/phase_coverage.bzl b/scala/private/phases/phase_coverage.bzl
index fa6a3ca..ad833d2 100644
--- a/scala/private/phases/phase_coverage.bzl
+++ b/scala/private/phases/phase_coverage.bzl
@@ -60,7 +60,7 @@ def _phase_coverage(ctx, p, srcjars):
outputs = [output_jar],
executable = ctx.attr._code_coverage_instrumentation_worker.files_to_run,
execution_requirements = {"supports-workers": "1"},
- arguments = [args],
+ arguments = ["--jvm_flag=-Djava.security.manager=allow", args],
)
replacements = {input_jar: output_jar}
diff --git a/scala/private/phases/phase_write_executable.bzl b/scala/private/phases/phase_write_executable.bzl
index 6f30595..5194158 100644
--- a/scala/private/phases/phase_write_executable.bzl
+++ b/scala/private/phases/phase_write_executable.bzl
@@ -104,7 +104,7 @@ def _write_executable_non_windows(ctx, executable, rjars, main_class, jvm_flags,
template = ctx.attr._java_stub_template.files.to_list()[0]
jvm_flags = " ".join(
- [ctx.expand_location(f, ctx.attr.data) for f in jvm_flags],
+ [ctx.expand_location(f, ctx.attr.data) for f in jvm_flags + ["-Djava.security.manager=allow"]],
)
javabin = "export REAL_EXTERNAL_JAVA_BIN=${JAVABIN};JAVABIN=${JAVABIN:-%s/%s}" % (coverage doesn't work with bazel 6.4 |
|
I'm having trouble following what needs to be done to get this working. I think we have to use our own scala_toolchain rather than scala_register_toolchains() and enable SM in scalac_jvm_flags, but I haven't been able to figure out the right combination to get it working. Is there any chance a fix will be added to the default toolchain or is there a good example of registering our own? |
|
@bmt-tock It depends on which features of rules_scala you're using. If you just need Scala compilation, you should be able to do something like this, and add These flags aren't passed to all actions though, so e.g. coverage doesn't work without the changes to rules_scala mentioned above. |
|
Is there a reason for reopening? |
|
When running with latest rules_scala and JDK 21, I still get the following when executing coverage: |
|
Building and testing code also does not work. |
|
@gergelyfabian yeah I noticed it doesn't work and that's the reason for reopening. Thought CI passed (probably everything was cached). Trying to solve it. |
|
Hi @gergelyfabian could you please check latest master of rules_scala on your repro? I did a quick check, basic use cases passed. However coverage failed with |
I think it's a different issue. I did not update my reproduction, but checking the monorepo I'm working on, the latest rules_scala works properly with |
|
I have seen an |
|
Updated my repro in https://github.com/gergelyfabian/bazel-scala-example/tree/java_21, and it reproduces the "unsupported class file major version 65" exception. However, it's a bit strange, not all coverage targets fail with this... (also saw the same in my monorepo) |
|
I'm wondering whether the "unsupported class file major version 65" exception is not related to how we execute the jacoco analyzer. I checked by upgrading my local JDK version to 21 and it did not solve this issue. So my guess is, that we are using a lower JDK version somewhere still, when starting the coverage analyzer tool. |
|
I think this exception is not thrown by the JVM, but by Jacoco->ASM. |
|
Probably this is related: bazelbuild/bazel#20845 |
I managed to solve this issue by building a custom JacocoRunner, with the updated Jacoco (0.8.11) and ASM from the linked bazel commit. |
|
It seems to me that also upgrading to Bazel 7.1.1 solves this issue (even when not using the custom JacocoRunner). The reason is most probably that the bazel fix is already included there. I also upgraded rules_java to the latest version (7.5.0). |
|
Tested in my repro, Bazel 7.0.2 + rules_java 7.3.2, 7.4.0, 7.5.0: does not work it's not necessary to build a custom JacocoRunner, you just have to update Bazel to 7.1.0+. |
|
For the record, when building the custom JacocoRunner from a bazel branch, after diverging from Bazel 7.1.1, it doesn't work, so it seems the Jacoco fixes have not yet been included in the 7.1.1 version (at least what concerns building the custom JacocoRunner). With Bazel 7.1.1 as I see the default jacocorunner is However this doesn't seem to have changed between Bazel 7.0.0 and 7.1.1, so I don't understand this. |
|
Hi, @gergelyfabian, thanks for looking into this and for your thorough analysis. Do you think this is same issue as reported originally? Or this should be a separate jacoco issue? I'm leaning towards closing this and opening new issue specifically for jacoco. wdyt? |
|
I think the class file version could be considered a new issue, and this one can be closed. How could you reproduce it, first and foremost? |
When running rules_scala (trying to compile some Scala code) with JDK 21, I receive the following exception:
Reproduced on Linux with:
The text was updated successfully, but these errors were encountered: