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

Propagate JAVA_TOOL_OPTIONS down to external tests in container #5498

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

judovana
Copy link
Contributor

Currently the external tests are swallowng the JAVA_TOOL_OPTIONS and thus the tested JDK does miss the global config. This pr should be fixing that

@judovana
Copy link
Contributor Author

@judovana
Copy link
Contributor Author

hi! I will be now away for a week, and @pmikova and @jandrlik are waiting for this change. Unless the concept is fundamentaly wrong and needs full revisit, pleae merge it. We are using JAVA_TOOL_OPTIONS to test different runtiem variants..

@sophia-guo
Copy link
Contributor

sophia-guo commented Aug 16, 2024

I'm not sure why we need to pass the global JAVA_TOOL_OPTIONS of the host environment to test containers. No matter what the host environment is tests will only run in the containers. Tests specific settings is set up by Image file( external/thirdparty/test.properties) and shell script test.sh (external/thirdparty/test.sh). If tests need special export JAVA_TOOL_OPTIONS setting, it should be done by test.sh?

Global one can be explicitly set by print_java_tool_options()

@judovana
Copy link
Contributor Author

JAVA_TOOL_OPTIONS is there to ensue, that you can orchestrate all your JVMs in same way. We use it eg to pass everywhere -XX+UseShenandoahGC and similar. BY not having it in contrainers means, that the JDK in container is tested differently then the jdsk on localhost. THis PR is removing this difference.

If you will disaprove this, then a different way how to pass global settings down through container is needed

@judovana
Copy link
Contributor Author

judovana commented Aug 17, 2024

If you will disaprove this, then a different way how to pass global settings down through container is needed

On top of that - whatever will be implement to workaround this, is simply reimplementing wheel. because this is exactly what JAVA_TOOL_OPTIONS is for, and unless there is real strong reason to not forward it in, the not forwarding it should be considered as bug.

@sophia-guo
Copy link
Contributor

That means tests will run with different JAVA_TOOL_OPTIONS on different environments but it's same for all other tests. Thanks @judovana

"\n" >> ${file}

echo -e "\nENV RANDFILE=/tmp/.rnd \\" \
"\n\t OPENJ9_JAVA_OPTIONS=\"-XX:+IgnoreUnrecognizedVMOptions -XX:+IdleTuningGcOnIdle -Dosgi.checkConfiguration=false\" " \
"\n\t OPENJ9_JAVA_OPTIONS=\"$OPENJ9_JAVA_OPTIONS -XX:+IgnoreUnrecognizedVMOptions -XX:+IdleTuningGcOnIdle -Dosgi.checkConfiguration=false\" " \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be $JAVA_TOOL_OPTIONS too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Merge now if need can be updated later.

@sophia-guo sophia-guo merged commit 6356956 into adoptium:master Aug 19, 2024
2 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.

3 participants