Add Valhalla Nestmates PR Build #1751

taliamccormick · 2018-04-24T00:45:49Z

This commit adds an automated Valhalla Nestmates PR build which compiles the VM with the J9VM_OPT_VALHALLA_NESTMATES build flag enabled and runs the Valhalla tests against it.

Test playlists will be updated in a subsequent commit to include the other sanity tests. Test config will be updated in a subsequent commit to update the Valhalla nestmates test options.

Signed-off-by: Talia McCormick Talia.McCormick@ibm.com

the VM with the J9VM_OPT_VALHALLA_NESTMATES build flag enabled and runs the Valhalla tests against it. Test playlists will be updated in a subsequent commit to include the other sanity tests. Test config will be updated in a subsequent commit to update the Valhalla nestmates test options. Signed-off-by: Talia McCormick <Talia.McCormick@ibm.com>

taliamccormick · 2018-04-24T00:48:44Z

Requesting review from @tajila @AdamBrousseau @smlambert @llxia

taliamccormick · 2018-04-24T00:52:19Z

This requires #1716 to handle the asm7 dependency

Tested here: https://ci.eclipse.org/openj9/job/PullRequest-Sanity-JDKValhalla-linux_ppc-64_cmprssptrs_le_valhalla_nestmates-OpenJ9/21/

Note that the Valhalla tests currently skipped because they were set up on linux x86 64 and this build is on ppc 64 le. Will update test in subsequent commit

Skipped due to jvm options ( -Xcompressedrefs ) and/or platform requirements (os.linux,arch.x86,bits.64) => NestAttributeTest_0_SKIPPED

smlambert · 2018-04-24T01:47:23Z

buildenv/jenkins/common/variables-functions

+        JAVA_VERSION = "Valhalla"
+    } else {
+        JAVA_VERSION = "SE" + "${SDK_VERSION}" + "0"
+    }


Do you need to set this variable here (change this file at all?), would you not just set this variable where you set valhalla_nestmates specs?

I think this is the correct spot as this is the only spot we set JAVA_VERSION (which is only used for test purposes). If we set it in the top level PR file then we would have to set it in every top level file we end up adding.

Possible options include:

Altering set_test_variables() to set JAVA_VERSION as Valhalla, based off of the spec. Then whenever set_job_variables() is run and the test variables are required, set_test_variables() will be called and the JAVA_VERSION will be set as needed.

Resetting JAVA_VERSION to Valhalla immediately after my call to set_test_variables(). Then whenever set_job_variables() is run, the JAVA_VERSION will initially be set as SE90/SE100 but redefined to Valhalla before it is used. (Setting it prior to running set_job_variables() would end with it being overwritten).

Ignoring the existence of set_job_variables() and defining all of the variables individually OR defining some sort of set_valhalla_job_variables() function.

I chose & prefer 1 for the following reasons

It makes use of the existing functionality within the jenkins build scripts

It might scale slightly better if/when Valhalla Lworld / Panama PR builds are added. I'm also adding a non-PR Valhalla nestmates build after this.

Setting JAVA_VERSION in only one place can make debugging easier

Is there another place/option for setting it and are there other factors that I'm missing?

Right, so stick with 1.

I'd like to see a pass of simplification on the pipeline scripts separate from this PR.

The set_test_variables() name is confusing as the method only sets JAVA_VERSION (RELEASE and JDK_FOLDER are not directly used by testing, must be mapped to the variables that tests expect... somewhere else).

I had an expectation to set the few environment variables needed for test in one spot, rather than scattered around the different pipeline functions. In that way, the pipeline code maps more closely to the standalone run on command line story, making it more readable/understandable/maintainable by developers who have run testing locally. We should have the goal of less levels of indirection.

I would suggest creating a naming standard for specs that gets followed, so this change doesn't become a giant if else, else else statement as it gets scaled up. You could change the regex to match on any spec with valhalla in it to set version Valhalla, and not have to keep growing the if statement.

I'll change the regex to check that it matches any spec whose name contains 'valhalla' instead of any check which ends in `valhalla_nestmates'

It looks like the other test variables are set within buildenv/jenkins/common/test and that the value of JAVA_BIN has to be set there. This is because set_job_variables() is called on the master machine and the JAVA_BIN value refers to a location on the node machine. The JAVA_BIN location is on the node machine because the tests run on the node. Since the JAVA_BIN is set as

JAVA_BIN = "${WORKSPACE}/${BUILD_RELEASE_IMAGES}${JDK_FOLDER}${JRE_FOLDER}/bin"

and the value of WORKSPACE is different between the two machines, it can't be set while running set_job_variables on the master one.

I could try copying/moving the test variable definitions from the variables-functions script so that they run on the node machine and can be set together. However, this will cause some duplication since some variables also need to be set on the master machine

RELEASE and JDK_FOLDER are used in JAVA_BIN which tests need. JAVA_BIN is set in the test file which is an exception and it is noted in a code comment. Assigning these values was moved to their own functions because we set them in Build jobs as well and did not want to repeat the code.

So within this PR, I will add another commit with a comment in set_test_variables() which indicates/clarifies that JAVA_BIN is set elsewhere and then open an issue for further discussion?

smlambert · 2018-04-24T01:48:19Z

...ins/jobs/pull-requests/PullRequest-Sanity-JDK9-linux_ppc-64_cmprssptrs_le_valhalla_nestmates

+
+SDK_VERSION = '10'
+SPEC = 'linux_ppc-64_cmprssptrs_le_valhalla_nestmates'
+TEST_TARGET = '_sanity'


JAVA_VERSION = Valhalla
if you set it here, you do not need to change variables-functions, correct?

smlambert · 2018-04-24T01:49:20Z

...ins/jobs/pull-requests/PullRequest-Sanity-JDK9-linux_ppc-64_cmprssptrs_le_valhalla_nestmates

+    buildfile.build_pr()
+
+    // Test
+    archive_test = [:]


what do these 3 calls to archive_test do? frustrating to have to find where those functions are defined and what each call does.

smlambert · 2018-04-24T01:54:20Z

buildspecs/linux_ppc-64_cmprssptrs_le_valhalla_nestmates.spec

+	<defaultSizes>desktop (256M + big OS stack)</defaultSizes>
+	<priority>150</priority>
+	<owners>
+		<owner>Talia.McCormick@ibm.com</owner>


do we still use these owners tag?

smlambert · 2018-04-24T01:56:15Z

buildspecs/linux_ppc-64_cmprssptrs_le_valhalla_nestmates.spec

+		<feature id="se"/>
+		<feature id="se60_26"/>
+		<feature id="se7"/>
+		<feature id="se70_27"/>


are all of these features still valid/used?
why se 6.26, 7, 7.27 and not 8/9/10?

smlambert · 2018-04-24T01:56:51Z

buildspecs/linux_ppc-64_cmprssptrs_le_valhalla_nestmates.spec

+		<feature id="se70_27"/>
+	</features>
+	<source>
+		<project id="com.ibm.jvmti.tests"/>


is this PR valid without jvmtitest.so / native tests compiled?

llxia · 2018-04-24T02:00:53Z

...ins/jobs/pull-requests/PullRequest-Sanity-JDK9-linux_ppc-64_cmprssptrs_le_valhalla_nestmates

+ * SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 OR LicenseRef-GPL-2.0 WITH Assembly-exception
+ *******************************************************************************/
+
+SDK_VERSION = '10'


is this for JDK9 or 10? The file name and SDK version are inconsistent.

llxia · 2018-04-24T02:03:20Z

...ins/jobs/pull-requests/PullRequest-Sanity-JDK9-linux_ppc-64_cmprssptrs_le_valhalla_nestmates

+}
+
+node("${NODE}") {
+    echo "creating node23"


should this be removed?

Woops, missed that - thank you

taliamccormick · 2018-04-24T12:11:21Z

@smlambert

Do you need to set this variable here (change this file at all?), would you not just set this variable where you set valhalla_nestmates specs?

JAVA_VERSION = Valhalla
if you set it here, you do not need to change variables-functions, correct?

I am trying to minimize script duplication and keep this pull request build similar to the others so that it's less likely to be broken during future refactoring. If I set it at the same time as SDK_VERSION , SPEC, and TEST_TARGET , then it will be reset by the call to set_job_variables - so I would have to either duplicate or change portions of set_job_variables. I will try setting it immediately after the call to set_job_variables and update here if that works.
Your approach should scale better, given that I've been told we are also adding a Valhalla LWorld PR build, Panama PR build, and a Valhalla nestmates non-PR build/test job.

taliamccormick · 2018-04-24T12:22:04Z

@smlambert

what do these 3 calls to archive_test do? frustrating to have to find where those functions are defined and what each call does.

wrt

    archive_test = [:]
    archive_test['Archive'] = { buildfile.archive() }
    archive_test['Test'] = { testfile.test_all() }
    parallel archive_test

Setting archive_test as a list of function class (archive() and test_all) lets us run the two in parallel. archive from buildenv/jenkins/common/build tars/archives the images from the build. test_all from buildenv/jenkins/common/test handles the test - it runs make -f configure.mk. gets dependent jars, compiles, runs the tests, publishes the TAP results, etc.

taliamccormick · 2018-04-24T12:35:15Z

@smlambert

do we still use these owners tag?

are all of these features still valid/used?
why se 6.26, 7, 7.27 and not 8/9/10?

I think that the owners tag is still used. I imitated the exisiting ppcle spec for the format/content and changed what was necessary to enable the nestmates pr build. The last time a spec was added (5f6f381#diff-ecf00c5c9b02bcdf4d8c6f300c21ec43), it included the owners tag. Same reason for features 6.26, 7, 7.27 - was not sure what they are for so I kept them to avoid breaking anything.

is this PR valid without jvmtitest.so / native tests compiled?

It should be. As of right now, the jvmti tests (and non Valhalla specific sanity tests) are not being run against the PR build. In a second separate PR I will update the playlist.xml files so that the jvmti tests (and non Valhalla specific sanity tests) are run against the PR build.

taliamccormick · 2018-04-24T12:38:22Z

@llxia

is this for JDK9 or 10? The file name and SDK version are inconsistent.

Nestmates is for JDK 11. Since the openj9-openjdk-jdk11 repo is not yet out, I'm temporarily using JDK 10 which requires the SDK version to be 10. File name was accidental, will correct

- Clarify nestmates are for JDK11 - Remove accidental debug comment Signed-off-by: Talia McCormick <Talia.McCormick@ibm.com>

AdamBrousseau · 2018-04-24T13:34:38Z

The correct/only way to add a spec is to copy the most similar spec and change only what you need. Unfortunately trying to trim down a spec is a rabbit hole. The hope is that the specs will be obsoleted by cmake.

Signed-off-by: Talia McCormick <Talia.McCormick@ibm.com>

smlambert · 2018-04-26T18:29:38Z

This looks ready to be merged, which I can do.

There is still one piece that should be changed/completed, which would be the trigger used to launch the PR testing. Right now it is "Jenkins test experimental plinux jdkvalhalla", etc., but should drop the experimental key word (as there is no test level called experimental) and be able to run either sanity or extended levels. I believe this change is not done in a repo, but in some configuration view of Jenkins.

smlambert · 2018-04-26T18:49:04Z

thanks @taliamccormick and @AdamBrousseau for your work on this
and updating the trigger to "Jenkins test sanity plinux jdk11-nestmates", etc.

smlambert · 2018-04-26T19:32:11Z

Will merge this now, understanding that we will update this to jdk11 once that repo is available (in a week or two).

taliamccormick force-pushed the valhalla_nestmates_pr_build branch from b5fb544 to d4df8aa Compare April 24, 2018 00:47

smlambert reviewed Apr 24, 2018

View reviewed changes

llxia reviewed Apr 24, 2018

View reviewed changes

Update per review

6790fd4

- Clarify nestmates are for JDK11 - Remove accidental debug comment Signed-off-by: Talia McCormick <Talia.McCormick@ibm.com>

pshipton added the comp:build label Apr 24, 2018

AdamBrousseau approved these changes Apr 25, 2018

View reviewed changes

Talia McCormick added 3 commits April 25, 2018 17:44

Pattern match to contain 'valhalla' instead of suffix 'valhalla

f700a53

Signed-off-by: Talia McCormick <Talia.McCormick@ibm.com>

add comment to clarify setting of JAVA_BIN

fb7d7b0

Signed-off-by: Talia McCormick <Talia.McCormick@ibm.com>

Pattern match to contain 'valhalla' instead of suffix 'valhalla

69a2e32

Signed-off-by: Talia McCormick <Talia.McCormick@ibm.com>

smlambert mentioned this pull request Apr 26, 2018

Refactor Valhalla nestmates tests #1716

Merged

smlambert approved these changes Apr 26, 2018

View reviewed changes

smlambert merged commit 998cba8 into eclipse-openj9:master Apr 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Valhalla Nestmates PR Build #1751

Add Valhalla Nestmates PR Build #1751

taliamccormick commented Apr 24, 2018

taliamccormick commented Apr 24, 2018 •

edited

Loading

taliamccormick commented Apr 24, 2018

smlambert Apr 24, 2018

AdamBrousseau Apr 24, 2018

taliamccormick Apr 25, 2018

smlambert Apr 25, 2018 •

edited

Loading

taliamccormick Apr 25, 2018

AdamBrousseau Apr 25, 2018

taliamccormick Apr 25, 2018

smlambert Apr 24, 2018

smlambert Apr 24, 2018

smlambert Apr 24, 2018

smlambert Apr 24, 2018

smlambert Apr 24, 2018

llxia Apr 24, 2018

llxia Apr 24, 2018

taliamccormick Apr 24, 2018

taliamccormick commented Apr 24, 2018

taliamccormick commented Apr 24, 2018

taliamccormick commented Apr 24, 2018

taliamccormick commented Apr 24, 2018

AdamBrousseau commented Apr 24, 2018

smlambert commented Apr 26, 2018 •

edited

Loading

smlambert commented Apr 26, 2018

smlambert commented Apr 26, 2018

Add Valhalla Nestmates PR Build #1751

Add Valhalla Nestmates PR Build #1751

Conversation

taliamccormick commented Apr 24, 2018

taliamccormick commented Apr 24, 2018 • edited Loading

taliamccormick commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smlambert Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taliamccormick commented Apr 24, 2018

taliamccormick commented Apr 24, 2018

taliamccormick commented Apr 24, 2018

taliamccormick commented Apr 24, 2018

AdamBrousseau commented Apr 24, 2018

smlambert commented Apr 26, 2018 • edited Loading

smlambert commented Apr 26, 2018

smlambert commented Apr 26, 2018

taliamccormick commented Apr 24, 2018 •

edited

Loading

smlambert Apr 25, 2018 •

edited

Loading

smlambert commented Apr 26, 2018 •

edited

Loading