Skip to content

Conversation

@luigidellaquila
Copy link
Contributor

@luigidellaquila luigidellaquila commented Feb 10, 2022

Previous JLine versions have a dependency from an old JAnsi version
that could lead to problems to terminal detection on new hardware (eg. M1/Silicon).

Fixes #83575

Previous JLine versions have a dependenency from an old JAnsi version
that could lead to problems to terminal detection on new hardware (eg. M1/Silicon)

See elastic#83575
@cla-checker-service
Copy link

cla-checker-service bot commented Feb 10, 2022

💚 CLA has been signed

@luigidellaquila luigidellaquila added the Team:QL (Deprecated) Meta label for query languages team label Feb 10, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

@elasticsearchmachine
Copy link
Collaborator

Hi @luigidellaquila, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

Hi @luigidellaquila, I've updated the changelog YAML for you.

1 similar comment
@elasticsearchmachine
Copy link
Collaborator

Hi @luigidellaquila, I've updated the changelog YAML for you.

@luigidellaquila
Copy link
Contributor Author

@elasticmachine retest this please

Copy link
Contributor

@Luegg Luegg left a comment

Choose a reason for hiding this comment

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

Lgtm, and congrats to your inaugural ES PR 🎊

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM - Can you confirm that jansi 2.4+ or higher is used?

@luigidellaquila
Copy link
Contributor Author

Good point @costin, given #83575 I thought it was the case. I also checked the pom of the 3.21.0 tag and it depends on jansi 2.4 indeed (see https://github.com/jline/jline3/blob/jline-parent-3.21.0/pom.xml#L86).

The problem (and the only thing I did not realise) is that we are using jline-terminal-jna, not jline-terminal-jansi, so unless I'm missing something, this upgrade is probably not needed.

@costin
Copy link
Member

costin commented Feb 11, 2022

Since we're already down this path, please take a look at the JNA dependency since version 5.10 contains a nice fix by @DaveCTurner.
If the CLI works (I believe you have a Apple M1), let's use that instead - and maybe there's a way to sync the JNA version in gradle with that of the server so any future upgrades on this front happen for us as well.

@luigidellaquila
Copy link
Contributor Author

Unfortunately I don't have an M1 (yet...), so I cannot test it.
And JLine 3.21.0 still depends on JNA v 5.9.0.
Still, having an updated dependency is not bad, but the scope of the "fix" is a bit different from what we expected

@luigidellaquila
Copy link
Contributor Author

As a side note, running ./gradlew x-pack:plugin:sql:sql-cli:dependencies I see we already have jna 5.10.0 in the classpath, probably it's included manually

runtimeClasspath - Runtime classpath of source set 'main'.
+--- org.jline:jline-terminal:3.14.1
+--- org.jline:jline-terminal-jna:3.14.1
+--- org.jline:jline-reader:3.14.1
+--- org.jline:jline-style:3.14.1
+--- project :x-pack:plugin:sql:sql-client
|    \--- project :x-pack:plugin:sql:sql-proto
|         +--- com.fasterxml.jackson.core:jackson-core:2.10.4
|         \--- com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:2.10.4
+--- project :libs:elasticsearch-cli
|    +--- net.sf.jopt-simple:jopt-simple:5.0.2
|    \--- project :libs:elasticsearch-core
\--- net.java.dev.jna:jna:5.10.0

@costin
Copy link
Member

costin commented Feb 11, 2022

Thanks for checking, I'm fine with closing the issue.

@luigidellaquila
Copy link
Contributor Author

Do you mean closing the PR (without merging) and closing #83575 ?

@luigidellaquila
Copy link
Contributor Author

Closing, since not really relevant to the issue we were addressing.
We can still re-consider it in case we have to upgrade JLine for other reasons

@luigidellaquila
Copy link
Contributor Author

@elasticmachine update branch

@luigidellaquila
Copy link
Contributor Author

luigidellaquila commented May 23, 2022

Tested on Apple M1, it seems also JNA implementation has problems (see #83575 (comment) ).
So even if we do not use JAnsi, we'll have to upgrade anyway.

With JLine v 3.21.0 the problem is fixed (Tested on a MBP M1 Max)

@costin
Copy link
Member

costin commented May 24, 2022

👍

@luigidellaquila luigidellaquila merged commit bf03552 into elastic:master May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/SQL SQL querying Team:QL (Deprecated) Meta label for query languages team >upgrade v8.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SQL: Upgrade JLine/Jansi

7 participants