Overhaul terminal io#5319
Conversation
| * Runs a thunk ensuring that the terminal is in canonical mode: | ||
| * [[https://www.gnu.org/software/libc/manual/html_node/Canonical-or-Not.html Canonical or Not]]. | ||
| * Most of the time sbt should be in canonical mode except when it is explicitly set to raw mode | ||
| * via [[withRawSystemIn]]. |
There was a problem hiding this comment.
| * via [[withRawSystemIn]]. | |
| * sbt should go back to canonical mode before giving the controls to `run` or `sbt new`. |
There was a problem hiding this comment.
Not sure I agree with this suggestion. sbt should only ever not be in canonical mode when in a Terminal.withRawSystemIn block so the onus should not actually be on the caller to go back to canonical mode.
| if (!closed.get()) { | ||
| buffer.add(wrapped.read()) | ||
| wrapped.read() match { | ||
| case -1 => closed.set(true) |
| * | ||
| * @return the wrapped InputStream | ||
| */ | ||
| private[sbt] def wrappedSystemIn: InputStream = WrappedSystemIn |
There was a problem hiding this comment.
Would it make sense to hide this from the rest of sbt code as well? And provide something like def read: Char?
There was a problem hiding this comment.
Possibly. ConsoleChannel uses Terminal.throwOnClosedSystemIn rather than wrapped system in but I think generally we want wrappedSystemIn.
eed3si9n
left a comment
There was a problem hiding this comment.
some comments here and there, but overall I think looks ok?
| case _ if matches("ignore") => s.log.warn(s"Ignoring load failure: $ignoreMsg."); s | ||
| case _ if matches("last") => LastCommand :: loadProjectCommand(LoadFailed, loadArg) :: s | ||
| case _ => println("Invalid response."); doLoadFailed(s, loadArg) | ||
| result.toChar match { |
There was a problem hiding this comment.
Not sure how many people have caps lock on but the original code had .toLowerCase(Locale.ENGLISH)
There was a problem hiding this comment.
Yeah. I should make that change.
There was a problem hiding this comment.
Done via explicit character matching rather than a call toLowerCase.
| ) | ||
| case multiple if multiple.size > 1 && logWarning => | ||
| val msg = | ||
| "Multiple main classes detected. Run 'show discoveredMainClasses' to see the list." |
There was a problem hiding this comment.
| "Multiple main classes detected. Run 'show discoveredMainClasses' to see the list." | |
| "multiple main classes detected: run 'show discoveredMainClasses' to see the list" |
This is purely cosmetic but I'd like to unify towards lowercase for warnings (already the case in most Scala compiler errors etc)
| for ((className, index) <- multiple.zipWithIndex) | ||
| println(" [" + (index + 1) + "] " + className) | ||
| promptIfMultipleChoices.flatMap { prompt => | ||
| val header = "\nMultiple main classes detected. Select one to run:\n" |
There was a problem hiding this comment.
| val header = "\nMultiple main classes detected. Select one to run:\n" | |
| val header = "\nmultiple main classes detected: select one to run:\n" |
There was a problem hiding this comment.
I am ok with the change above, but I disagree with this change. This is a prompt, not a warning message. The double colons are a poor way to express multiple sentences. It also is not at all inconsistent with sbt to output complete sentences or lines starting with a capital letter. For example:
[info] 2. Monitoring source files for sbtRoot/compile...
[info] Press <enter> to interrupt or '?' for more options.
[info] Build triggered by /Users/ethanatkins/work/oss/sbt/sbt/internal/util-logging/src/main/scala/sbt/internal/util/ConsoleAppender.scala. Running 'compile'.
[info] Formatting 1 Scala sources...
[info] Formatting 1 Scala sources...
[info] Formatting 1 Scala sources...
[info] Formatting 2 Scala sources...
[info] Compiling 2 Scala sources to /Users/ethanatkins/work/oss/sbt/sbt/main/target/scala-2.12/classes ...
[success] Total time: 7 s, completed May 1, 2020 1:02:55 PM
| append(Exec("exit", Some(Exec.newExecId), Some(CommandSource(name)))) | ||
| } | ||
| } catch { | ||
| case _: ClosedChannelException => |
There was a problem hiding this comment.
What would be a situation where the input would close itself?
There was a problem hiding this comment.
This is the purpose of Terminal.throwOnClosedSystemIn. The problem is that ConsoleReader.readLine returns null both if the user inputs EOF via <ctrl+d> and if the call to read returns -1. In that latter case, we don't want to shutdown sbt because we may be running scripted or a server session. Before the wrapped system in overrode read to poll until System.in.available > 0. This kept JLine from returning null when System.in wasn't attached.
| private[this] def active: Vector[Task[_]] = activeTasks.toVector.filterNot(Def.isDummy) | ||
| private[this] def activeExceedingThreshold: Vector[(Task[_], Long)] = active.flatMap { task => | ||
| val elapsed = timings.get(task).currentElapsedMicros | ||
| if (elapsed.micros > threshold) Some[(Task[_], Long)](task -> elapsed) else None |
There was a problem hiding this comment.
👍. Initially this looked confusing until I realized that the comparison of microsecond and millisecond is done using Duration.
The watch tests take forever on windows because wrapped.read() always returns -1 during scripted.
This commit aims to centralize all of the terminal interactions throughout sbt. It also seeks to hide the jline implementation details and only expose the apis that sbt needs for interacting with the terminal. In general, we should be able to assume that the terminal is in canonical (line buffered) mode with echo enabled. To switch to raw mode or to enable/disable echo, there are apis: Terminal.withRawSystemIn and Terminal.withEcho that take a thunk as parameter to ensure that the terminal is reset back to the canonical mode afterwards.
The ask user thread is a background thread so it's fine for it to block on System.in. By blocking rather than polling, the cpu utilization of sbt drops to 0 on idle. We have to explicitly handle <ctrl+d> if we block though because the JLine console reader will return null both if the input stream returns -1
When the user inputs `run` or `runMain` we shouldn't print the warning about multiple classes because in the case of run they already will be prompted to select the classes and in the case of runMain, they are already required to specify the class name. Bonus: * improve punctuation * add clear screen to selector dialogue * print selector dialogue in one call to println -- this should prevent the possibility of messages from other threads being interlaced with the dialogue
There typically are fewer than 10 main classes in a project so allow the user to just input a single digit in those cases. Otherwise fallback to a line reader.
It's a bit annoying to have to hit enter here. Also, this should fix #5162 because if there is no System.in attached, the read will return -1 which will cause sbt to quit.
Prior to this change, if a network command came in, it would run in the background with no real feedback in the server ui. Prior to this change, running compile from the thin client would look like: sbt:scala-compile> [success] Total time: 1 s, completed Dec 12, 2019, 7:24:43 PM sbt:scala-compile> Now it looks like: sbt:scala-compile> [info] Running remote command: compile [success] Total time: 1 s, completed Dec 12, 2019, 7:26:17 PM sbt:scala-compile>
Presently if a server command comes in while in the shell, the client output can appear on the same line as the command prompt and the command prompt will not appear again until the user hits enter. This is a confusing ux. For example, if I start an sbt server and type the partial command "comp" and then start up a client and run the clean command followed by a compile, the output looks like: [info] sbt server started at local:///Users/ethanatkins/.sbt/1.0/server/51cfad3281b3a8a1820a/sock sbt:scala-compile> comp[info] new client connected: network-1 [success] Total time: 0 s, completed Dec 12, 2019, 7:23:24 PM [success] Total time: 0 s, completed Dec 12, 2019, 7:23:27 PM [success] Total time: 2 s, completed Dec 12, 2019, 7:23:31 PM Now, if I type "ile\n", I get: [info] sbt server started at local:///Users/ethanatkins/.sbt/1.0/server/51cfad3281b3a8a1820a/sock ile [success] Total time: 0 s, completed Dec 12, 2019, 7:23:34 PM sbt:scala-compile> Following the same set of inputs after this change, I get: [info] sbt server started at local:///Users/ethanatkins/.sbt/1.0/server/51cfad3281b3a8a1820a/sock sbt:scala-compile> comp [info] new client connected: network-1 [success] Total time: 0 s, completed Dec 12, 2019, 7:25:58 PM sbt:scala-compile> comp [success] Total time: 0 s, completed Dec 12, 2019, 7:26:14 PM sbt:scala-compile> comp [success] Total time: 1 s, completed Dec 12, 2019, 7:26:17 PM sbt:scala-compile> compile [success] Total time: 0 s, completed Dec 12, 2019, 7:26:19 PM sbt:scala-compile> To implement this change, I added the redraw() method to LineReader which is a wrapper around ConsoleReader.drawLine; ConsoleReader.flush(). We invoke LineReader.redraw whenever the ConsoleChannel receives a ConsolePromptEvent and there is a running thread. To prevent log lines from being appended to the prompt line, in the CommandExchange we print a newline character whenever a new command is received from the network or a network client connects and we believe that there is an active prompt.
It is better that sbt not expose the implementation detail that LineReader is implemented by JLine. Other terminal related apis should be handled by sbt.internal.util.Terminal.
To reduce the noise of supershell, this commit imposes a 10 millisecond minimum duration before a task will appear in the supershell lines.
In order to make supershell work with println, this commit introduces a virtual System.out to sbt. While sbt is running, we override the default java.lang.System.out, java.lang.System.in, scala.Console.out and scala.Console.in (unless the property `sbt.io.virtual` is set to something other than true). When using virtual io, we buffer all of the bytes that are written to System.out and Console.out until flush is called. When flushing the output, we check if there are any progress lines. If so, we interleave them with the new lines to print. The flushing happens on a background thread so it should hopefully not impede task progress. This commit also adds logic for handling progress when the cursor is not all the way to the left. We now track all of the bytes that have been written since the last new line. Supershell will then calculate the cursor position from those bytes* and move the cursor back to the correct position. The motivation for this was to make the run command work with supershell even when multiple main classes were specified. * This might not be completely reliable if the string contains ansi cursor movement characters.
We do not need a large blank zone with the virtual system.out.
This communicates intent better than clearScreen(0).
There is push to move away from complete sentences in warnings.
Also display invalid response character.
|
@eed3si9n I rebased this branch, made a number of the requested changes and added commentary about some wording issues where I have some different opinions. |
|
@eatkins Thanks for the update. I'll take a closer look. |
| val exec: Exec = exchange.blockUntilNextExec(minGCInterval, s1.globalLogging.full) | ||
| if (exec.source.fold(true)(_.channelName != "console0")) { | ||
| s1.log.info(s"Running remote command: ${exec.commandLine}") | ||
| s1.log.info(s"received remote command: ${exec.commandLine}") |
| * @return the result of the thunk | ||
| */ | ||
| private[sbt] def withStreams[T](f: => T): T = | ||
| if (System.getProperty("sbt.io.virtual", "true") == "true") { |
| case 'i' => s.log.warn(s"Ignoring load failure: $ignoreMsg."); s | ||
| case 'l' => LastCommand :: loadProjectCommand(LoadFailed, loadArg) :: s | ||
| case _ => println("Invalid response."); doLoadFailed(s, loadArg) | ||
| case 'r' | 'R' => retry |
eed3si9n
left a comment
There was a problem hiding this comment.
LGTM. Thanks for this contribution!
This PR makes a number of enhancements to terminal io. Because of how the changes stacked on top of each other, it both was difficult to split this into many smaller PRs. Every individual commit in the PR compiles and should likely pass scripted.
The two main changes are:
Terminalclass which is used to do things like switch the tty mode between canonical and raw mode and also access properties like the height and width of the terminal.java.lang.System.outandscala.Console.outto go through a proxy. This enables us to make supershell work withprintln. It also allows us to make supershell work when output has been printed without a newline by reseting the cursor back to where the previous output left off.The impact of item 2 can be seen in some recorded terminal sessions. First I made a simple project that ran a task that depended on 8 other tasks that each just print some random lines to System.out. Here is what it looks like with 1.3.5:


And here is what it looks like with these changes:
The run task didn't really work with supershell before (#5246):


Now the cursor correctly moves back to the right place:
I also made it so that if there are fewer than 10 main classes that the user doesn't have to enter a newline to select the class.
I also made some improvements to


ConsoleChanneland theCommandExchangethat makes the output more sane when a client connects and runs commands. With 1.3.5, the output could get mixed with the shell prompt and the prompt wasn't restored after the command completed:Now the output is saner and sbt logs what command is being invoked by the network client:
Miscellaneous fixes:
ConsoleChanneland block instead. As a result the cpu utilization of sbt when idle drops from about 2% to 0% on my computer. Input latency should also be slightly lower.Fixes #5246
I think should fix #5162