Skip to content

Conversation

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Mar 6, 2024

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:

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18142/head:pull/18142
$ git checkout pull/18142

Update a local copy of the PR:
$ git checkout pull/18142
$ git pull https://git.openjdk.org/jdk.git pull/18142/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18142

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18142.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 6, 2024

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

@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 6, 2024
@openjdk
Copy link

openjdk bot commented Mar 6, 2024

@lahodaj The following labels will be automatically applied to this pull request:

  • build
  • kulla

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added build build-dev@openjdk.org kulla kulla-dev@openjdk.org labels Mar 6, 2024
@mlbridge
Copy link

mlbridge bot commented Mar 6, 2024

Webrevs

TCSADRAIN = 0x1;
TCSAFLUSH = 0x2;

VINTR = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


static {
System.loadLibrary("msvcrt");
System.loadLibrary("Kernel32");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency:

Suggested change
System.loadLibrary("Kernel32");
System.loadLibrary("kernel32");

@SuppressWarnings({"unused", "restricted"})
final class Kernel32 {

public static final int FORMAT_MESSAGE_FROM_SYSTEM = 0x00001000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@magicus
Copy link
Member

magicus commented Mar 22, 2024

Aha, now I understand! The new files in jdk.internal.org.jline.terminal.impl.ffm comes from upstream. I thought they were additions made by you.

I guess that means my comments are still valid, but should be asked upstream instead. And that you don't need to address them here.

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Apr 10, 2024
@openjdk openjdk bot removed merge-conflict Pull request has merge conflict with target branch rfr Pull request is ready for review labels May 3, 2024
@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 3, 2024
@lahodaj lahodaj changed the title 8327476: Upgrade JLine to 3.25.1 8327476: Upgrade JLine to 3.26.1 May 3, 2024
@openjdk openjdk bot removed the ready Pull request is ready to be integrated label May 3, 2024
@lahodaj
Copy link
Contributor Author

lahodaj commented May 3, 2024

Update: since JLine 3.26.1 was release, I tried to upgrade to that directly. I made a diff of the relevant files between JLine 3.25.1 and 3.26.1, and applied that diff to our code. Some additional cleanup was done as well, as suggested by Magnus.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 3, 2024
@lahodaj
Copy link
Contributor Author

lahodaj commented May 9, 2024

/integrate

@openjdk
Copy link

openjdk bot commented May 9, 2024

Going to push as commit aaa90b3.
Since your change was applied there have been 30 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 9, 2024
@openjdk openjdk bot closed this May 9, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 9, 2024
@openjdk
Copy link

openjdk bot commented May 9, 2024

@lahodaj Pushed as commit aaa90b3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build build-dev@openjdk.org integrated Pull request has been integrated kulla kulla-dev@openjdk.org

Development

Successfully merging this pull request may close these issues.

4 participants