socketry / nio4r

Cross-platform asynchronous I/O primitives for scalable network clients and servers.
Other
965 stars 86 forks source link

fix java 8 compatibility #292

Closed ahorek closed 1 year ago

ahorek commented 1 year ago

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

related to https://github.com/puma/puma/pull/3109

Types of Changes

Contribution

ioquatix commented 1 year ago

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 commented 1 year ago

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.

ioquatix commented 1 year ago

I've rebased this branch on main.

ioquatix commented 1 year ago

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

ahorek commented 1 year ago

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 https://github.com/rake-compiler/rake-compiler/pull/213

ioquatix commented 1 year ago

I'm fine with just using the newer version.

MSP-Greg commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

LGTM