On Wed, 20 Mar 2024 18:55:38 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> This is a patch that: >> a) upgrades the JLine inside the JDK to 3.25.1 >> b) since the new version of JLine has a FFM backend, our custom native >> backends are removed, and replaced with the FFM backend >> >> Some changes had to be made to the original JLine in order to fit into the >> JDK. Most of them were already done for the previous version (repackaging, >> avoiding non-ASCII characters, commenting out logging, adding ability to >> modify to wrap the InputStream used by the terminal), and have only been >> transferred to the new one. The main two new changes are: >> - fixes to the FFM backend, so that it works on Linux and JDK 22. These have >> been proposed to JLine itself: https://github.com/jline/jline3/pull/945 >> - disabling the `NativeFileDescriptorCreator`, as I believe we don't need >> it, and cannot make it work easily >> >> There's a full patch between the >> `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content >> of the corresponding sources of these original JLine sub-projects: >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal >> the patch is here: >> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff >> >> I've also cleaned the patch a little removing most of the changes for the >> rename. The result is here: >> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 26 commits: > > - Merge branch 'master' into jline-upgrade-3.25.1 > - Merge branch 'master' into jline-upgrade-3.25.1 > - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 > - Merge remote-tracking branch 'origin/native-access-modules1' into > native-access-modules1 > - Apply suggestions from code review > > Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> > Co-authored-by: Maurizio Cimadamore > <54672762+mcimadam...@users.noreply.github.com> > - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 > - Reflecting review feedback. > - Updating copyright headers. > - Re-enabling the exec provider. > - Cleanup. > - ... and 16 more: https://git.openjdk.org/jdk/compare/e5e7cd20...c8097592 src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java line 517: > 515: String hwName; > 516: try { > 517: Process p = Runtime.getRuntime().exec(new String[] > {"uname", "-m"}); This seems very much adhoc. What systems have you tested this on? If this is supposed to be a general method of locating a library in well-known locations on different Linux systems, maybe it needs to be elevated to a more prominent place where it can be shared by other parts of the JDK that might want to do the same. It seems like this is a bit of missing "glue" code from FFM -- how to actually locate a system library. I understand that it is not part of FFM proper, nor jextract, but I guess this is something that will need to be reimplemented many times, unless some common functionality is provided. We can perhaps not do that to save the world, but we can at least do that for the benefit of the JDK code, especially as I see FFM replacing JNI more and more, going forward. src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java line 832: > 830: TCSAFLUSH = 0x2; > 831: > 832: VINTR = 0; I'm just curious: How did you arrive at these values? They seem like they are generated somehow. (I guess you did not just make them up and write them down here). Can that process be documented? Or saved as a script in case they ever need to be updated? src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/Kernel32.java line 20: > 18: final class Kernel32 { > 19: > 20: public static final int FORMAT_MESSAGE_FROM_SYSTEM = 0x00001000; Here is another bunch of constants that seem generated. Is this file created by jextract? (I'm sorry, I have not played around enough with jextract to recognize the output it generates). In any way, some documentation on how this file was created, if any kind of automation was involved, would be nice. src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/Kernel32.java line 321: > 319: static { > 320: System.loadLibrary("msvcrt"); > 321: System.loadLibrary("Kernel32"); For consistency: Suggestion: System.loadLibrary("kernel32"); ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535466266 PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535468034 PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535471590 PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535469754
