Use the C API in native images #119
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I did try out your branch. I'm on Linux amd64. When using val exportProgress = terminal.progressAnimation {
percentage()
progressBar()
completed()
speed("layers/s")
timeRemaining()
}
exportProgress.start()
exportProgress.updateTotal(1)
exportProgress.advance(1)
exportProgress.stop()I get the following error: Also mind the question marks for the progress bar chars. How could I assist here? Not much of a C / JNA guy myself. |
|
@cmdjulian I could not reproduce the issue. The stacktrace doesn't seem related to this PR. |
Maybe it was related to the way I did replace the package in my build.gradle.kts file. I'm on GraalVM community version 17 the latest. |
|
Mmh, maybe. To use this PR in another gradle project, I did the following: In the mordant repo:
And in the project using mordant, repositories {
mavenCentral {
mavenContent {
excludeGroup("com.github.ajalt.mordant")
}
}
mavenLocal {
mavenContent {
includeGroup("com.github.ajalt.mordant")
}
}
} |
|
Yep it works now, must have been my mistake. Thanks :) Regarding Mac, static compile is currently not supported on it with Apple Silicon. However, for non static compile it does work. I had to let one of my coworkers check it out. Is there something else I can do in helping you in making this MR ready and getting merged? We're really looking forward into this getting merged and usable for one of our cli applications. |
|
Good news then :) What do you think @ajalt, should I remove the sample commit and set this PR as ready ? Or is there any change I need to do ? |
|
If it's been tested on all three platforms, then that's great. Do you know if it's feasible to set up graal on CI and use it to compile and run the test suite? That would certainly give me more confidence in it being correct. |
|
I tried a bit, and it is sadly not possible to apply the |
|
If a test submodule is our only option, then let's go with that and make a simple smoke test. I think the bulk of the code could be exercised by testing a progress animation and markdown rendering. Something like this: class GraalSmokeTest {
@Test
fun `progress animation test`() {
// Just make sure it doesn't crash, exact output is verified in the normal test suite
val t = Terminal(interactive = true, ansiLevel = AnsiLevel.TRUECOLOR)
val animation = t.progressAnimation { progressBar() }
animation.start()
Thread.sleep(100)
animation.clear()
}
@Test
fun `markdown test`() {
val vt = TerminalRecorder()
val t = Terminal(terminalInterface = vt)
t.print(Markdown("- Some **bold** text"))
assertEquals(" • Some ${bold("bold")} text", vt.output())
}
} |
5042aab
to
02a1975
Compare
ajalt
reviewed
Sep 26, 2023
mordant/src/jvmMain/kotlin/com/github/ajalt/mordant/internal/jna/JnaMacosMppImpls.kt
Show resolved
Hide resolved
This replaces all uses of JNA by the Native Image C API
On windows, the binary failed with:
```
Exception in thread "main" java.lang.IllegalArgumentException: Can't create an instance of class com.sun.jna.ptr.IntByReference, requires a public no-arg constructor: java.lang.NoSuchMethodException: com.sun.jna.ptr.IntByReference.<init>()
at com.sun.jna.Klass.newInstance(Klass.java:64)
at com.sun.jna.NativeMappedConverter.defaultValue(NativeMappedConverter.java:65)
at com.sun.jna.NativeMappedConverter.<init>(NativeMappedConverter.java:56)
at com.sun.jna.NativeMappedConverter.getInstance(NativeMappedConverter.java:45)
at com.sun.jna.Function.convertArgument(Function.java:509)
at com.sun.jna.Function.invoke(Function.java:345)
at com.sun.jna.Library$Handler.invoke(Library.java:270)
at com.github.ajalt.mordant.internal.$Proxy50.GetConsoleMode(Unknown Source)
at com.github.ajalt.mordant.internal.Win32MppImpls.stdinInteractive(JnaMppImplsWin32.kt:82)
...
```
On linux, JNA also caused issues when creating a static binary.
With `--static --libc=glibc`, this occured:
```
Exception in thread "main" java.lang.UnsatisfiedLinkError: Error looking up function 'isatty': native: undefined symbol: isatty
at com.sun.jna.Function.<init>(Function.java:252)
at com.sun.jna.NativeLibrary.getFunction(NativeLibrary.java:620)
at com.sun.jna.NativeLibrary.getFunction(NativeLibrary.java:596)
at com.sun.jna.NativeLibrary.getFunction(NativeLibrary.java:582)
at com.sun.jna.Library$Handler.invoke(Library.java:248)
at jdk.proxy4/jdk.proxy4.$Proxy49.isatty(Unknown Source)
at com.github.ajalt.mordant.internal.LinuxMppImpls.stdinInteractive(JnaMppImplsLinux.kt:46)
...
```
With `--static --libc=musl`, There was no error but Since there was an
`UnsatisfiedLinkError` thrown during the instanciation of
`JnaLinuxMppImpls`, the `FallbackJnaMppImpls` was used.
With this commit, we don't need the GraalVM reachability metadata.
hubvd
commented
Sep 27, 2023
| @@ -45,4 +50,4 @@ jobs: | |||
| path: build-reports.zip | |||
|
|
|||
| env: | |||
| GRADLE_OPTS: -Dorg.gradle.configureondemand=true -Dorg.gradle.parallel=true -Dkotlin.incremental=false -Dorg.gradle.project.kotlin.incremental.multiplatform=false -Dorg.gradle.project.kotlin.native.disableCompilerDaemon=true -Dorg.gradle.jvmargs="-Xmx5g -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8" | |||
| GRADLE_OPTS: -Dorg.gradle.configureondemand=true -Dorg.gradle.parallel=false -Dkotlin.incremental=false -Dorg.gradle.project.kotlin.incremental.multiplatform=false -Dorg.gradle.project.kotlin.native.disableCompilerDaemon=true -Dorg.gradle.jvmargs="-Xmx256m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8" | |||
There was a problem hiding this comment.
I had to do this, as the Windows action didn't have enough memory for building the native image
There was a problem hiding this comment.
That's too bad. It looks like the build take 50% longer now, but it's better than not testing.
| pluginManagement { | ||
| repositories { | ||
| gradlePluginPortal() | ||
| mavenCentral() |
There was a problem hiding this comment.
This is needed for the Native Build Tools Gradle plugin:
https://graalvm.github.io/native-build-tools/latest/gradle-plugin.html#_adding_the_plugin
| @@ -34,6 +34,12 @@ kotlin { | |||
| } | |||
| } | |||
|
|
|||
| val jvmMain by getting { | |||
| dependencies { | |||
| compileOnly(libs.graalvm.svm) | |||
There was a problem hiding this comment.
Since we are only using the annotations, we don't need to depend on the library at runtime, meaning users of mordant won't have a new dependency on the classpath :)
ajalt
approved these changes
Sep 27, 2023
|
Thank you for all the work! |
|
Is there a date when you do create a new release of clikt including this new version here? @ajalt |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #112
This fixes some issues when using mordant in a GraalVM Native Image. (More details in the commits)
I tested this on Linux x64 and Windows x64 but I don't have access to MacOS for testing. So maybe we would still need to fallback to
sttyif it doesn't work.I also added a commit with the native-build-tools. With the GRAALVM_HOME variable set, we can build a binary by running
./gradlew native:nativeCompile. This commit can be removed when the PR is ready to be merged.