-
-
Notifications
You must be signed in to change notification settings - Fork 244
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
fix: Amend build.sh to remove platform specific information from sources archive #2931
Conversation
Fix common.sh to replace misleading archive comment.
Add additional if clause to source archive creation function to strip out platform specific information.
Thank you for creating a pull request! |
sbin/build.sh
Outdated
@@ -535,6 +535,9 @@ createSourceArchive() { | |||
# Transform 'OpenJDK11U-jdk_aarch64_linux_hotspot_11.0.12_7.tar.gz' to 'OpenJDK11U-sources_11.0.12_7.tar.gz' | |||
# shellcheck disable=SC2001 | |||
srcArchiveName="$(echo "${BUILD_CONFIG[TARGET_FILE_NAME]}" | sed 's/-jdk_.*_temurin_/-sources_/g')" | |||
elif |
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.
How did the first if
block miss this very specific case? I feel like we're special casing something here but I can't see why so to speak
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.
For JDK17, the file names being provided to this function does not contain the "temurin" string in order to hit the first clause, as the sources for release are built alongside the X64 linux hotspot build, the file names have the following format OpenJDK17U-sources_x64_linux_hotspot_2022-05-15-12-05.tar.gz. The second clause is to allow for this whereby the temurin name isnt included/constructed as part of the target file name parameter.
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.
The same naming convention is used across the JDK11 & JDK18 build pipelines also as seen in the most recent source artifacts here https://ci.adoptopenjdk.net/job/build-scripts/job/openjdk18-pipeline/lastSuccessfulBuild/artifact/target/linux/x64/temurin/ & here https://ci.adoptopenjdk.net/job/build-scripts/job/openjdk11-pipeline/lastSuccessfulBuild/artifact/target/linux/x64/temurin/
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.
I'm not sure we ever produce filenames with temurin
in the name, so I suspect we should just adjust the above if
statement to use _temurin
and not have this else
case (or just use wildcards e.g. -jdk._[a-zA-Z0-9]_ for the search string
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.
FYI This was introduced by me in the hotspot->temurin switch a few months back: https://github.com/adoptium/temurin-build/pull/2818/files so I'm good with changing this line instead of the elif
Update if clauses in CreateSourceArchive to remove grep for temurin, as this wasnt updated when default platfom was switched to hotspot.
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.
LGTM
…ces archive (adoptium#2931) Update if clauses in CreateSourceArchive to remove grep for temurin, as this wasn't updated when default platform was switched to hotspot.
fixes #2909