Pass `ct.sym` to direct version of Turbine #20294

fmeum · 2023-11-22T20:50:19Z

This ensures that Turbine has access to ct.sym and thus supports the --release flag even if it is compiled into a Graal native image, which doesn't have access to a JAVA_HOME.

This requires exposing the ct.sym file on a new lib_ct_sym attribute of java_runtime and passing it into Turbine via the turbine.ctSymPath system property introduced in v0.3.1.

Along the way this commit fixes the setup code for testing unreleased versions of the remote Java and coverage tools, which silently broke with the flip of --enable_bzlmod.

Work towards bazelbuild/stardoc#195

fmeum · 2023-11-22T21:45:21Z

@cushon I am slightly confused why the integration test still fails - it does seem to pass the correct argument to turbine. But it looks like the toolchain_head version of the test still uses the native image from remote_java_tools instead of the one built from source. Do you happen to know why?

cushon · 2023-11-23T03:02:50Z

This looks great overall.

@cushon I am slightly confused why the integration test still fails - it does seem to pass the correct argument to turbine. But it looks like the toolchain_head version of the test still uses the native image from remote_java_tools instead of the one built from source. Do you happen to know why?

I am not sure, @hvadehra do you know?

hvadehra · 2023-11-23T07:51:48Z

bazel_java_test.sh is only run with released java_tools:

It's also run for java_tools built from source, but only run while creating the java_tools release:
3) https://github.com/bazelbuild/bazel/blob/master/src/test/shell/bazel/BUILD#L284-L305

Options:
a) Add this test in a new file, and invoke like 3). Once we have a java_tools release with this change, we can put it back into bazel_java_test.sh
b) Early exit in the test case if we're not running at head, drop the early exit once we have a release.

I guess a) is slightly better, since we will exercise the test in this PR, rather than during the java_tools release. But I'm okay with either option.

src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java

This ensures that Turbine has access to `ct.sym` and thus supports the `--release` flag even if it is compiled into a Graal native image, which doesn't have access to a `JAVA_HOME`. This requires exposing the `ct.sym` file on a new `lib_ct_sym` attribute of `java_runtime` and passing it into Turbine via the `turbine.ctSymPath` system property introduced in v0.3.1.

fmeum · 2023-11-24T20:38:20Z

bazel_java_test.sh is only run with released java_tools:

https://github.com/bazelbuild/bazel/blob/master/src/test/shell/bazel/BUILD#L236-L251

https://github.com/bazelbuild/bazel/blob/master/src/test/shell/bazel/BUILD#L258-L279

It's also run for java_tools built from source, but only run while creating the java_tools release: 3) https://github.com/bazelbuild/bazel/blob/master/src/test/shell/bazel/BUILD#L284-L305

I think that's not true, this test also used to run with Java tools built from HEAD:

bazel/src/test/shell/bazel/BUILD

Lines 264 to 275 in 7d10999

 # java_tools zips to test 

 "src/java_tools.zip", 

 "src/java_tools_prebuilt.zip", 

 # --java_language_version value 

 java_version, 

 # --java_runtime_version value 

 java_version, 

 ], 

 data = [ 

 ":test-deps", 

 "//src:java_tools_prebuilt_zip", 

 "//src:java_tools_zip",

I investigated this further and am now convinced that the mechanism used to override the remote Java and coverage tools simply stopped working when --enable_bzlmod was flipped. I modified the test setups to correctly override the repos and the test is now passing (and skipped with the current java_tools release).

src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java

fmeum · 2023-11-24T20:35:42Z

src/test/shell/bazel/bazel_coverage_java_test.sh

@@ -24,36 +24,34 @@ source "${CURRENT_DIR}/coverage_helpers.sh" \
 || { echo "coverage_helpers.sh not found!" >&2; exit 1; }


+RULES_JAVA_REPO_NAME=$(cat "$(rlocation io_bazel/src/test/shell/bazel/RULES_JAVA_REPO_NAME)")


The mechanism used to override the remote Java and coverage tools didn't work with --enable_bzlmod, so I rewrote it borrowing some existing tricks like this one.

@meteorcloudy Could you review the changes to the test files (except for the new test case, which is unrelated to Bzlmod)?

fmeum · 2023-11-24T20:39:51Z

@bazel-io flag

hvadehra · 2023-11-27T09:16:40Z

I think that's not true, this test also used to run with Java tools built from HEAD:

Yes, you're quite right, I was too hasty in reading the BUILD file :)

I investigated this further and am now convinced that the mechanism used to override the remote Java and coverage tools simply stopped working when --enable_bzlmod was flipped. I modified the test setups to correctly override the repos and the test is now passing (and skipped with the current java_tools release).

Thanks a lot for taking the time to fix this. @meteorcloudy We should probably audit all other tests that use a similar repo overridding mechanism to ensure they're still working as expected with bzlmod enabled.

@fmeum Do your changes not break the tests when making the java_tools release (bazel_java_test_local_java_tools_jdk*) ? ISTM we will need to update upload_all_java_tools.sh as well?

meteorcloudy · 2023-11-27T09:47:38Z

Yes, I have tracking issue for enabling Bzlmod in all tests. #19824. For shell tests, those are the leftovers: https://cs.opensource.google/search?q=%22disable_bzlmod%22&sq=&ss=bazel

fmeum · 2023-11-27T10:46:40Z

@fmeum Do your changes not break the tests when making the java_tools release (bazel_java_test_local_java_tools_jdk*) ? ISTM we will need to update upload_all_java_tools.sh as well?

Good catch, I pushed a new commit to hopefully fix this.

meteorcloudy · 2023-11-27T13:56:56Z

@bazel-io fork 7.0.0

meteorcloudy

LGTM for Bzlmod related change.

meteorcloudy · 2023-11-27T13:57:57Z

src/test/shell/bazel/bazel_coverage_java_test.sh

@@ -24,36 +24,34 @@ source "${CURRENT_DIR}/coverage_helpers.sh" \
 || { echo "coverage_helpers.sh not found!" >&2; exit 1; }


+RULES_JAVA_REPO_NAME=$(cat "$(rlocation io_bazel/src/test/shell/bazel/RULES_JAVA_REPO_NAME)")


honnix · 2023-11-28T09:10:11Z

Thank you for addressing this. I built the binary from bazel@HEAD (I made sure the commit 4a29f08 was there) but still see this problem happening. I might be doing something wrong.

$ /path/to/bazel/bazel-bin/src/bazel build //:foo --sandbox_debug
...
ERROR: /private/var/tmp/_bazel_honnix/7a71ac7a78af123987d625ef3d1d3eec/external/rules_jvm_external/private/tools/java/com/github/bazelbuild/rules_jvm_external/BUILD:1:13: Compiling Java headers external/rules_jvm_external/private/tools/java/com/github/bazelbuild/rules_jvm_external/librules_jvm_external-hjar.jar (4 source files) [for tool] failed: (Exit 1): sandbox-exec failed: error executing Turbine command
  (cd /private/var/tmp/_bazel_honnix/7a71ac7a78af123987d625ef3d1d3eec/sandbox/darwin-sandbox/866/execroot/_main && \
  exec env - \
    LC_CTYPE=en_US.UTF-8 \
    PATH='...' \
    TMPDIR=/var/folders/bd/4b4zq6s56tg01zdzqpw5b9gm0000gn/T/ \
  /usr/bin/sandbox-exec -f /private/var/tmp/_bazel_honnix/7a71ac7a78af123987d625ef3d1d3eec/sandbox/darwin-sandbox/866/sandbox.sb /var/tmp/_bazel_honnix/install/0f4666cf1082ebd83b24dc6a797d2675/process-wrapper '--timeout=0' '--kill_delay=15' '--stats=/private/var/tmp/_bazel_honnix/7a71ac7a78af123987d625ef3d1d3eec/sandbox/darwin-sandbox/866/stats.out' external/rules_java~7.3.0~toolchains~remote_java_tools_darwin_arm64/java_tools/turbine_direct_graal '-Dturbine.ctSymPath=external/rules_java~7.3.0~toolchains~remotejdk21_macos_aarch64/lib/ct.sym' --output bazel-out/darwin_arm64-opt-exec-ST-13d3ddad9198/bin/external/rules_jvm_external/private/tools/java/com/github/bazelbuild/rules_jvm_external/librules_jvm_external-hjar.jar --output_deps bazel-out/darwin_arm64-opt-exec-ST-13d3ddad9198/bin/external/rules_jvm_external/private/tools/java/com/github/bazelbuild/rules_jvm_external/librules_jvm_external-hjar.jdeps --bootclasspath bazel-out/darwin_arm64-opt-exec-ST-13d3ddad9198/bin/external/rules_java~7.3.0/toolchains/platformclasspath.jar --sources external/rules_jvm_external/private/tools/java/com/github/bazelbuild/rules_jvm_external/ByteStreams.java external/rules_jvm_external/private/tools/java/com/github/bazelbuild/rules_jvm_external/Coordinates.java external/rules_jvm_external/private/tools/java/com/github/bazelbuild/rules_jvm_external/Hasher.java external/rules_jvm_external/private/tools/java/com/github/bazelbuild/rules_jvm_external/MavenRepositoryPath.java --javacopts -source 17 -target 17 '-XDskipDuplicateBridges=true' '-XDcompilePolicy=simple' -g -parameters -Xep:ReturnValueIgnored:OFF -Xep:IgnoredPureGetter:OFF -Xep:EmptyTopLevelDeclaration:OFF -Xep:LenientFormatStringValidation:OFF -Xep:ReturnMissingNullable:OFF -Xep:UseCorrectAssertInTests:OFF --release 8 -- --target_label @@@rules_jvm_external//private/tools/java/com/github/bazelbuild/rules_jvm_external:rules_jvm_external --reduce_classpath_mode NONE)
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)
Target //:foo failed to build

I built bazel as bz build //src:bazel. Everything is done on macOS with M2 chip.

fmeum · 2023-11-28T09:13:26Z

@honnix Bazel uses a prebuilt version of Turbine that is shipped separately. This will require an update to the java_tools and ship with the next release of rules_java.

honnix · 2023-11-28T09:22:20Z

@honnix Bazel uses a prebuilt version of Turbine that is shipped separately. This will require an update to the java_tools and ship with the next release of rules_java.

Ah I see. Thank you. I will wait until then. I can temporarily disable header compilation.

Cherry-picked from 4a29f08 Compared to the cherry-picked commit, this does not contain the turbine update as turbine is not embedded into the Bazel binary and can be updated independently via rules_java. Original description follows: This ensures that Turbine has access to `ct.sym` and thus supports the `--release` flag even if it is compiled into a Graal native image, which doesn't have access to a `JAVA_HOME`. This requires exposing the `ct.sym` file on a new `lib_ct_sym` attribute of `java_runtime` and passing it into Turbine via the `turbine.ctSymPath` system property introduced in v0.3.1. Along the way this commit fixes the setup code for testing unreleased versions of the remote Java and coverage tools, which silently broke with the flip of `--enable_bzlmod`. Work towards bazelbuild/stardoc#195 Closes bazelbuild#20294. PiperOrigin-RevId: 585866870 Change-Id: I416b787e324bd3a01e223edbda4f9ba137c21241

Cherry-picked from 4a29f08 Compared to the cherry-picked commit, this does not contain the turbine update as turbine is not embedded into the Bazel binary and can be updated independently via rules_java. Original description follows: This ensures that Turbine has access to `ct.sym` and thus supports the `--release` flag even if it is compiled into a Graal native image, which doesn't have access to a `JAVA_HOME`. This requires exposing the `ct.sym` file on a new `lib_ct_sym` attribute of `java_runtime` and passing it into Turbine via the `turbine.ctSymPath` system property introduced in v0.3.1. Along the way this commit fixes the setup code for testing unreleased versions of the remote Java and coverage tools, which silently broke with the flip of `--enable_bzlmod`. Work towards bazelbuild/stardoc#195 Closes #20294. PiperOrigin-RevId: 585866870 Change-Id: I416b787e324bd3a01e223edbda4f9ba137c21241 Closes #20323

fmeum · 2023-11-28T14:12:24Z

@keertk Could you create patch releases of java_tools and rules_java from current HEAD?

meteorcloudy · 2023-11-28T14:16:28Z

@fmeum Just to clarify, Bazel 7 doesn't have to wait for the new releases, right?

fmeum · 2023-11-28T14:36:23Z

@fmeum Just to clarify, Bazel 7 doesn't have to wait for the new releases, right?

Exactly, with this PR merged it is fully independent.

keertk · 2023-11-28T14:36:47Z

@keertk Could you create patch releases of java_tools and rules_java from current HEAD?

@fmeum yes sure, though this is currently blocked by the ongoing network issues.
Tracked here - bazelbuild/java_tools#86

…ting that overriding actually works Should help avoid issues like: #21351 #20374 #20294 (comment) Closes #21364. PiperOrigin-RevId: 607785065 Change-Id: Ib8c75d597911034091c3dafd638acfe72d2eb9c5

fmeum force-pushed the update-turbine branch 2 times, most recently from 5dea561 to fa11afa Compare November 22, 2023 21:35

fmeum marked this pull request as ready for review November 22, 2023 21:44

fmeum requested a review from lberki as a code owner November 22, 2023 21:44

fmeum requested review from cushon and removed request for lberki November 22, 2023 21:44

github-actions bot added team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels Nov 22, 2023

cushon requested a review from hvadehra November 23, 2023 03:01

hvadehra reviewed Nov 23, 2023

View reviewed changes

src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java Show resolved Hide resolved

fmeum mentioned this pull request Nov 23, 2023

Make Turbine x86_64 native image more compatible #20306

Closed

fmeum force-pushed the update-turbine branch from fa11afa to e54ae09 Compare November 24, 2023 08:09

fmeum force-pushed the update-turbine branch from dbdfc95 to 7648e0a Compare November 24, 2023 19:51

Attempt to fix Bzlmod java tools override

8bdff69

fmeum force-pushed the update-turbine branch from 7648e0a to 8bdff69 Compare November 24, 2023 20:30

fmeum commented Nov 24, 2023

View reviewed changes

fmeum requested review from hvadehra and meteorcloudy November 24, 2023 20:38

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 24, 2023

Support absolute paths for "at head" test overrides

30f0f58

bazel-io mentioned this pull request Nov 27, 2023

[7.0.0] Pass ct.sym to direct version of Turbine #20323

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 27, 2023

meteorcloudy approved these changes Nov 27, 2023

View reviewed changes

cushon approved these changes Nov 27, 2023

View reviewed changes

hvadehra approved these changes Nov 27, 2023

View reviewed changes

hvadehra added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Nov 27, 2023

copybara-service bot closed this in 4a29f08 Nov 28, 2023

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Nov 28, 2023

fmeum deleted the update-turbine branch November 28, 2023 09:12

fmeum mentioned this pull request Nov 28, 2023

[7.0.0] Pass ct.sym to direct version of Turbine #20341

Merged

keertk mentioned this pull request Nov 28, 2023

Release: java_tools v13.3 bazelbuild/java_tools#86

Closed

hvadehra mentioned this pull request Feb 15, 2024

Remove duplication in overriding java_tools and add a test for validating that overriding actually works #21364

Closed

comius mentioned this pull request Sep 4, 2024

Upgrade rules_java to 7.10.0 bazel-contrib/rules_jvm_external#1226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass `ct.sym` to direct version of Turbine #20294

Pass `ct.sym` to direct version of Turbine #20294

fmeum commented Nov 22, 2023 •

edited

Loading

fmeum commented Nov 22, 2023

cushon commented Nov 23, 2023

hvadehra commented Nov 23, 2023 •

edited

Loading

fmeum commented Nov 24, 2023

fmeum Nov 24, 2023

meteorcloudy Nov 27, 2023

fmeum commented Nov 24, 2023

hvadehra commented Nov 27, 2023

meteorcloudy commented Nov 27, 2023 •

edited

Loading

fmeum commented Nov 27, 2023

meteorcloudy commented Nov 27, 2023

meteorcloudy left a comment

meteorcloudy Nov 27, 2023

honnix commented Nov 28, 2023

fmeum commented Nov 28, 2023

honnix commented Nov 28, 2023

fmeum commented Nov 28, 2023

meteorcloudy commented Nov 28, 2023

fmeum commented Nov 28, 2023 •

edited

Loading

keertk commented Nov 28, 2023

		@@ -24,36 +24,34 @@ source "${CURRENT_DIR}/coverage_helpers.sh" \
		\|\| { echo "coverage_helpers.sh not found!" >&2; exit 1; }


		RULES_JAVA_REPO_NAME=$(cat "$(rlocation io_bazel/src/test/shell/bazel/RULES_JAVA_REPO_NAME)")

Pass ct.sym to direct version of Turbine #20294

Pass ct.sym to direct version of Turbine #20294

Conversation

fmeum commented Nov 22, 2023 • edited Loading

fmeum commented Nov 22, 2023

cushon commented Nov 23, 2023

hvadehra commented Nov 23, 2023 • edited Loading

fmeum commented Nov 24, 2023

fmeum Nov 24, 2023

Choose a reason for hiding this comment

meteorcloudy Nov 27, 2023

Choose a reason for hiding this comment

fmeum commented Nov 24, 2023

hvadehra commented Nov 27, 2023

meteorcloudy commented Nov 27, 2023 • edited Loading

fmeum commented Nov 27, 2023

meteorcloudy commented Nov 27, 2023

meteorcloudy left a comment

Choose a reason for hiding this comment

meteorcloudy Nov 27, 2023

Choose a reason for hiding this comment

honnix commented Nov 28, 2023

fmeum commented Nov 28, 2023

honnix commented Nov 28, 2023

fmeum commented Nov 28, 2023

meteorcloudy commented Nov 28, 2023

fmeum commented Nov 28, 2023 • edited Loading

keertk commented Nov 28, 2023

Pass `ct.sym` to direct version of Turbine #20294

Pass `ct.sym` to direct version of Turbine #20294

fmeum commented Nov 22, 2023 •

edited

Loading

hvadehra commented Nov 23, 2023 •

edited

Loading

meteorcloudy commented Nov 27, 2023 •

edited

Loading

fmeum commented Nov 28, 2023 •

edited

Loading