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.

Sign up for GitHub

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

Jump to bottom

Update parsing TEMURIN_BUILD_ARGS #3824

Merged
merged 14 commits into from
Jun 21, 2024
Merged

Update parsing TEMURIN_BUILD_ARGS #3824

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5ebfb93
Archive SBOM file and exit instant if build jdk fails
sophia-guo May 23, 2024
d98d20e
Testing
sophia-guo May 23, 2024
55ce82b
Install strace
sophia-guo May 28, 2024
1415629
Add --cap-add SYS_PTRACE for docker run
sophia-guo May 28, 2024
8af8e39
Turn off --enable-sbom-strace
sophia-guo May 30, 2024
75e969a
update
sophia-guo Jun 3, 2024
b2050b5
Update build args setting
sophia-guo Jun 5, 2024
6919b17
Add --build-reproducible-date args
sophia-guo Jun 7, 2024
5ebad2c
Leave windows alone
sophia-guo Jun 7, 2024
636999a
USING_DEVKIT is set the wrong way around
sophia-guo Jun 13, 2024
0ee465d
Keep old method of parsing configure-args to work for old GAs
sophia-guo Jun 18, 2024
2ccf570
Adding back the extra space
sophia-guo Jun 19, 2024
c614969
Comment update
sophia-guo Jun 19, 2024
baafa00
Naming change
sophia-guo Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion test/system/reproducibleCompare/playlist.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@
<include>reproducible.mk</include>
<test>
<testCaseName>Rebuild_Same_JDK_Reproducibility_Test</testCaseName>
<command>docker run -v "$(TEST_RESROOT):/home/jenkins/test" -w "/home/jenkins/" -v "$(TEST_JDK_HOME):/home/jenkins/jdkbinary/" --name reproducibleCompare centos:7 /bin/bash /home/jenkins/test/linux_repro_build_compare.sh $(SBOM_FILE) /home/jenkins/jdkbinary; \
<command>docker container rm reproducibleCompare; \
docker run -v "$(TEST_RESROOT):/home/jenkins/test" -w "/home/jenkins/" -v "$(TEST_JDK_HOME):/home/jenkins/jdkbinary/" --name reproducibleCompare centos:7 /bin/bash /home/jenkins/test/linux_repro_build_compare.sh $(SBOM_FILE) /home/jenkins/jdkbinary; \
$(TEST_STATUS); \
docker cp reproducibleCompare:/home/jenkins/reprotest.diff ./; \
docker cp reproducibleCompare:/home/jenkins/reproJDK.tar.gz ./; \
docker cp reproducibleCompare:/home/jenkins/SBOM.json ./; \
docker container rm reproducibleCompare
</command>
<levels>
Expand Down
58 changes: 43 additions & 15 deletions tooling/reproducible/linux_repro_build_compare.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/sh
#!/bin/bash
# ********************************************************************************
karianna marked this conversation as resolved.
Show resolved Hide resolved
# Copyright (c) 2024 Contributors to the Eclipse Foundation
#
Expand All @@ -25,7 +25,7 @@ ANT_SHA=9028e2fc64491cca0f991acc09b06ee7fe644afe41d1d6caf72702ca25c4613c
ANT_CONTRIB_VERSION=1.0b3
ANT_CONTRIB_SHA=4d93e07ae6479049bb28071b069b7107322adaee5b70016674a0bffd4aac47f9
isJdkDir=false

USING_DEVKIT="true"
installPrereqs() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should assume no DevKit default

USING_DEVKIT="false"

if test -r /etc/redhat-release; then
yum install -y gcc gcc-c++ make autoconf unzip zip alsa-lib-devel cups-devel libXtst-devel libXt-devel libXrender-devel libXrandr-devel libXi-devel
Expand Down Expand Up @@ -75,28 +75,47 @@ downloadAnt() {
fi
}

setEnvironment() {
setGccEnvironment() {
export CC="${LOCALGCCDIR}/bin/gcc-${GCCVERSION}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes agree, that is what was going through my head when I suggested it, ie.set the Gcc environment when not using a DevKit...

export CXX="${LOCALGCCDIR}/bin/g++-${GCCVERSION}"
export LD_LIBRARY_PATH="${LOCALGCCDIR}/lib64"
# /usr/local/bin required to pick up the new autoconf if required
}

setAntEnvironment() {
export PATH="${LOCALGCCDIR}/bin:/usr/local/bin:/usr/bin:$PATH:/usr/local/apache-ant-${ANT_VERSION}/bin"
# /usr/local/bin required to pick up the new autoconf if required
ls -ld "$CC" "$CXX" "/usr/lib/jvm/jdk-${BOOTJDK_VERSION}/bin/javac" || exit 1
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split this method into separate:
setGccEnvironment()
setAntEnvironment()
Also remove check on javac, as it might not be located in /usr/lib/jvm/jdk-


cleanBuildInfo() {
# shellcheck disable=SC3043
local DIR="$1"
# BUILD_INFO name of OS level build was built on will likely differ
sed -i '/^BUILD_INFO=.*$/d' "${DIR}/release"
}

setTemurinBuildArgs() {
local buildArgs="$1"
local bootJdk="$2"
local ignoreOptions=("--enable-sbom-strace ")
for ignoreOption in "${ignoreOptions[@]}"; do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
local ignoreOptions=("--enable-sbom-strace ")
# Remove arguments that are not necessary for reproducibility comparison
buildArgs="$(echo buildArgs | sed -e 's/--enable-sbom-strace//g')"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sophia-guo sophia-guo Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@sxa sxa Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:~$

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, updated.

buildArgs="${buildArgs/${ignoreOption}//}"
done
#reset --jdk-boot-dir
# shellcheck disable=SC2001
buildArgs="$(echo "$buildArgs" | sed -e "s|--jdk-boot-dir [^ ]*|--jdk-boot-dir /usr/lib/jvm/jdk-${bootJdk}|")"
echo "${buildArgs}"
}

downloadTooling() {
local using_DEVKIT=$1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if [ ! -r "/usr/lib/jvm/jdk-${BOOTJDK_VERSION}/bin/javac" ]; then
echo "Retrieving boot JDK $BOOTJDK_VERSION" && mkdir -p /usr/lib/jvm && curl -L "https://api.adoptium.net/v3/binary/version/jdk-${BOOTJDK_VERSION}/linux/${NATIVE_API_ARCH}/jdk/hotspot/normal/eclipse?project=jdk" | (cd /usr/lib/jvm && tar xpzf -)
fi
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
if [[ "${using_DEVKIT}" == "false" ]]; 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
if [ ! -r temurin-build ]; then
git clone https://github.com/adoptium/temurin-build || exit 1
Expand All @@ -113,7 +132,6 @@ checkAllVariablesSet() {
installPrereqs
downloadAnt

# shellcheck disable=SC3010
if [[ $SBOM_PARAM =~ ^https?:// ]]; then
echo "Retrieving and parsing SBOM from $SBOM_PARAM"
curl -LO "$SBOM_PARAM"
Expand All @@ -126,23 +144,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")
TEMURIN_VERSION=$(jq -r '.metadata.component.version' "$SBOM" | sed 's/-beta//' | cut -f1 -d"-")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@sxa sxa Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@sxa sxa Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume those covers all possible combinations of configure-args of former GAs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. @sxa


#BUILDSTAMP=$(jq -r '.components[0].properties[] | select(.name == "Build Timestamp") | .value' "$SBOM")
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"
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be:

  USING_DEVKIT="true"


checkAllVariablesSet
downloadTooling "$USING_DEVKIT"
if [[ "${USING_DEVKIT}" == "false" ]]; then
setGccEnvironment
fi
setAntEnvironment
echo "original temurin build args is ${TEMURIN_BUILD_ARGS}"
TEMURIN_BUILD_ARGS=$(setTemurinBuildArgs "$TEMURIN_BUILD_ARGS" "$BOOTJDK_VERSION")

downloadTooling
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line here should be removed

setEnvironment

if [ -z "$JDK_PARAM" ] && [ ! -d "jdk-${TEMURIN_VERSION}" ] ; then
JDK_PARAM="https://api.adoptium.net/v3/binary/version/jdk-${TEMURIN_VERSION}/linux/${NATIVE_API_ARCH}/jdk/hotspot/normal/eclipse?project=jdk"
fi

# shellcheck disable=SC3010
if [[ $JDK_PARAM =~ ^https?:// ]]; then
echo Retrieving original tarball from adoptium.net && curl -L "$JDK_PARAM" | tar xpfz - && ls -lart "$PWD/jdk-${TEMURIN_VERSION}" || exit 1
elif [[ $JDK_PARAM =~ tar.gz ]]; then
Expand All @@ -158,20 +183,23 @@ if [ "${isJdkDir}" = true ]; then
comparedDir=$JDK_PARAM
fi

echo "Rebuild Temurin build args is $TEMURIN_BUILD_ARGS"
echo " cd temurin-build && ./makejdk-any-platform.sh $TEMURIN_BUILD_ARGS 2>&1 | tee build.$$.log" | sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it's nice to make this explicit for anyone that wants to replicate this outside the build jobs:

Suggested change
echo "Rebuild Temurin build args is $TEMURIN_BUILD_ARGS"
echo "makejdk_any_platform.sh build args are: $TEMURIN_BUILD_ARGS"


echo Comparing ...
mkdir compare.$$
tar xpfz temurin-build/workspace/target/OpenJDK*-jdk_*tar.gz -C compare.$$
cp temurin-build/workspace/target/OpenJDK*-jdk_*tar.gz reproJDK.tar.gz
cp "$SBOM" SBOM.json

cleanBuildInfo "${comparedDir}"
cleanBuildInfo "compare.$$/jdk-$TEMURIN_VERSION"

rc=0

# shellcheck disable=SC2069
diff -r "${comparedDir}" "compare.$$/jdk-$TEMURIN_VERSION" 2>&1 > "reprotest.diff" || rc=$?

if [ $rc = 0 ]; then
if [ $rc -eq 0 ]; then
echo "Compare identical !"
else
cat "reprotest.diff"
Expand Down
3 changes: 2 additions & 1 deletion tooling/reproducible/windows_repro_build_compare.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea maybe confirm with @steelhead31

CONFIG_ARGS=("--disable-warnings-as-errors" "--disable-ccache" "--with-toolchain-version" "--with-ucrt-dll-dir" "--with-version-opt")
NOTUSE_ARGS=("--assemble-exploded-image" "--configure-args")
NOTUSE_ARGS=("--assemble-exploded-image" "--configure-args" "--enable-sbom-strace")

# Addiitonal Working Variables Defined For Use By This Script
SBOMLocalPath="$WORK_DIR/src_sbom.json"
DISTLocalPath="$WORK_DIR/src_jdk_dist.zip"
ScriptPath=$(dirname "$(realpath "$0")")
rc=0

# Function to check if a string is a valid URL
is_url() {
local url=$1
Expand Down
Loading