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

Fix build for JDK 22+ #823

Merged
merged 5 commits into from
Aug 7, 2024
Merged

Fix build for JDK 22+ #823

merged 5 commits into from
Aug 7, 2024

Conversation

cpovirk
Copy link

@cpovirk cpovirk commented Jul 30, 2024

The dependency changes work around the Gradle issue identified in
#806.

The flag changes prepare for new javac warnings.

With this PR, my build with JDK 22 worked. My build with JDK 23
initially got far enough for me to encounter the new warnings but then
started failing with the kind of weirdly nondeterministic Gradle errors
we've seen before:

BUG! exception in phase 'semantic analysis' in source unit '_BuildScript_' Unsupported class file major version 67

The dependency changes work around the Gradle issue identified in
eisop#806.

The flag changes prepare for new javac warnings.

With this PR, my build with JDK 22 worked. My build with JDK 23
initially got far enough for me to encounter the new warnings but then
started failing with the kind of weirdly nondeterministic Gradle errors
we've seen before:

```
BUG! exception in phase 'semantic analysis' in source unit '_BuildScript_' Unsupported class file major version 67
```
Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracking down the dependency errors @cpovirk!

I don't quite see the connection to JDK-8225377 you make in 806.
Shouldn't there be a separate javac issue if we manage to crash javac?

I've added #825 to run on JDK 22, 23-ea, and 24-ea.
On JDK 22 I see one jtreg failure and filed #826 for it. Otherwise, JDK 22 seems to work.

I tried a few things I found online to run gradle with one version of the JDK and the compiler with a newer version, but all my attempts failed.
If you know how to get gradle working on JDK 23 and JDK 24, I would appreciate a pointer.

@wmdietl
Copy link
Member

wmdietl commented Aug 2, 2024

As #825 seem to already successfully build, I'm not sure this PR actually changes anything for JDK 22.
Should the title be changed to JDK 23+?
Or is the dependency problem happening non-deterministically on JDK 22?

@cpovirk
Copy link
Author

cpovirk commented Aug 5, 2024

The reason that I cite JDK-8225377 is that the stack trace runs through addTypeAnnotationsToSymbol, which was added during that work.

As for the fact that the javac problem is a crash: Good point, thanks. I have started talking to @cushon about that. He agrees that the compiler shouldn't crash. Now we have to figure out what it should do instead (error, warning, -Xlint diagnostic, nothing?).

RE: JDK 22 behavior: Strange, I am definitely seeing problems with 22 consistently. Even with the current head of #825 (d88cb01), I can reproduce the problem with Temurin ("OpenJDK22U-jdk_x64_linux_hotspot_22.0.2_9.tar.gz," which exactly matches the CI value), both with ./gradlew nonJunitTests -x javadoc -x allJavadoc --console=plain --warning-mode=all (from the CI) and with plain ./gradlew assemble. In all that, I'm setting JAVA_HOME to point to the JDK 22. I don't know if that exactly matches what the CI does, but it seems very close, and I would expect any kind of use of the JDK 22 compiler to cause the same problem. Overall, I don't know how to square my results with the successful :javacutil:compileJava step of "Test cftests-nonjunit on JDK 22."

For Gradle setup, I have no hands-on knowledge. It seems in principle that the approach should look similar to this approach for testing under different versions, but I don't know if it works out that easily in practice.

@cpovirk
Copy link
Author

cpovirk commented Aug 5, 2024

Ah: The CI sets useJdk17Compiler=true. With that change, I can build successfully. I guess this is part of the general policy of settling on a fixed set of javac versions that the Checker Framework can build against. I don't know exactly what policy you want in general, but at least now we know where the difference comes from :)

@cpovirk
Copy link
Author

cpovirk commented Aug 5, 2024

...and in theory, that also answers your question about how to use a different JDK's compiler :)

@wmdietl
Copy link
Member

wmdietl commented Aug 6, 2024

@cpovirk Thanks for your comments!
In #825 I've refactored from useJdk17Compiler to a more flexible useJdkCompiler. This now tests on JDK 22 with the actual JDK 22 compiler and we do get the expected failure!
It should now also run on JDK 23/24. It uses JDK 21 to run gradle and then uses the gradle toolchain to switch to Java 23/24.
Unfortunately, this only switches the compiler version. The tests are executed on the same JVM as gradle is, so only JDK21.
Switching that out would be a bit more work.
Once gradle supports 23/24, that platform can be marked as non-experimental and then the test cases should also run on the newer platform.

I also added a new test that compiles with JDK 21 and then runs on the multiple other JDKs. This tests that the released EISOP CF, which is built with JDK 21, can execute on the different other JDKS.

Any comments here or on #825 would be welcome! I'll merge #825 first and then we'll see that this PR fixes the JDK 22 failures.
Thanks!

cpovirk and others added 2 commits August 6, 2024 12:11
This won't build until after that PR, but I haven't rebased onto it yet.
build.gradle Outdated Show resolved Hide resolved
@Ao-senXiong
Copy link
Member

@wmdietl there is one job still failling, looks like reltaed to permission on forked PR.

Maybe something like this is needed in the CI.yml.

permissions:
  contents: write
``

@wmdietl
Copy link
Member

wmdietl commented Aug 7, 2024

@cpovirk Thanks again for these fixes! Java 22 works now and 23/24 seem to start off well.
On Java 23 and 24 I see one Error Prone warning:

/home/runner/work/checker-framework/checker-framework/checker/src/main/java/org/checkerframework/checker/optional/OptionalVisitor.java:513: warning: [InvalidLink] `handleNestedOptionalCreation` is not known here. Should it be a reference to a method?
     * to construction of an Optional. Method {@link handleNestedOptionalCreation} does so.
                                              ^
    (see https://errorprone.info/bugpattern/InvalidLink)
  Did you mean '* to construction of an Optional. Method {@link #handleNestedOptionalCreation} does so.'?

The warning seems correct, but I'm wondering why it doesn't show up on earlier JDKs.

In a separate PR, I'll try to fix the setup to see this warning also on earlier JDKs, unless you know of why this only shows up on Java 23/24.

@wmdietl
Copy link
Member

wmdietl commented Aug 7, 2024

@wmdietl there is one job still failling, looks like reltaed to permission on forked PR.

I guess the assumption is that external PRs should not get write access to the repository, which makes sense.
However, I also don't quite understand what the dependency submission gives us...

@wmdietl
Copy link
Member

wmdietl commented Aug 7, 2024

In a separate PR, I'll try to fix the setup to see this warning also on earlier JDKs, unless you know of why this only shows up on Java 23/24.

In earlier JDKs, I do get the Error Prone warning when having an invalid link, but not when missing the #.
Maybe something in javadoc got stricter. I've fixed the warning in 1e4a6dd .

@wmdietl
Copy link
Member

wmdietl commented Aug 7, 2024

@wmdietl there is one job still failling, looks like reltaed to permission on forked PR.

I guess the assumption is that external PRs should not get write access to the repository, which makes sense. However, I also don't quite understand what the dependency submission gives us...

@Ao-senXiong Can you prepare a separate PR that disables the dependency submission job when in a fork? Or see what the benefit of that job is at all?

@Ao-senXiong
Copy link
Member

@wmdietl there is one job still failling, looks like reltaed to permission on forked PR.

I guess the assumption is that external PRs should not get write access to the repository, which makes sense. However, I also don't quite understand what the dependency submission gives us...

@Ao-senXiong Can you prepare a separate PR that disables the dependency submission job when in a fork? Or see what the benefit of that job is at all?

@wmdietl Opened a PR in #831

@wmdietl wmdietl changed the title Prepare build for JDK 22 (and a little for 23+). Fix build for JDK 22+ Aug 7, 2024
Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This fixes the builds on JDK 22+!

@wmdietl wmdietl merged commit 61d8e8e into eisop:master Aug 7, 2024
62 of 65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants