Add support for Java modules

anthonyvdotbe commented 1 year ago

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)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] 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.
[x] All new and existing tests passed.

PhilipRoman commented 1 year ago

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

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

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

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

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

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

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

TooTallNate / Java-WebSocket