bazelbuild / rules_scala

Scala rules for Bazel
Apache License 2.0
360 stars 273 forks source link

rules_scala support for JDK 21 #1521

Closed gergelyfabian closed 4 months ago

gergelyfabian commented 10 months ago

When running rules_scala (trying to compile some Scala code) with JDK 21, I receive the following exception:

Exception in thread "main" java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release
    at java.base/java.lang.System.setSecurityManager(System.java:429)
    at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:47)
    at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:39)
    at io.bazel.rulesscala.scalac.ScalacWorker.main(ScalacWorker.java:36)

Reproduced on Linux with:

# WORKSPACE:
# Idea from: https://gist.github.com/ghostbear/b0aa3bf1a323b38be98bb1a66db65b41
# Link can be found at: https://www.azul.com/downloads/?package=jdk#zulu
remote_java_repository(
    name = "java_21_repository",
    prefix = "zulujdk",
    sha256 = "bf4842dd3a17cfe85523be5848b5ec3bc3d811afc74feab791befa4c895c4449",
    strip_prefix = "zulu21.30.15-ca-jdk21.0.1-linux_x64",
    target_compatible_with = ["@platforms//os:linux"],
    urls = ["https://cdn.azul.com/zulu/bin/zulu21.30.15-ca-jdk21.0.1-linux_x64.tar.gz"],
    version = "21",
)

register_toolchains("//tools/jdk:java21_toolchain_definition")

# //tools/jdk:BUILD

default_java_toolchain(
    name = "java21_toolchain",
    configuration = DEFAULT_TOOLCHAIN_CONFIGURATION,
    java_runtime = "@java_21_repository//:jdk",
    javacopts = DEFAULT_JAVACOPTS,
    # jvm_opts needs to stay this one also for Java 11, it seems to be common for Java 9+ versions.
    # It includes e.g. Java module system options.
    jvm_opts = BASE_JDK9_JVM_OPTS,
    source_version = "21",
    target_version = "21",
    visibility = ["//visibility:public"],
)
gergelyfabian commented 10 months ago

Seems this exception is starting with JDK 18 (before it was only a warning):

https://bz.apache.org/bugzilla/show_bug.cgi?id=65381

gergelyfabian commented 10 months ago

Trying to localize this, it seems to occur when trying to compile any Scala code (tried scala_macro_library).

gergelyfabian commented 10 months ago

Reproduction: https://github.com/gergelyfabian/bazel-scala-example/tree/java_21

bazel build //...

The following changes are necessary in .bazelrc, to ensure this is reproducible:

build --java_language_version=21
build --java_runtime_version=21
build --tool_java_language_version=21
build --tool_java_runtime_version=21
gergelyfabian commented 10 months ago

Even when running on Java 17, when first using rules_scala (and it gets built), there is a warning:

INFO: From Building external/io_bazel_rules_scala/src/java/io/bazel/rulesscala/worker/libworker.jar (1 source file) [for tool]:
external/io_bazel_rules_scala/src/java/io/bazel/rulesscala/worker/Worker.java:48: warning: [removal] SecurityManager in java.lang has been deprecated and marked for removal
        new SecurityManager() {
            ^
external/io_bazel_rules_scala/src/java/io/bazel/rulesscala/worker/Worker.java:47: warning: [removal] setSecurityManager(SecurityManager) in System has been deprecated and marked for removal
    System.setSecurityManager(
          ^
gergelyfabian commented 10 months ago

Related to #1425.

gergelyfabian commented 10 months ago

The workaround from #1425 works. just need to add -Djava.security.manager=allow to the scala_toolchain:

scala_toolchain(
    name = "my_toolchain_impl",
    # ...
    scalac_jvm_flags = [
        "-Djava.security.manager=allow",
    ],
    # ...
)
gergelyfabian commented 10 months ago

The workaround fixes bazel build //... in my example project, but bazel coverage //... still fails with the same error:

    at java.base/java.lang.System.setSecurityManager(System.java:429)
    at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:47)
    at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:39)
    at io.bazel.rulesscala.coverage.instrumenter.JacocoInstrumenter.main(JacocoInstrumenter.java:27)
gergelyfabian commented 10 months ago

If scalafmt is enabled, it will also fail:

Exception in thread "main" java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release
    at java.base/java.lang.System.setSecurityManager(System.java:429)
    at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:47)
    at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:39)
    at io.bazel.rules_scala.scalafmt.ScalafmtWorker$.main(ScalafmtWorker.scala:14)
    at io.bazel.rules_scala.scalafmt.ScalafmtWorker.main(ScalafmtWorker.scala)
gergelyfabian commented 10 months ago

Note: JDK 21 does not work with Scala 2.12.17, after upgrading to Scala 2.12.18 it works (with the above limitations):

https://github.com/scala/scala/releases/tag/v2.12.18

gergelyfabian commented 10 months ago

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",
            )
gergelyfabian commented 10 months ago

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],
        )
gergelyfabian commented 10 months ago

Uploaded my temporary fixes here: https://github.com/bazelbuild/rules_scala/compare/master...gergelyfabian:rules_scala:jdk_21?expand=1

gergelyfabian commented 10 months ago

One more thing that will need to get fixed I guess, when running coverage:

java.io.IOException: Error while analyzing com/.../.../DummyClazz.class.uninstrumented.
    at org.jacoco.core.analysis.Analyzer.analyzerError(Analyzer.java:163)
    at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:134)
    at com.google.testing.coverage.JacocoCoverageRunner.analyzeStructure(JacocoCoverageRunner.java:187)
    at com.google.testing.coverage.JacocoCoverageRunner.create(JacocoCoverageRunner.java:129)
    at com.google.testing.coverage.JacocoCoverageRunner$2.run(JacocoCoverageRunner.java:568)
Caused by: java.lang.IllegalArgumentException: Unsupported class file major version 65
    at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.<init>(ClassReader.java:199)
    at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.<init>(ClassReader.java:180)
    at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.<init>(ClassReader.java:166)
    at org.jacoco.core.internal.instr.InstrSupport.classReaderFor(InstrSupport.java:280)
    at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:107)
    at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:132)
gergelyfabian commented 10 months ago

Found the following related issue: https://github.com/bazelbuild/rules_java/issues/141

It seems may need to retry this after upgrading to Bazel 7.

srdo commented 10 months ago

The reason I didn't fix this when I reported https://github.com/bazelbuild/rules_scala/issues/1425 is that it's a little more complicated than just adding -Djava.security.manager=allow to the places we invoke Java :)

Adding this flag will break compatibility with Java 11 and below. This is because on versions older than 12, allow is not special cased in this code, and so it is interpreted as being the class name of a SecurityManager the JVM should load.

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 >= 12.

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 JavaRuntimeInfo Starlark provider, so it is now reasonably doable for us to only add this flag if the toolchain says the Java version is new enough.

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.

gergelyfabian commented 10 months ago

As looking at https://github.com/bazelbuild/rules_java/issues/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.

gergelyfabian commented 10 months ago

Here is the PR with fixes for Bazel 7.0.0rc2: https://github.com/bazelbuild/rules_scala/pull/1524

gergelyfabian commented 10 months ago

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:

https://github.com/bazelbuild/rules_java/issues/141#issuecomment-1772306539

gergelyfabian commented 10 months ago

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.

srdo commented 10 months ago

I believe the JDK team are aware that trapping System.exit is a common reason people use the SM, and so I would expect there to be a replacement in the JDK before they remove the SM completely.

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.

srdo commented 10 months ago

On a different note, after my initial investigations I'm not sure enabling SecurityManager will be all we need to change.

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.

cristifalcas commented 8 months ago

I'm also getting this error when compiling proto files:

BUILD.bazel:7:14: ProtoScalaPBRule apps/aqueduct/protobuf/api/api_proto_fs2_scalapb.srcjar failed: Worker process did not return a WorkResponse:

---8<---8<--- Start of log, file at /private/var/tmp/_bazel_cfalcas/a2947156e74a17b98ed94d535238794c/bazel-workers/worker-59-ProtoScalaPBRule.log ---8<---8<---
Exception in thread "main" java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release
    at java.base/java.lang.System.setSecurityManager(System.java:429)
    at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:47)
    at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:39)
    at scripts.ScalaPBWorker$.main(ScalaPBWorker.scala:39)
    at scripts.ScalaPBWorker.main(ScalaPBWorker.scala)
---8<---8<--- End of log ---8<---8<---

And I can't seem to find a solution :(

cristifalcas commented 8 months ago

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

bmt-tock commented 8 months ago

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?

srdo-humio commented 8 months ago

@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 "-Djava.security.manager=allow" to the scalac_jvm_flags. That works for us.

These flags aren't passed to all actions though, so e.g. coverage doesn't work without the changes to rules_scala mentioned above.

gergelyfabian commented 5 months ago

Is there a reason for reopening?

gergelyfabian commented 5 months ago

When running with latest rules_scala and JDK 21, I still get the following when executing coverage:

Exception in thread "main" java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release
    at java.base/java.lang.System.setSecurityManager(System.java:429)
    at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:47)
    at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:39)
    at io.bazel.rulesscala.coverage.instrumenter.JacocoInstrumenter.main(JacocoInstrumenter.java:27)
gergelyfabian commented 5 months ago

Building and testing code also does not work.

simuons commented 5 months ago

@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.

simuons commented 5 months ago

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 Unsupported class file major version 65 but I think this is a different issue.

gergelyfabian commented 5 months ago

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 Unsupported class file major version 65 but I think this is a different issue.

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 coverage.

gergelyfabian commented 5 months ago

I have seen an Caused by: java.lang.IllegalArgumentException: Unsupported class file major version 65 too, when running all tests. I don't have a direct reproduction yet.

gergelyfabian commented 5 months ago

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)

//example-lib:test                                                       PASSED in 0.8s
  /home/user/.cache/bazel/_bazel_user/64527517dacf7170f7d914889f83481c/execroot/scala_example/bazel-out/k8-fastbuild/testlogs/example-lib/test/coverage.dat
//example-maven:test                                                     PASSED in 0.8s
  /home/user/.cache/bazel/_bazel_user/64527517dacf7170f7d914889f83481c/execroot/scala_example/bazel-out/k8-fastbuild/testlogs/example-maven/test/coverage.dat
//example-lib:java_test                                                  FAILED in 1.8s
  /home/user/.cache/bazel/_bazel_user/64527517dacf7170f7d914889f83481c/execroot/scala_example/bazel-out/k8-fastbuild/testlogs/example-lib/java_test/test.log
gergelyfabian commented 5 months ago

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.

gergelyfabian commented 5 months ago

I think this exception is not thrown by the JVM, but by Jacoco->ASM.

gergelyfabian commented 5 months ago

Probably this is related: https://github.com/bazelbuild/bazel/issues/20845

gergelyfabian commented 5 months ago

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. I have created an update for the custom jacocorunner building script. Used this to fix the issue on my side.

gergelyfabian commented 5 months ago

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).

gergelyfabian commented 5 months ago

Tested in my repro,

Bazel 7.0.2 + rules_java 7.3.2, 7.4.0, 7.5.0: does not work Bazel 7.1.0 + rules_java 7.3.2, 7.4.0, 7.5.0: works

it's not necessary to build a custom JacocoRunner, you just have to update Bazel to 7.1.0+. However, when using the custom JacocoRunner, you need to take the updated version to fix this issue.

gergelyfabian commented 4 months ago

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 "@remote_java_tools//:jacoco_coverage_runner_filegroup", then bazel query --output=build //external:remote_java_tools says:

http_archive(
  name = "remote_java_tools",
  generator_name = "",
  generator_function = "rules_jvm_external_setup",
  urls = ["https://mirror.bazel.build/bazel_java_tools/releases/java/v13.5/java_tools-v13.5.zip", "https://github.com/bazelbuild/java_tools/releases/download/java_v13.5/java_tools-v13.5.zip"],
  sha256 = "c1e1045ed067777fe2be3b0369ee186bef42885f4ec59e500c2c92d03e71e28f",
)

However this doesn't seem to have changed between Bazel 7.0.0 and 7.1.1, so I don't understand this.

simuons commented 4 months ago

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?

gergelyfabian commented 4 months ago

I think the class file version could be considered a new issue, and this one can be closed. Please add me on the new issue, maybe I can help with something :)

How could you reproduce it, first and foremost?