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

Java --add-opens should not be passed to javac; breaks with -Werror #19876

Closed
timothyg-stripe opened this issue Oct 18, 2023 · 19 comments
Closed
Labels

Comments

@timothyg-stripe
Copy link
Contributor

Description of the bug:

If a target is declared with an add_opens attribute, it appears that the --add-opens flag is getting passed to javac. But this is incorrect, as --add-opens is a runtime-only flag. The consequence is that if -Werror is part of javacopts and JDK ≥17 is used, then build will fail. See reproduction case below.

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.

https://github.com/timothyg-stripe/bazel-add-opens-repro

bazel-add-opens-repro timothyg$ bazel version
Build label: 6.3.2
Build target: bazel-out/darwin_arm64-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Tue Aug 8 15:51:44 2023 (1691509904)
Build timestamp: 1691509904
Build timestamp as int: 1691509904

bazel-add-opens-repro timothyg$ cat .bazelrc
build --java_language_version=17 --javacopt=-Werror

bazel-add-opens-repro timothyg$ bazel build //:TestMain
INFO: Analyzed target //:TestMain (1 packages loaded, 3 targets configured).
INFO: Found 1 target...
ERROR: ~/bazel-add-opens-repro/BUILD.bazel:1:13: Building libtest.jar (1 source file) failed: (Exit 1): java failed: error executing command (from target //:test) external/remotejdk17_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 18 arguments skipped)
warning: [options] --add-opens has no effect at compile time
error: warnings found and -Werror specified
Target //:TestMain failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.782s, Critical Path: 0.66s
INFO: 4 processes: 3 internal, 1 worker.
FAILED: Build did NOT complete successfully

cc @cushon who wrote 2217b13

Which operating system are you running Bazel on?

macOS

What is the output of bazel info release?

release 6.3.2

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 master; 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?

No response

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

No response

@cushon
Copy link
Contributor

cushon commented Oct 18, 2023

Thanks for the report, I noticed this in #19850 (comment) and have a pending fix

@timothyg-stripe
Copy link
Contributor Author

Lucky timing! Could you also backport the PR to 7.x and also ideally 6.x if possible?

@cushon
Copy link
Contributor

cushon commented Oct 18, 2023

@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 Oct 18, 2023
copybara-service bot pushed a commit that referenced this issue Oct 18, 2023
They are unnecessary at compile-time, and this avoids a warning on recent
javac versions:

```
warning: [options] --add-opens has no effect at compile time
```

They are still collected and passed as runtime JVM flags.

See also

* #19850 (comment)
* #19876

Closes: #19859
PiperOrigin-RevId: 574527220
Change-Id: Ie9bfb8162869a3479b9401945e140c09875cc05e
@iancha1992
Copy link
Member

@timothyg-stripe We can cherry-pick this into 7.0.0, but unfortunately won't be able to cherry-pick into 6.4.0 because we're scheduled to do the final release tomorrow. But if this is critical and have to be included for 6.4.0, then let us know. Thank you!

cc: @bazelbuild/triage

@iancha1992
Copy link
Member

@bazel-io fork 7.0.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 Oct 18, 2023
@timothyg-stripe
Copy link
Contributor Author

@iancha1992 that's fine, thanks for letting me know!

@timothyg-stripe
Copy link
Contributor Author

@iancha1992 One more question: are future bugfix releases planned for 6.x? like 6.4.1 / 6.5.0? Or will the release of 7.0.0 mean that 6.x is no longer supported

@iancha1992 iancha1992 added the team-Rules-Java Issues for Java rules label Oct 18, 2023
@hvadehra
Copy link
Member

One more question: are future bugfix releases planned for 6.x? like 6.4.1 / 6.5.0? Or will the release of 7.0.0 mean that 6.x is no longer supported

https://bazel.build/release#support-matrix

@guw
Copy link
Contributor

guw commented Oct 25, 2023

@cushon I am not sure this should be merged. I have a concern with --add-opens being needed by annotation processors.

@cushon
Copy link
Contributor

cushon commented Oct 25, 2023

@guw this PR should be a no-op, passing --add-opens as a command line argument to javac doesn't do anything. If an annotation processor was reflecting on internal JDK APIs, it would need --add-opens to be passed as a runtime JVM flag to javac, which is still possible.

i.e. this is the difference between javac -J--add-opens= Foo.java (-J means the flag is passed as a VM argument) and javac --add-opens=... Foo.java (without -J it's a regular javac flag).

@Wyverald
Copy link
Member

This is fixed by 367994a, right?

iancha1992 pushed a commit to iancha1992/bazel that referenced this issue Nov 1, 2023
They are unnecessary at compile-time, and this avoids a warning on recent
javac versions:

```
warning: [options] --add-opens has no effect at compile time
```

They are still collected and passed as runtime JVM flags.

See also

* bazelbuild#19850 (comment)
* bazelbuild#19876

Closes: bazelbuild#19859
PiperOrigin-RevId: 574527220
Change-Id: Ie9bfb8162869a3479b9401945e140c09875cc05e
@iancha1992
Copy link
Member

Cherry-picked in #20016

iancha1992 added a commit that referenced this issue Nov 1, 2023
They are unnecessary at compile-time, and this avoids a warning on
recent javac versions:

```
warning: [options] --add-opens has no effect at compile time
```

They are still collected and passed as runtime JVM flags.

See also

*
#19850 (comment)
* #19876

Closes: #19859

Commit
367994a

PiperOrigin-RevId: 574527220
Change-Id: Ie9bfb8162869a3479b9401945e140c09875cc05e

Co-authored-by: Googler <[email protected]>
@timothyg-stripe
Copy link
Contributor Author

@bazel-io fork 6.0.0

@iancha1992
Copy link
Member

@timothyg-stripe this will be included in the release-7.0.0 which is set to release its RC3 next week. We will keep you updated! Thanks :)

@timothyg-stripe
Copy link
Contributor Author

timothyg-stripe commented Nov 3, 2023

@iancha1992 Would it be possible to cherry-pick into 6.x also? It will likely take us some time to upgrade to 7.0.0.

I'm happy to supply a patch, considering the 7.0.0 commit earlier doesn't apply cleanly to 6.x.

@Wyverald
Copy link
Member

Wyverald commented Nov 3, 2023

We don't have plans to create a 6.5.0 as of yet; but can consider it if more backport requests show up.

@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.0.0 RC5. Please test out the release candidate and report any issues as soon as possible. Thanks!

@keertk
Copy link
Member

keertk commented Dec 7, 2023

@bazel-io fork 6.5.0

@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 6.5.0 RC1 as well. Please test out the release candidate and report any issues as soon as possible. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants