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

Check testenv.properties JDKxx_BRANCH tags #5102

Merged
merged 27 commits into from
Feb 28, 2024

Conversation

smlambert
Copy link
Contributor

Fixes #4373
Add a script to check the existence of testenv.properties tags for the JDKxx_BRANCH values when USE_TESTENV_PROPERTIES=true. If the value is a valid branch or tag name, then use it, otherwise get the latest tag and use that.

A second pass of this should be done to cover the case where the value in the testenv.properties files are SHAs (instead of branch or tag names).

Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
@smlambert
Copy link
Contributor Author

smlambert commented Feb 27, 2024

Various tests

Grinder/8973 (JDK8, dev branch)

21:09:05  Running checkTags with ./testenv/testenv.properties and 8
21:09:05  Use branch name dev from ./testenv/testenv.properties

Grinder/8979 (JDK11, non-existent jdk-11.0.23-ga tag)

21:55:45  Running checkTags with ./testenv/testenv.properties and 11
21:55:46  Override non-existent jdk-11.0.23-ga with JDK11_BRANCH=jdk-11.0.23+4 and write to ./testenv/testenv.properties 

Grinder/8980 (JDK11, non-existent blahblah tag)

22:13:50  Running checkTags with ./testenv/testenv.properties and 11
22:13:51  Unable to resolve the latest JDK11_BRANCH=blahblah tag from JDK11_REPO

and exits immediately

Grinder/8982 (JDK17, with JDK17_BRANCH=latestSHA)

22:31:49  Running checkTags with ./testenv/testenv.properties and 17
22:31:49  Unable to resolve the latest JDK17_BRANCH=37cf29ab1f33c827b7cf67f44f7698ab8c292b88 tag from JDK17_REPO

does not yet handle this case well

because the SHA case needs work, shifting this PR to draft

Signed-off-by: Shelley Lambert <[email protected]>
@smlambert smlambert marked this pull request as draft February 27, 2024 03:35
Signed-off-by: Shelley Lambert <[email protected]>
@smlambert
Copy link
Contributor Author

Grinder/8983 (JDK17 with JDK17_BRANCH=latestSHA)

23:06:57  Running checkTags with ./testenv/testenv.properties and 17
23:06:59  SHA 37cf29ab1f33c827b7cf67f44f7698ab8c292b88 exists on https://github.com/adoptium/jdk17u.git

Grinder fails later, as presumably, we do not handle the case for getting the OpenJDK test material for a particular commit SHA at present. So this may come in as a separate PR.

Signed-off-by: Shelley Lambert <[email protected]>
Signed-off-by: Shelley Lambert <[email protected]>
@llxia
Copy link
Contributor

llxia commented Feb 27, 2024

From the console output, cmd for a branch is expected (see below). We need to update openjdk logic.

23:07:21       [exec] git clone --depth 1 -q --reference-if-able /home/jenkins/openjdk_cache -b 37cf29ab1f33c827b7cf67f44f7698ab8c292b88 https://github.com/adoptium/jdk17u.git openjdktemp
23:07:21       [exec] info: Could not add alternate for '/home/jenkins/openjdk_cache': reference repository '/home/jenkins/openjdk_cache' is not a local repository.
23:07:22       [exec] error code: 128. Sleep 300 secs, then retry 1...
23:07:22       [exec] warning: Could not find remote branch 37cf29ab1f33c827b7cf67f44f7698ab8c292b88 to clone.

@smlambert
Copy link
Contributor Author

Given #5102 (comment), I will not worry about the SHA edge case for this PR. This PR does not break anything, since the functionality is not there. This PR does goes as far as identifying when we have a SHA which is good, and we can address the update to the logic in the openjdk/build.xml and openjdk.mk file in a subsequent PR.

@smlambert smlambert marked this pull request as ready for review February 27, 2024 22:07
@smlambert smlambert requested a review from sophia-guo February 28, 2024 16:53
@sophia-guo
Copy link
Contributor

@ShelleyLambert just to double check the following scenario of

Grinder/8979 (JDK11, non-existent jdk-11.0.23-ga tag)

There is the output of Override non-existent jdk-11.0.23-ga with JDK11_BRANCH=jdk-11.0.23+4 and write to ./testenv/testenv.properties , which is contradictory to originally story 21:57:28 USE_TESTENV_PROPERTIES was set, not writing to testenv.properties. Might need to update some print message? And if the ./testenv/testenv.properties is overwritten do we still need to manually create the PR to do the update? Otherwise the overwritten will be done everytime called during release?

@smlambert
Copy link
Contributor Author

smlambert commented Feb 28, 2024

There is the output of Override non-existent jdk-11.0.23-ga with JDK11_BRANCH=jdk-11.0.23+4 and write to ./testenv/testenv.properties , which is contradictory to originally story 21:57:28 USE_TESTENV_PROPERTIES was set, not writing to testenv.properties. Might need to update some print message? And if the ./testenv/testenv.properties is overwritten do we still need to manually create the PR to do the update? Otherwise the overwritten will be done everytime called during release?

This PR was created so that we will no longer need to manually update the testenv.properties file. One less step in release checklist!

The bit of code below, which updates the contents of the testenv.properties call only gets called in a very specific case, it is called when all of the following criteria are met:

  • the current value for JDKxx_BRANCH in the testenv.properties file is a tag that does not exist (example if this entry is in the properties file, JDK11_BRANCH=jdk-11.0.23-ga which doesn't exist)
  • we strip off the -ga substring of the tag value (if it contains -ga), take the remaining string and try and fetch the latest tag that starts with that substring (example, take the prefix of the non-existent tag, jdk-11.0.23 and find the latest tag with that prefix)
        if [[ $latestTag ]]; then
          echo "Override non-existent $workingTag with $branchFromPropFile=$latestTag and write to $teFile "
          export $branchFromPropFile=$latestTag
          setProperty $branchFromPropFile $latestTag 

if $latestTag is not found, we do not write it to testenv.properties file, we exit / fail.

You are correct that I should update the output message to state why in this case we are writing to the testenv.properties file when we said we would not be doing so, if USE_TESTENV_PROPERTIES=true. testenv.properties should contain the property values that were used in the test run. When USE_TESTENV_PROPERTIES=false, we grab the SHAs that were used and write them to the file. When USE_TESTENV_PROPERTIES=true and we find that the tag value does not exist, but we find a suitable fallback tag and proceed to use it, we need to capture that in the testenv.properties file.

Let me see if I can improve the print message a bit.

Signed-off-by: Shelley Lambert <[email protected]>
@smlambert
Copy link
Contributor Author

@sophia-guo - I have updated my output message, and can create a TKG pull request to change the wording of the other output message, which could read something like "USE_TESTENV_PROPERTIES was set, not overwriting the testenv.properties file with SHAs"

@sophia-guo sophia-guo merged commit d9b0387 into adoptium:master Feb 28, 2024
1 check 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.

Automate or semi-automate update of testenv.properties file
3 participants