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

EXTRA_OPTIONS are appended to exisiting JVM options #4861

Merged
merged 2 commits into from
Nov 17, 2023
Merged

EXTRA_OPTIONS are appended to exisiting JVM options #4861

merged 2 commits into from
Nov 17, 2023

Conversation

annaibm
Copy link
Contributor

@annaibm annaibm commented Nov 9, 2023 •
edited
Loading

Append user specified jvmOpts to exisitng jvm options

resolves : #4849

llxia
llxia requested changes Nov 9, 2023
View reviewed changes
jck/jtrunner/JavaTestRunner.java Outdated Show resolved Hide resolved
@annaibm annaibm force-pushed the append_extraoptions branch from 3e0bb73 to 6c6b9a5 Compare November 9, 2023 19:11
@annaibm
Copy link
Contributor Author

annaibm commented Nov 10, 2023 •
edited
Loading

Grinder_JCK output:

1906

@annaibm annaibm marked this pull request as ready for review November 10, 2023 15:01
llxia
llxia requested changes Nov 10, 2023
View reviewed changes
jck/jtrunner/JavatestUtil.java
}
else {
jvmOpts = System.getProperty("jvm.options");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not quite correct. We need to check jvm.options as well.

if (System.getProperty("other.options") != null) {
   jvmOpts += System.getProperty("other.options").trim() + " ";
}
if (System.getProperty("jvm.options") != null) {
   jvmOpts += System.getProperty("jvm.options").trim() + " ";
}

jck/jtrunner/JavatestUtil.java Outdated
@@ -51,7 +51,7 @@ public class JavatestUtil {
private static String testExecutionType;
private static String withAgent;
private static String interactive;
private static String extraJvmOptions;
private static String extraJvmOptions= "";
Copy link
Contributor

Choose a reason for hiding this comment

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

add a space before =

jck/jtrunner/JavatestUtil.java

// Add the JVM options supplied by the user plus those added in this method to the jtb file option.
fileContent += "set jck.env.devtools.refExecute.otherOpts \" " + extraJvmOptions + " \"" + ";\n";
fileContent += "set jck.env.devtools.refExecute.otherOpts \" " + extraJvmOptions + " " + jvmOpts + " \"" + ";\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

aqa-tests/jck/jtrunner/JavatestUtil.java

Line 678 in 6c6b9a5

fileContent += "set jck.env.runtime.testExecute.otherOpts \" " + extraJvmOptions + " \"" + ";\n";
should be updated as well.

jck/jtrunner/JavaTestRunner.java Outdated
@@ -168,8 +168,7 @@ public static void main(String args[]) throws Exception {
}
}
}

jvmOpts = System.getProperty("jvm.options").trim() + " " + System.getProperty("other.opts");
jvmOpts = System.getProperty("other.options").trim() + " " + System.getProperty("jvm.options");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as JavatestUtil. This needs to be updated

jck/jtrunner/JavaTestRunner.java

// Add the JVM options supplied by the user plus those added in this method to the jtb file option.
fileContent += "set jck.env.devtools.refExecute.otherOpts \" " + extraJvmOptions + " \"" + ";\n";
fileContent += "set jck.env.devtools.refExecute.otherOpts \" " + extraJvmOptions + " " + jvmOpts + " \"" + ";\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

aqa-tests/jck/jtrunner/JavaTestRunner.java

Line 733 in 6c6b9a5

fileContent += "set jck.env.runtime.testExecute.otherOpts \" " + extraJvmOptions + " \"" + ";\n";
should be updated as well.

@annaibm annaibm marked this pull request as draft November 10, 2023 16:28
@annaibm
Copy link
Contributor Author

annaibm commented Nov 10, 2023 •
edited
Loading

Grinder Output:
JavatestUtil : 1907
JavaTestrunner: 1911

@annaibm annaibm marked this pull request as ready for review November 10, 2023 19:11
@annaibm annaibm force-pushed the append_extraoptions branch from 3491a95 to 2f90818 Compare November 10, 2023 21:11
@llxia
Copy link
Contributor

llxia commented Nov 10, 2023

@annaibm Please refer to https://github.com/eclipse-openj9/openj9/blob/master/CONTRIBUTING.md#commit-guidelines. We do not need to put code in the commit message. Also, could you update the description related: aqa-tests/issues/4849 above? Thanks

@annaibm
User specfic extra_options appended to JVM options
716d47e 
- Append user specified jvmOpts to exisitng jvm options

- Added the null pointer check for other options and jvm options

resolves : adoptium#4849

Signed-off-by: Anna Babu Palathingal <anna.bp@ibm.com>
@annaibm annaibm force-pushed the append_extraoptions branch from 2f90818 to 716d47e Compare November 10, 2023 21:17
llxia
llxia approved these changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

Thanks @annaibm

@llxia llxia requested a review from smlambert November 10, 2023 21:38
@smlambert
Copy link
Contributor

Publicly visible grinder: https://ci.adoptium.net/view/Test_grinder/job/Grinder/8026

@llxia
Copy link
Contributor

llxia commented Nov 17, 2023

@smlambert could you review this PR when you have time? Thanks

smlambert
smlambert approved these changes Nov 17, 2023
View reviewed changes
Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

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

thanks @annaibm !

@smlambert smlambert merged commit ec4ef4a into adoptium:master Nov 17, 2023
@annaibm
Copy link
Contributor Author

annaibm commented Nov 17, 2023

Thank you @llxia and @smlambert!

@annaibm annaibm deleted the append_extraoptions branch November 20, 2023 18:22
annaibm added a commit to annaibm/aqa-tests that referenced this pull request Nov 22, 2023
@annaibm
User specfic extra_options appended to JVM options (adoptium#4861)
37e1bc2 
- Append user specified jvmOpts to exisitng jvm options

- Added the null pointer check for other options and jvm options

resolves : adoptium#4849

Signed-off-by: Anna Babu Palathingal <anna.bp@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smlambert smlambert smlambert approved these changes

@llxia llxia llxia 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.

EXTRA_OPTIONS should append instead of prepend to the existing JVM options
3 participants
@annaibm @llxia @smlambert