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 native-linux-riscv64 package #111

Merged
merged 10 commits into from
Oct 25, 2023

Conversation

luhenry
Copy link
Contributor

@luhenry luhenry commented Oct 23, 2023

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:

This patch adds support for linux-riscv64.

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).
@hyperxpro
Copy link
Owner

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
Copy link
Contributor Author

luhenry commented Oct 23, 2023

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
Copy link
Owner

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

@luhenry
Copy link
Contributor Author

luhenry commented Oct 23, 2023

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
Copy link
Owner

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

@luhenry
Copy link
Contributor Author

luhenry commented Oct 23, 2023

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
Copy link
Owner

I'd prefer that. :)

@slandelle
Copy link
Contributor

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
Copy link
Contributor Author

luhenry commented Oct 23, 2023

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

@hyperxpro
Copy link
Owner

+1 for running tests JDK 8 distros.

@slandelle
Copy link
Contributor

@slandelle seems like we can use <maven.compiler.release>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
Copy link
Owner

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

@luhenry
Copy link
Contributor Author

luhenry commented Oct 23, 2023

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.

This JDK has a dependency on glibc 2.34 which is not available in Ubuntu 20.04
@luhenry
Copy link
Contributor Author

luhenry commented Oct 24, 2023

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
Copy link
Owner

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
Copy link
Owner

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.

Copy link

@gctony gctony left a comment

Choose a reason for hiding this comment

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

Not familiar with this codebase. But, to me, the changes look good.

@luhenry
Copy link
Contributor Author

luhenry commented Oct 25, 2023

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.

Except on RISC-V where JDK 8 is not supported. But given there is no RISC-V specific code, it would be verified that compiling with JDK 11/17 and running on JDK 8 still functions.
@hyperxpro
Copy link
Owner

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
Copy link
Contributor Author

luhenry commented Oct 25, 2023

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 ace44cb 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
Copy link
Owner

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

@hyperxpro
Copy link
Owner

Let me know if you need helping hands for this.

@luhenry
Copy link
Contributor Author

luhenry commented Oct 25, 2023

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
Copy link
Contributor Author

luhenry commented Oct 25, 2023

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!

export MAVEN_OPTS="-Djdk.lang.Process.launchMechanism=vfork"
chmod +x ./mvnw
# Build native-linux-aarch64 only
./mvnw -B --show-version -ntp clean package -pl :native-linux-aarch64 -DskipTests
Copy link
Owner

Choose a reason for hiding this comment

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

When we do this, it will compile native-linux-aarch64 and run its tests not native-linux-riscv64.

I guess this is not easily achievable, let me think of something. I will do a direct push.

@hyperxpro hyperxpro changed the base branch from main to riscv64 October 25, 2023 17:16
Copy link
Owner

@hyperxpro hyperxpro left a comment

Choose a reason for hiding this comment

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

I will take it from here. :)

@hyperxpro hyperxpro merged commit b51e960 into hyperxpro:riscv64 Oct 25, 2023
19 checks passed
hyperxpro added a commit that referenced this pull request Oct 26, 2023
* Add native-linux-riscv64 package (#111)

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:

Support for riscv64 architecture.
---------

Co-authored-by: Ludovic Henry <git@ludovic.dev>
@hyperxpro
Copy link
Owner

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

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

Successfully merging this pull request may close these issues.

4 participants