Move to Java 17 on CI

Hi devs,

We’re supposed to support Java 17 and thus we need to prove this. I’m proposing that we move our CI to build using Java 17 so that our tests are executed on Java 17.

WDYT?

Thanks

Note that @mflorea found a problem this morning with Java 17 and CKEditor’s build:

Starting CKBuilder…
Your JDK version is not supported, there may be a problem to finish build process. Please change the JDK version to 15 or lower. Archived OpenJDK GA Releases
Actual version of JDK: 17

It would be good to find these problems.

Don’t we need to continue building with Java 11 for the release (unless we want to drop Java 11 support) and thus wouldn’t building with Java 17 on CI make the release risky as we would discover problems with Java 11 during release only?

The source level is still 11 so that reduces the risks.

IMO, it’s more important to ensure that we execute fine with Java 17 than Java 11 if we have to pick one since we’re supposed to support Java 17 and 11 is EOL soon (Java/OpenJDK | endoflife.date). Now, what we’ve done in the past is mix the java version in the docker tests so that we have at least one functional test executing with the older java version in xwiki-jenkins-pipeline/dockerConfigurations.groovy at master · xwiki/xwiki-jenkins-pipeline · GitHub (see [Misc] XWiki 14.x doesn't run anymore on Java 8 · xwiki/xwiki-jenkins-pipeline@3fb70d7 · GitHub for example).

So I would propose to do the same.

And BTW we should also do 2 things:

Done

There’s no java 20 docker tag yet for Tomcat at Docker

For the docker tests this means:

  • 'tomcat' : [ 'latest' : '9-jdk11', 'lts' : '9-jdk11', 'special' : '9-jdk8' ]'tomcat' : [ 'latest' : '9-jdk17', 'lts' : '9-jdk17', 'special' : '9-jdk8' ]
  • 'jetty' : [ 'latest' : '10-jdk11', 'lts' : '10-jdk11' ]'jetty' : [ 'latest' : '10-jdk17', 'lts' : '10-jdk17' ]

And add a special category to continue having one test with 9-jdk11 for Tomcat.

Do you mean 9-jdk11 instead of 9-jdk8 for special? Because I think with JDK 8, the tests are all failing, at least they did when I saw them recently.

Yes my bad, it’s a mistake.

I don’t expect any compilation related surprises, thanks to https://github.com/xwiki/xwiki-commons/blob/2d48237e10a764cf93d6ee4db8504947b7062ce6/pom.xml#L2180. But we should still run integration tests with Java 11 too, ideally, as there are some runtime differences.

For the CKBuilder issue I found Java exception when running builder/build.sh · Issue #34 · ckeditor/ckbuilder · GitHub . They suggest adding this Java option:

--add-exports java.desktop/sun.java2d=ALL-UNNAMED

Seems to work then adding it manually to build.sh, but unfortunately there’s no easy / clean way to add this from the POM.

Done for the Docker tests, they now uses Java 17 and there’s one on Java 11 to make sure it keeps working at runtime, see [Misc] Move tests to Java 17 since it's now the latest supported Java… · xwiki/xwiki-jenkins-pipeline@35a5453 · GitHub

I wonder if you couldn’t use the jdk toolchain to force Java 11, see Exec Maven Plugin – Using toolchains with the exec:exec goal

AFAICS the code in builder.sh is:

jdk_version=$( echo `java -version 2>&1 | grep 'version' 2>&1 | awk -F\" '{ split($2,a,"."); print a[1]}'` | bc -l);
regex='^[0-9]+$';
# Builder is crashing when JDK version is newer than 15.
if ! [[ $jdk_version =~ $regex ]] || [ $jdk_version -gt 15 ]; then
        printf "${MSG_INCORRECT_JDK_VERSION}\n";
        printf "${UNDERLINE}${YELLOW}Actual version of JDK: ${jdk_version}${RESET_STYLE}\n";
fi

It seems to be just checking the version of java and doesn’t care about the --add-exports. I guess one other option would be to modify builder.sh as part of our build.

We should open an issue in their issue tracker ASAP too.

For the --add-exports java.desktop/sun.java2d=ALL-UNNAMED arguments, seems we can pass them to builder.sh and they’ll propagate to the java call.

See:

  • ARGS=" $@ "
  • JAVA_ARGS=${ARGS// -t / } # Remove -t from args.
  • and java -jar ckbuilder/$CKBUILDER_VERSION/ckbuilder.jar --build ../../ release $JAVA_ARGS --version="$VERSION" --revision="$REVISION" --overwrite

Yes, but it only logs a warning message and continues. The --add-exports fixes the issue for which the Java check was added. When I tried adding --add-exports the warning message was still showing of course but the build passed.

No, I tried this of course and it doesn’t work because --add-exports needs to be specified before -jar otherwise CKBuilder tries to parse it as its own parameter and fails. Java treats every argument passed after the class name or the jar file name as the arguments of the application (CKBuilder in this case).

I haven’t tried this, but wouldn’t it fail on CI when running in Docker because there we’ll have only one Java version available?

We have several versions of java available on the CI, see xwiki-docker-build/Dockerfile at master · xwiki/xwiki-docker-build · GitHub

It would mean using a profile to define the location of the jdk on the CI. What’s not great though is that it also requires a location when running the build locally so all devs would also need to define that in their settings.xml probably.

Patching the build.sh using the Maven Ant plugin (for example) is simpler I think and more portable. WDYT?

Yes, I prefer this. I’ll handle it.

Docker tests are now executed with java 17 but the build is still running on java 11 (see xwiki-docker-build/build/Dockerfile at master · xwiki/xwiki-docker-build · GitHub).