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

Move to vanilla JLine as an external dependency for the repl #7604

Closed
scabug opened this issue Jun 24, 2013 · 32 comments
Closed

Move to vanilla JLine as an external dependency for the repl #7604

scabug opened this issue Jun 24, 2013 · 32 comments
Assignees
Labels

Comments

@scabug
Copy link

scabug commented Jun 24, 2013

It is common, when using cygwin + jline, to set the -Djline.terminal= property. While Scala supports jline 2.x where you can instead use the string "unix" or "windows" here, it does not support the same class names as standard jline. Jline 1.x only supported custom terminals via class name, forcing users cross between jline 1.x and 2.x to use class names. HOwever, if you wish to use the scala REPL in an environment with other jlines, you are now unable to specify "jline.UnixTerminal" as the terminal for cygwin, while jline 2.x vanilla handles this perfectly.

SO, what it comes down to is that if Scala is going to "shade" the classes, it should NOT break the the canonical settings of jline. I see one of two options:

(1) Provide a hack so that unshaded settings that may be class names are shaded and attempted.
(2) Use a shaded property name (scala.jline.terminal) for configuring Scala's terminal.

@scabug
Copy link
Author

scabug commented Jun 24, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7604?orig=1
Reporter: @jsuereth
Affected Versions: 2.9.2, 2.10.0, 2.10.1, 2.10.2

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@retronym said:
+100

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@harrah said:
I would pick (2), but fall back to jline.terminal if scala.jline.terminal doesn't exist.

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@paulp said:
Correct move is (3), use regular jline2 as a normal dependency. I've tried a number of times to get someone interested in completing that task, though I don't see any jline branches left so it seems I gave up a while ago.

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@paulp said:
Here's maybe the last attempt: https://groups.google.com/forum/#!topic/scala-internals/YiwPvCcAOAg

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@jsuereth said:
I agree the correct move would be #3. I'd even be willing to just accept regressions from moving to jline 2 and push users that need features to contribute upstream.

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@adriaanm said:
I'm working on porting our fixes to jline2. We're mostly in synch.

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@adriaanm said:
And by "mostly", it turns out I meant "completely". I went through all the diffs in our fork and verified that they have been fixed independently.

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@jsuereth said:
Great news! We should definitely migrate soon. If I can spare another day on windows madness, I'll see if I can even attempt it.

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@paulp said:
You've integrated it, tested it on windows, etc? Having walked this path before, I will be extremely surprised if it "just works".

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@adriaanm said:
I've diffed their master against our fork and didn't find any difference (modulo renaming method names).

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@paulp said:
Then differences are likely to arise from the exact manner in which it is built and what exactly is included in the jar. If they don't, great. But don't count your jline chickens until they are serving chicken sandwiches.

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@adriaanm said:
they independently fixed e.g., jline/jline2#13, jline/jline2#15

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@adriaanm said:
Ok, I'm looking at the SBT build now. It looks like it creates one fat jar. I was thinking of including jansi as a separate jar on the classpath -- is that problematic?

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@paulp said:
Not sure. Differences on that front are either inertial (stemming from how it was in jline 1.0) or reactive to something not working. I would try the most vanilla setup possible and see what doesn't work.

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@harrah said:
That mimics what the standard jline does. I'm not sure why it does that, though. It would seem like it should be a normal dependency and not packaged in the same jar.

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@paulp said:
I would say it's for size reasons based on some old mailing list chatter, but if there is any savings being realized, it is negligible.

jline jar: 208K
jansi jar: 114K
jline jar after throwing away jansi and rejarring: 108K

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@paulp said:
Or maybe this is why.

/r/jvminfo/bin/dump-class jline |& grep jansi.internal
  call  jline.WindowsTerminal.getConsoleMode                  org.fusesource.jansi.internal.WindowsSupport#getConsoleMode
  call  jline.WindowsTerminal.getWindowsTerminalHeight        org.fusesource.jansi.internal.WindowsSupport#getWindowsTerminalHeight
  call  jline.WindowsTerminal.getWindowsTerminalWidth         org.fusesource.jansi.internal.WindowsSupport#getWindowsTerminalWidth
  call  jline.WindowsTerminal.readByte                        org.fusesource.jansi.internal.WindowsSupport#readByte
  call  jline.WindowsTerminal.setConsoleMode                  org.fusesource.jansi.internal.WindowsSupport#setConsoleMode

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@jsuereth said:
Those are the calls now causing us a lot of fun issues in windows crossing JANSI dependencies and projects...

@scabug
Copy link
Author

scabug commented Jun 24, 2013

@adriaanm said:
How can I test/reproduce those issues? I have a build using jline 2.11 ready.

@scabug
Copy link
Author

scabug commented Jun 25, 2013

huynhjl said:
So sbt and scala will now use the same jline 2.x? Or sbt will continue to use jline 0.9.x/1.x and scala use jline 2.x?

Will there be any issue with the fact that jline1 and jline2 have class name collisions?

https://github.com/jline/jline/blob/master/src/main/java/jline/Terminal.java
https://github.com/jline/jline2/blob/master/src/main/java/jline/Terminal.java

@scabug
Copy link
Author

scabug commented Jun 25, 2013

@jsuereth said:
sbt 0.13 is on jline 2.x. sbt also uses classloader isolation between the scala used for the project and itself. There's no direct class conflict issue between the two, although native library loading can be an issue, which I'm working on now.

@scabug
Copy link
Author

scabug commented Jun 25, 2013

@adriaanm said (edited on Jun 25, 2013 5:19:04 PM UTC):
https://github.com/adriaanm/scala/tree/modularize-jline
artifacts will be available as soon as https://scala-webapps.epfl.ch/jenkins/job/scala-checkin-manual/923/ finishes

@scabug
Copy link
Author

scabug commented Jun 25, 2013

@adriaanm said:
Artifacts for testing are here: http://scala-webapps.epfl.ch/artifacts-manual/af1be5048c3e5d9a249403e3bc442c6aaeafe3c2/
It works fine on my mac.

@scabug
Copy link
Author

scabug commented Jun 25, 2013

@adriaanm said (edited on Jun 25, 2013 10:29:43 PM UTC):
I also tried in a win7 vm. Backspace, del, home, end, history up/down all work.
It did complain about not finding jline.console.completer.Completer

https://www.evernote.com/shard/s7/sh/50f6b2f3-c618-4166-b874-26cbb8a79445/20bb38cf9f1cfa41397987529e5d9c2d

@scabug
Copy link
Author

scabug commented Jun 25, 2013

@adriaanm said (edited on Jun 25, 2013 11:37:10 PM UTC):
Ah, figured out the problem. Creating new artifacts here: https://scala-webapps.epfl.ch/jenkins/job/scala-checkin-manual/925

@scabug
Copy link
Author

scabug commented Jun 26, 2013

huynhjl said:
For those like me who haven't downloaded artifacts before: the link to the artifacts for build 925 is http://scala-webapps.epfl.ch/artifacts-manual/74790d1533636ef11efc4a6b4f94c8d3e17073c4/

@scabug
Copy link
Author

scabug commented Jun 27, 2013

huynhjl said:
I tested dist.tgz on

  • Mac OSX 10.8.4
  • Win7 64 bits
  • with cmd.exe
  • with cygwin inside cmd.exe
  • with mintty/cygwin
  • Ubuntu 10.10 VM

All console stuff works fine (line wrapping, backspace, history, paste, arrows, reverse search). For completeness I'll try a win 32 bits when I have time.

I started to browse the recent commits in jline2 and was reminded that it has an "expand event" feature that defaults to true. So !! recalls the previous line and in general anything starting with ! may cause IllegalArgumentException.

For instance try typing 1 != 1 with this 925 build.

You may need to call ConsoleReader.setExpandEvents(false).

@scabug
Copy link
Author

scabug commented May 18, 2014

@som-snytt said:
Is this issue still open as a reminder to rm ./src/jline from 2.12.x?

@scabug
Copy link
Author

scabug commented May 19, 2014

@adriaanm said:
Excellent point, I'd gladly see src/jline disappear in 2.11.x and 2.12.x

@scabug
Copy link
Author

scabug commented Aug 8, 2014

@gourlaysama said:
Oh, so that's where @som-snytt read that src/jline could go away.

So yes, it is gone: scala/scala#3921

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

No branches or pull requests

2 participants