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

Update Jansi to 2.4.1 #106

Merged

Conversation

Friendseeker
Copy link
Contributor

@Friendseeker
Copy link
Contributor Author

Friendseeker commented Nov 9, 2024

Btw since the change involve native code, want to be more careful than usual.

What procedures should we perform to ensure nothing goes wrong from this PR? I had checked

private static final List<String> JNI_CLASS_NAMES = Arrays.asList(
"org.fusesource.jansi.internal.CLibrary",
"org.fusesource.jansi.internal.CLibrary$WinSize",
"org.fusesource.jansi.internal.CLibrary$Termios",
"org.fusesource.jansi.internal.Kernel32",
"org.fusesource.jansi.internal.Kernel32$SMALL_RECT",
"org.fusesource.jansi.internal.Kernel32$COORD",
"org.fusesource.jansi.internal.Kernel32$CONSOLE_SCREEN_BUFFER_INFO",
"org.fusesource.jansi.internal.Kernel32$CHAR_INFO",
"org.fusesource.jansi.internal.Kernel32$KEY_EVENT_RECORD",
"org.fusesource.jansi.internal.Kernel32$MOUSE_EVENT_RECORD",
"org.fusesource.jansi.internal.Kernel32$WINDOW_BUFFER_SIZE_RECORD",
"org.fusesource.jansi.internal.Kernel32$FOCUS_EVENT_RECORD",
"org.fusesource.jansi.internal.Kernel32$MENU_EVENT_RECORD",
"org.fusesource.jansi.internal.Kernel32$INPUT_RECORD");

And Jansi 2.4.1 still has the same set of classes, does that mean we don't need to change NativeImageFeature?

Also I saw recent Jansi build includes a jni-config.json, does that mean we can remove NativeImageFeature or do we still need to keep it?

https://github.com/fusesource/jansi/blob/012fdaa2efdb1bd11bc2a4b30736f958e1fa0482/pom.xml#L247

(introduced in fusesource/jansi@8bafdf7)

@alexarchambault
Copy link
Owner

What procedures should we perform to ensure nothing goes wrong from this PR?

I'd say publishing windows-ansi locally with these changes, building a coursier CLI native image with it and testing it (./mill -i nativeTests), then checking manually that one gets progress bars (rather than garbage…) when downloading things, by running a command like out/cli/base-image/nativeImage.dest/cs java --env --jvm zulu:21 in the Windows terminal, to trigger a new download (the zulu:21 JVM)

@alexarchambault
Copy link
Owner

Going to try that soon(ish), unless it's particularly straightforward for you or someone else reading this to try that?

@alexarchambault
Copy link
Owner

alexarchambault commented Dec 4, 2024

The tests don't have to be run on Windows ARM64, as:

  • there's no native image there (no GraalVM on Windows ARM64)
  • I can build a JAR-based coursier CLI launcher and run it from a Windows ARM64 VM (I have one that's working fine)

@Friendseeker
Copy link
Contributor Author

Going to try that soon(ish), unless it's particularly straightforward for you or someone else reading this to try that?

Thanks a lot! I currently don't have access to a Windows Machine so having you trying these out can really speed things up.

@alexarchambault
Copy link
Owner

alexarchambault commented Dec 17, 2024

Building a coursier launcher with jansi bumped to a locally published version of this PR, and running it in a Windows ARM64 VM currently gives me:

> java -jar coursier-2.1.21-new version
Exception in thread "main" java.lang.UnsatisfiedLinkError: 'long org.fusesource.jansi.internal.Kernel32.GetStdHandle(int)'
        at org.fusesource.jansi.internal.Kernel32.GetStdHandle(Native Method)
        at io.github.alexarchambault.windowsansi.WindowsAnsi.setup(WindowsAnsi.java:30)
        at coursier.cli.Coursier$.main(Coursier.scala:83)
        at coursier.cli.Coursier.main(Coursier.scala)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at coursier.bootstrap.launcher.a.a(Unknown Source)
        at coursier.bootstrap.launcher.ResourcesLauncher.main(Unknown Source)

(while it complains about being unable to load an AMD64 DLL on ARM64 with windows-ansi 0.0.5)

@alexarchambault
Copy link
Owner

alexarchambault commented Dec 17, 2024

It seems jansi may not be loading the Windows ARM64 DLL. It appears with a .so extension in the jansi 2.4.1 JAR:

$ unzip -l "$(cs get https://repo1.maven.org/maven2/org/fusesource/jansi/jansi/2.4.1/jansi-2.4.1.jar)"
…
    10196  10-12-2023 08:38   org/fusesource/jansi/internal/native/FreeBSD/x86/libjansi.so
    13228  10-12-2023 08:38   org/fusesource/jansi/internal/native/FreeBSD/x86_64/libjansi.so
    22032  10-12-2023 08:38   org/fusesource/jansi/internal/native/Linux/arm/libjansi.so
    19544  10-12-2023 08:38   org/fusesource/jansi/internal/native/Linux/arm64/libjansi.so
    15088  10-12-2023 08:38   org/fusesource/jansi/internal/native/Linux/armv6/libjansi.so
    14424  10-12-2023 08:38   org/fusesource/jansi/internal/native/Linux/armv7/libjansi.so
    73208  10-12-2023 08:38   org/fusesource/jansi/internal/native/Linux/ppc64/libjansi.so
    17376  10-12-2023 08:38   org/fusesource/jansi/internal/native/Linux/x86/libjansi.so
    18952  10-12-2023 08:38   org/fusesource/jansi/internal/native/Linux/x86_64/libjansi.so
    53036  10-12-2023 08:38   org/fusesource/jansi/internal/native/Mac/arm64/libjansi.jnilib
    14748  10-12-2023 08:38   org/fusesource/jansi/internal/native/Mac/x86/libjansi.jnilib
    15612  10-12-2023 08:38   org/fusesource/jansi/internal/native/Mac/x86_64/libjansi.jnilib
    82432  10-12-2023 08:38   org/fusesource/jansi/internal/native/Windows/arm64/libjansi.so
   115972  10-12-2023 08:38   org/fusesource/jansi/internal/native/Windows/x86/jansi.dll
   130522  10-12-2023 08:38   org/fusesource/jansi/internal/native/Windows/x86_64/jansi.dll
…

fusesource/jansi#292 probably fixes that, but it's not part of a jansi release yet.

@alexarchambault
Copy link
Owner

We might be able to work around it in the mean time by manually loading the .so file, trying that

@alexarchambault
Copy link
Owner

alexarchambault commented Dec 17, 2024

Seems it works by customizing the right Java property, like

> java -Dlibrary.jansi.name=libjansi.so -jar coursier-2.1.21-new-jansi version
2.1.21

@alexarchambault
Copy link
Owner

So the workaround should consist in detecting we're on Windows ARM64, checking if org/fusesource/jansi/internal/native/Windows/arm64/libjansi.so is in the resources (while org/fusesource/jansi/internal/native/Windows/arm64/jansi.dll is not), and setting this Java property

@Friendseeker
Copy link
Contributor Author

Friendseeker commented Dec 17, 2024

So the workaround should consist in detecting we're on Windows ARM64, checking if org/fusesource/jansi/internal/native/Windows/arm64/libjansi.so is in the resources (while org/fusesource/jansi/internal/native/Windows/arm64/jansi.dll is not), and setting this Java property

Hmm. Maybe we can wait for a few days to see if Jansi 2.4.2 will be released. I just made a comment fusesource/jansi#292 (comment) to request for a new Jansi release.

@alexarchambault
Copy link
Owner

alexarchambault commented Dec 17, 2024

@Friendseeker I added the workaround anyway, so that we don't have to wait any longer

@alexarchambault
Copy link
Owner

Merging, thanks @Friendseeker!

@alexarchambault alexarchambault merged commit 9ecdcde into alexarchambault:main Dec 17, 2024
2 checks passed
@Friendseeker Friendseeker deleted the bump-jansi-to-2-4.1 branch December 18, 2024 03:04
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.

coursier.jar fails to run on Windows Arm64
2 participants