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

support --generate-cds-archive for Java 22 or higher? #34

Closed
chirontt opened this issue Dec 1, 2024 · 17 comments
Closed

support --generate-cds-archive for Java 22 or higher? #34

chirontt opened this issue Dec 1, 2024 · 17 comments

Comments

@chirontt
Copy link
Contributor

chirontt commented Dec 1, 2024

According to the comments here in build-jre.sh script, Java 22 or higher would support the --generate-cds-archive option of jlink:

# Not all vendors support --generate-cds-archive
# Also only Java version 22 or higher support it.

But then in line 289:
if [[ $java_version =~ ^[2-9][0-9]+(.[0-9]+(.[0-9]+)?)?$ ]]; then

as well as line 298:
if [[ $java_version =~ ^[2-9][0-9]+(.[0-9]+(.[0-9]+)?)?$ ]]; then

the pattern to compare $java_version seems wrong, i.e. the compare pattern should be valid for Java 22+:

if [[ $java_version =~ ^[2-9][2-9]+(.[0-9]+(.[0-9]+)?)?$ ]]; then

Also, for JDKs which are not from Temurin, e.g. IBM Semeru JDKs, the current code to handle them:

else
vendor_url="https://jdk.java.net/"
vendor_label="OpenJDK Hotspot"
vendor_prefix="openjdk.hotspot"
if [[ $java_version =~ ^[2-9][0-9]+(.[0-9]+(.[0-9]+)?)?$ ]]; then
generate_cds_archive="--generate-cds-archive"
fi
fi

doesn't handle OpenJ9 JVM like that of the Temurin's, i.e. the code should be like:

else
  # Assume other JDKs are derived from OpenJDK
  vendor_url="https://jdk.java.net/"
  if grep "OpenJ9" all.properties; then
    vendor_label="OpenJDK J9"
    vendor_prefix="openjdk.j9"
  else
    vendor_label="OpenJDK Hotspot"
    vendor_prefix="openjdk.hotspot"

    if [[ $java_version =~ ^[2-9][2-9]+(.[0-9]+(.[0-9]+)?)?$ ]]; then
      generate_cds_archive="--generate-cds-archive"
    fi
  fi
fi

I've got a PR for the above changes ready to submit, if the above issues are considered valid and acceptable.

@merks
Copy link
Contributor

merks commented Dec 2, 2024

Yes, I already tried to fix this missing it up the first time:

Of course I'm happy with improvements.

Note though that the comment about Java 22 is wrong and in fact we're already doing this for Java 21:

#34

Probably Java 30 is going to be a problem. 😨

@merks
Copy link
Contributor

merks commented Dec 2, 2024

Another point to keep in mind is that we no longer will produce new Java 20 JRE because it's EOL. So only LTS versions (11, 17, 21) and Java 23 (until EOL) and higher right now...

There is currently a problem with Java 24 since +23:

adoptium/adoptium-support#1198

@chirontt
Copy link
Contributor Author

chirontt commented Dec 3, 2024

Note though that the comment about Java 22 is wrong and in fact we're already doing this for Java 21

Yes, then the compare pattern could be, to support Java 21+:

if [[ $java_version =~ ^[2-9][1-9]+(.[0-9]+(.[0-9]+)?)?$ ]]; then

Probably Java 30 is going to be a problem

Yes, Java 30 will be missed out, but it will be some 3+ years before we start worrying about it.

But I don't know how to handle the LTS versions of 11 and 17, though. Perhaps via multiple OR conditions to cover Java 11, 17 and 21+, like:

if [[ $java_version =~ ^[1][1,7]+(.[0-9]+(.[0-9]+)?)?$ || $java_version =~ ^[2-9][1-9]+(.[0-9]+(.[0-9]+)?)?$ ]]; then

@chirontt
Copy link
Contributor Author

chirontt commented Dec 3, 2024

A complete compare pattern can be, for all Java 11, 17 and 21+ (including Java 30):

if [[ $java_version =~ ^[1][1,7]+(.[0-9]+(.[0-9]+)?)?$ || \
      $java_version =~ ^[2][1-9]+(.[0-9]+(.[0-9]+)?)?$ || \
      $java_version =~ ^[3-9][0-9]+(.[0-9]+(.[0-9]+)?)?$ ]]; then

@merks
Copy link
Contributor

merks commented Dec 4, 2024

I'm not a shell expert... It would be nice just to pull out the major version number and treat it like a number that we can compare to another number...

https://unix.stackexchange.com/questions/232384/argument-string-to-integer-in-bash

@chirontt
Copy link
Contributor Author

chirontt commented Dec 4, 2024

If you only need the Java major version number greater than 20, the regex will be simpler:

major_version_regex="^([0-9]+)"
if [[ $java_version =~ $major_version_regex ]] && (( 10#${BASH_REMATCH[1]} > 20 )); then
  generate_cds_archive="--generate-cds-archive"
fi

@merks
Copy link
Contributor

merks commented Dec 5, 2024

I created this little test script:

for java_version in "17.0.12+7" "17" "30" "30." "30.0.1" "21" "21.1000"; do
java_major_version=${java_version%%.*}
echo $java_version " -> " $java_major_version
if (($java_major_version > 20)); then
echo "(($java_major_version > 20)) == true"
else
echo "(($java_major_version > 20)) == false"
fi
done

It prints these results:

$. version-compare.sh 
17.0.12+7  ->  17
((17 > 20)) == false
17  ->  17
((17 > 20)) == false
30  ->  30
((30 > 20)) == true
30.  ->  30
((30 > 20)) == true
30.0.1  ->  30
((30 > 20)) == true
21  ->  21
((21 > 20)) == true
21.1000  ->  21
((21 > 20)) == true

This seems simple and readable. I will change the actual script this weekend when I have time.

@merks
Copy link
Contributor

merks commented Dec 6, 2024

@chirontt

I simplified the logic; I haven't tested yet.

For other cases, I wonder if we might initialize variables like this in some places, e.g.,

generate_cds_archive=${GENERATE_CDS_ARCHIVE:-"--generate-cds-archive"}

This way an environment variable can be set outside the script to determine the desired value, otherwise it's computed as the default value currently.

In any case, I'm not sure at this point what exactly you want to change further...

@chirontt
Copy link
Contributor Author

chirontt commented Dec 6, 2024

For other cases, I wonder if we might initialize variables like this in some places, e.g.,
generate_cds_archive=${GENERATE_CDS_ARCHIVE:-"--generate-cds-archive"}

Yes, it does no harm, and it may help in overriding the default value when/if needed.

I'm not sure at this point what exactly you want to change further

I've provided some comment in your above commit bec1c33, regarding other vendors' JDKs (other than Temurin's), about the possible OpenJ9 JVM in them.

@merks merks closed this as completed in 14a4c04 Dec 7, 2024
@merks
Copy link
Contributor

merks commented Dec 7, 2024

FYI, I tested on Windows x64 the following:

build-jre.sh  https://github.com/ibmruntimes/semeru21-binaries/releases/download/jdk-21.0.5%2B11_openj9-0.48.0/ibm-semeru-open-jdk_x64_windows_21.0.5_11_openj9-0.48.0.zip 
build-jre.sh https://corretto.aws/downloads/latest/amazon-corretto-23-x64-windows-jdk.zip 
build-jre.sh  https://github.com/adoptium/temurin24-binaries/releases/download/jdk-24+23-ea-beta/OpenJDK-jdk_x64_windows_hotspot_24_23-ea.zip
build-jre.sh https://download.java.net/openjdk/jdk23/ri/openjdk-23+37_windows-x64_bin.zip 

I'm not so happy with overlapping vendor prefixes which was only done historically to make it easy to switch from openjdk URLs to adoptium URL without changing the bundle symbolic names. But environment variables can be used to override the default.

@chirontt
Copy link
Contributor Author

chirontt commented Dec 7, 2024

Using the latest from master, I've tested similar JDKs on my Linux x64 box:

export JDK_URLS_LINUX="https://github.com/ibmruntimes/semeru21-binaries/releases/download/jdk-21.0.5%2B11_openj9-0.48.0/ibm-semeru-open-jdk_x64_linux_21.0.5_11_openj9-0.48.0.tar.gz \
https://corretto.aws/downloads/latest/amazon-corretto-21-x64-linux-jdk.tar.gz \
https://github.com/adoptium/temurin24-binaries/releases/download/jdk-24%2B23-ea-beta/OpenJDK-jdk_x64_linux_hotspot_24_23-ea.tar.gz \
https://download.oracle.com/java/23/latest/jdk-23_linux-x64_bin.tar.gz"
./build-jre.sh

and I've encountered some errors:

./build-jre.sh: line 202: jdk[1-9-]*/bin/java: No such file or directory

I'm not sure what we can do about this JDK, apart from considering it an "edge case" to be ignored for now.

+ jdk='jdk-23.0.1 jdk-23_linux-x64_bin.tar.gz'

So I consider this Oracle-provided JDK as another "edge case" to be ignored for now.

Anyway, thanks @merks for all your help.

Last question: is there some task/goal in the existing Maven build script to generate the JREs and then create the p2 repo for them at the same time?

@merks
Copy link
Contributor

merks commented Dec 8, 2024

It seems odd that they would name the folder in the windows zip different from the one in the tar.gz:

image

image

This job does everything:

https://ci.eclipse.org/justj/job/build-jres/

It uses this:

https://github.com/eclipse-justj/justj/blob/master/releng/org.eclipse.justj.releng/Jenkinsfile

It will be helpful to read this:

https://eclipse.dev/justj/?page=developer

The builds involves producing a manifest for the results of build-jre.sh

https://download.eclipse.org/justj/jres/21/downloads/20241101_1100/justj.manifest

This is then used/reuse in the generation step to generate the maven structure used to drive the build of the p2 update sites.

@merks
Copy link
Contributor

merks commented Dec 8, 2024

For macos it has yet another name:

image

It all seems a bit "random". 😢

@merks
Copy link
Contributor

merks commented Dec 8, 2024

Something like this could get the root name from the tar file:

tar ztf /d/stuff/amazon-corretto-23.0.1.8.1-macosx-x64.tar.gz | head -1

Or this the root of the zip:

unzip -qql amazon-corretto-21-x64-windows-jdk.zip | head -n1 | tr -s ' ' | cut -d' ' -f5-

And then hope all the OSes produce the expected results from such a script...

merks added a commit that referenced this issue Dec 9, 2024
@merks
Copy link
Contributor

merks commented Dec 9, 2024

The issue with the folder name in the archive should be working now too, at least as far as I've tested in on Windows and in the build

https://ci.eclipse.org/justj/job/build-jres/

(The build is failing because of signing problems on Windows and MacOs for Java 24+26.)

@chirontt
Copy link
Contributor Author

I've re-run my script for Linux and it works now for the Amazon JDK (thanks for your hard work!), but still fails for the Oracle-provided JDK: https://download.oracle.com/java/23/latest/jdk-23_linux-x64_bin.tar.gz, as it works out the uncompressed JDK folder name wrongly:

+ jdk=jdk-23.0.1LICENSE

where the correct folder name should have been:

+ jdk=jdk-23.0.1

I think it's due to this line:

jdk=$(tar -ztf $file | head -1 | sed 's%/%%')

Something's screwy there, but as I said before, it's an "edge case" to be ignored for now.

merks added a commit that referenced this issue Dec 11, 2024
- The tar file might not include folder names but only file names.

Fixes #34
@merks
Copy link
Contributor

merks commented Dec 11, 2024

It looks like that tar file doesn't include folder names but only file names. Stripping out /.* rather than just the / should cover that case:

$echo $(tar -ztf jdk-23_linux-x64_bin.tar.gz | head -1 | sed 's%/.*%%')
jdk-23.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants