Update parsing TEMURIN_BUILD_ARGS #3824
Conversation
Signed-off-by: Sophia Guo <[email protected]>
Signed-off-by: Sophia Guo <[email protected]>
Signed-off-by: Sophia Guo <[email protected]>
|
Grinder runs extremely long. Seems like doing some unexpected steps Is this related with #3746? @andrew-m-leonard |
|
Linter also unhappy. |
@sophia-guo yes it is, the strace anaylsis was incorrectly analysing openjdk source and due to the large number of files it takes ages! |
Signed-off-by: Sophia Guo <[email protected]>
NO need to install strace and corresponding docker run options for strace Change to /bin/bash Signed-off-by: Sophia Guo <[email protected]>
|
Turn off strace for reproducing build |
|
Lots of binary diff. @andrew-m-leonard would it be related with turn off strace? PR is ready. |
no strace should have no affect. I've done some analysis, and it looks like the Grinder test build is using the wrong gcc compiler: vs the original build: |
| } | ||
|
|
||
| setBuildArgs() { | ||
| local CONFIG_ARGS=("--disable-warnings-as-errors" "--enable-dtrace" "--without-version-pre" "--without-version-opt" "--with-version-opt") |
There was a problem hiding this comment.
We should be using the original build configure_args, not specifying these...?
There was a problem hiding this comment.
we need to fix the SBOM makejdk_any_platform_args which is losing the --configure-args "quotes"...
replace the code here:
temurin-build/makejdk-any-platform.sh
Line 54 in 39a0b00
with this, I "think" should fix that:
# Store params to this script as "buildinfo"
makeJdkArgs=""
for arg in "$@"; do
# Quote the argument if it contains spaces
if [[ "${arg}" =~ .*" ".* ]]; then
makeJdkArgs="${makeJdkArgs} \"${arg}\""
else
makeJdkArgs="${makeJdkArgs} ${arg}"
fi
done
echo "${makeJdkArgs}" > ./workspace/config/makejdk-any-platform.args
There was a problem hiding this comment.
yes just tested it, seems to do the right thing, you can then hopefully consume configure_args with this fix:
{
"name" : "makejdk_any_platform_args",
"value" : "--clean-git-repo --jdk-boot-dir /home/jenkins/temurin-build/jdk-22 --configure-args \" --disable-warnings-as-errors --with-version-opt=LTS --with-version-pre=ea\" --target-file-name jdk-hotspot.tar.gz --release --clean-libs --tag jdk-23+24_adopt --create-sbom --user-openjdk-build-root-directory /home/jenkins/build2 --use-adoptium-devkit gcc-11.3.0-Centos7.6.1810-b03 --freetype-dir bundled --use-jep319-certs --create-debug-image --build-variant temurin jdk"
},
There was a problem hiding this comment.
I'm not sure about this --configure-args. will this work for other cases? Like the value of it may be variable as --disable-warnings-as-errors --with-version-opt=LTS --enable-dtrace --without-version-pre?
There was a problem hiding this comment.
@sophia-guo i've created a PR to fix this, so we can get some builds you can test this with...
#3835
|
|
||
| setBuildArgs() { | ||
| local CONFIG_ARGS=("--disable-warnings-as-errors" "--enable-dtrace" "--without-version-pre" "--without-version-opt" "--with-version-opt") | ||
| local NOTUSE_ARGS=("--configure-args" "--enable-sbom-strace") |
There was a problem hiding this comment.
--configure-args should not be being ignored, only --enable-sbom-strace
There was a problem hiding this comment.
--configure-args is not ignored. It basically uses the same logic as window repro_build code. it was added back final_params="$build_string --configure-args \"$config_string\" $jdk". The idea is do with --configure-args differently as it need the double-quoted.
There was a problem hiding this comment.
Yes, you should not need this if we fix the SBOM --configure-args BUILD_ARG, which is currently missing the double-quotes
The problem here is we are adding assumptions about what configure args are expected, we should just pass them through.
The Windows logic is wrong too.
| # Check if fixed_param is in CONFIG_ARGS | ||
| if containsElement "$fixed_param" "${CONFIG_ARGS[@]}"; then | ||
| # Add Config Arg To New Array | ||
| if [ "$fixed_param" == "--with-version-opt" ] ; then |
There was a problem hiding this comment.
shouldn't be needed, use original --configure-args
| @@ -84,8 +84,116 @@ setEnvironment() { | |||
| ls -ld "$CC" "$CXX" "/usr/lib/jvm/jdk-${BOOTJDK_VERSION}/bin/javac" || exit 1 | |||
There was a problem hiding this comment.
split this method into separate:
setGccEnvironment()
setAntEnvironment()
Also remove check on javac, as it might not be located in /usr/lib/jvm/jdk-
| NATIVE_API_ARCH=$(uname -m) | ||
| if [ "${NATIVE_API_ARCH}" = "x86_64" ]; then NATIVE_API_ARCH=x64; fi | ||
| if [ "${NATIVE_API_ARCH}" = "armv7l" ]; then NATIVE_API_ARCH=arm; fi | ||
|
|
||
| checkAllVariablesSet | ||
|
|
||
| downloadTooling | ||
| setEnvironment |
There was a problem hiding this comment.
split this into two:
setAntEnvironment()
if [[ NOT_USING_DEVKIT ]]; then
setGccEnvironment()
fi
There was a problem hiding this comment.
in downloadTooling() add DEVKIT check... :
if [[ NOT_USING_DEVKIT ]]; then
if [ ! -r "${LOCALGCCDIR}/bin/g++-${GCCVERSION}" ]; then
echo "Retrieving gcc $GCCVERSION" && curl "https://ci.adoptium.net/userContent/gcc/gcc$(echo "$GCCVERSION" | tr -d .).$(uname -m).tar.xz" | (cd /usr/local && tar xJpf -) || exit 1
fi
fi
There was a problem hiding this comment.
@andrew-m-leonard how is NOT_USING_DEVKIT defined? Is it defined by following options?
if [[ "${CONFIGURE_ARGS}" =~ .*"--with-devkit=".* ]]; then
echo "Using gcc from DevKit toolchain specified in configure args"
elif [[ "${BUILD_ARGS}" =~ .*"--use-adoptium-devkit".* ]]; then
echo "Using gcc from Adoptium DevKit toolchain specified in --use-adoptium-devkit build args"
There was a problem hiding this comment.
SBOM has the args --use-adoptium-devkit
There was a problem hiding this comment.
@andrew-m-leonard how is
NOT_USING_DEVKITdefined? Is it defined by following options?if [[ "${CONFIGURE_ARGS}" =~ .*"--with-devkit=".* ]]; then echo "Using gcc from DevKit toolchain specified in configure args" elif [[ "${BUILD_ARGS}" =~ .*"--use-adoptium-devkit".* ]]; then echo "Using gcc from Adoptium DevKit toolchain specified in --use-adoptium-devkit build args"
Correct, --use-adoptium-devkit will download the required devkit
| @@ -57,13 +57,14 @@ SIGNTOOL_BASE="C:/Program Files (x86)/Windows Kits/10" | |||
| # Define What Are Configure Args & Redundant Args | |||
| # This MAY Need Updating If Additional Configure Args Are Passed | |||
There was a problem hiding this comment.
ditto, not sure this all of this right, we should use original configure-args, and update args that need to point to local path equivalents, eg.--with-ucrt-dll-dir needs to point at local Windows dir path
There was a problem hiding this comment.
No idea maybe confirm with @steelhead31
|
Using temurin master with #3835 in https://ci.adoptium.net/view/Test_grinder/job/Grinder/10155/ - failed as the temurin-build ref in sbom is personal repo and branch https://ci.adoptium.net/view/Test_grinder/job/Grinder/10167/ |
|
@andrew-m-leonard tests is running in the contain with basic docker centos:7 . I also double checked in the container there is no local /usr/local/gcc11 /usr/local/gcc11 C Compiler: Version 11.3.0 (at /usr/local/gcc11/bin/gcc-11.3) is set by So CC and CXX environment was setup by download adoptium gcc tar and export if original sbom says '--use-adoptium-devkit'. |
|
This PR is fixing the parameter parsing, I think we can get this done first. Any related reproducible issues or underlying temurin-build or sbom issues we can open specific issues correspondingly. @andrew-m-leonard opinion? |
| NATIVE_API_ARCH=$(uname -m) | ||
| if [ "${NATIVE_API_ARCH}" = "x86_64" ]; then NATIVE_API_ARCH=x64; fi | ||
| if [ "${NATIVE_API_ARCH}" = "armv7l" ]; then NATIVE_API_ARCH=arm; fi | ||
| if [[ "$TEMURIN_BUILD_ARGS" =~ .*"--use-adoptium-devkit".* ]]; then | ||
| USING_DEVKIT="false" |
There was a problem hiding this comment.
This should be:
USING_DEVKIT="true"
| @@ -25,7 +25,7 @@ ANT_SHA=9028e2fc64491cca0f991acc09b06ee7fe644afe41d1d6caf72702ca25c4613c | |||
| ANT_CONTRIB_VERSION=1.0b3 | |||
| ANT_CONTRIB_SHA=4d93e07ae6479049bb28071b069b7107322adaee5b70016674a0bffd4aac47f9 | |||
| isJdkDir=false | |||
|
|
|||
| USING_DEVKIT="true" | |||
There was a problem hiding this comment.
Should assume no DevKit default
USING_DEVKIT="false"
@sophia-guo The USING_DEVKIT boolean is the wrong way around... |
Signed-off-by: Sophia Guo <[email protected]>
|
That's great, only these differ now: the reason for that is the failures further up, eg: You need to work out why those are failing.... not sure why? |
There was a problem hiding this comment.
Looks great overall and great to see this script getting a few updates! I've added a few comments, mostly just suggestions, but I've marked this as "Request Changes" because we should definitely retain the configure_args parsing for the last GA release to keep the "old" quoting working for that version to make sure we have a viable story for end users to rebuild the GA version.
One other note: When this is merged we should make the commit message a bit clearer to indicate that it is the parsing for the Linux reproducible build script e.g. Update build args parsing in Linux reproducible build script
| local buildArgs="$1" | ||
| local bootJdk="$2" | ||
| local timeStamp="$3" | ||
| local ignoreOptions=("--enable-sbom-strace ") |
There was a problem hiding this comment.
I'd personally go with this as it feels less complex than using an array - you can add extra things to filter out later with multiple -e parameters if needed. But I'm ok with either approach. We could also combine this with the jdk-boot-dir sed below.
| local ignoreOptions=("--enable-sbom-strace ") | |
| # Remove arguments that are not necessary for reproducibility comparison | |
| buildArgs="$(echo buildArgs | sed -e 's/--enable-sbom-strace//g')" |
There was a problem hiding this comment.
It's not a code standard issue. I personally feel the array is simpler if extra options need to be filtered out. Using multiple -e make it less readable and more error-prone for me.
There was a problem hiding this comment.
I'm not convinced this logic is working. When I tried it out it converted:
--create-sbom --enable-sbom-strace --use-adoptium-devkit
in the string to:
--create-sbom /--use-adoptium-devkit
so if you want to leave it with the bash-specific construct, I think you'll need to switch // to / in the translation expression.
There was a problem hiding this comment.
Confused. @sxa which logic did you try? Seems you were trying buildArgs="$(echo buildArgs | sed -e 's/--enable-sbom-strace//g')"? As I said I would prefer using the array, similar to the window reproducible comparing script.
There was a problem hiding this comment.
Seems you were trying buildArgs="$(echo buildArgs | sed -e 's/--enable-sbom-strace//g')"
No, the version with sed works ok - the current version with the array adds in an extra / unless you remove one of them as you can see in this example:
sxa@fedora:~$ buildArgs="abc --enable-sbom-strace def"
sxa@fedora:~$ echo $buildArgs | sed -e 's/--enable-sbom-strace//g'
abc def
sxa@fedora:~$ ignoreOptions=("--enable-sbom-strace ")
sxa@fedora:~$ for ignoreOption in "${ignoreOptions[@]}"; do
buildArgs="${buildArgs/${ignoreOption}//}"
done
sxa@fedora:~$ echo $buildArgs
abc /def
sxa@fedora:~$
| downloadTooling() { | ||
| local using_DEVKIT=$1 |
There was a problem hiding this comment.
Noting that this may be a little confusing to anyone reading this to see a local variable using_DEVKIT and a global one USING_DEVKIT
| @@ -75,28 +75,50 @@ downloadAnt() { | |||
| fi | |||
| } | |||
|
|
|||
| setEnvironment() { | |||
| setGccEnvironment() { | |||
There was a problem hiding this comment.
I'd be a bit happier if we changed the name of this. The devkit on Linux still using gcc so this may cause some confusion. Maybe setNonDevkitGccEnvironment?
There was a problem hiding this comment.
yes agree, that is what was going through my head when I suggested it, ie.set the Gcc environment when not using a DevKit...
| @@ -158,20 +186,23 @@ if [ "${isJdkDir}" = true ]; then | |||
| comparedDir=$JDK_PARAM | |||
| fi | |||
|
|
|||
| echo "Rebuild Temurin build args is $TEMURIN_BUILD_ARGS" | |||
There was a problem hiding this comment.
I feel it's nice to make this explicit for anyone that wants to replicate this outside the build jobs:
| echo "Rebuild Temurin build args is $TEMURIN_BUILD_ARGS" | |
| echo "makejdk_any_platform.sh build args are: $TEMURIN_BUILD_ARGS" |
| @@ -126,23 +147,30 @@ BOOTJDK_VERSION=$(jq -r '.metadata.tools[] | select(.name == "BOOTJDK") | .versi | |||
| GCCVERSION=$(jq -r '.metadata.tools[] | select(.name == "GCC") | .version' "$SBOM" | sed 's/.0$//') | |||
| LOCALGCCDIR=/usr/local/gcc$(echo "$GCCVERSION" | cut -d. -f1) | |||
| TEMURIN_BUILD_SHA=$(jq -r '.components[0] | .properties[] | select (.name == "Temurin Build Ref") | .value' "$SBOM" | awk -F/ '{print $NF}') | |||
| TEMURIN_BUILD_ARGS=$(jq -r '.components[0] | .properties[] | select (.name == "makejdk_any_platform_args") | .value' "$SBOM" | cut -d\" -f4 | sed -e "s/--disable-warnings-as-errors --enable-dtrace --without-version-pre --without-version-opt/'--disable-warnings-as-errors --enable-dtrace --without-version-pre --without-version-opt'/" -e "s/ --disable-warnings-as-errors --enable-dtrace/ '--disable-warnings-as-errors --enable-dtrace'/" -e 's/\\n//g' -e "s,--jdk-boot-dir [^ ]*,--jdk-boot-dir /usr/lib/jvm/jdk-$BOOTJDK_VERSION,g") | |||
| TEMURIN_BUILD_ARGS=$(jq -r '.components[0] | .properties[] | select (.name == "makejdk_any_platform_args") | .value' "$SBOM") | |||
There was a problem hiding this comment.
I don't think we should remove these bits of sed that quote the configure args completely at the moment.
While the format of the SBOM changed to ensure the configure args quoted in the SBOM this is still needed if we want to rebuild the last GA with these scripts (which require the devkit adjustments too, so we can't just rely on using an old version of the temurin-build repo to rebuild the latest GA)
Ref: Old GA SBOM without quotes - New SBOM with quotes
Can we (at least until we've done a few more GA levels) do something to detect if the makejdk_any_platform_args is already quoted, and if not apply this sed command?
There was a problem hiding this comment.
My question would be it's currently used by anyone outside of Adoptium for former GAs? It's more like under development instead of production, which should allow the failures? For now if the PR is not in the issue with the devkit still exists. That is the issue with using the an old version of the temurin-build repo to rebuild without devkit adjustments will be an known issue. If necessary I would prefer to update some related documents to address this.
There was a problem hiding this comment.
Since we are actively pushing the story that we're reproducible it would be a mistake not to be able to do this with the scripts easily. I've done videos in the past and if we now have to say "Well we can't do that just now" it's not a good story for the community. I strongly believe we should support that scenario and that these scripts should never be considered only for our internal use.
It should be fairly simple to support this. Going forward we will be in the situation where we can point people at the GA SHA of temurin-build (although this should be a fallback - I would rather have the HEAD version able to support as a minimum one year of GA builds so that users wanting to verify our builds don't have to figure out how to get the correct version) but in the meantime I feel it's very important to allow end users to be able to prove reproducibility easily given how much we're pushing this in our marketing at the moment.
There was a problem hiding this comment.
If necessary I would prefer to update some related documents to address this.
Any such document would be very time limited (since we'll have this sorted in a release or two) and I'm not sure how we'd publish that in a way that our end users interested in verifying our reproducibility would be likely to find it. The reproducible build script has already been publicised in the past (I wrote it specifically with the goal of having others able to run it to verify what we're saying about reproducibility) so I feel it's important that it can be seen to work as demonstrated previously with the most recent GA level.
There was a problem hiding this comment.
The following after the jq line that sets TEMURIN_BUILD_ARGS should be ok for all situations:
if echo "$TEMURIN_BUILD_ARGS" | grep -v configure-args\ \\\"; then
echo "Using old method to escape configure-args in SBOM (build PR 3835)"
TEMURIN_BUILD_ARGS=$(echo $TEMURIN_BUILD_ARGS | \
cut -d\" -f4 | \
sed -e "s/--with-jobs=[0-9]*//g" \
-e "s/--disable-warnings-as-errors --enable-dtrace --without-version-pre --without-version-opt/'--disable-warnings-as-errors --enable-dtrace --without-version-pre --without-version-opt'/" \
-e "s/--disable-warnings-as-errors --enable-dtrace *--with-version-opt=ea/'--disable-warnings-as-errors --enable-dtrace --with-version-opt=ea'/" \
-e "s/--disable-warnings-as-errors --enable-dtrace/'--disable-warnings-as-errors --enable-dtrace'/" \
-e 's/\\n//g' \
-e "s,--jdk-boot-dir [^ ]*,--jdk-boot-dir /usr/lib/jvm/jdk-$BOOTJDK_VERSION,g")
fi
Noting that the removal of --with-jobs= is because it's not necessary for reproducing the builds, and was added to work around an issue when enable-sbom-strace is enabled (Ref: adoptium/ci-jenkins-pipelines@68ffc9e). This could potentially be done in the same place as the removal of enable-sbom-strace but it needs to be done before the other translations in the above sed are done.
I've verified the above changes with the following and it looks good:
- Latest in jenkins:
./linux_repro_build_compare.sh https://ci.adoptium.net/job/build-scripts/job/jobs/job/jdk21u/job/jdk21u-linux-aarch64-temurin/195/artifact/workspace/target/OpenJDK21U-sbom_aarch64_linux_hotspot_2024-06-13-10-35.json https://ci.adoptium.net/job/build-scripts/job/jobs/job/jdk21u/job/jdk21u-linux-aarch64-temurin/195/artifact/workspace/target/OpenJDK21U-jdk_aarch64_linux_hotspot_2024-06-13-10-35.tar.gz(Now deleted so can't be re-built again) - Latest EA:
./linux_repro_build_compare.sh https://github.com/adoptium/temurin21-binaries/releases/download/jdk-21.0.4%2B5-ea-beta/OpenJDK21U-sbom_aarch64_linux_hotspot_21.0.4_5-ea.json https://github.com/adoptium/temurin21-binaries/releases/download/jdk-21.0.4%2B5-ea-beta/OpenJDK21U-jdk_aarch64_linux_hotspot_21.0.4_5-ea.tar.gz - Latest GA:
./linux_repro_build_compare.sh https://github.com/adoptium/temurin21-binaries/releases/download/jdk-21.0.3%2B9/OpenJDK21U-sbom_aarch64_linux_hotspot_21.0.3_9.json https://github.com/adoptium/temurin21-binaries/releases/download/jdk-21.0.3%2B9/OpenJDK21U-jdk_aarch64_linux_hotspot_21.0.3_9.tar.gz
Note to self: The location specified by --user-openjdk-build-root-directory should be cleared before each subsequent attempt.
Second note to self: This script does not work if a fork of the adoptium/temurin-build is used as that repository is hard coded in the script, so - for example - jdk21u-linux-aarch64-temurin/197 does not work without a change to the script.
There was a problem hiding this comment.
Assume those covers all possible combinations of configure-args of former GAs.
|
Those diff has been discussed before #3739 Reproducing build use linux_repro_build_compare.sh instead of build_farm which adds BUILD_SOURCE , BUILD_REPO, BUILD_SOURCE_REPO. addBuildSHA, addBuildSourceRepo, addOpenJDKSourceRepo use ${BUILD_CONFIG[WORKSPACE_DIR]} to generate those information. In repro_common.sh those information has been cleaned explicitly. Linux_repro_build_compare.sh doesn't use repro_common.h. Though I believe the logic is the same. I have asked if we need to update to clean those information too, and was confirmed to 'leave the cleanBuildInfo() exactly as it was originally, ie.just containing BUILD_INFO The release file content should contain the others, I suspect the build.sh is getting an error during the release.txt file generation and not adding them, i've seen that happen before when for example ant can't be found I think'. I thought @andrew-m-leonard maybe there are some other issues? @andrew-m-leonard I did take a look at the build.sh. So I think the right way is to open an issue to update the build.sh which says if it's not the build farm environment another way need to get those information, which shouldn't be difficult. Thoughts? |
|
@sophia-guo yeah, I wasn't quite right in my previous discussion on the release.txt, we need to discover why addBuildSourceRepo is failing: Line 2184 in 8ae8d28 I have seen this issue before, and I think it maybe the "git -C " option, if I remember correctly the "-C" option is only valid on certain versions of git, I suspect we should do this differently, maybe "cd repo && git config ..." ? |
|
@sophia-guo yes, it looks like the test job container is using centos:7 with git package: Package git.x86_64 0:1.8.3.1-25.el7_9 will be installed |
| fi | ||
| setAntEnvironment | ||
| echo "original temurin build args is ${TEMURIN_BUILD_ARGS}" | ||
| TEMURIN_BUILD_ARGS=$(setTemurinBuildArgs "$TEMURIN_BUILD_ARGS" "$BOOTJDK_VERSION" "$BUILDSTAMP") | ||
|
|
There was a problem hiding this comment.
Extra blank line here should be removed
@sophia-guo This should be resolved by #3854 |
Signed-off-by: Sophia Guo <[email protected]>
| sed -e "s/--with-jobs=[0-9]*//g" \ | ||
| -e "s/--disable-warnings-as-errors --enable-dtrace --without-version-pre --without-version-opt/'--disable-warnings-as-errors --enable-dtrace --without-version-pre --without-version-opt'/" \ | ||
| -e "s/--disable-warnings-as-errors --enable-dtrace *--with-version-opt=ea/'--disable-warnings-as-errors --enable-dtrace --with-version-opt=ea'/" \ | ||
| -e "s/--disable-warnings-as-errors --enable-dtrace/'--disable-warnings-as-errors --enable-dtrace'/" \ |
There was a problem hiding this comment.
Apologies - copy error on my part - I accidentally removed the space at the beginning of this expression when I copied it from my test script. Without it some of the parameters get escaped twice:
| -e "s/--disable-warnings-as-errors --enable-dtrace/'--disable-warnings-as-errors --enable-dtrace'/" \ | |
| -e "s/ --disable-warnings-as-errors --enable-dtrace/ '--disable-warnings-as-errors --enable-dtrace'/" \ |
|
Noting that with the changes to support older GA builds, both 21.0.3 and 21.0.2 GA builds are reproducible with this script. 21.0.1 does not work due to a checksum failure on temurin-build/cyclonedx-lib/build.xml Line 47 in 16ac032 Those checksum verification were changed in https://github.com/adoptium/temurin-build/pull/3709/files (I assume, without further investigation, that the previous code was still good for 21.0.2 but not compatible with 21.0.1) |
Signed-off-by: Sophia Guo <[email protected]>
|
@andrew-m-leonard @sxa updated. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Ignore my change request from a few minutes ago if you saw it - I've been staring at my screen for too long today and my suggestion was wrong ;-)
LGTM Although it would be good to get a response to #3824 (comment) that would be good (WDYT @andrew-m-leonard since I think that name was your suggestion?)
Also reiterating my earlier comment in case it gets lost when this is merged that it would be good to make the commit message a bit more descriptive when this is merged e.g. Update build args parsing in Linux reproducible build script
Signed-off-by: Sophia Guo <[email protected]>
|
@sophia-guo just change the setGccEnvironment() method name as per Stewart's suggestion, then we're good to go i think |
Signed-off-by: Sophia Guo <[email protected]>
Fix adoptium/aqa-tests#5345