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

Add support for JDK 21+ #1712

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add support for JDK 21+ #1712

wants to merge 4 commits into from

Conversation

geoff-powell
Copy link
Collaborator

@geoff-powell geoff-powell commented Nov 21, 2024

closed #565

@@ -23,7 +23,7 @@ jobs:
windows-latest,
ubuntu-latest
]
java-version: [17, 18]
java-version: [17, 18, 22]
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider:

Suggested change
java-version: [17, 18, 22]
java-version: [17, 21, 22]

So that it definitely works on LTS 21. Testing only on 22 could result in the library using 22-only features that are not in 21.

build.gradle Outdated
Comment on lines 54 to 55
// Enabled dynamic agent loading (ByteBuddy) for JDK 21+ https://openjdk.org/jeps/451
jvmArgs '-XX:+EnableDynamicAgentLoading'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do consumers need to add this too?

(i.e. should this be part of the plugin, behind a version check: JavaVersion.current() or Test.javaVersion)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point let me update this 👍🏽

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about making it part of the Paparazzi Gradle Plugin? If I added Paparazzi to a project running CI on 22, would I need to add this line too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, I added this in my latest commit

Copy link
Contributor

Choose a reason for hiding this comment

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

note: if you have it in plugin, then is it still necessary in root build.gradle?

@geoff-powell geoff-powell marked this pull request as ready for review November 21, 2024 19:25
@geoff-powell
Copy link
Collaborator Author

verified gradle java sdk and compile java sdk to 22 builds successfully.
https://github.com/cashapp/paparazzi/actions/runs/11959970311/job/33342949222

build.gradle Outdated Show resolved Hide resolved
@geoff-powell geoff-powell force-pushed the gpowell/jdk-versions branch 2 times, most recently from f6b6214 to 4797778 Compare November 21, 2024 20:13
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.

java.lang.NoSuchFieldException: modifiers
2 participants