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. Build changes look good. ------------- Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13687#pullrequestreview-1414782534