Add support for Java 17 #186

mstruk · 2023-03-06T09:17:11Z

Added three profiles: java-8, java-11, and java-17 that activate automatically based on the JDK used
Fixed JavaDoc warnings so the project builds with Java 17
Fixed .travis/build.sh script to use Java 11 and run testsuite on x86_64

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

…ava-11 and java-17 Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

…te on `x86_64` Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

scholzj · 2023-03-06T09:48:16Z

.travis.yml

@@ -18,7 +18,6 @@ jobs:
        packages:
        - maven
 jdk:
- openjdk8
 - openjdk11


I guess you should add Java 17 here?

Yeah, makes sense.

.travis/build.sh

scholzj · 2023-03-06T09:54:55Z

.travis/build.sh

-if [ ${JAVA_MAJOR_VERSION} -eq 1 ] ; then
-  # some parts of the workflow should be done only once on the main build which is currently Java 8
+if [ "$arch" == 'x86_64' ] || [ "$arch" == 'arm64' ] ; then
+  # some parts of the workflow should be done only once on the main build which is currently Java 11


Should you check the Java version here?

If we have multiple java versions enabled as you suggested above then yes.

Actually, the question is do we want to run as many of the tests on as many of the platforms as possible.

I'm not sure but IIRC the Arquillian Cube based testsuite only worked on Java 8, that has been fixed by moving to TestContainers.
Another thing I remember is that the testsuite run would take so long or produce so much output that it triggered some limits and was killed by Travis.
I guess we can try again running the full testsuite on both Java 11 and Java 17, and if that will hit limits then I suggest we limit the testsuite to Java 11, that's due to the next paragraph ...

We have to decide which class version libraries we want to push to maven upon release.
I would still prefer Java 8 as that makes the jars most widely usable, but we decided to move on from there. Therefore I suggest Java 11 for the same reason.

I don't think anyone is prescribing what Java versions should Oauth support. We need Java 11 as a minimum. But if you want to support Java 8, then fine. But Java 8 is super old, Kafka will stop supporting it soon etc. So I think it is time to cut it. But even if you want to release with Java 8 as a target version, we need to have testing with Java 11 and Java 17

Ok, I added openjdk8 back to the Travis build, and reworked logic so that testsuite is run for all java versions on all platforms. Only Java 8 build on x86_64 will be published if it's a release build.

…ease 1.8 class version artifacts Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

…oo long Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

…e on s390x Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

…rimzi kafka images for this platform on quay.io Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

scholzj · 2023-03-06T19:47:57Z

.travis/build.sh

@@ -18,70 +18,58 @@ exitIfError() {
  [ "$EXIT" != "0" ] && exit $EXIT
 }

+arch=$(uname -m)
+echo "arch: $arch"


If you want to keep this, maybe it can be a bit nicer? Architecture: ?

Yes, let's have the printout to be sure that the actual architecture label is not something different then I'm expecting.
I can make it a bit nicer :)

scholzj · 2023-03-06T19:51:57Z

pom.xml

