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

Support using FFM in native-image #269

Merged
merged 16 commits into from
Oct 4, 2023
Merged

Conversation

Glavo
Copy link
Contributor

@Glavo Glavo commented Oct 2, 2023

See: https://www.graalvm.org/latest/reference-manual/native-image/dynamic-features/foreign-interface/

This PR provides FFM support for native-image.

Since FFM support in both JDK 21 and GraalVM is in preview, this PR is also experimental until GraalVM provides stable support for FFM.

Users need to provide the following options to the native-image tool to enable FFM support:

--enable-preview --enable-native-access=org.fusesource.jansi -Djansi.providers=ffm

If jansi is on the class path instead of the module path, replace org.fusesource.jansi with ALL-UNNAMED.

This PR is working, but there seems to be some glitch that I still need to look into

@Glavo
Copy link
Contributor Author

Glavo commented Oct 2, 2023

The purpose of this PR is to fix #246, but currently building static images will fail. I need to investigate more, maybe because GraalVM doesn't currently support using musl and ffm at the same time?

@Glavo Glavo marked this pull request as ready for review October 3, 2023 14:36
@Glavo
Copy link
Contributor Author

Glavo commented Oct 3, 2023

@gnodet This PR is completed. Unfortunately, it still doesn't solve #246 , so I need to try something else.

I refactored AnsiConsoleSupportHolder and OSInfo so that they can be initialized at build time.

In addition, I also made some adjustments so that only the JNI library of the corresponding platform is retained in native-image, and exclude all JNI libraries when users use FFM. But this only works for GraalVM 22.3+, so users of older GraalVM must add the -H:IncludeResources=org/fusesource/jansi/internal/native/.* option.

@Glavo
Copy link
Contributor Author

Glavo commented Oct 3, 2023

I have some new ideas for #246, and if this PR is merged, I can try new things based on it.

}

public static AnsiConsoleSupport.Kernel32 getKernel32() {
if (KERNEL32 == null) {
if (LibraryHolder.KERNEL32 == null) {
if (OSInfo.isWindows()) {
throw new RuntimeException("Unable to get the instance of Kernel32", ERR);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be LibraryHolder.ERR.

On a side note, I'm wondering if we should use singleton patterns (a single static field for the instance holding fields for the variables) for AnsiConsoleSupportHolder and LibraryHolder instead of using a bunch of static fields...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a side note, I'm wondering if we should use singleton patterns (a single static field for the instance holding fields for the variables) for AnsiConsoleSupportHolder and LibraryHolder instead of using a bunch of static fields...

I converted AnsiConsoleSupport to an abstract class and cached CLibrary and Kernel32 in it.

@@ -169,7 +182,7 @@ private static String readFully(InputStream in) throws IOException {
while ((readLen = in.read(buf, 0, buf.length)) >= 0) {
b.write(buf, 0, readLen);
}
return b.toString();
return b.toString("UTF-8");
Copy link
Member

Choose a reason for hiding this comment

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

StandardCharsets.UTF_8 ?

Copy link
Contributor Author

@Glavo Glavo Oct 4, 2023

Choose a reason for hiding this comment

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

StandardCharsets.UTF_8 ?

ByteArrayOutputStream::toString(Charset) is only available since Java 10, so we cannot use it here.

Copy link
Member

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

Two minor points to check...

@Glavo Glavo requested a review from gnodet October 4, 2023 06:49
@gnodet gnodet added this to the 2.5.0 milestone Oct 4, 2023
@gnodet gnodet merged commit 9d60bc5 into fusesource:master Oct 4, 2023
3 checks passed
@Glavo Glavo deleted the graal-ffm branch October 4, 2023 10:02
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.

2 participants