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 java 8 compatibility #292

Merged
merged 1 commit into from
Apr 2, 2023
Merged

fix java 8 compatibility #292

merged 1 commit into from
Apr 2, 2023

Conversation

ahorek
Copy link
Contributor

@ahorek ahorek commented Mar 29, 2023

see https://www.morling.dev/blog/bytebuffer-and-the-dreaded-nosuchmethoderror/

related to puma/puma#3109

Types of Changes

  • Bug fix.

Contribution

@ioquatix
Copy link
Member

This looks okay to me in principle, however, please educate me.

What is Java 1.8?

What is Java 8?

Is Java 1.8, 7 major versions less than Java 8?

Is this just setting the minimum required "interface" version to something a little more recent?

Is Java 8 a reasonable minimum if that's the case?

@ahorek
Copy link
Contributor Author

ahorek commented Mar 29, 2023

Is Java 1.8, 7 major versions less than Java 8?

no, Java 1.8 is actually equal to Java 8 - it's just a historical versioning scheme kept for compatibility since Java ever existed :)

Java 8 (which is 10 years old) is the reasonable minimum. JRuby and Puma (except EOL versions) do support Java 8+ as well.

Is this just setting the minimum required "interface" version to something a little more recent?
--target_version doesn't actually ensure binary compatibility with older versions. --release flag does.

typical situations are:
1/ if you compile the gem using JDK 8, you can run it with JRE 8+ without problems
2/ but if you compile it using JDK 9+, but run it with an older version like JRE 8 it may not work if you're using a code that was changed between these versions, like https://www.morling.dev/blog/bytebuffer-and-the-dreaded-nosuchmethoderror/

--release flag ensures that you can safely compile the gem with any recent JDK without breaking compatibility with previous Java versions.

kou pushed a commit to rake-compiler/rake-compiler that referenced this pull request Mar 30, 2023
this allows using
```
  Rake::JavaExtensionTask.new("name", gemspec) do |ext|
    ext.release = '8'
  end
```
on Java 8 (for building the gem), because the flag is available since
Java 9, see
https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-source-and-target.html

this flag is for backward compatibility, so it's safe to just skip it if
we can't use it.

relates to puma/puma#3109
socketry/nio4r#292
@ioquatix
Copy link
Member

I've rebased this branch on main.

@ioquatix
Copy link
Member

It seems like macOS jruby doesn't like this new flag?

@ahorek
Copy link
Contributor Author

ahorek commented Mar 30, 2023

macOS 11 still uses Java 8 by default, but the compatibility flag is available since Java 9 (the resulting binary remains Java 8 compatible)

we could use macOS 12 instead which has a newer version I think... Or skip the test until there's an official release of rake-compiler/rake-compiler#213

@ioquatix
Copy link
Member

I'm fine with just using the newer version.

@MSP-Greg
Copy link
Contributor

JFYI, GitHub Actions macOS 12 image is setup with the following:

JAVA_HOME         /Users/runner/hostedtoolcache/Java_Temurin-Hotspot_jdk/8.0.362-9/x64/Contents/Home/
JAVA_HOME_11_X64  /Users/runner/hostedtoolcache/Java_Temurin-Hotspot_jdk/11.0.18-10/x64/Contents/Home/
JAVA_HOME_17_X64  /Users/runner/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.6-10/x64/Contents/Home/
JAVA_HOME_8_X64   /Users/runner/hostedtoolcache/Java_Temurin-Hotspot_jdk/8.0.362-9/x64/Contents/Home/

Don't know about standard macOS...

@ahorek
Copy link
Contributor Author

ahorek commented Mar 30, 2023

thanks @MSP-Greg , so we can just switch the JAVA_HOME to the right version like https://github.com/puma/puma/blob/master/.github/workflows/tests.yaml#L154 ?

@MSP-Greg
Copy link
Contributor

LGTM

@ioquatix ioquatix merged commit 640386b into socketry:main Apr 2, 2023
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