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

added support for graalvm version strings without minor version #1234

Merged
merged 2 commits into from
Nov 19, 2023

Conversation

ennerf
Copy link
Contributor

@ennerf ennerf commented Nov 7, 2023

Substrate currently fails for some GraalVM builds that print versions without minor revision numbers, e.g., graalvm-jdk-21+35.1/bin/java -version

java version "21" 2023-09-19
Java(TM) SE Runtime Environment Oracle GraalVM 21+35.1 (build 21+35-jvmci-23.1-b15)
Java HotSpot(TM) 64-Bit Server VM Oracle GraalVM 21+35.1 (build 21+35-jvmci-23.1-b15, mixed mode, sharing)

This PR changes the Regex from GraalVM .*?(\d\d.\d.\d) to GraalVM (\d{1,2}(\.\d+){0,2}) (same as for the Java version) and adds unit tests for a variety of old and new GraalVM builds on different platforms

Current Regex

GraalVM .*?(\d\d.\d.\d)

image

Updated Regex

GraalVM (\d{1,2}(.\d+){0,2})

image

@ennerf ennerf mentioned this pull request Nov 9, 2023
3 tasks
Arguments.of(new Version("21.0.1"), new Version("21.0.1"), "java version \"21.0.1\" 2023-10-17\n" +
"Java(TM) SE Runtime Environment Oracle GraalVM 21.0.1+12.1 (build 21.0.1+12-jvmci-23.1-b19)\n" +
"Java HotSpot(TM) 64-Bit Server VM Oracle GraalVM 21.0.1+12.1 (build 21.0.1+12-jvmci-23.1-b19, mixed mode, sharing)"),

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: add a comment about GraalVM CE builds?

Copy link
Contributor

@jperedadnr jperedadnr Nov 15, 2023

Choose a reason for hiding this comment

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

I've tested against recent GraalVM 21, the changes from this PR are fine, but to complete the build it needs the changes from #1236

Copy link
Contributor

Choose a reason for hiding this comment

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

@kristofdho we'll integrate this PR before yours. Would be good if you can do another review?

Copy link
Contributor

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

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

Looks good, minor comment

@jperedadnr jperedadnr mentioned this pull request Nov 15, 2023
3 tasks
@jperedadnr jperedadnr requested review from jperedadnr and tiainen and removed request for johanvos and kristofdho November 16, 2023 19:04
Copy link
Contributor

@kkriske kkriske left a comment

Choose a reason for hiding this comment

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

LGTM

I might still look into removing code duplication in #1236 after this is merged, since both version parsers are running java -version. After my PR these are stored in the constructor so makes sense to only call that once.

Copy link
Contributor

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

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

looks good

@jperedadnr jperedadnr merged commit e812729 into gluonhq:master Nov 19, 2023
3 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.

4 participants