Add Java 24 compatibility #1719

srdo-humio · 2025-03-18T08:58:00Z

The SecurityManager is a hollow shell that just throws exceptions in Java 24. We should avoid calling it. This will make tests that call System.exit crash the worker.

Bazel is doing the same thing for now, see
bazelbuild/bazel#24354

It is often possible to edit code to avoid calling System.exit, and guarding against it requires more effort in Java 24+, likely involving attaching an agent. Until someone actually has that need, it doesn't seem worth doing.

srdo-humio · 2025-03-18T09:01:38Z

In a future Java release, the SM classes will go away entirely. At that point, we'll have to either access the SM via reflection, or split the worker into an old-JDK version and a new-JDK version if we want to maintain support for blocking System.exit on old JDKs.

I think it might be fine to just drop support for the SM entirely across all versions instead of trying to maintain partial support.

Do you have opinions on this?

simuons

Thanks, @srdo-humio, if I remember correctly JEP promised a replacement for SecurityManager. If it's not the case anymore then I think we will get rid of SM entirely, because all other options you listed doesn't seem to be worth the effort/maintainance.

srdo-humio · 2025-03-18T11:28:33Z

@simuons The tracking issue for the JDK at https://bugs.openjdk.org/browse/JDK-8199704 seems to have no movement, and the JEP that degrades the SM is not suggesting a replacement API, but instead contains an appendix describing a bytecode rewriting agent that people can use as a starting point if they really need this functionality https://openjdk.org/jeps/486.

I don't think there's a built-in replacement for this in Java 24, and I don't get the impression a replacement is planned.

Assuming we agree that we should drop the System.exit-blocking code entirely, would you prefer that I updated this PR to remove the SM code, or should we delay doing that until the future JDK that will remove the SM interfaces entirely releases?

simuons · 2025-03-25T07:32:04Z

@srdo-humio please update PR and remove SM.

Since replacement is not planned and everyone will eventually move to jdk 24 we will be left with obsolate code.

srdo-humio · 2025-03-25T12:26:59Z

scala/private/phases/phase_write_executable.bzl

@@ -1,20 +1,19 @@
+load("//scala/private:common.bzl", "rlocationpath_from_file")


Buildifier moves this line up here. Should I leave these changes in?

Yes, if it's made by buildifier then keep it. We do not object to buildifier :)

Java 24 degrades the SecurityManager into a hollow shell that just throws exceptions. Later versions will remove the interface entirely. This commit removes references to the SecurityManager from rules_scala, which will make tests that call System.exit crash the worker. It is often possible to edit code to avoid calling System.exit, and guarding against it requires more effort in Java 24+, likely involving attaching an agent. Until someone actually has that need, it doesn't seem worth doing. Bazel is doing the same thing for now, see bazelbuild/bazel#24354 Add -Dcom.google.testing.junit.runner.shouldInstallTestSecurityManager=false for junit tests. While recent Bazel versions set this automatically, older Bazel versions do not, and since we're removing the flag that allows the SM, we need to set this to prevent crashing for people running older Bazel versions with Java 17+ Also exclude the .ijwb directory from the buildifier lint check. Failures due to those files are irritating when running the test locally.

srdo-humio · 2025-03-25T13:07:33Z

@simuons Sounds good. I've updated the PR to remove the SM.

simuons

Thanks, @srdo-humio!

mbland · 2025-03-26T12:25:50Z

@srdo-humio @simuons I noticed that src/java/io/bazel/rulesscala/worker/WorkerTest.java is now broken:

$ bazel build //src/...

INFO: Analyzed 33 targets (0 packages loaded, 0 targets configured).
ERROR: /Users/mbland/src/bazelbuild/rules_scala/src/java/io/bazel/rulesscala/worker/BUILD:12:10: Building src/java/io/bazel/rulesscala/worker/worker_test.jar (1 source file) failed: (Exit 1): java failed: error executing Javac command (from target //src/java/io/bazel/rulesscala/worker:worker_test) external/remotejdk21_macos_aarch64/bin/java '--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED' ... (remaining 19 arguments skipped)
src/java/io/bazel/rulesscala/worker/WorkerTest.java:40: error: cannot find symbol
        assertThrows(Worker.ExitTrapped.class, () -> Worker.workerMain(new String[] {}, worker))
                           ^
  symbol:   class ExitTrapped
  location: class Worker
src/java/io/bazel/rulesscala/worker/WorkerTest.java:40: error: unreported exception Exception; must be caught or declared to be thrown
        assertThrows(Worker.ExitTrapped.class, () -> Worker.workerMain(new String[] {}, worker))
                                                                      ^
INFO: Elapsed time: 0.211s, Critical Path: 0.07s
INFO: 2 processes: 2 internal.
ERROR: Build did NOT complete successfully

This test isn't exercised by our CI, or by ./test_all.sh, but I often build this package as a smoke test before ./test_all.sh. Should this test be updated or removed?

…removed WorkerTest was failing after bazelbuild#1719

simuons · 2025-03-26T13:15:21Z

Thanks and sorry @mbland. I'm addressing this in #1721
Will address tests not being run by ci afterwards

…removed (#1721) WorkerTest was failing after #1719

`test_lint.sh` now contains tests to lint `MODULE.bazel` files, so this runs the script in CI instead of just //tools:lint_check. Adds `src/...` to the `bazel build` and `bazel test` commands in `test_rules_scala.sh` to follow up on bazelbuild#1719 and bazelbuild#1721.

Adds `src/...` to the `bazel build` and `bazel test` commands in `test_rules_scala.sh` to follow up on bazelbuild#1719 and bazelbuild#1721. Also includes: - Adding `--test_output=errors` to each `bazel test` invocation to make failure messages visible in the CI logs. - Joining the `contents` lines in `WorkerTest.testPersistentWorkerArgsfile` using the `line.separator` system property to fix a test failure on Windows. - Swapping the arguments of `assertEquals()` assertions to `expected, actual` instead of `actual, expected` to fit the assertion failure messages. --- After adding `src/...` to the `bazel test` commands in `test_rules_scala.sh` the first time, the Windows build failed with: - https://buildkite.com/bazel/rules-scala-scala/builds/5511#0195e775-498c-4ae5-b308-34cc70da65c4/75-257 ```txt //src/java/io/bazel/rulesscala/worker:worker_test FAILED in 1.1s C:/.../testlogs/src/java/io/bazel/rulesscala/worker/worker_test/test.log Executed 120 out of 120 tests: 119 tests pass and 1 fails locally. Test "bazel test src/... test/..." failed (18 sec) Traceback (most recent call last): File "c:\b\bk-windows-gd7g\bazel\rules-scala-scala\bazelci.py", line 4528, in <module> sys.exit(main()) ^^^^^^ File "c:\b\bk-windows-gd7g\bazel\rules-scala-scala\bazelci.py", line 4496, in main execute_commands( File "c:\b\bk-windows-gd7g\bazel\rules-scala-scala\bazelci.py", line 1232, in execute_commands PrepareRepoInCwd(True, initial_setup=True) File "c:\b\bk-windows-gd7g\bazel\rules-scala-scala\bazelci.py", line 1228, in PrepareRepoInCwd execute_batch_commands(task_config.get("batch_commands", None), print_cmd_groups) File "c:\b\bk-windows-gd7g\bazel\rules-scala-scala\bazelci.py", line 1741, in execute_batch_commands return subprocess.run(batch_commands, shell=True, check=True, env=os.environ).returncode ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "C:\python3\Lib\subprocess.py", line 571, in run raise CalledProcessError(retcode, process.args, subprocess.CalledProcessError: Command 'set PATH=/usr/bin;%PATH%&bash -lc "pacman --noconfirm --needed -S libxml2"&bash test_rules_scala.sh' returned non-zero exit status 3 ``` Updating the `bazel test` command to add `--test_output=errors` made the actual failure visible: - https://buildkite.com/bazel/rules-scala-scala/builds/5513#0195e78b-6abc-42ea-95ee-8ddce41a64fb ```txt FAIL: //src/java/io/bazel/rulesscala/worker:worker_test (see C:/.../testlogs/src/java/io/bazel/rulesscala/worker/worker_test/test.log) INFO: From Testing //src/java/io/bazel/rulesscala/worker:worker_test: ===== Test output for //src/java/io/bazel/rulesscala/worker:worker_test: JUnit4 Test Runner ...E Time: 0.432 There was 1 failure: 1) testPersistentWorkerArgsfile(io.bazel.rulesscala.worker.WorkerTest) org.junit.ComparisonFailure: expected:<line 1[ --flag_1 ]ome arg > but was:<line 1[ --flag_1 some arg] > at org.junit.Assert.assertEquals(Assert.java:117) at org.junit.Assert.assertEquals(Assert.java:146) at io.bazel.rulesscala.worker.WorkerTest.testPersistentWorkerArgsfile(WorkerTest.java:73) FAILURES!!! Tests run: 3, Failures: 1 ``` This was due to the `line.separator` system property being `\r\n` on Windows, and `\n` on every other platform. Notice the `]ome arg` line, and the fact that this appeared as the `expected:` value in the assertion failure message.

srdo-humio requested review from liucijus and simuons as code owners March 18, 2025 08:58

srdo-humio mentioned this pull request Mar 18, 2025

Use of the SecurityManager #1617

Open

simuons approved these changes Mar 18, 2025

View reviewed changes

srdo-humio commented Mar 25, 2025

View reviewed changes

srdo-humio force-pushed the java-24-compat branch 3 times, most recently from d454236 to 7596cc8 Compare March 25, 2025 12:41

srdo-humio force-pushed the java-24-compat branch from 7596cc8 to dcf6b6d Compare March 25, 2025 13:07

simuons approved these changes Mar 26, 2025

View reviewed changes

simuons merged commit 818f015 into bazelbuild:master Mar 26, 2025
2 checks passed

mbland mentioned this pull request Mar 26, 2025

Bazel 8 + rules_java 8 updates, protoc toolchain #1710

Merged

simuons added a commit to simuons/rules_scala that referenced this pull request Mar 26, 2025

Remove leftover test cases from WorkerTest after SecurityManager was …

39d08fe

…removed WorkerTest was failing after bazelbuild#1719

simuons mentioned this pull request Mar 26, 2025

Cleanup WorkerTest after SecurityManager was removed #1721

Merged

simuons added a commit that referenced this pull request Mar 26, 2025

Remove leftover test cases from WorkerTest after SecurityManager was …

dc8fb8f

…removed (#1721) WorkerTest was failing after #1719

This was referenced Mar 31, 2025

Enable Bzlmod #1722

Open

Support Bzlmod and add rules_scala to bazel-central-registry #1482

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Java 24 compatibility #1719

Add Java 24 compatibility #1719

srdo-humio commented Mar 18, 2025 •

edited

Loading

srdo-humio commented Mar 18, 2025 •

edited

Loading

simuons left a comment

srdo-humio commented Mar 18, 2025 •

edited

Loading

simuons commented Mar 25, 2025

srdo-humio Mar 25, 2025

simuons Mar 25, 2025

srdo-humio commented Mar 25, 2025

simuons left a comment

mbland commented Mar 26, 2025

simuons commented Mar 26, 2025

		@@ -1,20 +1,19 @@
		load("//scala/private:common.bzl", "rlocationpath_from_file")

Add Java 24 compatibility #1719

Add Java 24 compatibility #1719

Conversation

srdo-humio commented Mar 18, 2025 • edited Loading

srdo-humio commented Mar 18, 2025 • edited Loading

simuons left a comment

Choose a reason for hiding this comment

srdo-humio commented Mar 18, 2025 • edited Loading

simuons commented Mar 25, 2025

srdo-humio Mar 25, 2025

Choose a reason for hiding this comment

simuons Mar 25, 2025

Choose a reason for hiding this comment

srdo-humio commented Mar 25, 2025

simuons left a comment

Choose a reason for hiding this comment

mbland commented Mar 26, 2025

simuons commented Mar 26, 2025

srdo-humio commented Mar 18, 2025 •

edited

Loading

srdo-humio commented Mar 18, 2025 •

edited

Loading

srdo-humio commented Mar 18, 2025 •

edited

Loading