Skip to content

Overhaul terminal io#5319

Merged
eed3si9n merged 18 commits intosbt:developfrom
eatkins:terminal
May 2, 2020
Merged

Overhaul terminal io#5319
eed3si9n merged 18 commits intosbt:developfrom
eatkins:terminal

Conversation

@eatkins
Copy link
Contributor

@eatkins eatkins commented Dec 18, 2019

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:

  1. Adds a new Terminal class 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.
  2. Changes the global java.lang.System.out and scala.Console.out to go through a proxy. This enables us to make supershell work with println. 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:
asciicast
And here is what it looks like with these changes:
asciicast

The run task didn't really work with supershell before (#5246):
asciicast
Now the cursor correctly moves back to the right place:
asciicast
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 ConsoleChannel and the CommandExchange that 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:
asciicast
Now the output is saner and sbt logs what command is being invoked by the network client:
asciicast

Miscellaneous fixes:

  • Clear the current line and the screen to the bottom in the shell prompt. This prevents stray supershell lines from ending up stuck at the bottom.
  • We no longer poll System.in in ConsoleChannel and 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.
  • In continuous, we stop trying to read from System.in if we read a -1 byte that indicates the channel has closed.

Fixes #5246
I think should fix #5162

@eatkins eatkins changed the title Terminal Overhaul terminal io Dec 18, 2019
* 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]].
Copy link
Member

@eed3si9n eed3si9n Dec 18, 2019

Choose a reason for hiding this comment

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

Suggested change
* via [[withRawSystemIn]].
* sbt should go back to canonical mode before giving the controls to `run` or `sbt new`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
Copy link
Member

@eed3si9n eed3si9n Dec 18, 2019

Choose a reason for hiding this comment

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

👍

*
* @return the wrapped InputStream
*/
private[sbt] def wrappedSystemIn: InputStream = WrappedSystemIn
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to hide this from the rest of sbt code as well? And provide something like def read: Char?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly. ConsoleChannel uses Terminal.throwOnClosedSystemIn rather than wrapped system in but I think generally we want wrappedSystemIn.

Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how many people have caps lock on but the original code had .toLowerCase(Locale.ENGLISH)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I should make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

for ((className, index) <- multiple.zipWithIndex)
println(" [" + (index + 1) + "] " + className)
promptIfMultipleChoices.flatMap { prompt =>
val header = "\nMultiple main classes detected. Select one to run:\n"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val header = "\nMultiple main classes detected. Select one to run:\n"
val header = "\nmultiple main classes detected: select one to run:\n"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 =>
Copy link
Member

Choose a reason for hiding this comment

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

What would be a situation where the input would close itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

👍. Initially this looked confusing until I realized that the comparison of microsecond and millisecond is done using Duration.

eatkins added 18 commits May 1, 2020 12:28
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.
@eatkins
Copy link
Contributor Author

eatkins commented May 1, 2020

@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.

@eed3si9n
Copy link
Member

eed3si9n commented May 1, 2020

@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}")
Copy link
Member

Choose a reason for hiding this comment

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

👍

* @return the result of the thunk
*/
private[sbt] def withStreams[T](f: => T): T =
if (System.getProperty("sbt.io.virtual", "true") == "true") {
Copy link
Member

Choose a reason for hiding this comment

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

We should document this.

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
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for this contribution!

@eed3si9n eed3si9n added this to the 1.4.0 milestone May 2, 2020
@eed3si9n eed3si9n merged commit 5a529bf into sbt:develop May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants