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.

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

Update parsing TEMURIN_BUILD_ARGS #3824

Merged
merged 14 commits into from
Jun 21, 2024
Merged

Update parsing TEMURIN_BUILD_ARGS #3824

merged 14 commits into from
Jun 21, 2024

Conversation

sophia-guo
Copy link
Contributor

Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
@github-actions github-actions bot added the testing Issues that enhance or fix our test suites label May 27, 2024
@sophia-guo sophia-guo marked this pull request as draft May 27, 2024 18:47
Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
@sophia-guo
Copy link
Contributor Author

@sophia-guo sophia-guo marked this pull request as ready for review May 28, 2024 19:05
@sophia-guo sophia-guo marked this pull request as draft May 28, 2024 20:50
@sophia-guo
Copy link
Contributor Author

Grinder runs extremely long. Seems like doing some unexpected steps

13:52:53  
13:53:06  find "/home/jenkins/temurin-build/workspace/./build//straceOutput" -type f -name 'outputFile.*' | xargs -n100 grep -v ENOENT | cut -d'"' -f2 | grep "^/" | eval "grep -Ev '(\.java$|\.d$|\.o$|\.d\.targets$|^/home/jenkins/temurin-build|\+\+\+|\-\-\-|^/dev/|^/proc/|^/sys/|^/tmp/)'" | sort | uniq
13:53:06  /bin/sh
13:53:06  /etc/ld.so.cache
13:53:06  /etc/nsswitch.conf
13:53:06  /etc/passwd
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/build.log
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes/_the.CLASSLIST_JAR-javacserver.conf
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes/_the.CLASSLIST_JAR.config_vardeps
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes/_the.CLASSLIST_JAR.vardeps
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes/_the.CLASSLIST_JAR_batch
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes/_the.CLASSLIST_JAR_batch.cmdline
1

Is this related with #3746? @andrew-m-leonard

@sophia-guo sophia-guo marked this pull request as ready for review May 28, 2024 20:56
@karianna
Copy link
Contributor

Linter also unhappy.

@andrew-m-leonard
Copy link
Contributor

Grinder runs extremely long. Seems like doing some unexpected steps

13:52:53  
13:53:06  find "/home/jenkins/temurin-build/workspace/./build//straceOutput" -type f -name 'outputFile.*' | xargs -n100 grep -v ENOENT | cut -d'"' -f2 | grep "^/" | eval "grep -Ev '(\.java$|\.d$|\.o$|\.d\.targets$|^/home/jenkins/temurin-build|\+\+\+|\-\-\-|^/dev/|^/proc/|^/sys/|^/tmp/)'" | sort | uniq
13:53:06  /bin/sh
13:53:06  /etc/ld.so.cache
13:53:06  /etc/nsswitch.conf
13:53:06  /etc/passwd
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/build.log
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes/_the.CLASSLIST_JAR-javacserver.conf
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes/_the.CLASSLIST_JAR.config_vardeps
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes/_the.CLASSLIST_JAR.vardeps
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes/_the.CLASSLIST_JAR_batch
13:53:06  /home/jenkins/workspace/build-scripts/jobs/jdk21u/jdk21u-linux-x64-temurin/workspace/build/openjdkbuild/buildtools/classlist_classes/_the.CLASSLIST_JAR_batch.cmdline
1

Is this related with #3746? @andrew-m-leonard

@sophia-guo yes it is, the strace anaylsis was incorrectly analysing openjdk source and due to the large number of files it takes ages!
This PR #3826 should fix it, just needs one more review please?

Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
NO need to install strace and corresponding docker run options for
strace
Change to /bin/bash

Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
@sophia-guo
Copy link
Contributor Author

sophia-guo commented May 30, 2024

Turn off strace for reproducing build
https://ci.adoptium.net/view/Test_grinder/job/Grinder/10145/

@sophia-guo sophia-guo marked this pull request as draft May 30, 2024 15:18
@sophia-guo
Copy link
Contributor Author

Lots of binary diff. @andrew-m-leonard would it be related with turn off strace?

PR is ready.

@sophia-guo sophia-guo marked this pull request as ready for review May 30, 2024 17:01
@andrew-m-leonard
Copy link
Contributor

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:

17:14:20  configure: Using gcc C++ compiler version 11.3.0 [g++-11.3 (Adoptium/SXA 11.3.0) 11.3.0]

vs the original build:

20:02:16  configure: Using gcc C++ compiler version 11.3.0 [g++ (GCC) 11.3.0]

}

setBuildArgs() {
local CONFIG_ARGS=("--disable-warnings-as-errors" "--enable-dtrace" "--without-version-pre" "--without-version-opt" "--with-version-opt")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be using the original build configure_args, not specifying these...?

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to fix the SBOM makejdk_any_platform_args which is losing the --configure-args "quotes"...

replace the code here:

echo "$@" > ./workspace/config/makejdk-any-platform.args

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

Copy link
Contributor

@andrew-m-leonard andrew-m-leonard May 31, 2024

Choose a reason for hiding this comment

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

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"
        },

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'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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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")
Copy link
Contributor

Choose a reason for hiding this comment

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

--configure-args should not be being ignored, only --enable-sbom-strace

Copy link
Contributor Author

@sophia-guo sophia-guo May 31, 2024

Choose a reason for hiding this comment

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

--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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
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-

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
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 into two:

setAntEnvironment()

if [[ NOT_USING_DEVKIT ]]; then
  setGccEnvironment()
fi

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SBOM has the args --use-adoptium-devkit

Copy link
Contributor

Choose a reason for hiding this comment

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

@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"

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
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

@sophia-guo
Copy link
Contributor Author

sophia-guo commented Jun 3, 2024

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/

@sophia-guo
Copy link
Contributor Author

sophia-guo commented Jun 11, 2024

@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
https://ci.adoptium.net/view/Test_grinder/job/Grinder/10352/console

C Compiler: Version 11.3.0 (at /usr/local/gcc11/bin/gcc-11.3) is set by

https://github.com/adoptium/temurin-build/pull/3824/files#diff-65a42dfc13c7c802493b4d5ddf3040f762df150947ea544841d387a4b82da73fR120-R123

and https://github.com/adoptium/temurin-build/pull/3824/files#diff-65a42dfc13c7c802493b4d5ddf3040f762df150947ea544841d387a4b82da73fR163-R166.

So CC and CXX environment was setup by download adoptium gcc tar and export if original sbom says '--use-adoptium-devkit'.

@sophia-guo
Copy link
Contributor Author

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"
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"

@@ -25,7 +25,7 @@ ANT_SHA=9028e2fc64491cca0f991acc09b06ee7fe644afe41d1d6caf72702ca25c4613c
ANT_CONTRIB_VERSION=1.0b3
ANT_CONTRIB_SHA=4d93e07ae6479049bb28071b069b7107322adaee5b70016674a0bffd4aac47f9
isJdkDir=false

USING_DEVKIT="true"
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"

@andrew-m-leonard
Copy link
Contributor

@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 https://ci.adoptium.net/view/Test_grinder/job/Grinder/10352/console

C Compiler: Version 11.3.0 (at /usr/local/gcc11/bin/gcc-11.3) is set by

https://github.com/adoptium/temurin-build/pull/3824/files#diff-65a42dfc13c7c802493b4d5ddf3040f762df150947ea544841d387a4b82da73fR120-R123

and https://github.com/adoptium/temurin-build/pull/3824/files#diff-65a42dfc13c7c802493b4d5ddf3040f762df150947ea544841d387a4b82da73fR163-R166.

So CC and CXX environment was setup by download adoptium gcc tar and export if original sbom says '--use-adoptium-devkit'.

@sophia-guo The USING_DEVKIT boolean is the wrong way around...

Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
@sophia-guo
Copy link
Contributor Author

@andrew-m-leonard
Copy link
Contributor

That's great, only these differ now:

17:27:46  < BUILD_SOURCE="git:2ad2f7c73bb23e23cd4f20208c01576bec75f1f3"
17:27:46  < BUILD_SOURCE_REPO="https://github.com/adoptium/temurin-build.git"
17:27:46  < SOURCE_REPO="https://github.com/adoptium/jdk21u.git"

the reason for that is the failures further up, eg:

17:25:03  Unable to fetch build SHA, does a work tree exist?...
17:25:03  Unable to fetch OpenJDK Source SHA, does a work tree exist?...
17:25:03  fatal: attempt to fetch/clone from a shallow repository
17:25:03  fatal: Could not read from remote repository.
17:25:03  
17:25:03  Please make sure you have the correct access rights
17:25:03  and the repository exists.
17:25:03  fatal: attempt to fetch/clone from a shallow repository
17:25:03  fatal: Could not read from remote repository.
17:25:03  
17:25:03  Please make sure you have the correct access rights
17:25:03  and the repository exists.
17:25:03  fatal: attempt to fetch/clone from a shallow repository
17:25:03  fatal: Could not read from remote repository.

You need to work out why those are failing.... not sure why?

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

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 ")
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

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

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.

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

@@ -75,28 +75,50 @@ downloadAnt() {
fi
}

setEnvironment() {
setGccEnvironment() {
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...

@@ -158,20 +186,23 @@ if [ "${isJdkDir}" = true ]; then
comparedDir=$JDK_PARAM
fi

echo "Rebuild Temurin build args is $TEMURIN_BUILD_ARGS"
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"

@@ -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")
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

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

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

@sophia-guo
Copy link
Contributor Author

#3824 (comment)

Those diff has been discussed before #3739
Screenshot 2024-06-14 at 10 29 10 AM (2)

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?

@andrew-m-leonard
Copy link
Contributor

@sophia-guo yeah, I wasn't quite right in my previous discussion on the release.txt, we need to discover why addBuildSourceRepo is failing:

local buildSourceRepo=$(git -C "${BUILD_CONFIG[WORKSPACE_DIR]}" config --get remote.origin.url 2>/dev/null)

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 ..." ?

@andrew-m-leonard
Copy link
Contributor

andrew-m-leonard commented Jun 14, 2024

@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
I believe git 1.8 does not support "git -C"
So we can either upgrade git used in the test container, or fix the build.sh to use "cd <dir> && git config ..."

fi
setAntEnvironment
echo "original temurin build args is ${TEMURIN_BUILD_ARGS}"
TEMURIN_BUILD_ARGS=$(setTemurinBuildArgs "$TEMURIN_BUILD_ARGS" "$BOOTJDK_VERSION" "$BUILDSTAMP")

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

@andrew-m-leonard
Copy link
Contributor

That's great, only these differ now:

17:27:46  < BUILD_SOURCE="git:2ad2f7c73bb23e23cd4f20208c01576bec75f1f3"
17:27:46  < BUILD_SOURCE_REPO="https://github.com/adoptium/temurin-build.git"
17:27:46  < SOURCE_REPO="https://github.com/adoptium/jdk21u.git"

@sophia-guo This should be resolved by #3854
now, so I think if we can just add the former GA "sed" hack in a simple manner, then I think we're good to go!

Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
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'/" \
Copy link
Member

@sxa sxa Jun 19, 2024

Choose a reason for hiding this comment

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

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:

Suggested change
-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'/" \

@sxa
Copy link
Member

sxa commented Jun 19, 2024

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 cyclonedx-core-java.jar (which is likely because it's using the "latest" version in

srcurl="https://ci.adoptium.net/view/all/job/build.getDependency/lastSuccessfulBuild/artifact/sbom_dependencies/cyclonedx-core-java.jar"
and there has been a subsequent update)
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 <sophia.gwf@gmail.com>
@sophia-guo
Copy link
Contributor Author

@andrew-m-leonard @sxa updated.

sxa

This comment was marked as outdated.

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

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 <sophia.gwf@gmail.com>
@andrew-m-leonard
Copy link
Contributor

andrew-m-leonard commented Jun 20, 2024

@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 <sophia.gwf@gmail.com>
Copy link
Contributor

@andrew-m-leonard andrew-m-leonard left a comment

Choose a reason for hiding this comment

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

lgtm

@sxa sxa merged commit d937e1f into adoptium:master Jun 21, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Issues that enhance or fix our test suites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reproduciable comparing tests on linux failed with Invalid build.sh option: --with-version-opt=ea
4 participants