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

Lift Java version requirement #659

Merged
merged 1 commit into from
Mar 10, 2024
Merged

Conversation

Bananeweizen
Copy link
Collaborator

The minimum version of 17 is needed since a while and should have been changed with the Tycho 4 upgrade already. The previous limit of the upper version is no longer needed, all involved tools work fine with 21.

Noticed because my personal default Java is 21 meanwhile, and the build would stop there, for no good reason.

The minimum version of 17 is needed since a while and should have been
changed with the Tycho 4 upgrade already. The previous limit of the
upper version is no longer needed, all involved tools work fine with 21.
@rnveach
Copy link
Member

rnveach commented Mar 9, 2024

If you want and is supported, a higher jdk can be added to show it is supported too.
https://github.com/checkstyle/eclipse-cs/blob/master/.github/workflows/maven.yml#L12

@Bananeweizen
Copy link
Collaborator Author

While that's technically possible (see https://maven.apache.org/enforcer/enforcer-rules/versionRanges.html) it's very uncommon to have something that is not an open/closed interval, and some tools might stumble then. In fact, that's exactly the root cause for checkstyle/checkstyle#14376. So I would really leave this as an open interval instead.

@rnveach
Copy link
Member

rnveach commented Mar 9, 2024

I would really leave this as an open interval instead.

I was speaking of the CI, not the POM, to showcase the upper JDK is supported. Sorry if I wasn't clear.

@romani
Copy link
Member

romani commented Mar 9, 2024

Keep moving forward to jdk 17, do not wait for main library to update, we will do it very soon.

@Bananeweizen
Copy link
Collaborator Author

Bananeweizen commented Mar 10, 2024

I was speaking of the CI, not the POM, to showcase the upper JDK is supported. Sorry if I wasn't clear.

Ah, got that wrong then. #660
Also if my comment on this PR isn't clear, the Java 17 requirement is only for the Maven JVM, not for the Eclipse plugin itself.

Keep moving forward to jdk 17, do not wait for main library to update, we will do it very soon.

Great news. I'll create another change for that. If you are doing some braking changes anyway, would you also consider removing Guava completely? Checkstyle is a bad guy in that regard: https://twitter.com/aalmiray/status/1757710195201823125. The Eclipse plugin is almost free of Guava on my local branch, so the plugin would not be affected by the core project removing it.

@rnveach
Copy link
Member

rnveach commented Mar 10, 2024

If you are doing some braking changes anyway, would you also consider removing Guava completely?

I doubt we will be doing breaking changes for the java upgrade. We already compile with the JRE.

When removing Guava only 2 files break. One seems very minor, but the other is ModuleReflectionUtil. We use Guava for our classpath scan to identify and pull in 3rd party modules we find on the path. We would need to find something similar to continue this functionality if we were to remove Guava.

This is besides the fact that we would need an approved issue to do this removal.

@romani
Copy link
Member

romani commented Mar 10, 2024

As far as I remember, this is the only usage of guava in our project, we need to find alternative to remove guava.

@rnveach
Copy link
Member

rnveach commented Mar 10, 2024

Someone needs to create an issue, approve it and move it forward, otherwise nothing will happen. Like I said there are 2 files that need to be modified and an alternative is needed. We also need to remove reflections library because since the move from Java 8, it hasn't been updated and basically does nothing. ronmamo/reflections#465 ronmamo/reflections#186

@Calixte Calixte merged commit 7d79931 into checkstyle:master Mar 10, 2024
7 checks passed
@Bananeweizen Bananeweizen deleted the java_17 branch March 11, 2024 17:42
Bananeweizen added a commit to Bananeweizen/eclipse-cs that referenced this pull request Mar 17, 2024
* require Java 17 at runtime
* require Eclipse 2022-09 as minimum target platform version (that's the
first platform version requiring Java 17)
* require eclipse.core 3.26, which is the plugin version contained in
2022-09. This avoids someone installing an upgrade in an older version
* remove Java 11 build

See checkstyle#659 for a discussion, that the main checkstyle project will switch
to Java 17 soon. We need to switch latest at the same time, but it's
also fine to switch earlier.
@Bananeweizen Bananeweizen mentioned this pull request Mar 17, 2024
Bananeweizen added a commit to Bananeweizen/eclipse-cs that referenced this pull request Mar 17, 2024
* require Java 17 at runtime
* require Eclipse 2022-09 as minimum target platform version (that's the
first platform version requiring Java 17)
* require eclipse.core 3.26, which is the plugin version contained in
2022-09. This avoids someone installing an upgrade in an older version
* remove Java 11 build
* require JUnit 5.9, since that version was delivered with Eclipse
2022-09

See checkstyle#659 for a discussion, that the main checkstyle project will switch
to Java 17 soon. We need to switch latest at the same time, but it's
also fine to switch earlier.
Calixte pushed a commit that referenced this pull request Mar 19, 2024
* require Java 17 at runtime
* require Eclipse 2022-09 as minimum target platform version (that's the
first platform version requiring Java 17)
* require eclipse.core 3.26, which is the plugin version contained in
2022-09. This avoids someone installing an upgrade in an older version
* remove Java 11 build
* require JUnit 5.9, since that version was delivered with Eclipse
2022-09

See #659 for a discussion, that the main checkstyle project will switch
to Java 17 soon. We need to switch latest at the same time, but it's
also fine to switch earlier.
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.

4 participants