On Fri, 5 May 2023 12:34:29 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> To support JShell and other usecases, the JDK uses JLine, which provides
>> line-editing functionality inside a terminal.
>>
>> JLine has several ways to work with the terminal, based on various
>> additional libraries and tools. Most of them are not directly usable inside
>> the JDK, so currently, on non-Windows platforms, the JLine inside the JDK
>> will use external programs, like `stty`, to inspect the terminal's
>> properties and to switch the terminal to raw mode.
>>
>> This is slightly problematic, as the external programs might be missing, and
>> while this is not that big problem for JShell, it might be a problem for
>> other potential uses of JLine, like using it inside `System.console()`.
>>
>> So, the proposal here is to call the corresponding native methods directly,
>> on selected platforms for now (Linux and Mac OS/X), instead of invoking the
>> external programs. On Windows, this has always been the case, we are using a
>> custom implementation of the interface that maps native and Java function
>> for JNA there, and the proposal is to do the same here. We take the
>> appropriate mapping interface for JNA, and provide hand-written
>> implementation for it, using JNI.
>>
>> The Windows implementation is mostly unchanged, the changes are mostly
>> non-Windows only. The overview of the changes is:
>> - `LastErrorException` is moved from the Windows-specific code to the
>> platform-neutral code, as it is used by all the native implementations. This
>> is the only change that directly affects the Windows-specific code
>> -
>> `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaNativePty.java`
>> and
>> `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaTerminalProvider.java`
>> are created based on the corresponding files from JLine. In JLine, these
>> files directly call platform-specific implementations, but in this patch,
>> the platform specific code is commented out, and replaced with calls to
>> `JDKNativePty`, which is a new platform-specific class, that delegates to
>> the actual platform-specific implementations. Note the `JnaNativePty.java`
>> and `JnaTerminalProvider.java` already exist in the Windows part, but have
>> different pieces of code commented out, which makes them substantially
>> different.
>> - for Linux and Mac OS/X, there are platform-specific implementations based
>> on the corresponding code from JLine, with the hand-written JNI-based
>> implementation of the JNA-based existing interfaces. They also have an
>> implementation of `JDKNativePty`, which just delegates to the given
>> platform's `"NativePty"` implementation.
>> - the `JdkConsoleProviderImpl.java` has been tweaked to not allow the
>> implementation based on the external programs, and gracefully falling back
>> to the standard implementation. JShell is kept unchanged.
>>
>> The reasons for this organization are: limiting duplication, mostly adhering
>> to the JDK's platform specific build (which is different from the normal
>> JLine's platform-neutral build) and limiting the difference between standard
>> JLine and JLine inside JDK, to help future upgrades.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adjusting build script as suggested on the review.
src/jdk.internal.le/linux/native/lible/CLibrary.cpp line 2:
> 1: /*
> 2: * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights
> reserved.
Copyright year 2023 only if this is new.
src/jdk.internal.le/linux/native/lible/CLibrary.cpp line 103:
> 101:
> 102: if (tcgetattr(fd, &data) != 0) {
> 103: jobject exc = env->NewObject(lastErrorExceptionClass,
Could lines 103-106 be converted to a utility function which would then be
invoked at current lines 103, 141, 163, and 197?
src/jdk.internal.le/linux/native/lible/CLibrary.cpp line 116:
> 114: env->SetIntField(result, c_line, data.c_line);
> 115: jbyteArray c_ccValue = (jbyteArray) env->GetObjectField(result,
> c_cc);
> 116: env->SetByteArrayRegion(c_ccValue, 0, NCCS, (signed char *)
> data.c_cc);//TODO: cast?
Cast instead to `(jbyte*)` here and at lines 136 and 204?
src/jdk.internal.le/linux/native/lible/CLibrary.cpp line 183:
> 181: JNIEXPORT jint JNICALL
> Java_jdk_internal_org_jline_terminal_impl_jna_linux_CLibraryImpl_isatty
> 182: (JNIEnv *, jobject, jint fd) {
> 183: return isatty(fd);
Do we care if the native `isatty()` returns zero? Or is this dealt with
somewhere else?
src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 2:
> 1: /*
> 2: * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights
> reserved.
Copyright year 2023 only?
src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 59:
> 57:
> 58: JNIEXPORT void JNICALL
> Java_jdk_internal_org_jline_terminal_impl_jna_osx_CLibraryImpl_initIDs
> 59: (JNIEnv *env, jclass) {
Missing parameter for `jclass`? Maybe add `clazz` or `unused` or something?
src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 109:
> 107:
> 108: if (tcgetattr(fd, &data) != 0) {
> 109: jobject exc = env->NewObject(lastErrorExceptionClass,
Could lines 109-112 be converted to a utility function which would then be
invoked at current lines 109, 145, 167, and 197?
src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 121:
> 119: env->SetLongField(env->GetObjectField(result, c_lflag),
> nativelong_value, data.c_lflag);
> 120: jbyteArray c_ccValue = (jbyteArray) env->GetObjectField(result,
> c_cc);
> 121: env->SetByteArrayRegion(c_ccValue, 0, NCCS, (signed char *)
> data.c_cc);
Cast instead to `(jbyte*)` here and at lines 140 and 208?
src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 187:
> 185: JNIEXPORT jint JNICALL
> Java_jdk_internal_org_jline_terminal_impl_jna_osx_CLibraryImpl_isatty
> 186: (JNIEnv *, jobject, jint fd) {
> 187: return isatty(fd);
Do we care if the native `isatty()` returns zero? Or is this dealt with
somewhere else?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189114958
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189116097
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189116683
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189117836
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189123411
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189118782
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189120027
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189121218
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189121521
