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

Require JDK 17 for building #536

Closed
gnodet opened this issue Jul 23, 2024 · 6 comments · Fixed by #537
Closed

Require JDK 17 for building #536

gnodet opened this issue Jul 23, 2024 · 6 comments · Fixed by #537
Labels
feature request A request for a new feature releng Anything related to release engineering or CI
Milestone

Comments

@gnodet
Copy link
Contributor

gnodet commented Jul 23, 2024

Description

Bump the minimal JDK version required for building to java 17.

Motivation

This will streamline the build, allow upgrading to more recent plugins.
The minimal JDK requirement for runtime will be unchanged at JDK 8.

@gnodet gnodet changed the title Require JDK 9 for building Require JDK 11 for building Jul 23, 2024
@garydgregory
Copy link
Member

+1 here.

gnodet added a commit to gnodet/mina-sshd that referenced this issue Jul 23, 2024
@tomaswolf
Copy link
Member

This might require changing the CI builds. We still should test on Java 8. Might require maven toolchains.

Otherwise fine by me.

@gnodet
Copy link
Contributor Author

gnodet commented Jul 23, 2024

This might require changing the CI builds. We still should test on Java 8. Might require maven toolchains.

I'm working on CI. I've set up a jdk8 profile which removes some plugins that require JDK 11. I don't think toolchains are required here.

@tomaswolf tomaswolf added feature request A request for a new feature releng Anything related to release engineering or CI labels Jul 23, 2024
@tomaswolf
Copy link
Member

I'm working on CI. I've set up a jdk8 profile which removes some plugins that require JDK 11. I don't think toolchains are required here.

Then we're not really requiring Java 11 for building. So still no multi-release.

I would require 11 (or even 17) for compiling/building always; just run the tests on different JVMs (8, 11, 17). That's why I mentioned toolchains. I think there is a way to tell surefire to use a specific toolchain.

gnodet added a commit to gnodet/mina-sshd that referenced this issue Jul 25, 2024
@gnodet
Copy link
Contributor Author

gnodet commented Jul 25, 2024

I'm working on CI. I've set up a jdk8 profile which removes some plugins that require JDK 11. I don't think toolchains are required here.

Then we're not really requiring Java 11 for building. So still no multi-release.

I would require 11 (or even 17) for compiling/building always; just run the tests on different JVMs (8, 11, 17). That's why I mentioned toolchains. I think there is a way to tell surefire to use a specific toolchain.

The #537 PR now does that. I've bumped to JDK 21, but we can go down if we want.

@tomaswolf
Copy link
Member

You can use JDK 21 in CI, but I would not require it as minimum for building.

Still looks more complicated that needed. I'm not at my development machine, so I cannot verify myself that this would work, but it seems to me the following patch would be a minimal change to build with Java 11 and test on JDK 8, 11, 17:

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 0eb87ee..7ab3828 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -29,15 +29,14 @@
     strategy:
       matrix:
         os: [ ubuntu-latest, windows-latest ]
-        java: [ '8' ]
     steps:
       - uses: actions/checkout@v4
 
-      - name: Set up JDK ${{ matrix.java }}
+      - name: Set up JDK 11
         uses: actions/setup-java@v4
         with:
           distribution: temurin
-          java-version: ${{ matrix.java }}
+          java-version: '11'
 
       - uses: actions/cache@v4
         with:
@@ -63,7 +62,10 @@
         uses: actions/setup-java@v4
         with:
           distribution: temurin
-          java-version: ${{ matrix.java }}
+          # Unsure whether this works if matrix.java == 11. If not, need two different setups with a conditional
+          java-version: |
+            ${{ matrix.java }}
+            '11'
 
       - uses: actions/cache@v4
         with:
@@ -74,7 +76,7 @@
 
       - name: Build and test with maven
         # Skip all static checks, they were already done in the compile jobs
-        run: mvn -B --errors --activate-profiles ci --no-transfer-progress package
+        run: mvn -B --errors --activate-profiles ci -Dtest.jdk.vendor=temurin -Dtest.jdk.version=${{ matrix.java }} --no-transfer-progress package
 
       - name: Archive test results and logs
         # if: success() || failure() to also get the test results on successful runs.
diff --git a/.github/workflows/master-build.yml b/.github/workflows/master-build.yml
index 7aa899e..1d5a991 100644
--- a/.github/workflows/master-build.yml
+++ b/.github/workflows/master-build.yml
@@ -47,7 +47,7 @@
         uses: actions/setup-java@v4
         with:
           distribution: temurin
-          java-version: '8'
+          java-version: '11'
           # Create a ~/.m2/settings.xml referencing these environment variable names
           server-id: 'apache.snapshots.https'
           server-username: NEXUS_USERNAME
diff --git a/pom.xml b/pom.xml
index fcbc28d..60ea820 100644
--- a/pom.xml
+++ b/pom.xml
@@ -138,9 +138,6 @@
     <profiles>
         <profile>
             <id>release</id>
-            <properties>
-                <required.java.version>[1.8,1.9)</required.java.version>
-            </properties>
         </profile>
 
         <profile>
@@ -149,6 +146,20 @@
                 <sshd.tests.timeout.factor>4.0</sshd.tests.timeout.factor>
                 <sshd.tests.rerun.count>4</sshd.tests.rerun.count>
             </properties>
+            <build>
+                <plugins>
+                    <plugin>
+                        <groupId>org.apache.maven.plugins</groupId>
+                        <artifactId>maven-surefire-plugin</artifactId>
+                        <configuration>
+                            <jdkToolchain>
+                               <version>${test.jdk.version}</version>
+                               <vendor>${test.jdk.vendor}</vendor>
+                            </jdkToolchain>
+                        </configuration>
+                    </plugin>
+                </plugins>
+            </build>
         </profile>
 
         <profile>

This should use toolchains only in CI, so building locally doesn't require people to define toolchains manually. Adapting to build with Java 21 and running tests on 8,11,17,21 would be simple.

gnodet added a commit to gnodet/mina-sshd that referenced this issue Jul 25, 2024
@gnodet gnodet changed the title Require JDK 11 for building Require JDK 17 for building Jul 25, 2024
@gnodet gnodet closed this as completed in 9dcb788 Jul 26, 2024
@tomaswolf tomaswolf added this to the 2.14.0 milestone Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A request for a new feature releng Anything related to release engineering or CI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants