Support jdk11 #208

Zwiterrion · 2023-01-09T09:49:30Z

Hi,

We want to use Extism on our project Otoroshi but we need to run it on jdk11

This pull request makes everything run smoothly on jdk11

If you have any suggestion about this pull request, i'm open to it

Thanks for your time

bhelx · 2023-01-09T14:05:15Z

.github/workflows/ci-java.yml

@@ -28,7 +28,7 @@ jobs:
        uses: actions/setup-java@v3
        with:
          distribution: 'temurin'
-          java-version: '17'
+          java-version: '11'


Should we perhaps setup a matrix here and test on both versions, or should we just be testing the lowest version and assuming the latest version is ok? I'm a bit unsure about what the best strategy is here for java.

I think having a matrix, consisting of the minimum supported version (v11 with this change) and at least a couple more recent versions (including the most recent version, v17 prior to this change) would be a good idea. The evolution of Java between version 11 and 17 is not something I am familiar with though, so this could be an impossible request. Feedback appreciated!

I tried to use a version of Extism compiled with jdk17 on a jvm running on 11, and of course, I got the following error:

java.lang.RuntimeException: java.lang.UnsupportedClassVersionError: org/extism/sdk/wasm/WasmSourceResolver has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 55.0

The solution may be to compile the library in two versions using a java version array in github actions..

Yea, I think the array of versions makes sense. I think we'd also need to inject that version from the matrix into the pom.xml on CI before running it though, right? Maybe other places too?

java/pom.xml

bhelx · 2023-01-09T14:06:54Z

Thanks @Zwiterrion !

nilslice · 2023-01-10T16:28:50Z

@Zwiterrion - I've added the matrix for version testing in this branch: https://github.com/extism/extism/tree/test-java-versions

Within that new branch, I've updated the CI to manipulate pom.xml with the version being tested, but not sure if there are other files that need similar changes. Would you be able to rebase your PR off that branch and push to see if your current changes work across these versions too?

I've got it set up to run versions 8, 11, and 17. But I'm not sure we need to go down to 8.. just figured it would be a good experiment. Please advise which version support you think is worth supporting!

bhelx · 2023-01-10T18:02:14Z

Perhaps @thomasdarimont has an opinion on this PR.

I support adding more version to the test matrix. But when it comes to publishing jars it gets unclear to me what the consequences are. I think priorities, in this order, would be:

Support as many JVM versions as possible for our users
Keep the build and release system as simple as possible
Allow us to write the source in the newest language but still target older JVMs.

thomasdarimont · 2023-01-10T18:32:18Z

Thanks for the ping. I think having support for JDK 11 is fine if we want to keep the JNA library for accessing the extism native lib.

So I'm fine with downgrading the current JDK version to 11.

Perhaps at a later stage we could ditch the JNA library and leverage the FFI support that's coming to the JDK.
I'm currently experimenting with leveraging the native FFI support from project panama that's already available with Java 19 and it already works quite well. JDK 21 will then be the next LTS (long term support) release for Java that will provide native support for FFI integrations.

thomasdarimont

A few remarks.

thomasdarimont · 2023-01-10T18:33:57Z

java/src/main/java/org/extism/sdk/manifest/ManifestHttpRequest.java

-public record ManifestHttpRequest(String url, Map<String, String> header, String method) {
+public class ManifestHttpRequest {
+
+    private String url;


Suggested change

private String url;

private final String url;

thomasdarimont · 2023-01-10T18:34:13Z

java/src/main/java/org/extism/sdk/manifest/ManifestHttpRequest.java

+
+    private String url;
+
+    private Map<String, String> header;


Suggested change

private Map<String, String> header;

private final Map<String, String> header;

thomasdarimont · 2023-01-10T18:34:26Z

java/src/main/java/org/extism/sdk/manifest/ManifestHttpRequest.java

+
+    private Map<String, String> header;
+
+    private String method;


Suggested change

private String method;

private final String method;

thomasdarimont · 2023-01-10T18:35:06Z

java/src/main/java/org/extism/sdk/manifest/ManifestHttpRequest.java

@@ -3,6 +3,12 @@
 import java.util.Map;

 // FIXME remove this and related stuff if not supported in java-sdk
-public record ManifestHttpRequest(String url, Map<String, String> header, String method) {
+public class ManifestHttpRequest {


I'd make the fields final, create accessor methods and create a constructor for passing the required parameters in.

thomasdarimont · 2023-01-10T18:35:30Z

java/src/main/java/org/extism/sdk/manifest/MemoryOptions.java

-public record MemoryOptions(@SerializedName("max") Integer max) {
+public class MemoryOptions {
+
+    private @SerializedName("max") Integer max;


Suggested change

private @SerializedName("max") Integer max;

@SerializedName("max")

private final Integer max;

thomasdarimont · 2023-01-10T18:37:37Z

java/src/main/java/org/extism/sdk/wasm/ByteArrayWasmSource.java

+    @Override
+    public String hash() {
+        return hash;
+    }
 }


provide an accessor method for data.

thomasdarimont · 2023-01-10T18:38:00Z

java/src/main/java/org/extism/sdk/wasm/PathWasmSource.java

-public record PathWasmSource(String name, String path, String hash) implements WasmSource {
+public class PathWasmSource implements WasmSource {
+
+    private String name;


Suggested change

private String name;

private final String name;

thomasdarimont · 2023-01-10T18:38:09Z

java/src/main/java/org/extism/sdk/wasm/PathWasmSource.java

+
+    private String name;
+
+    private String path;


Suggested change

private String path;

private final String path;

thomasdarimont · 2023-01-10T18:38:21Z

java/src/main/java/org/extism/sdk/wasm/PathWasmSource.java

+
+    private String path;
+
+    private String hash;


Suggested change

private String hash;

private final String hash;

thomasdarimont · 2023-01-10T18:38:38Z

java/src/main/java/org/extism/sdk/wasm/PathWasmSource.java

+    @Override
+    public String hash() {
+        return hash;
+    }
 }


Provide an accessor method for path.

chabala · 2023-01-11T06:15:37Z

@nilslice regarding:

I think we'd also need to inject that version from the matrix into the pom.xml on CI before running it though, right?

and

I've added the matrix for version testing in this branch: https://github.com/extism/extism/tree/test-java-versions

Within that new branch, I've updated the CI to manipulate pom.xml with the version being tested, but not sure if there are other files that need similar changes.

I don't think that's a good idea. Java is backward compatible in the sense that newer JREs can recognize older bytecode, but not vice versa. The pom.xml should contain the oldest version the project targets, and the produced JAR tested on multiple JRE versions, not changed on the fly and built multiple times. The JAR built with a pom specifying 17 will absolutely fail on earlier versions as @Zwiterrion noted.

nilslice · 2023-01-11T06:19:54Z

@chabala - 😄 uh oh, my "I don't know Java" is showing...

I just wanted to make sure we continue to test newer versions as well. I assumed we'd need to update the pom.xml to coincide with the version of Java being used, but if that's not the case then that simplifies things!

chabala · 2023-01-11T06:27:34Z

No worries 😆 I figured I'd chime in as things looked to be going in a weird direction. I had asked about why Java 17? in the r/java announcement post, so I figured I'd check in on how supporting earlier versions was going. I'd support your idea of going as far back as version 8 as well, unless you have other requirements that need newer language features; 8 will be in use for a long time yet.

thomasdarimont · 2023-01-11T11:40:03Z

I suggest using at least Java 11. Java 8 is now almost 10 years old and many Java open source projects have removed support for Java 8.

nilslice · 2023-01-11T17:35:44Z

I suggest using at least Java 11. Java 8 is now almost 10 years old and many Java open source projects have removed support for Java 8.

This sounds good to me. Unless someone feels very strongly about supporting Java 8, and would like to test & make whatever library changes are needed to support it, I think we can use Java 11 as the minimum supported version.

nilslice · 2023-01-11T17:39:35Z

@Zwiterrion - thank you for your work on this! When you have a minute, would you be able to include the changes suggested by @thomasdarimont? Thanks to my new knowledge from @chabala, the pom.xml seems like it's already good as-is in your branch, and we should just add the matrix from my branch to include [11, 17] versions in the Java CI workflow.

What do you think?

Zwiterrion · 2023-01-12T08:48:32Z

@nilslice - It's fine for me, i'll try to include all changes suggested by @thomasdarimont on my branch.

nilslice · 2023-01-18T00:20:56Z

Hi @Zwiterrion - just wanted to check in on this. I looks like you made most of the changes - is my observation correct? We'd love to include this in the upcoming 0.2.0 release, but let me know if there is anything left here or if you would like some help!

Zwiterrion · 2023-01-18T13:39:53Z

Hi @nilslice, I made the needed changes. The branch is ready to be merged.

Hi, We want to use Extism on our project [Otoroshi](https://github.com/MAIF/otoroshi) but we need to run it on jdk11 This pull request makes everything run smoothly on jdk11 If you have any suggestion about this pull request, i'm open to it Thanks for your time

Zwiterrion added 2 commits January 9, 2023 10:38

support jdk11

85da084

support jdk11

ab500f5

bhelx reviewed Jan 9, 2023

View reviewed changes

java/pom.xml Show resolved Hide resolved

thomasdarimont suggested changes Jan 10, 2023

View reviewed changes

rewrite records and add multiple version test strategy for java sdk

8218c7b

Zwiterrion mentioned this pull request Jan 18, 2023

research around WASM integration MAIF/otoroshi#1278

Closed

bhelx approved these changes Jan 18, 2023

View reviewed changes

bhelx merged commit ac7e1ae into extism:main Jan 18, 2023

Zwiterrion mentioned this pull request Jan 18, 2023

Add support for wasm plugins in a route MAIF/otoroshi#1414

Merged

7 tasks

Zwiterrion deleted the support-jdk11 branch January 30, 2024 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support jdk11 #208

Support jdk11 #208

Zwiterrion commented Jan 9, 2023

bhelx Jan 9, 2023

nilslice Jan 10, 2023

Zwiterrion Jan 10, 2023

nilslice Jan 10, 2023

bhelx commented Jan 9, 2023

nilslice commented Jan 10, 2023 •

edited

bhelx commented Jan 10, 2023

thomasdarimont commented Jan 10, 2023

thomasdarimont left a comment

thomasdarimont Jan 10, 2023

thomasdarimont Jan 10, 2023

thomasdarimont Jan 10, 2023

thomasdarimont Jan 10, 2023

thomasdarimont Jan 10, 2023

thomasdarimont Jan 10, 2023

thomasdarimont Jan 10, 2023

thomasdarimont Jan 10, 2023

thomasdarimont Jan 10, 2023

thomasdarimont Jan 10, 2023

chabala commented Jan 11, 2023

nilslice commented Jan 11, 2023

chabala commented Jan 11, 2023

thomasdarimont commented Jan 11, 2023

nilslice commented Jan 11, 2023

nilslice commented Jan 11, 2023

Zwiterrion commented Jan 12, 2023

nilslice commented Jan 18, 2023

Zwiterrion commented Jan 18, 2023

	private Map<String, String> header;
	private final Map<String, String> header;

	private @SerializedName("max") Integer max;
	@SerializedName("max")
	private final Integer max;

Support jdk11 #208

Support jdk11 #208

Conversation

Zwiterrion commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhelx commented Jan 9, 2023

nilslice commented Jan 10, 2023 • edited

bhelx commented Jan 10, 2023

thomasdarimont commented Jan 10, 2023

thomasdarimont left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chabala commented Jan 11, 2023

nilslice commented Jan 11, 2023

chabala commented Jan 11, 2023

thomasdarimont commented Jan 11, 2023

nilslice commented Jan 11, 2023

nilslice commented Jan 11, 2023

Zwiterrion commented Jan 12, 2023

nilslice commented Jan 18, 2023

Zwiterrion commented Jan 18, 2023

nilslice commented Jan 10, 2023 •

edited