8327476: Upgrade JLine to 3.26.1 #1413

GoeLin · 2025-02-19T09:37:11Z

I backport this change for parity with 21.0.7-oracle.

It brings updates of upstream library jline 3.26.1 to jdk21.
Jdk21 currently includes jline 3.22.0, see JDK-8297587.
The original sources of jline 3.26.1 can be found here:
https://github.com/jline/jline3/archive/refs/tags/jline-parent-3.26.1.tar.gz

The backport of this change needed larger adaptions.

The original jline library dynamically selects how to access
the operating system in two means:

Which operating system? linux, windows, mac?
JDK functionality to do native calls: JNI, jna, ffm?

The implementation in OpenJDK does this selection at JDK build time.
The code is split into more subdirectories for the operating systems.
One of the methods to call native is picked, the code for the others
is removed. Unfortunately this differs for the JDK releases:

jdk21 uses jna.
jdk23 uses ffm.
The original patch of this change includes all the edits to
replace jna by ffm.

Further changes that fell into the eye during the backport

jline changed the format of its Copyright header
jline changed sorting of imports
jline changed a lot of formatting, especially how
arguments to methods are listed.
jline removed the Stream enum from TerminalProvider.java and moved it
into a class of its own: SystemStream.java. This makes changes to
import statements and argument lists necessary.
OpenJDK added a new parameter inputStreamWrapper to the
functions which are also affected by the Stream enum change.
This also made changes to import statements and argument lists
necessary.

OpenJDK 23 added jline to the list of modules needed for a jre.
I don't think we should do that in 21, so I omitted that change.

jline 3.26.1 and OpenJDK 23 added a new method systemStreamWidth() to TerminalProvider.
I don't think we should extend the functionlaity of jline in 21,
so I omitted this. Also, I would need additional parts of the
jline windows files that are not yet included in OpenJDK to
implement this.

For the backport, I dropped the following files from the patch:

src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/FfmNativePty.java
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/FfmTerminalProvider.java
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/Kernel32.java
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/NativeWinConsoleWriter.java
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/NativeWinSysTerminal.java
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/WindowsAnsiWriter.java

Also, I did not remove the following files as the original change did:

make/modules/jdk.internal.le/Lib.gmk

src/jdk.internal.le/aix/classes/jdk/internal/org/jline/terminal/impl/jna/JDKNativePty.java

src/jdk.internal.le/linux/classes/jdk/internal/org/jline/terminal/impl/jna/JDKNativePty.java
src/jdk.internal.le/linux/classes/jdk/internal/org/jline/terminal/impl/jna/linux/CLibrary.java
src/jdk.internal.le/linux/classes/jdk/internal/org/jline/terminal/impl/jna/linux/CLibraryImpl.java
src/jdk.internal.le/linux/classes/jdk/internal/org/jline/terminal/impl/jna/linux/LinuxNativePty.java
src/jdk.internal.le/linux/classes/jdk/internal/org/jline/terminal/impl/jna/linux/UtilLibraryImpl.java
src/jdk.internal.le/linux/native/lible/CLibrary.cpp

src/jdk.internal.le/macosx/classes/jdk/internal/org/jline/terminal/impl/jna/JDKNativePty.java
src/jdk.internal.le/macosx/classes/jdk/internal/org/jline/terminal/impl/jna/osx/CLibrary.java
src/jdk.internal.le/macosx/classes/jdk/internal/org/jline/terminal/impl/jna/osx/CLibraryImpl.java
src/jdk.internal.le/macosx/classes/jdk/internal/org/jline/terminal/impl/jna/osx/NativeLong.java
src/jdk.internal.le/macosx/classes/jdk/internal/org/jline/terminal/impl/jna/osx/OsXNativePty.java
src/jdk.internal.le/macosx/native/lible/CLibrary.cpp

src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/jna/LastErrorException.java

src/jdk.internal.le/unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaNativePty.java
src/jdk.internal.le/unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaTerminalProvider.java

src/jdk.internal.le/windows/classes/jdk/internal/org/jline/terminal/impl/jna/JnaTerminalProvider.java
src/jdk.internal.le/windows/classes/jdk/internal/org/jline/terminal/impl/jna/win/IntByReference.java
src/jdk.internal.le/windows/classes/jdk/internal/org/jline/terminal/impl/jna/win/JnaWinConsoleWriter.java
src/jdk.internal.le/windows/classes/jdk/internal/org/jline/terminal/impl/jna/win/JnaWinSysTerminal.java
src/jdk.internal.le/windows/classes/jdk/internal/org/jline/terminal/impl/jna/win/Kernel32.java
src/jdk.internal.le/windows/classes/jdk/internal/org/jline/terminal/impl/jna/win/Kernel32Impl.java
src/jdk.internal.le/windows/classes/jdk/internal/org/jline/terminal/impl/jna/win/Pointer.java
src/jdk.internal.le/windows/classes/jdk/internal/org/jline/terminal/impl/jna/win/WindowsAnsiWriter.java
src/jdk.internal.le/windows/native/lible/Kernel32.cpp

I added the changes for the new parameter "inputStreamWrapper" in the jna files.

I patched all differences between jline 3.22.0 and 3.26.1 into the jna files
that were deleted in the original change for jdk23, as far as there is a direct
relation between OpenJDK 21 and upstream jline.

I split the changes needed on top of the origin patch into several commits.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
JDK-8327476 needs maintainer approval

Issue

JDK-8327476: Upgrade JLine to 3.26.1 (Task - P4 - Approved)

Reviewers

Martin Doerr (@TheRealMDoerr - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk21u-dev.git pull/1413/head:pull/1413
$ git checkout pull/1413

Update a local copy of the PR:
$ git checkout pull/1413
$ git pull https://git.openjdk.org/jdk21u-dev.git pull/1413/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1413

View PR using the GUI difftool:
$ git pr show -t 1413

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk21u-dev/pull/1413.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-19T09:37:51Z

👋 Welcome back goetz! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-02-19T09:38:19Z

@GoeLin This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8327476: Upgrade JLine to 3.26.1

Reviewed-by: mdoerr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 8 new commits pushed to the master branch:

af639f3: 8339728: [Accessibility,Windows,JAWS] Bug in the getKeyChar method of the AccessBridge class
1b49a33: 8342098: Write a test to compare the images
2ff0232: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java
fdf6dbe: 8328730: Convert java/awt/print/bug8023392/bug8023392.html applet test to main
6cc0ec2: 8347267: [macOS]: UnixOperatingSystem.c:67:40: runtime error: division by zero
c9bf948: 8347268: [ubsan] logOutput.cpp:357:21: runtime error: applying non-zero offset 1 to null pointer
58f3acc: 8345676: [ubsan] ProcessImpl_md.c:561:40: runtime error: applying zero offset to null pointer on macOS aarch64
9d496c0: 8322983: Virtual Threads: exclude 2 tests

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-02-19T09:38:33Z

This backport pull request has now been updated with issue from the original commit.

mlbridge · 2025-02-20T15:07:19Z

Webrevs

00: Full (5df5ee4f)

TheRealMDoerr · 2025-02-20T15:21:04Z

Indentation looks bad in some commented lines:

+            //            state = State.INTERRUPT;
+        //        addBuiltinWidget(widgets, QUIT, this::quit);
+        //        bind(vicmd, INSERT_COMMENT,                         alt('#'));
+        //        bind(vicmd, INSERT_COMPLETIONS,                     alt('*'));
+        //        if (attributes.getLocalFlag(Attributes.LocalFlag.ECHO)) {
+        //            processOutputByte(c);
+        //            masterOutput.flush();
+        //        }
+                                //                        case 'l':
+                                //                            rawPrint('\l');
+                                //                            break;
+                    //                    if (charRead < 0) {
+                    //                        continue;
+                    //                    }
+                    //                    charRead = -1;

(The PR is too large to review it in the GUI.)

The 4th Commit is from https://github.com/jline/jline3/archive/refs/tags/jline-parent-3.26.1.tar.gz ?

Thanks for splitting the PR into the individual commits!

GoeLin · 2025-02-20T15:27:57Z

Hi,
the fourth commit combines the changes from jline-parent-3.26.1 and what is needed to implement changes by the original OpenJDK 3.26.1 change. In detail this is passing inputStreamWrapper to the functions in the jna part.
I'll have a look at the indentation.

GoeLin · 2025-02-20T15:31:34Z

Indentation:
That part is broken in the original patch as well, so I'd rather keep it.

TheRealMDoerr · 2025-02-21T11:37:40Z

I've compared with https://github.com/jline/jline3/archive/refs/tags/jline-parent-3.26.1.tar.gz and your resolution looks good to me.
Only one question: Why is GetConsoleOutputCP removed (src/jdk.internal.le/windows/native/lible/Kernel32.cpp)?

GoeLin · 2025-02-21T16:02:00Z

Thanks for looking at this change!
GetConsoleOutputCP was removed in the original lib from .../win/Kernel32.java
So I removed it from Kernel32Impl.java and Kernel32.cpp.

TheRealMDoerr · 2025-02-21T16:13:53Z

I can still see GetConsoleOutputCP in https://github.com/jline/jline3/archive/refs/tags/jline-parent-3.26.1.tar.gz. Where did you find the removal?

TheRealMDoerr · 2025-02-21T16:17:44Z

Ah, I see it only in

jline3-jline-parent-3.26.1/native/src/main/java/org/jline/nativ/Kernel32.java:    public static native int GetConsoleOutputCP();
jline3-jline-parent-3.26.1/native/src/main/native/kernel32.c:JNIEXPORT jint JNICALL Kernel32_NATIVE(GetConsoleOutputCP)
jline3-jline-parent-3.26.1/native/src/main/native/kernel32.c:	rc = (jint)GetConsoleOutputCP();

So, it seems to be no longer in the windows files.

openjdk · 2025-02-21T16:19:37Z

⚠️ @GoeLin This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

GoeLin · 2025-02-21T16:56:09Z

I think it is unused, and this was cleaned up in the win files, but not in the rest. At least is't nowhere called within jline.
Thanks for the review!

GoeLin added 4 commits February 19, 2025 10:22

Backport aaa90b3005c85852971203ce6feb88e7091e167b

8c81dfe

Dont put jline into jre, keep the status of 21

02bfa9f

Don't add new method systemStreamWidth()

924a4bf

Adapt jdk21 jna parts to changes from OpenJDk head and from jline 3.26.1

5df5ee4

GoeLin changed the title ~~Goetz backport 8327476~~ Backport aaa90b3005c85852971203ce6feb88e7091e167b Feb 19, 2025

openjdk bot changed the title ~~Backport aaa90b3005c85852971203ce6feb88e7091e167b~~ 8327476: Upgrade JLine to 3.26.1 Feb 19, 2025

openjdk bot added the backport label Feb 19, 2025

GoeLin marked this pull request as ready for review February 20, 2025 15:02

openjdk bot added the rfr Pull request is ready for review label Feb 20, 2025

TheRealMDoerr approved these changes Feb 21, 2025

View reviewed changes

openjdk bot added approval ready Pull request is ready to be integrated and removed approval labels Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8327476: Upgrade JLine to 3.26.1 #1413

8327476: Upgrade JLine to 3.26.1 #1413

GoeLin commented Feb 19, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 19, 2025

openjdk bot commented Feb 19, 2025 •

edited

Loading

openjdk bot commented Feb 19, 2025

mlbridge bot commented Feb 20, 2025

TheRealMDoerr commented Feb 20, 2025

GoeLin commented Feb 20, 2025

GoeLin commented Feb 20, 2025

TheRealMDoerr commented Feb 21, 2025

GoeLin commented Feb 21, 2025

TheRealMDoerr commented Feb 21, 2025

TheRealMDoerr commented Feb 21, 2025

openjdk bot commented Feb 21, 2025

GoeLin commented Feb 21, 2025

8327476: Upgrade JLine to 3.26.1 #1413

Are you sure you want to change the base?

8327476: Upgrade JLine to 3.26.1 #1413

Conversation

GoeLin commented Feb 19, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 19, 2025

openjdk bot commented Feb 19, 2025 • edited Loading

openjdk bot commented Feb 19, 2025

mlbridge bot commented Feb 20, 2025

Webrevs

TheRealMDoerr commented Feb 20, 2025

GoeLin commented Feb 20, 2025

GoeLin commented Feb 20, 2025

TheRealMDoerr commented Feb 21, 2025

GoeLin commented Feb 21, 2025

TheRealMDoerr commented Feb 21, 2025

TheRealMDoerr commented Feb 21, 2025

openjdk bot commented Feb 21, 2025

GoeLin commented Feb 21, 2025

GoeLin commented Feb 19, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 19, 2025 •

edited

Loading