Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Java8 #105

Closed
snuyanzin opened this issue Aug 12, 2018 · 11 comments
Closed

Upgrade to Java8 #105

snuyanzin opened this issue Aug 12, 2018 · 11 comments

Comments

@snuyanzin
Copy link
Collaborator

Is there something that blocks upgrading to Java 8?

@julianhyde
Copy link
Owner

We'd like to be compatible with as wide a set of JDKs as possible. It wouldn't do much harm to drop support for 1.6 and 1.7, but neither (in my opinion) would it do a huge amount of good to allow Java 8 features (e.g. lambdas) in source code.

But I am open to persuasion, if you feel otherwise...

@snuyanzin
Copy link
Collaborator Author

I was also thinking about moving to jline3 (I think not in 1.5.0) which requires java8. So besides java 8 features there could come jline3 features.

@snuyanzin
Copy link
Collaborator Author

Moving to Jline3 and jdk8 is implemented at #115
Such movement also provides features requested in #73 and #60

@julianhyde
Copy link
Owner

How about we make one last release without jline3 (and continuing support for JDK 1.6 and 1.7), then merge this PR. We said we'd make a release in the first week of September, so we are due.

@snuyanzin
Copy link
Collaborator Author

Yes, I think it makes sense as people could get sqlline 1.5 related features for JDK 1.6, 1.7.

@julianhyde
Copy link
Owner

I'm getting close to this (and I've retitled the commit to 'Upgrde to jline3, and add !rerun command'). See my scratch branch, 5468b5c.

Tests pass on both Linux and Windows, but a lot of errors appear on stdout on Windows during tests:

java.io.IOException: Cannot run program "infocmp.exe": CreateProcess error=2, The system cannot find the file specified
        at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1128)
        at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1071)
        at org.jline.utils.InfoCmp.getInfoCmp(InfoCmp.java:547)
        at org.jline.terminal.impl.AbstractTerminal.parseInfoCmp(AbstractTerminal.java:187)
        at org.jline.terminal.impl.LineDisciplineTerminal.<init>(LineDisciplineTerminal.java:111)
        at org.jline.terminal.impl.ExternalTerminal.<init>(ExternalTerminal.java:60)
        at org.jline.terminal.TerminalBuilder.doBuild(TerminalBuilder.java:428)
        at org.jline.terminal.TerminalBuilder.build(TerminalBuilder.java:262)
        at sqlline.SqlLine.getConsoleReader(SqlLine.java:568)

Is there a solution? Do we need to tell people to install infocmp.exe or something?

I added a WARNING about incompatible history file formats to the commit message. We will need to include that in the release notes.

@julianhyde
Copy link
Owner

Also, how do I enable vi mode?

@snuyanzin
Copy link
Collaborator Author

snuyanzin commented Oct 4, 2018

about vi mode:
Now I guess I got what you mean. That is my fault, currently there is no support to switch to vi mode in a non programmatic way here. Jline provides org.jline.builtins.Commands#keymap to switch to different modes but I have not found any built in possibilities to switch between keymaps. So some additional code on sqlline side should be added to have complete support of #60.

So far I did not make any special (everything came with jline3). Unfortunately they do not have docs about it. However there is a well commented test where described supported features (usually keys with ctrl)

Ctrl-J, M enter
Ctrl-K should delete to end-of-line 
Ctrl-L clears the screen
Ctrl-T transposes every character exactly
Ctrl-U "backward-kill-line", it deletes everything prior to the current cursor position
Ctrl-W word rubout

for more detailed please have a look in comments inside
https://github.com/jline/jline3/blob/master/reader/src/test/java/org/jline/reader/impl/ViMoveModeTest.java
+ not vi but also with Ctrl

Ctrl-N next command from the history
Ctrl-P previous command from the history
Ctrl-R reverse search in history
Ctrl-S forward search in history

a lot of errors appear on stdout on Windows during tests:

currently do not have access to Windows but do not remember such issue. Will try to check today when I'll have Windows by me.

@snuyanzin
Copy link
Collaborator Author

snuyanzin commented Oct 4, 2018

I'm getting close to this (and I've retitled the commit to 'Upgrde to jline3, and add !rerun command'). See my scratch branch, 5468b5c.
Tests pass on both Linux and Windows, but a lot of errors appear on stdout on Windows during tests:
Is there a solution? Do we need to tell people to install infocmp.exe or something?

I just scheduled a Windows build with the mentioned commit 5468b5c at appveyor [1] and there are no such warnings for any of jdk8, 9, 10.
Based on your trace and code I was able to reproduce it but to do it I have to set system property org.jline.terminal.type to non null value otherwise everything works without warnings. (Also do not have infocmp on a local machine and tests go without such warnings). Are you sure you do not have this property somehow set to non null?

[1] https://ci.appveyor.com/project/snuyanzin/sqlline/build/257

@julianhyde
Copy link
Owner

Fixed in bf495f9, PR #155. Thanks @snuyanzin!

@julianhyde
Copy link
Owner

Regarding windows. I decided to commit anyway, and we can follow up later with bug-fixes (if necessary). There is plenty in this commit, and plenty that is working. (The control-R feature, to search back for multi-line commands is amazing.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants