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.

Sign up for GitHub

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

Jump to bottom

Compensate for letters in jck version string #4635

Merged
merged 2 commits into from
Jun 21, 2023
Merged

Compensate for letters in jck version string #4635

merged 2 commits into from
Jun 21, 2023

Conversation

adamfarley
Copy link
Contributor

This change should prevent the trailing letters in new jck version strings from causing errors.

Test run: https://ci.eclipse.org/temurin-compliance/view/JCK11/job/Test_openjdk11_hs_extended.jck_aarch64_linux/51/console

@adamfarley
Copy link
Contributor Author

Error found in test run:

09:35:46  Exception in thread "main" java.lang.Error: Unexpected jck version : jck11a
09:35:46  	at JavaTestRunner.getTestSpecificJvmOptions(JavaTestRunner.java:1277)
09:35:46  	at JavaTestRunner.jckConfigurationForMultijvmWithNoAgent(JavaTestRunner.java:722)
09:35:46  	at JavaTestRunner.generateJTB(JavaTestRunner.java:418)
09:35:46  	at JavaTestRunner.main(JavaTestRunner.java:178)

The code seems to balk at the letter "a", as it only allows b or c. I've modified the code to allow the full alphabet, and to simplify the regex.

Test run: https://ci.eclipse.org/temurin-compliance/view/JCK11/job/Test_openjdk11_hs_extended.jck_aarch64_linux/52/console

@adamfarley adamfarley force-pushed the fix_int_parsing_in_jdkversion_interpreter branch from a173f2c to 45f093a Compare June 21, 2023 09:16
@adamfarley adamfarley changed the title WIP: Compensate for letters in jck version string Compensate for letters in jck version string Jun 21, 2023
sxa
sxa approved these changes Jun 21, 2023
View reviewed changes
Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

I'd probably go for [a-z]? rather than [a-z]* since I would not expect us to have multiple suffix letters, but I'm happy that this will work regardless so approving.

Noting that this is a follow up fix to #4601

@adamfarley
Copy link
Contributor Author

Having the jck version turned into an int in many different places seems messy. Let's see if we can set it once as a global variable.

@adamfarley adamfarley force-pushed the fix_int_parsing_in_jdkversion_interpreter branch 2 times, most recently from 71939a0 to 4919420 Compare June 21, 2023 11:35
@adamfarley
Copy link
Contributor Author

Ok, ready for review.

Note: the big changes to the "execute" and "getTestSpecificJvmOptions" functions should just be formatting changes. I reversed a couple of if statements so they didn't indent the entire remainder of the method.

Though, tbh, the entire getTestSpecificJvmOptions "jckVerNum < 11" section happens within a "> 8" section, so we could probably remove that entire section unless we plan on building 9 or 10 again.

@adamfarley
Copy link
Contributor Author

Rerunning the test here to ensure the formatting tweaks didn't break anything:

https://ci.eclipse.org/temurin-compliance/view/JCK11/job/Test_openjdk11_hs_extended.jck_aarch64_linux/53/

andrew-m-leonard
andrew-m-leonard approved these changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@andrew-m-leonard andrew-m-leonard left a comment

Choose a reason for hiding this comment

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

Assuming builds and runs ok..?

@adamfarley adamfarley force-pushed the fix_int_parsing_in_jdkversion_interpreter branch from 4919420 to 87e5c9e Compare June 21, 2023 11:57
@adamfarley
Compensate for letters in jck version string
819dfea 
This change should prevent the trailing letters in new jck version
strings from causing errors.

This change also includes some formatting changes intended
to aid readability. These should not change the functional
behaviour of the code.

Signed-off-by: Adam Farley <adfarley@redhat.com>
@adamfarley adamfarley force-pushed the fix_int_parsing_in_jdkversion_interpreter branch from 87e5c9e to 819dfea Compare June 21, 2023 12:06
@adamfarley
Copy link
Contributor Author

Typo. Fixed and re-running test.

https://ci.eclipse.org/temurin-compliance/view/JCK11/job/Test_openjdk11_hs_extended.jck_aarch64_linux/54/

@adamfarley
Copy link
Contributor Author

Test has passed, no errors. Ready for merge. Thanks for the feedback!

@andrew-m-leonard andrew-m-leonard merged commit ee588d3 into adoptium:master Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sxa sxa sxa approved these changes

@andrew-m-leonard andrew-m-leonard andrew-m-leonard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamfarley @sxa @andrew-m-leonard