+        <profile>
+            <id>java-8</id>
+            <activation>
+                <jdk>1.8</jdk>
+            </activation>
+            <properties>
+                <maven.compiler.source>1.8</maven.compiler.source>
+                <maven.compiler.target>1.8</maven.compiler.target>
+            </properties>
+        </profile>
+        <profile>
+            <id>java-11</id>
+            <activation>
+                <jdk>[11,17)</jdk>
+            </activation>
+            <properties>
+                <maven.compiler.source>11</maven.compiler.source>
+                <maven.compiler.target>11</maven.compiler.target>
+                <maven.compiler.release>11</maven.compiler.release>
+            </properties>
+        </profile>
+        <profile>
+            <id>java-17</id>
+            <activation>
+                <jdk>[17,</jdk>
+            </activation>
+            <properties>
+                <maven.compiler.source>11</maven.compiler.source>
+                <maven.compiler.target>17</maven.compiler.target>
+                <maven.compiler.release>17</maven.compiler.release>
+            </properties>
+        </profile>


Both here and in the other pom.xml ... Do we really need the profiles here? The main point of these Java 11 and Java 17 builds is to ensure that it works in the environments using Java 11 and Java 17. So I do not think we need to change the source / target versions as we want it to be as close as possible to the code you will actually release.

I do not think we did anything like this on the other Strimzi projects either.

I found that maven.compiler.release option was necessary to build with Java 11 or 17. Without it I was getting an error like: system modules path not set in conjunction with -source

However, Java-8 build would not build if this option was present.

It could be the specifics of my local environment, but I just did whatever was necessary to get things to work.

The other thing there is the compiler source 11 in java-17 profile. This one I kind of forgot in there.
I'll set it to 17 so it's in sync with the others.

Not sure what maven.compiler.release does TBH. But maybe we can at least keep the source and target?

I mean ideally, we would want to have a single build and then test the binaries it produces with the different JDKs. But that would be quite complicated. But I think it would be great to get at least close to it.

In that case I would most prefer1.8. We could remove java-8 profile, and remove source and target overrides from java-11 and java-17 profiles, but they would still need the release set to 8.
Would that be better?

…x from testsuite build - hostname aliases not working Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

…vity on s390x Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

…jdks Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

mstruk · 2023-03-07T13:26:24Z

I modified the profiles and versions in pom.xml as described above, and fixed hostname aliases as .travis/build.sh so that a full testsuite run is passing on all java versions on amd64, and on the configured Java 11 version on s390x, where as the testsuite run is skipped on ppc64le due to unavailable docker images for that platform.

scholzj

Left two nits / questions. But if you think it is needed as it s, then its fine with me.

scholzj · 2023-03-07T13:49:04Z

testsuite/pom.xml

+        <profile>
+            <id>java-8</id>
+            <activation>
+                <jdk>1.8</jdk>
+            </activation>
+            <properties>
+                <maven.compiler.source>1.8</maven.compiler.source>
+                <maven.compiler.target>1.8</maven.compiler.target>
+            </properties>
+        </profile>
+        <profile>
+            <id>java-11</id>
+            <activation>
+                <jdk>[11,17)</jdk>
+            </activation>
+            <properties>
+                <maven.compiler.source>11</maven.compiler.source>
+                <maven.compiler.target>11</maven.compiler.target>
+                <maven.compiler.release>11</maven.compiler.release>
+            </properties>
+        </profile>
+        <profile>
+            <id>java-17</id>
+            <activation>
+                <jdk>[17,</jdk>
+            </activation>
+            <properties>
+                <maven.compiler.source>11</maven.compiler.source>
+                <maven.compiler.target>17</maven.compiler.target>
+                <maven.compiler.release>17</maven.compiler.release>
+            </properties>
+        </profile>


Should this use the same profile as the main pom.xml? Or is there some reason to have it different?

Ugh, missed that. Good catch.

pom.xml

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

mstruk added 4 commits March 6, 2023 09:58

JavaDoc fixes to be able to build with Java 17

b91e5f5

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Add three auto-activating profiles based on the jdk in use: java-8, j…

a641a12

…ava-11 and java-17 Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Modify Travis config to only use OpenJDK 11 and only run full testsui…

880d5c8

…te on `x86_64` Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Fix running the testsuite with Java 17 and via .travis/build.sh script

725a494

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

mstruk added this to the 0.12.0 milestone Mar 6, 2023

mstruk requested a review from scholzj March 6, 2023 09:25

scholzj reviewed Mar 6, 2023

View reviewed changes

mstruk added 8 commits March 6, 2023 15:07

Travis updates: add openjdk8 back, always run the full testsuite, rel…

44919a8

…ease 1.8 class version artifacts Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Reduce testsuite logging

78efb34

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Update .travis/build.sh

f7a54a1

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Update .travis/build.sh again

69bb9cb

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Update .travis/build.sh again, try to shorten the run that's taking t…

e318a6e

…oo long Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Update mock-oauth-server Dockerfile - switch to parent image availabl…

db00995

…e on s390x Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Update .travis/build.sh - exclude testsuite build for ppc64le - no St…

f35f4fb

…rimzi kafka images for this platform on quay.io Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Update mock-oauth-server Dockerfile again

116cf18

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

scholzj reviewed Mar 6, 2023

View reviewed changes

mstruk added 6 commits March 6, 2023 22:08

Fix java-17 profile + update .travis/build.sh output and exclude s390…

ed0ecc1

…x from testsuite build - hostname aliases not working Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Fix .travis/build.sh - properly exclude s390x from testsuite build

672665a

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Debug .travis/build.sh - add SKIP_DISABLED + debug localhost connecti…

7edef3a

…vity on s390x Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Try Fix s390x by adding host aliases to .travis.yml

ce9965c

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Try Fix s390x again + set target release to Java 1.8 for all the open…

4aa3d67

…jdks Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Hostname aliases should work now

b766e35

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

scholzj approved these changes Mar 7, 2023

View reviewed changes

Some more Travis and testsuite pom fixing

6504ace

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

mstruk merged commit 6f997c1 into strimzi:main Mar 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Java 17 #186

Add support for Java 17 #186

mstruk commented Mar 6, 2023

scholzj Mar 6, 2023

mstruk Mar 6, 2023

scholzj Mar 6, 2023

mstruk Mar 6, 2023

mstruk Mar 6, 2023

scholzj Mar 6, 2023

mstruk Mar 6, 2023

scholzj Mar 6, 2023

mstruk Mar 6, 2023

scholzj Mar 6, 2023

mstruk Mar 6, 2023

scholzj Mar 6, 2023

scholzj Mar 6, 2023

mstruk Mar 7, 2023 •

edited

mstruk commented Mar 7, 2023

scholzj left a comment

scholzj Mar 7, 2023

mstruk Mar 7, 2023

Add support for Java 17 #186

Add support for Java 17 #186

Conversation

mstruk commented Mar 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstruk Mar 7, 2023 • edited

Choose a reason for hiding this comment

mstruk commented Mar 7, 2023

scholzj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstruk Mar 7, 2023 •

edited