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

Add support for Java modules #1309

Merged
merged 3 commits into from
Mar 7, 2023

Conversation

anthonyvdotbe
Copy link
Contributor

@anthonyvdotbe anthonyvdotbe commented Feb 23, 2023

Description

Add support for Java modules. The slf4j-api dependency was also updated to use a modularized version.

Related Issue

Fixes #1308

Motivation and Context

It allows projects that depend on this project to be modularized as well. And it allows encapsulating packages (such as the utils package).

How Has This Been Tested?

Building the project and verifying that the created JAR is now modular with jar -d -f .\Java-WebSocket-1.5.4-SNAPSHOT.jar --release 9

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@anthonyvdotbe anthonyvdotbe force-pushed the issue-1308 branch 2 times, most recently from aaa5ac1 to a312558 Compare February 24, 2023 12:30
@PhilipRoman
Copy link
Collaborator

Hi, I would like to merge this, but from what I see this is a breaking change - slf4j api requires java 8 after version 2.0.0 and the new build process requires at least java 9.
From README it seems like we currently promise JDK 7 compatibility.

@marci4 what are your thoughts on this?

If modules and slf4j-api are the only thing we need from java 9, and codebase remains java 7 compatible, maybe it makes sense to build two jars and release the new one under different name.

@marci4
Copy link
Collaborator

marci4 commented Mar 6, 2023

@PhilipRoman java 7 is only needed for android, but as far as I know these versions are still supported by google (they have at least access to the playstore) so I dont think we can drop this support.

We had a lot of problems when we used a java 7 api (https://github.com/TooTallNate/Java-WebSocket/wiki/No-such-method-error-setEndpointIdentificationAlgorithm) so...

@anthonyvdotbe
Copy link
Contributor Author

Hi, thanks for considering this.

the new build process requires at least java 9

Good point, I've updated .github/workflows. Note that, since I've used JDK 17, this will expose the issue that I've addressed in #1307, so that one should probably be merged first.

from what I see this is a breaking change

Unless I'm missing something, it's merely a build-breaking change under specific conditions: if a project X depends on Java-WebSocket, and X requires Java SE 7 compatibility, and its build tool determines the version of slf4j-api by taking the version as declared in Java-WebSocket, then yes, its build will break. However, to fix it, X simply has to explicitly declare the slf4j-api dependency with version 1.7.25, instead of relying on the declaration in Java-WebSocket.

If I'm missing something, please elaborate on your concerns.

maybe it makes sense to build two jars and release the new one under different name

The JAR as built will work correctly on any Java SE 7+ JVM, on 1 condition: Java-WebSocket must only use slf4j APIs which are available in both versions 1.7.25 and 2.0.6. But this is unlikely to ever be an issue as the project only uses a minimal number of essential slf4j methods.

On a final note: build.gradle hasn't been updated.
I'm not familiar enough with it and have been struggling to try to get it to do the separate compilation (i.e. compile module-info.java with 9+ and everything else with 7). If anyone wants to give it a try, here's some resources I've used during my attempts: 1, 2, 3.
However, this can actually be seen as "it's not a bug, it's a feature": by building with Gradle, you won't get a modular JAR (but as I understand, Maven is used for publishing the artifacts, so that's acceptable), but it will verify that the project still builds correctly with slf4j-api 1.7.25

@PhilipRoman
Copy link
Collaborator

Java-WebSocket must only use slf4j APIs which are available in both versions 1.7.25 and 2.0.6

sounds good

On a final note: build.gradle hasn't been updated.

That's not a problem. I usually use Gradle for building but that's only because of some extra scripts I have

I want to do some more tests before merging, in particular to verify that cross compiling doesn't introduce the dreaded ByteBuffer ABI incompatibility.

@PhilipRoman PhilipRoman changed the base branch from master to issue-1308 March 7, 2023 19:12
@PhilipRoman PhilipRoman merged commit 48a1cbf into TooTallNate:issue-1308 Mar 7, 2023
@PhilipRoman
Copy link
Collaborator

Looks like CI is failing
https://github.com/TooTallNate/Java-WebSocket/actions/runs/4357698514

Run actions/setup-java@v[3](https://github.com/TooTallNate/Java-WebSocket/actions/runs/4357698514/jobs/7617329596#step:3:3)
  with:
    java-version: 1[7](https://github.com/TooTallNate/Java-WebSocket/actions/runs/4357698514/jobs/7617329596#step:3:7)
    java-package: jdk
    check-latest: false
    server-id: github
    server-username: GITHUB_ACTOR
    server-password: GITHUB_TOKEN
    overwrite-settings: true
    job-status: success
    token: ***
Error: Input required and not supplied: distribution

@anthonyvdotbe
Copy link
Contributor Author

@PhilipRoman my bad. The docs also have a distribution attribute. I assumed it was optional, but apparently it isn't. Would you mind to add the attribute yourself? Or do you want me to provide a PR for it? Either way, sorry for the trouble.

@PhilipRoman
Copy link
Collaborator

Thanks for adding it, I have no idea how Github CI works 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants