hyperxpro / Brotli4j

Brotli4j provides Brotli compression and decompression for Java.
Apache License 2.0
102 stars 35 forks source link

Add native-linux-riscv64 package #111

Closed luhenry closed 10 months ago

luhenry commented 10 months ago

Motivation:

This patch adds support for linux-riscv64.

There was no code change other than adding support for linux-riscv64.

Modification:

It requires updating uraimo/run-on-arch-action to v2.4.0 for support for riscv64.

It also uses JDK 21 for riscv64 as it's the first LTS version with support for riscv64 (backports to JDK 17 done and JDK 11 in progress).

Result:

hyperxpro commented 10 months ago

Actually the problem is, if I compile this with JDK 21 then Maven will be pulling this automatically to downstream JDK(s) like 8 or 11 which will eventually break them badly.

luhenry commented 10 months ago

Actually the problem is, if I compile this with JDK 21 then Maven will be pulling this automatically to downstream JDK(s) like 8 or 11 which will eventually break them badly.

I'm not sure I understand the issue here. Do you mean that if we build with JDK 21, then it will mark the package as requiring JDK 21 which will break packages depending on brotli4j?

hyperxpro commented 10 months ago

Yup, that's right. Also, Netty 4.1 will never go beyond Java 6 and Netty 5 is also built with Java 11.

luhenry commented 10 months ago

I'm working on adding CI for JDK 11 and 17 on CI as well. Once it's working there, I'll add it to that PR.

Practically speaking nothing stops us from building on JDK 11 and 17 as well. They are supported on linux-riscv64, even if more slowly (until backport are complete) as it uses the full interpreter mode in the JDK (no architecture specific depencencies).

hyperxpro commented 10 months ago

Right but even running JDK 11 code on JDK 8 will throw a direct error. Is there any hopes for JDK 8?

luhenry commented 10 months ago

We can easily specify to build the packages to be runnable on JDK 8 with setting -source and -target passed to javac, and that works with JDK 11+ as well.

hyperxpro commented 10 months ago

I'd prefer that. :)

slandelle commented 10 months ago

We can easily specify to build the packages to be runnable on JDK 8 with setting -source and -target passed to javac, and that works with JDK 11+ as well.

@luhenry That's not necessarily true, see an example here.

Given how ByteBuffer is central to this library, I would really not recommend releasing with JDK11 while hoping to retain compatibility with Java 8. You would have to be able to run the test suite with Java 8 after compiling the main code with JDK11.

luhenry commented 10 months ago

@slandelle seems like we can use <maven.compiler.release>8</maven.compiler.release> then which does what we expect.

hyperxpro commented 10 months ago

+1 for running tests JDK 8 distros.

slandelle commented 10 months ago

@slandelle seems like we can use 8</maven.compiler.release> then which does what we expect.

Ah cool. Actually, JEP 247 (or maven) was probably buggy last time I tried. My sample now works.

hyperxpro commented 10 months ago

@slandelle These incompatibility issues have actually prevented me from adding more variety of architectures. 🥲

luhenry commented 10 months ago

I've added the use of --release for JDK 9+. For JDK 11 and 17 on RISC-V on CI (inside QEMU), it's requiring the backport of https://bugs.openjdk.org/browse/JDK-8310265, which I'm looking into now.

luhenry commented 10 months ago

I've added JDK 11 and 17 using the same workaround as s390x and aarch64 platforms (export MAVEN_OPTS="-Djdk.lang.Process.launchMechanism=vfork").

On the issue of no building on JDK 8, I don't expect JDK 8 to support RISC-V is the near-term. Where that change is IMO ok is that:

  1. given it doesn't change any other platform, all other platforms are still JDK 8 compatible, and,
  2. given RISC-V doesn't have JDK 8 across the board (not just Broatli4j), then there isn't really a need to support it.

Additionally we know it supports it because there is no RISC-V specific compilation flags for the Java code, so the same code is generated for RISC-V or any other platforms. Given Brotli4j, Netty, and any other project depending on Brotli4j will only run on JDK 11+ on RISC-V, checking that it runs on JDK 11 on RISC-V in Brotli4j is acceptable.

hyperxpro commented 10 months ago

I'd need a special test case to ensure the following:

When we run this on JDK 8, it should run fine, except the native library file is not being loaded. We need some conditional checks for JDK 8. And JDK 11+, we need to make sure the library loads fine. Basically, I want to test this:

<maven.compiler.release>8</maven.compiler.release>
hyperxpro commented 10 months ago

Let's compile this on JDK 11, and make sure tests pass normally. Then on same Docker runtime, install JDK 8 and run the special case without recompiling.

luhenry commented 10 months ago

Let's compile this on JDK 11, and make sure tests pass normally. Then on same Docker runtime, install JDK 8 and run the special case without recompiling.

Would you do that for RISC-V only, or all platforms? If for all platforms, that would help verify that the code generated by newer JDK is indeed compatible with JDK 8, however given you're only distributing the code generated by JDK 8, you're already pretty much guaranteed of that. And for RISC-V, the issue is there is no JDK 8 on RISC-V and I don't expect there will be one any time soon. The oldest supported version of the JDK on RISC-V is JDK 11.

I'm currently working on testing with JDK 8 on all platforms (expect RISC-V) when built with JDK 11 and 17.

hyperxpro commented 10 months ago

Yup, that's right. I want to test for all platforms (preferably x64 for now) on JDK 8 compiled with JDK 11 just to be double sure (since I have some trust issues with Maven builds).

luhenry commented 10 months ago

I want to test for all platforms (preferably x64 for now) on JDK 8 compiled with JDK 11 just to be double sure (since I have some trust issues with Maven builds).

@hyperxpro https://github.com/hyperxpro/Brotli4j/pull/111/commits/ace44cb164a114b5a9b564568e3f5dbd3c5f70a8 adds just that for all JDK 11 and 17 configurations (except RISC-V again given we don't have JDK 8 on RISC-V).

hyperxpro commented 10 months ago

Actually, I mean, running RISC-V build (JDK 11) on x64 (JDK 8) without recompilation.

hyperxpro commented 10 months ago

Let me know if you need helping hands for this.

luhenry commented 10 months ago

Actually, I mean, running RISC-V build (JDK 11) on x64 (JDK 8) without recompilation.

@hyperxpro that's done with running RISC-V build (with JDK 11) on aarch64 (with JDK 8) without recompilation. I picked aarch64 over x86_64 to make it easier to use uraimo/run-on-arch-action@v2.4.0 (which doesn't support x86_64).

luhenry commented 10 months ago

It should now be ready for final review. It tests on JDK 8 the RISC-V package build with JDK 11.

@hyperxpro @slandelle let me know if there is anything you'd like me to change. Thanks!

hyperxpro commented 10 months ago

Released https://github.com/hyperxpro/Brotli4j/releases/tag/v1.13.0