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

Support JDK EA builds in JavaVersionDetector #279

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

Conversation

estekhin
Copy link

@estekhin estekhin commented Apr 5, 2024

The (current) scheme for JDK EA builds is <major>-ea.

The first commit extracts getJavaMajorVersion(String) to make it testable.
The second commit adds test cases for version string for the current LTS builds and the EA build (the EA test case fails).
The third commit adds EA detection and fixes the EA test case.

This PR should close #275 and close #277.

Copy link

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

This seems like a good patch. Especially since it allows for testing various strings. The case of java.version containing - would need a more generic solution, I think. Details on how java.version can look like is in: https://openjdk.org/jeps/223 In particular this:

Name                            Syntax
------------------------------  --------------
java.version                    $VNUM(\-$PRE)?  

Comment on lines 17 to 18
} else if (javaVersion.endsWith("-ea")) {
normalizedJavaVersion = javaVersion.substring(0, javaVersion.length() - 3);
Copy link

Choose a reason for hiding this comment

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

Temurin 23 EA builds have java.version == 23-beta. For example:

jdk-23+15/bin/java -XshowSettings:properties --version 2>&1 | grep java\\.version
    java.version = 23-beta
    java.version.date = 2024-09-17

This would need to be handled as well.

Copy link
Author

Choose a reason for hiding this comment

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

Added a test for -beta, and replaced the explicit -ea handling with uniform handling for -<anything>.

Copy link

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

LGTM.

@estekhin
Copy link
Author

estekhin commented Apr 9, 2024

@johanhaleby can you check this please?

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