-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
Fix: Java 18+ Builds #7087
Fix: Java 18+ Builds #7087
Conversation
cc / @cpovirk Sorry for the flood of tags; this first one is ready to go for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think this looks good. Just one question below, and then I'll see about getting it submitted internally and mirrored out.
(The flood of mentions is fine with me. I'll try to keep up as best I can, but I can say in advance that I won't be as on top of things as I'd like at the moment.)
@cpovirk Thanks for the quick review! I'll see if I can run down that I don't think I did, I run the actions on my fork, but the feedback would help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, here's an approval.
(Our internal tooling is going to scold me to not approve Guava PRs because we don't have a good import configuration for them. I will ignore it :) (Approving doesn't actually hurt anything; we just have it set up so that anyone who approves a Guava PR realizes that it won't get automatically imported. It is not ideal that that discourages me from approving to run CI.))
...annnnnd that isn't even the kind of approval it needed :) Now actually approved for CI. |
Ha ha! I didn't even notice. It looks like everything but Copybara is happy, as you'd predicted @cpovirk. I'm not exactly sure why the Windows run isn't making it in; should I take a look, or is that expected? I'll also rebase onto latest. |
77cd546
to
3feda73
Compare
I couldn't easily find the old results anymore, so I re-approved. It might be that what you're seeing is that "pom.xml on JDK 17 on windows-latest" is listed as "Expected — Waiting for status to be reported" because our branch-protection settings have that listed as a requirement. But what runs now is "CI / pom.xml on JDK 21 on windows-latest (pull_request)," which does appear to be running. (And I assume that I will see it pass soon.) If that's all that's going on, then I can update our settings to look under the new name (and, for that matter, to look for the JDK 21 runs in general) when we merge this. Anyway, I think I'm going to send this out for internal review (minus the additional |
Or I guess we might not need the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I can never press the right button in GitHub's review UI, so I published one of my comments separately.
@cpovirk Going to rebase this one now that JDK srczip has been merged. |
923aead
to
8a7df43
Compare
Okay, this is cleaned up and all review comments have been addressed, I think. |
8a7df43
to
a484650
Compare
- fix: build against embedded jdk sources needs `--enable-preview` when built against jvm21 - fix: add `-Djava.security.manager=allow` to fix security manager tests under modern java Fixes and closes google#7065 Signed-off-by: Sam Gammon <[email protected]>
a484650
to
6bb8533
Compare
The main changes is that tests need `-Djava.security.manager=allow`. Fixes and closes #7065 Helps with #6790 Fixes #6245 (at least the remaining parts that we actually care about) Fixes #7087 Signed-off-by: Sam Gammon <[email protected]> RELNOTES=n/a PiperOrigin-RevId: 614671411
The main changes is that tests need `-Djava.security.manager=allow`. Fixes and closes #7065 Helps with #6790 Fixes #6245 (at least the remaining parts that we actually care about) Fixes #7087 Signed-off-by: Sam Gammon <[email protected]> RELNOTES=n/a PiperOrigin-RevId: 614671411
6bb8533
to
5aa1838
Compare
Signed-off-by: Sam Gammon <[email protected]>
5aa1838
to
cbd19ee
Compare
@cpovirk I've adjusted the PR to meet all comments. Re/your notes about JVM testing and CI resources, I'm happy to file a change like this one once this is merged, if you are open to it. That approach changes things a bit because Guava would build at latest JVM only, and then test at each JVM tier using Maven toolchains and specific bytecode settings for each suite. The effect is the same except that no expectation is provided that the codebase builds at earlier JDKs. I've found it to be very effective in my workflows. |
Oh, also, thank you for the note about credit! But I know Copybara doesn't align perfectly with the PR process, and I expect the conclusion of this PR to be closing it no matter what, even if it appears on |
The main changes is that tests need `-Djava.security.manager=allow`. Fixes and closes #7065 Helps with #6790 Fixes #6245 (at least the remaining parts that we actually care about) Fixes #7087 Signed-off-by: Sam Gammon <[email protected]> RELNOTES=n/a PiperOrigin-RevId: 614671411
Yes, please. I think it's time to give that a try. It should help with everything from "actually being able to compile a |
Summary
Amends the build to be compatible with JDKs newer than 18. Adds JDK21 to the CI matrix.
Related issues
master
with JDK 21 #7065SecurityManager
build failures (and maybe other failures on some older JDKs) #6245Changelog
--enable-preview
when built against jvm21-Djava.security.manager=allow
to fix security manager tests under modern java