-
-
Notifications
You must be signed in to change notification settings - Fork 251
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
Archive the test image for after the fact testing. #1251
Conversation
I'm still trying to figure out how I can test this PR before merging. Hence, the WIP :) |
Can one of the admins verify this patch? |
In past, we have hijacked existing build jobs, and pointed it to the working branch, jerboaa:testimages, to test, I am happy to set that up to verify this if no one opposes. |
Ugh, sounds ugly. But I suppose better than not testing it at all :) Thanks for your help getting this set up. |
run tests |
Note that the build-scripts-pr-tester will probably not do entirely what you need atm as they do not run tests, but could enable tests on them temporarily to test your stuff. However they do show up your initial error. FYI the initial problem you have is that unfortunately you have to declare the members of the BUILD_CONFIG array, unfortunately its not just a map you can throw things in. I.e add |
@johnoliver Thanks for the pointers. Should be fixed now. Could you please retrigger a test? |
run tests |
Previous run failed on JDK 8 with:
Otherwise, testimage tar.gz archives get created as expected. |
run tests |
run tests |
This now seems to work correctly. Latest test-run: https://ci.adoptopenjdk.net/job/build-scripts-pr-tester/job/openjdk-build-pr-tester/164/ Please review! |
@@ -356,6 +356,8 @@ class Build { | |||
def type = "jdk" | |||
if (file.contains("-jre")) { | |||
type = "jre" | |||
} else if (file.contains("-testimage")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the change needed so that foo.tar.gz.json
metadata gets testimage
for binary_type
. It would be jdk
otherwise. Example: https://ci.adoptopenjdk.net/job/build-scripts-pr-tester/job/build-test/job/jobs/job/jdk11u/job/jdk11u-linux-x64-hotspot/135/artifact/workspace/target/OpenJDK11U-testimage_x64_linux_hotspot_2019-09-03-15-11.tar.gz.json
FWIW, that build passed. @johnoliver @karianna I'd appreciate a review when you get a chance. |
The change looks good, and works from test perspective. There is one codacy suggestion (https://app.codacy.com/app/AdoptOpenJDK/openjdk-build/pullRequest?prid=4071457) on this PR. Does the suggestion make sense for you to apply? |
run tests |
Thanks for the review!
Trying that now. I hope old bash versions (3.x?) understand this syntax. |
@smlambert Should be all set now. More thoughts? |
Closes #248