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 #3109

Merged
merged 1 commit into from Mar 31, 2023
Merged

fix java 8 compatibility #3109

merged 1 commit into from Mar 31, 2023

Conversation

ahorek
Copy link
Contributor

@ahorek ahorek commented Mar 28, 2023

Description

it's a similar issue as #1772 unfortunately, that PR didn't fix all usage of buffers

since rake-compiler/rake-compiler#200 does support the --release flag for compatibility with older java versions, it's cleaner and safer to fix it this way instead of explicitly casting buffer methods as (Buffer) buffer

the downside is, it requires at least Java 9 for building the extension, but I guess it won't be a problem.

closes #3108

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

the only way to test it would be to build the extension with Java 9+, but run tests with Java 8.

@nateberkopec
Copy link
Member

Does this change our compatibility with older Java versions?

Copy link
Contributor

@kares kares left a comment

Choose a reason for hiding this comment

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

LGTM

Does this change our compatibility with older Java versions?

It will require Puma's JRuby ext to be compiled on Java >= 9 but the resulting gem (when the extension is build) stays compatible with Java 8.

Also according to CI there aren't any targets running with Java 8, which is fine it's just that with the release flag added those wouldn't work (there's a way around with having to switch Java versions while building the ext and running the test suite).

@ahorek
Copy link
Contributor Author

ahorek commented Mar 29, 2023

Does this change our compatibility with older Java versions?

Java 8 should be the minimum version for supported Puma and JRuby versions. According to the Puma 6.x changelog:

No longer supporting Java 1.7 or below (JRuby 9.1 was the last release to support this) (#2849)

https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-source-and-target.html

Merely setting the target option does not guarantee that your code actually runs on a JRE with the specified version. The pitfall is unintended usage of APIs that only exist in later JREs which would make your code fail at runtime with a linkage error. Better yet use the release option supported since JDK 9.

It will require Puma's JRuby ext to be compiled on Java >= 9 but the resulting gem (when the extension is build) stays compatible with Java 8.

if we also want to keep Java 8 compatibility for building the gem (maintainers, java 8 tests, or people who want to build the gem from the source on their own)
we can toggle the flag based on the Java version (which would be better to do in the rake-compiler itself). But does it matter? Java 8 is really old...

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
@nateberkopec
Copy link
Member

I'm fine dropping support for Java 8, but it just makes this a breaking change that needs to wait for Puma 7.

@kares
Copy link
Contributor

kares commented Mar 30, 2023

I'm fine dropping support for Java 8, but it just makes this a breaking change that needs to wait for Puma 7.

Just to clarify we do not do that here, the discussion Pavel and I had is mostly about being able to build from source on 8.
So the gem with this patch included should not break Java 8 compat, only the possibility to build from source on Java 8 (that would require more work but I think @ahorek and I agree it's not needed and that we can revisit that later, if anyone complains).

@ahorek
Copy link
Contributor Author

ahorek commented Mar 30, 2023

this change doesn't intend to break any existing compatibility. Regular users won't be affected, they still can use Java 8 + Puma.

here's a workaround rake-compiler/rake-compiler#213 but this is just for building a gem from the source (maintainers, tests on old environments with Java 8 etc.). I don't think it's a blocker.

@nateberkopec nateberkopec merged commit 1b53bc2 into puma:master Mar 31, 2023
66 of 67 checks passed
@nateberkopec
Copy link
Member

Thanks for the clarification 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug jruby waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running JRuby 9.4 and Puma with SSL
3 participants