[textinput] Keep the prompt on the terminal when stdout is redirected - #22948
Conversation
dpiparo
left a comment
dpiparo
left a comment
There was a problem hiding this comment.
Impressive.
Before merging though, I think we could provide a test, to never have the problem again.
Now, could the tool we use for testing TTabCom be used for that? https://github.com/root-project/root/tree/master/roottest/root/rint
Basically what happens is that we have in ascii files the input we want emulate as typed by the user at the prompt, and with reference files we cross-check what Rint should produce. In this case, another minimal comparison, a diff really, should be performed on the piped output.
Would it make sense
The `.> file` meta command redirects stdout to a file by dup2-ing fd 1, and the redirection persists across prompts. This left the interactive command line unusable, because textinput decided where to write the prompt/line editing and whether to configure the terminal based on stdout, which is no longer the terminal after the redirection: * TerminalDisplayUnix wrote the prompt, the echo of typed characters and the cursor movements to stdout, so they ended up in the file instead of on the terminal, leaving the command line invisible. * TerminalConfigUnix::IsInteractive() required stdout to be a tty, so once stdout was redirected the terminal was no longer switched to raw mode. The terminal stayed in cooked mode, so e.g. the up arrow was echoed verbatim as "^[[A" instead of recalling the previous command. Base both decisions on the terminal we actually read from and configure, i.e. stdin (never touched by ".> file"): * Keep a handle on the controlling terminal (/dev/tty) whenever stdin is a tty, and make WriteRawString() fall back to it when stdout is not (or no longer) connected to a terminal. As long as stdout is a terminal the prompt still goes to stdout exactly as before, so nothing changes for the common interactive case; only once stdout is redirected does the prompt go to the controlling terminal, while just the executed code's output follows the redirection. * Make IsInteractive() depend on stdin (fFD) instead of stdout, so the terminal keeps being switched to raw mode and line editing keeps working. Closes root-project#7626 🤖 Done with the help of AI.
Drive ROOT through a pseudo terminal (like driveTabCom.py) so it believes it talks to an interactive terminal; pty.fork() also makes that pty the controlling terminal, so /dev/tty resolves to it even on headless machines. The emulated input redirects stdout to a file, runs two commands, recalls the first one with the up arrow and re-executes it, then un-redirects and quits. The driver prints, for comparison against the reference: * the content of the redirected file: it must contain only the output of the executed code, including the line produced by the recalled command. Before the fix the prompt and the terminal escape sequences leaked into the file. * whether the up arrow was echoed verbatim to the terminal: this happens when the terminal is stuck in cooked mode while stdout is redirected. Before the fix it was, so the arrow was echoed instead of recalling a command. To keep the second check reliable, each emulated line is sent only once ROOT has displayed the next prompt, i.e. once it has switched the terminal to raw mode and is waiting for input. Typing while ROOT is still starting up or busy would let the line discipline echo the keystrokes in cooked mode and produce a false positive. Both regressions were verified to make the test fail. 🤖 Done with the help of AI.
After the previous commit fOutputID is never assigned and stays equal to STDOUT_FILENO for the whole lifetime of the object: the prompt output target is now chosen dynamically in WriteRawString() (stdout, or the controlling terminal when stdout is redirected). Drop the member and use STDOUT_FILENO directly. This also removes the now-dead destructor branch that closed fOutputID and the always-true condition in WriteRawString(). No behavior change. 🤖 Done with the help of AI.
|
Thanks for the review! Indeed, it's good to implement a unit test, which I added in an additional commit to this PR. |
|
thanks! As far as I am concerned, if all tests pass, this PR is good to be merged. |
|
FWIW commit ec7998a breaks |
|
I see that failure too, when I run the tests locally on a computer that I don't ssh into. I guess it depends on whether the ctest process inherits a controlling terminal. Investigating. |
|
Ah yes, I'm in SSH but inside |
3 participants
The
.> filemeta command redirects stdout to a file by dup2-ing fd 1, and the redirection persists across prompts. TerminalDisplayUnix, however, decided only once at construction where to write the prompt and line-editing output: it switched to /dev/tty only if stdout was already not a tty at startup. In an interactive session stdout is a tty at startup, so it kept writing to fd 1. Once.> fileredirected fd 1 onto the file, the prompt, the echo of typed characters and the cursor movements all went into the file, leaving the command line invisible and unusable (arrow keys leaked raw escape sequences).Keep a handle on the controlling terminal (/dev/tty) whenever stdin is a tty, and make WriteRawString() fall back to it when stdout is not (or no longer) connected to a terminal. As long as stdout is a terminal the prompt still goes to stdout exactly as before, so nothing changes for the common interactive case; only once stdout is redirected (e.g. by
.> file) does the prompt go to the controlling terminal, keeping the command line usable while just the executed code's output follows the redirection.This also guards the open("/dev/tty") result: on failure we keep writing to stdout instead of leaving the descriptor at -1.
Closes #7626
🤖 Done with the help of AI.
A regression test is added in a separate commit.