Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turbine mishandles --release flag in some cases #21895

Closed
srdo opened this issue Apr 4, 2024 · 11 comments
Closed

Turbine mishandles --release flag in some cases #21895

srdo opened this issue Apr 4, 2024 · 11 comments
Labels
team-Rules-Java Issues for Java rules turbine Issues related to https://github.com/google/turbine type: bug untriaged

Comments

@srdo
Copy link

srdo commented Apr 4, 2024

Description of the bug:

Turbine contains code that inspects the current JDK to determine where to look for class file signatures. I believe this is incorrect, since as I understand it, Turbine runs as a native Graal image which doesn't have these signatures.

The two places I noticed where Turbine looks at the running current JDK are here and here.

The second of those is only important if you're targeting pre-Java-12, but the former will cause Turbine to use JimageClassBinder.bindDefault to look up signatures if you pass it --release N where N is the version of the JDK running Turbine (21 in my case), and that will get you an error since jrt is not available on the native image.

I think the reason this code exists at all is that ct.sym doesn't contain signatures for the current JDK. As far as I can tell, that has changed as of Java 22, and ct.sym now contains signatures for the latest JDK version as well.

I think the jrt lookup doesn't work at all, so maybe it should be removed? From a user perspective, the thing that doesn't work is passing --release N if Turbine is running on Java N. For ct.sym files from Java 22+, it should work to look in ct.sym no matter what --release is set to, so that would at least fix this on the newest JDK.

In order to fix this for ct.sym files for older JDKs as well, I think it would be necessary to hand Turbine a reference to the module files, in addition to the reference to ct.sym it's already getting, since those ct.sym files don't contain signatures for the JDK they're from.

Which category does this issue belong to?

Java Rules

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Try to build a Java target with --release 21.

Which operating system are you running Bazel on?

MacOS x64

What is the output of bazel info release?

7.1.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

Other work recently took place in this area in #21610 and in #20323

Any other information, logs, or outputs that you want to share?

No response

@cushon
Copy link
Contributor

cushon commented Apr 4, 2024

Thanks for the report, I agree we shouldn't be relying on those paths for the turbine native-image when --release is being used.

It's been a while since I looked at the status of jrt and Graal, but it looks like it has a AllowJRTFileSystem flag and some jrt support now, at least for reading from a separate JDK (but not the host runtime).

I think it would be necessary to hand Turbine a reference to the module files, in addition to the reference to ct.sym it's already getting, since those ct.sym files don't contain signatures for the JDK they're from.

I think the invocations are already passing both the 'bootclasspath' jar we generate and ct.sym, I wonder if it makes sense for it to recognize the case where ct.sym doesn't contain the desired --release and call back to using the 'bootclasspath' jar.

@srdo-humio
Copy link

Thanks for responding.

Since the purpose of the --release flag is to allow building on a new JDK while targeting an old one, I think even an incomplete fix that only works for ct.sym from Java 22 would be useful. Fixing the issue when the runtime JDK is 21 or below could come later, when someone actually has that need?

I think a partial fix for Java 22 would be to simply swap the two lookups here https://github.com/google/turbine/blob/ff3b0f75b4df04b1dd04c2bd4f6934934049738c/java/com/google/turbine/main/Main.java#L304-L309, so we check ct.sym first, and only then check jrt if we're targeting the current JDK and didn't find anything in the sym file.

Would you accept a PR to that effect, or do you think another solution is better?

srdo-humio added a commit to srdo-humio/turbine that referenced this issue Apr 5, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not
enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so
there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895
srdo-humio added a commit to srdo-humio/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not
enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so
there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

FUTURE_COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 622308437
copybara-service bot pushed a commit to google/turbine that referenced this issue Apr 9, 2024
Since Turbine is used via a GraalVM native image in Bazel, jrt is not enabled, so looking up the bootclasspath that way doesn't work.

As of Java 22, ct.sym contains signatures for all supported JDKs, so there is no need to look in jrt.

Partial fix for bazelbuild/bazel#21895

Fixes #318

COPYBARA_INTEGRATE_REVIEW=#318 from srdo-humio:stig/check-ct-sym-first 2a48a7d
PiperOrigin-RevId: 623187179
@hvadehra
Copy link
Member

hvadehra commented May 8, 2024

Looks like google/turbine#320 made it into Turbine 0.6.0, which landed in Bazel in 81117aa

Does that resolve this issue, or is there anything else we're waiting for?

@illicitonion
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 8, 2024
@srdo-humio
Copy link

@hvadehra The issue I described should have been fixed, so IMO feel free to close.

@iancha1992
Copy link
Member

@bazel-io fork 7.2.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 8, 2024
Wyverald added a commit that referenced this issue May 8, 2024
Same as 81117aa which was accidentally reverted in 5e63f2d

Fixes #21895
Wyverald referenced this issue May 8, 2024
This brings AVX-512 support on Linux.

Also adds a JMH benchmark pitting BLAKE3 against SHA2-256.

Results with `-f 1` (single JVM fork) and for `hashBytesOneShot` only:
<details>
<summary>Intel Core i5-8520U, Linux: BLAKE3 has ~8x the throughput on large inputs</summary>
<pre>
Benchmark                                      (size)    (type)   Mode  Cnt        Score        Error  Units
BazelHashFunctionsBenchmark.hashBytesOneShot        1    BLAKE3  thrpt    5  3897193.109 ± 104089.759  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot        1  SHA2_256  thrpt    5  9773250.840 ± 919565.969  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot       16    BLAKE3  thrpt    5  4058401.127 ±  69345.382  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot       16  SHA2_256  thrpt    5  9338184.696 ± 575903.627  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot      128    BLAKE3  thrpt    5  3883335.405 ± 197131.021  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot      128  SHA2_256  thrpt    5  3931746.804 ± 111963.068  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot      512    BLAKE3  thrpt    5  3165886.130 ± 105001.405  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot      512  SHA2_256  thrpt    5  1689377.092 ±  67006.025  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     1024    BLAKE3  thrpt    5  2137151.012 ±  71425.961  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     1024  SHA2_256  thrpt    5   971335.403 ±  43622.796  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     4096    BLAKE3  thrpt    5  1266551.855 ±  77312.865  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     4096  SHA2_256  thrpt    5   271217.035 ±  15770.310  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot    16384    BLAKE3  thrpt    5   562124.458 ±  47243.736  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot    16384  SHA2_256  thrpt    5    72281.652 ±  10734.186  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot  1048576    BLAKE3  thrpt    5     9800.524 ±    230.269  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot  1048576  SHA2_256  thrpt    5     1124.542 ±     40.938  ops/s
</pre>
</details>
<details>
<summary>MacBook Pro with M3 Max, macOS: BLAKE3 has ~0.75x the throughput on large inputs</summary>
<pre>
Benchmark                                      (size)    (type)   Mode  Cnt         Score        Error  Units
BazelHashFunctionsBenchmark.hashBytesOneShot        1    BLAKE3  thrpt    5   9262824.819 ±  12194.067  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot        1  SHA2_256  thrpt    5  76557346.275 ± 548738.127  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot       16    BLAKE3  thrpt    5   9254500.192 ±  22138.081  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot       16  SHA2_256  thrpt    5  81029076.629 ± 748425.519  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot      128    BLAKE3  thrpt    5   8304084.839 ±  20398.724  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot      128  SHA2_256  thrpt    5  4146027.256 ± 106648.234  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     1024    BLAKE3  thrpt    5   3092086.580 ±   1301.806  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     1024  SHA2_256  thrpt    5   9355426.285 ±   7352.032  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     4096    BLAKE3  thrpt    5   1670833.346 ±   1809.726  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     4096  SHA2_256  thrpt    5   2562509.914 ±  29303.110  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot    16384    BLAKE3  thrpt    5    484960.116 ±    146.961  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot    16384  SHA2_256  thrpt    5    658392.748 ±   3364.324  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot  1048576    BLAKE3  thrpt    5      7987.472 ±     19.194  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot  1048576  SHA2_256  thrpt    5     10380.444 ±      8.804  ops/s
</pre>
</details>
<details>
<summary>AMD Ryzen 7 PRO 5850U, Windows: BLAKE3 has ~1.5x the throughput on large inputs</summary>
<pre>
BazelHashFunctionsBenchmark.hashBytesOneShot        1    BLAKE3  thrpt    5   5569003,683 ± 125621,794  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot        1  SHA2_256  thrpt    5  21202138,257 ± 458127,205  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot       16    BLAKE3  thrpt    5   5539298,273 ±  77378,097  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot       16  SHA2_256  thrpt    5  21618815,496 ± 208338,556  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot      128    BLAKE3  thrpt    5   5047579,827 ± 118690,537  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot      128  SHA2_256  thrpt    5  15806244,512 ± 258848,826  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot      512    BLAKE3  thrpt    5   3300538,392 ±  53754,778  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot      512  SHA2_256  thrpt    5   8353887,852 ±  47076,094  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     1024    BLAKE3  thrpt    5   2062144,084 ±  14557,116  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     1024  SHA2_256  thrpt    5   5120693,705 ±  30640,599  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     4096    BLAKE3  thrpt    5   1437595,889 ±  34088,637  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot     4096  SHA2_256  thrpt    5   1552307,356 ±  25584,819  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot    16384    BLAKE3  thrpt    5    558955,757 ±   8647,716  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot    16384  SHA2_256  thrpt    5    411619,868 ±   1179,203  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot  1048576    BLAKE3  thrpt    5      9576,940 ±    460,875  ops/s
BazelHashFunctionsBenchmark.hashBytesOneShot  1048576  SHA2_256  thrpt    5      6470,682 ±     41,223  ops/s
</pre>
</details>

Closes #22017.

PiperOrigin-RevId: 628330908
Change-Id: Ic635027d020d60b79d2e498fcebb0cc42fae712b
@Wyverald
Copy link
Member

Wyverald commented May 8, 2024

Looks like google/turbine#320 made it into Turbine 0.6.0, which landed in Bazel in 81117aa

Does that resolve this issue, or is there anything else we're waiting for?

Looks like that commit was accidentally reverted. I've sent #22301 to roll forward.

Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
Same as bazelbuild@81117aa which was accidentally reverted in bazelbuild@5e63f2d

Fixes bazelbuild#21895

Closes bazelbuild#22301.

PiperOrigin-RevId: 632186641
Change-Id: If30c5db8a6d177b23d0a50628b11d37bdfaab88b
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.2.0 RC1. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.2.0rc1. Thanks!

@mattnworb
Copy link
Contributor

Hi, this does not appear to be fixed in 7.2.0rc1 if the runtime JDK is 21 and the --release 21 javacopt is passed to a java_library.

In that case we get an error like:

ERROR: /Users/mattbrown/turbine-issue/BUILD.bazel:18:13: Compiling Java headers turbine-issue/libhello_lib-hjar.jar (1 source file) failed: (Exit 1): turbine_direct_graal failed: error executing Turbine command (from target //turbine-issue:hello_lib) external/remote_java_tools_darwin_arm64/java_tools/turbine_direct_graal '-Dturbine.ctSymPath=external/companyname_remotejdk_21_macos_aarch64/lib/ct.sym' --output ... (remaining 29 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
not a supported release: OptionalInt[21]

Usage: turbine [options]
...

The fix referenced above seems to only be for a runtime JDK of 22+:

Since the purpose of the --release flag is to allow building on a new JDK while targeting an old one, I think even an incomplete fix that only works for ct.sym from Java 22 would be useful. Fixing the issue when the runtime JDK is 21 or below could come later, when someone actually has that need?

here is a minimal repro case, minus any WORKSPACE or toolchain setup: https://gist.github.com/mattnworb/b3043807d9da9bd9d6953938002c579d. There is no error when the --release flag in the java_library is --release 11 or --release 17, or when omitted.

@srdo
Copy link
Author

srdo commented May 22, 2024

@mattnworb Yes, before Java 22, ct.sym doesn't contain symbols for the current JDK, so you can't pass --release 21 to Java 21, because Turbine won't know where to find the signatures, it only knows how to look in ct.sym. The fix we implemented should be effective as of Java 22, so as of that version you should be able to use --release N on JDK N.

Fixing this for Java 21 requires a more complicated fix, since Turbine can't just look in ct.sym for that version.

If you don't want to work on that fix, consider upgrading the JDK you use for building to Java 22 instead. As long as you continue passing --release 21, the emitted code should stay compatible with Java 21.

@cushon cushon added the turbine Issues related to https://github.com/google/turbine label May 22, 2024
@mattnworb
Copy link
Contributor

I see, thanks. We are wrapping most of our usages of java_library in a macro where it is pretty simple for us to avoid setting a --release flag if it is the same value as our (default) toolchain version of 21, so the fix isn't too bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-Java Issues for Java rules turbine Issues related to https://github.com/google/turbine type: bug untriaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.