-
Notifications
You must be signed in to change notification settings - Fork 252
8342988: GHA: Build JTReg in single step #2996
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
Conversation
|
👋 Welcome back vieiro! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
This backport pull request has now been updated with issue from the original commit. |
jerboaa
left a comment
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.
There seems to be a problem of still building JTREG if it's not found in the cache in actions/get-jreg/action.yml. Instead, we should not look in the cache but use download-artifact as is being done on the JDK 17 version of this. Am I missing something?
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 still builds jtreg? Shouldn't it use actions/download-artifact@v4 to get the built version like is done on the JDK 17 version of this patch:
openjdk/jdk17u-dev@087e8a2#diff-3a815a14d4d572818da85d29537d07cf3199fc15a6db7eaa9427a73f024d23dcR42
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.
Hi @jerboaa . Thanks for the review!
The cache is checked against using the if: steps.get-cached-jtreg.outputs.cache-hit != 'true' , so no rebuilds are required if cached. But we're indeed missing the actions/download-artifact@v4 (even though the upload-artifact@v4 is in place).
I tried to make a minimum-change PR, for an easier review, so I missed this thunk.
Thinking of it I think I'll do a new PR with all changes in JDK17 (for instance, renaming the select job in 11 to prepare in 17, instead of keeping the JDK11 version). This will make the PR slightly more difficult to review, but we'll be in-sync with JDK17, so future changes will be easier to backport.
|
I'm afraid I won't be able to redo this anytime soon, so let's close it and reopen later on. |
Backport of JDK-8349829 from 17u to build jtreg and cache it for subsequent builds. Low risk (only GHA actions affected).
The backport is not clean as JDK-8338286 has not been backported to JDK-11, on purpose. Also
JAVA_HOME_11_X64is being used now to buildjtreg.As expected jtreg is now being built & cached in subsequent builds:
All tests pass but I detected an intermittent time- out in serviceability agent in macos (possibly because of JDK-8260389? despite JDK-8294316 having been already integrated. This is under investigation).
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/2996/head:pull/2996$ git checkout pull/2996Update a local copy of the PR:
$ git checkout pull/2996$ git pull https://git.openjdk.org/jdk11u-dev.git pull/2996/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2996View PR using the GUI difftool:
$ git pr show -t 2996Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/2996.diff
Using Webrev
Link to Webrev Comment