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

Fix build JDK version check to match documentation #148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sirhcel
Copy link

@sirhcel sirhcel commented Jan 19, 2024

The current documentation at https://badass-runtime-plugin.beryx.org/releases/1.13.0/ states:

The plugin requires Java 11 and Gradle 7.0 or newer. [...]

Building with JDK 17 actually works.

The current documentation at
https://badass-runtime-plugin.beryx.org/releases/1.13.0/ states 'The
plugin requires Java 11 and Gradle 7.0 or newer. [...]'. Building with
JDK 17 actually works.
@sirhcel
Copy link
Author

sirhcel commented Jan 19, 2024

Fixed faulty condition working by accident when committing late. Thank you for spotting this @srehlig!

@hakanai
Copy link
Collaborator

hakanai commented Jan 22, 2024

A common pitfall you might want to check is to build on JDK 17 and make sure the jar that comes out is still JDK 11 compatible. Just in case this project has just let Gradle choose the target version...

@janosvitok
Copy link

A common pitfall you might want to check is to build on JDK 17 and make sure the jar that comes out is still JDK 11 compatible. Just in case this project has just let Gradle choose the target version...

This may be useful: https://docs.gradle.org/current/userguide/toolchains.html and this https://blog.gradle.org/java-toolchains

As I understand it, the targetCompatibility should be defined to make sure the bytecode is compatible with JDK 11 or whatever is selected (when compiling under higher version. Toolkits should take care of this better.

FWIW, I tried to compile under JDK 21 (temurin, macos):

  • I had to upgrade gradle to 8.5
  • compilation when fine with this change
  • RuntimePluginSpec tests failed due to gradle 7.x not understanding java 21 (java.lang.IllegalArgumentException: Unsupported class file major version 65)

@sirhcel
Copy link
Author

sirhcel commented Feb 3, 2024

A common pitfall you might want to check is to build on JDK 17 and make sure the jar that comes out is still JDK 11 compatible. Just in case this project has just let Gradle choose the target version...

I'm controlling the build output in my project from build.gradle with:

sourceCompatibility = 1.8
targetCompatibility = 1.8

The original version of the check I'm addressing prevents me from using the plugin with a newer JDK at all.

Or is the documentation meant to read as (requires JDK 11) and (Gradle 7.0 or newer)?

@hakanai
Copy link
Collaborator

hakanai commented Feb 3, 2024

Oh, I meant in this project. The plugin project itself, as opposed to the project where you're using the plugin.

Because if some future maintainer ran the build on JDK 17, you wouldn't want them inadvertently publishing jars which can only be used on JDK 17. Some people using this plugin would expect it to work with JDK 11 still.

If it turned out that JDK 17 cannot build jars which are compatible with JDK 11 - then yeah, you'd want to update the docs instead. (I vaguely remember that it can do it, but I haven't tried it myself.)

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