Skip to content

Conversation

@ppkarwasz
Copy link
Contributor

This change:

  • Bumps the build requirement for the project to JDK 17.
  • Adds a java8-tests profile that runs tests using JRE 8. The profile activates if a Maven Toolchains configuration file is present, i.e., the user knows about Maven Toolchains. Toolchains are automatically configured by actions/setup-java.
  • Bumps the version of Error Prone to a version compatible with JDK 17 (i.e., latest).
  • Bumps the version of Maven Surefire to a version that supports the jdkToolchain property.
  • Locks the version of the remaining plugins to the currently used versions.

This change:

- Bumps the build requirement for the project to JDK 17.
- Adds a `java8-tests` profile that runs tests using JRE 8. The profile activates if a Maven Toolchains configuration file is present, i.e., the user knows about Maven Toolchains. Toolchains are automatically configured by `actions/setup-java`.
- Bumps the version of Error Prone to a version compatible with JDK 17 (i.e., latest).
- Bumps the version of Maven Surefire to a version that supports the `jdkToolchain` property.
- Locks the version of the remaining plugins to the currently used versions.
@jeremylong
Copy link
Collaborator

Thanks! However, for the the CI to work we also need to update

java-version: [ 8 ]
distro: [ 'zulu', 'temurin' ]

@jeremylong
Copy link
Collaborator

Any chance you could add a .java-version as well?

@ppkarwasz
Copy link
Contributor Author

Thanks! However, for the the CI to work we also need to update

Fixed in 5dd8b8f . Since tests run using JRE 8, two JDKs must be installed (or JRE 8 and JDK 17 if your prefer).

Any chance you could add a .java-version as well?

Fixed in 91d8d04

@dwalluck
Copy link
Contributor

Just a note that the spotbugs plugin would have to be bumped to 4.9.0 to support JDK 23.

@dwalluck dwalluck mentioned this pull request Mar 11, 2025
@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Mar 11, 2025

Just a note that the spotbugs plugin would have to be bumped to 4.9.0 to support JDK 23.

I would bump all Maven plugins. Previously many plugin versions were not specified, so their versions were determined by the Maven Lifecycle Bindings and were different for each version of Maven.

In this PR I fixed all the unspecified plugin versions to those of the Maven 3.8.7 Lifecycle Bindings (Debian 12 ships with Maven 3.8.7), but these are really old versions.

@dwalluck
Copy link
Contributor

I created #180 to try to prevent this, but I am not sure if your editor supports it https://editorconfig.org/#pre-installed.

A missing newline in a text file actually violates the POSIX standard as far as I know.

@ppkarwasz
Copy link
Contributor Author

I created #180 to try to prevent this, but I am not sure if your editor supports it https://editorconfig.org/#pre-installed.

Sure, I'll enable the EditorConfig plugin. For Java code, however, I would enforce a more precise formatting (see #183 ).

A missing newline in a text file actually violates the POSIX standard as far as I know.

Yes it does.

Comment on lines +347 to +353
<profile>
<id>java8-tests</id>
<activation>
<file>
<exists>${user.home}/.m2/toolchains.xml</exists>
</file>
</activation>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand this profile. If the build requires Java 17, how could this be activated and work? What am I missing?

Copy link
Contributor Author

@ppkarwasz ppkarwasz Mar 11, 2025

Choose a reason for hiding this comment

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

The purpose of this profile is to run tests using JRE 8 in forked JVMs.
Since this can be only done through Maven Toolchains, which might be hard to configure for single-time contributors, the profile only activates if a toolchains configuration is present.

Note that the CI always has a toolchains configuration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it - I've never set the Maven Toolchain up. Thanks for the explanation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't really used it either, but since GitHub automatically sets up the file, it's pretty painless.

I think this pull request makes it so that it's not required (presumably, the CI will test Java 8 if the developer does not), but if you're interested, it has a mojo to automatically try to generate the config file mvn toolchains:generate-jdk-toolchains-xml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! 💯
It discovered all 8 JDKs installed on my system.

@jeremylong jeremylong merged commit 1c5067d into package-url:master Mar 12, 2025
2 checks passed
@ppkarwasz ppkarwasz deleted the feature/jdk17-build branch March 12, 2025 11:52
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

Successfully merging this pull request may close these issues.

3 participants