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

Set up jdk for jck interactive #4239

Merged
merged 2 commits into from
Feb 7, 2023
Merged

Set up jdk for jck interactive #4239

merged 2 commits into from
Feb 7, 2023

Conversation

sophia-guo
Copy link
Contributor

Signed-off-by: Sophia Guo [email protected]

@sophia-guo sophia-guo marked this pull request as draft January 13, 2023 15:21
@smlambert smlambert requested a review from llxia January 13, 2023 15:56
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 Sophia, we will continue to test and annotate https://github.com/temurin-compliance/temurin-compliance/issues/151 with results, but from preliminary runs, looks good.

@@ -469,6 +516,9 @@ def get_sources() {
sh "$GET_SH_CMD"
}
}
if (env.BUILD_LIST.contains('jck') && CUSTOMIZED_SDK_URL != "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I suggested checking CUSTOMIZED_SDK_URL was populated, but could also use SDK_RESOURCE.contains('customized').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even better. Updated

@@ -469,6 +516,9 @@ def get_sources() {
sh "$GET_SH_CMD"
}
}
if (env.BUILD_LIST.contains('jck') && CUSTOMIZED_SDK_URL != "") {
setup_jck_interactives()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

setup_jck_interactives() is an additional step added for preparing SDK for JCK interactives. Since it has nothing to do with automatic JCK test execution, I think this setup_jck_interactives() should happen after the test execution in the Post stage

or create its own stage after Post stage. Mixing the step in get_sources() will cause confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed putting it at the end. Why we decided not to was, if we wait to do this step in the post stage, due to the long running jck tests, it means that the setup might take several hours (in some cases 12+ hrs), which then reduces our ability to make our target release goals (2 days for primaries, 7 days for secondaries).

Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that for those not using this approach, the call to setup_jck_interactives() does nearly nothing (checks for existence of a directory that is not present and returns).

@sophia-guo sophia-guo force-pushed the getHack branch 2 times, most recently from e134851 to 1f4bf86 Compare January 13, 2023 19:12
@smlambert
Copy link
Contributor

From our Slack discussion, the request to add a flag, SETUP_JCK_RUN (false by default) to ensure this code does not run for others.

	if (env.BUILD_LIST.contains('jck') && SDK_RESOURCE.contains('customized') && (params.SETUP_JCK_RUN)) {
		setup_jck_interactives()
	}

In terms of setting the flag, for the upcoming release, either we prepare a PR for ci-jenkins-pipeline to adjust the remoteTrigger, or modify aqaTestPipeline.groovy to have that same param, which on Temurin-compliance server could be configured to be true by default.

cd ${jdkDir}
cp -r ${TEST_JDK_HOME}/* .
chgrp -R jck ${targetDir}/${jdkDir}
chmod -R 755 ${targetDir}/${jdkDir}
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be 775 so that jck users can delete the files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@sophia-guo
Copy link
Contributor Author

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.

I am fine with the changes as they are, but @llxia would like to see an explicit flag added (see #4239 (comment)) for extra safety. And of course to update to 775 as per Andrew's review.

@sophia-guo
Copy link
Contributor Author

Updated with extra SETUP_JCK_RUN flag or jenkins parameters. Eclipse temurin job AQA_Test_Pipeline updated

@sophia-guo
Copy link
Contributor Author

TODO: Eclipse temurin tests jobs need to regen.

@sophia-guo
Copy link
Contributor Author

Other enhancement like stage jdk on all machines with same label can be done separately.

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 @sophia-guo

@llxia llxia merged commit fbe94bc into adoptium:master Feb 7, 2023
sophia-guo added a commit to sophia-guo/openjdk-tests that referenced this pull request Mar 10, 2023
* Set up jdk for jck interactive

Signed-off-by: Sophia Guo <[email protected]>

* Add parameter SETUP_JCK_RUN explicitely

Signed-off-by: Sophia Guo <[email protected]>

---------

Signed-off-by: Sophia Guo <[email protected]>
sophia-guo added a commit to sophia-guo/openjdk-tests that referenced this pull request Mar 10, 2023
* Set up jdk for jck interactive

Signed-off-by: Sophia Guo <[email protected]>

* Add parameter SETUP_JCK_RUN explicitely

Signed-off-by: Sophia Guo <[email protected]>

---------

Signed-off-by: Sophia Guo <[email protected]>
smlambert added a commit that referenced this pull request Mar 10, 2023
* Set up jdk for jck interactive (#4239)

* Set up jdk for jck interactive

Signed-off-by: Sophia Guo <[email protected]>

* Add parameter SETUP_JCK_RUN explicitely

Signed-off-by: Sophia Guo <[email protected]>

---------

Signed-off-by: Sophia Guo <[email protected]>

* [Jtreg/FFI] Disable FFI related test suite in JDK20 (#4318)

The change is to exclude the FFI related test cases
in AttachTest.java captured at
eclipse-openj9/openj9#16656
given the FFI related code in JDK20 has been disabled
for the moment and will be enabled once the code has
been updated against the latest APIs.

Signed-off-by: ChengJin01 <[email protected]>

* Fix if condition for tck interactive setup (#4320)

Signed-off-by: Mesbah Alam <[email protected]>

* [Jtreg/FFI] update the issue no for FFI test suites in JDK20 (#4324)

The change updates the issue no by replacing
#16656 with #16565 as #16656 will be closed
as duplicate of #16565, in which case #16565
serves as the only issue that keeps track of
the excluded FFI test suites in JDK20.

Signed-off-by: ChengJin01 <[email protected]>

* Add OpenJ9 timeout handler to security and rmi extended tests (#4333)

Signed-off-by: Peter Shipton <[email protected]>

* Disable GetStackTraceSuspendedStressTest (#4360)

Related: eclipse-openj9/openj9#16751

Signed-off-by: Babneet Singh <[email protected]>

* Update JDK20 exclude list (#4371)

Re-enable Thread related tests fixed in JDK19

Signed-off-by: Jack Lu <[email protected]>

* Disable ContinuationTest (#4380)

ContinuationTest is being disabled for OpenJ9 since it is specific to
the reference implementation and it does not apply to OpenJ9.

Signed-off-by: Babneet Singh <[email protected]>

* Permanently disable ContinuationTest (#4382)

Refer to #1297 to permanently exclude.

Related: eclipse-openj9/openj9#16792
Related: #1297

Signed-off-by: Babneet Singh <[email protected]>

* Exclude jdk/internal/platform/docker/TestDockerCpuMetrics.java (#4387)

Signed-off-by: Jason Feng <[email protected]>

* AUTO: auto exclude test jdk_svc_sanity plat=ppc64_aix impl=hotspot (#4389)

- related: #4218 (comment)

Signed-off-by: GitHub <[email protected]>
Co-authored-by: smlambert <[email protected]>

---------

Signed-off-by: Sophia Guo <[email protected]>
Signed-off-by: ChengJin01 <[email protected]>
Signed-off-by: Mesbah Alam <[email protected]>
Signed-off-by: Peter Shipton <[email protected]>
Signed-off-by: Babneet Singh <[email protected]>
Signed-off-by: Jack Lu <[email protected]>
Signed-off-by: Jason Feng <[email protected]>
Signed-off-by: GitHub <[email protected]>
Co-authored-by: Cheng Jin <[email protected]>
Co-authored-by: Mesbah Alam <[email protected]>
Co-authored-by: Peter Shipton <[email protected]>
Co-authored-by: Babneet Singh <[email protected]>
Co-authored-by: Jack Lu <[email protected]>
Co-authored-by: Jason Feng <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: smlambert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants