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

SI-8308 Fix REPL tab completion after suspend/resume #5032

Closed
wants to merge 1 commit into from

Conversation

retronym
Copy link
Member

Implementation borrowed from SBT. For best results, we need to use
sun.misc.SignalHandler to repair JLine immediately after SIGCONT.
If that isn't avaialable, we avoid crashing, and get the terminal
working after the next newline is entered.

The signal handler registration can be disabled with
-Dscala.repl.disable.cont.

Tested manually on MacOS X: http://recordit.co/mjOTxvu84K

Review by @som-snytt

@scala-jenkins scala-jenkins added this to the 2.11.9 milestone Mar 11, 2016
@retronym
Copy link
Member Author

/cc @eed3si9n @jsuereth

@som-snytt
Copy link
Contributor

Wow, it works on Ubuntu. I had to build with ant, sbt package says: [error] (compiler/compile:packageBin) java.util.zip.ZipException: duplicate entry: repl-jline.properties which I am too dumb or lazy to investigate ATM.

@lrytz
Copy link
Member

lrytz commented Mar 11, 2016

i've had the same complaint about sbt package, but @szeiger educated me that we're not supposed to use that command, see https://github.com/scala/scala/blob/2.12.x/build.sbt#L4-L10.

we should fix that, since it violates common expectations. could be by just printing a doc message on sbt package, or by re-wiring it to dist/mkPack.

@som-snytt
Copy link
Contributor

@lrytz Thx for spreading the education. I haven't deep-dived into the sbt build yet.

@som-snytt
Copy link
Contributor

I must have seen The required jar file has not been built by sbt. Please run "sbt package" in dotty.

@retronym
Copy link
Member Author

Reworked to indirect reference to sun.misc through reflection so as not to need to declare it as a dependency in OSGi metadata.

@retronym retronym force-pushed the topic/resume branch 2 times, most recently from f56a7ec to ebb7f15 Compare March 12, 2016 21:17
@retronym
Copy link
Member Author

IDE build is failing due to scala/scala-dev#97

@som-snytt
Copy link
Contributor

I was going to joke, "That's what they all say." But it's a classic thread. "I have no idea what is going on but I have to find it out." (NB order tee-shirt.) And "The .sbt format evolved." I thought the beauty of sbt is that the build file is just Scala code. (Modulo blank lines.) I don't understand the comment about ueber-build props, but even your crummy build issues are interesting!

@szeiger
Copy link
Member

szeiger commented Mar 14, 2016

I was actually surprised that several people (including myself at one point...) ran into the problem with sbt package because I had never used the package command before and expected it to be for internal use only. Our OSGi code currently does the same as the sbt-osgi plugin, i.e. change the behavior of packageResults but not package. I don't know why sbt-osgi made this decision but I wouldn't expect any problems from changing this to override package instead.

@retronym
Copy link
Member Author

I thought the beauty of sbt is that the build file is just Scala code

There are also restrictions about the types of each expression. In 0.13.5 and earlier, you had to manually squash a sequence of settings into a single setting:

foo := "bar"

seq(somePluginSettings: _*)

Seems like this was improved in:

sbt/sbt@746583e

But not release noted: http://www.scala-sbt.org/0.13/docs/sbt-0.13-Tech-Previews.html#sbt+0.13.6

seq itself was deprecated in the same release.

@retronym
Copy link
Member Author

Community Testing Drive.

  • Follow the approach in http://recordit.co/mjOTxvu84K on a variety of platforms.
  • Engage the help of local monkeys / toddlers / fuzzing tool to bash at the keyboard to see if you can break it.

Platforms:

@som-snytt
Copy link
Contributor

Cygwin 64 on windows 7.

$ ./scala-2.11.9-ebb7f15-nightly/bin/scala
Welcome to Scala 2.11.9-ebb7f15-nightly (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_60).
Type in expressions for evaluation. Or try :help.
Exception in thread "main" java.lang.IllegalArgumentException: Unknown signal: CONT
        at sun.misc.Signal.<init>(Signal.java:143)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.lang.reflect.Constructor.newInstance(Constructor.java:422)
        at scala.tools.nsc.interpreter.jline.Signalling$SunMisSignalViaReflection.newSignal(JLineReader.scala:231)
        at scala.tools.nsc.interpreter.jline.Signalling$SunMisSignalViaReflection.withHandler(JLineReader.scala:218)
        at scala.tools.nsc.interpreter.jline.InteractiveReader.withReader(JLineReader.scala:74)
        at scala.tools.nsc.interpreter.jline.InteractiveReader.readOneLine(JLineReader.scala:58)
        at scala.tools.nsc.interpreter.InteractiveReader$class.readLine(InteractiveReader.scala:38)
        at scala.tools.nsc.interpreter.jline.InteractiveReader.readLine(JLineReader.scala:29)
        at scala.tools.nsc.interpreter.ILoop.readOneLine(ILoop.scala:404)
        at scala.tools.nsc.interpreter.ILoop.loop(ILoop.scala:413)
        at scala.tools.nsc.interpreter.ILoop$$anonfun$process$1.apply$mcZ$sp(ILoop.scala:923)
        at scala.tools.nsc.interpreter.ILoop$$anonfun$process$1.apply(ILoop.scala:909)
        at scala.tools.nsc.interpreter.ILoop$$anonfun$process$1.apply(ILoop.scala:909)
        at scala.reflect.internal.util.ScalaClassLoader$.savingContextLoader(ScalaClassLoader.scala:97)
        at scala.tools.nsc.interpreter.ILoop.process(ILoop.scala:909)
        at scala.tools.nsc.MainGenericRunner.runTarget$1(MainGenericRunner.scala:74)
        at scala.tools.nsc.MainGenericRunner.run$1(MainGenericRunner.scala:87)
        at scala.tools.nsc.MainGenericRunner.process(MainGenericRunner.scala:98)
        at scala.tools.nsc.MainGenericRunner$.main(MainGenericRunner.scala:103)
        at scala.tools.nsc.MainGenericRunner.main(MainGenericRunner.scala)

@retronym
Copy link
Member Author

Off to a flying start :)

@retronym
Copy link
Member Author

I'll refactor to eagerly call newSigal("CONT") while in the try/catch, which should fix the graceful
degradation on windows.

However, I think we'll still get an stacktrace spew on J9, if sbt/sbt#1027 is to be believed.

Implementation borrowed from SBT. For best results, we need to use
`sun.misc.SignalHandler` to repair JLine immediately after SIGCONT.
If that isn't avaialable, we avoid crashing, and get the terminal
working after the next newline is entered.

The signal handler registration can be disabled with
`-Dscala.repl.disable.cont`.

Access to `sun.misc` is performed via reflection.

Tested manually on MacOS X: http://recordit.co/mjOTxvu84K
@retronym
Copy link
Member Author

@som-snytt Could you please re-test with the latest build on Windows and Windows+cygwin? My expectation is that you should no longer see the IllegalArgumentException, and that JLine will work again after the first \n after resuming.

@som-snytt
Copy link
Contributor

Sure, it's the least I could do that isn't Nothing.

@som-snytt
Copy link
Contributor

Quickly sanity checked on Win 7 and cygwin 64.

The autocomplete had an extra line feed, maybe due to wrapping, in an 80-column windows shell with scala.bat:

scala> 42.
!=   >             floatValue      isValidInt     to               toRadians

%    >=            floor           isValidLong    toBinaryString   toShort

&    >>            getClass        isValidShort   toByte           unary_+

*    >>>           intValue        isWhole        toChar           unary_-

+    ^             isInfinite      longValue      toDegrees        unary_~

Starting scala.bat under cygwin console doesn't hook up inputs, but ctl-C works to get bash back which reads the lines:

scala> 42




ls


amarki@AMARKI-531490 ~/tmp/scala-2.11.9-bfdee84-nightly
$ 42
-bash: 42: command not found

amarki@AMARKI-531490 ~/tmp/scala-2.11.9-bfdee84-nightly
$

[snip]

amarki@AMARKI-531490 ~/tmp/scala-2.11.9-bfdee84-nightly
$ ls
bin  doc  lib  man

I don't get my scala back in cygwin:

scala>
[1]+  Stopped                 ./bin/scala

amarki@AMARKI-531490 ~/tmp/scala-2.11.9-bfdee84-nightly
$ ps aux
      PID    PPID    PGID     WINPID   TTY         UID    STIME COMMAND
     3596       1    3596       3596  ?        1090335 13:00:17 /usr/bin/mintty
S    6416    5032    6416       6184  pty1     1090335 13:15:28 /usr/bin/bash
I    5504    1240    5504       3452  pty0     1090335 12:48:09 /usr/bin/bash
     6672    5032    6672       6492  pty1     1090335 13:15:40 /usr/bin/ps
     1240       1    1240       1240  ?        1090335 12:48:09 /usr/bin/mintty
     4148       1    4148       4148  ?        1090335 12:48:10 /usr/bin/ssh-agent
     5032    3596    5032       4232  pty1     1090335 13:00:17 /usr/bin/bash
     6992    6416    6416       1260  pty1     1090335 13:15:29 /cygdrive/c/Program Files/Java/jdk1.8.0_60/bin/java <defunct>

amarki@AMARKI-531490 ~/tmp/scala-2.11.9-bfdee84-nightly
$ fg
./bin/scala
./bin/scala: line 21:  6992 Stopped                 "$@"

@retronym
Copy link
Member Author

@som-snytt Do you know which of those problems are regressions and which are existing issues?

@som-snytt
Copy link
Contributor

Lack of fg is existing. Actually, it's all existing. Increasing the windows console to 82 width solves the wrapping issue. Now I can say I opened a windows console in 2016.

@retronym
Copy link
Member Author

retronym commented Apr 27, 2016

I'm going to close this in favour of the upgrade to JLine 2.14.1 (#5129), which natively supports this with -Djline.sigcont : jline/jline2@3715461

@retronym retronym closed this Apr 27, 2016
@SethTisue SethTisue removed this from the 2.11.9 milestone Oct 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants