Fix "Failed tcsetattr(TCSADRAIN)" when nix repl is not a TTY
#10010
Conversation
Before: ``` $ echo builtins.nixVersion | nix repl Welcome to Nix 2.18.1. Type :? for help. Failed tcsetattr(TCSADRAIN): Inappropriate ioctl for device "2.18.1" Failed tcsetattr(TCSADRAIN): Inappropriate ioctl for device ``` After: ``` $ echo builtins.nixVersion | nix repl Nix 2.21.0pre20240131_dirty Type :? for help. "2.21.0pre20240131_dirty" ```
github-actions
bot
added
the
repl
| @@ -351,7 +351,6 @@ bool NixRepl::getLine(std::string & input, const std::string & prompt) | |||
| }; | |||
|
|
|||
| setupSignals(); | |||
| Finally resetTerminal([&]() { rl_deprep_terminal(); }); | |||
There was a problem hiding this comment.
editline already calls rl_prep_terminal and rl_deprep_terminal in readline, so I'm not sure what the point of this was, especially since there's no matching rl_prep_terminal call.
It was added in #5665 without comment about what sort of "exceptional exits" can lead to the terminal not being reset or any tests.
There was a problem hiding this comment.
I doubt I did #5665 for no reason, so I assume it was needed at the time. So if we remove this, we have to be sure that there are no exceptions that can leave the terminal in a raw state.
There was a problem hiding this comment.
Can we test this assumption it is necessary by putting some code in there that throws an uncaught exception and see if it indeed fails to deinitialize the terminal?
There was a problem hiding this comment.
in gnu readline also calls deprep automatically. we can't find anything in the readline history that would tell us why calling deprep manually should be necessary unless you're hooking into readline itself, which nix does not do.
There was a problem hiding this comment.
What if a completer throws? (n.b. it is also probably bad to unwind through libreadline, which is c, in such an instance)
There was a problem hiding this comment.
we'd consider a throwing completer a bug for the exact reason you mentioned. this kind of thing should be handled by wrappers that stash away the exception, return through the normal readline path, and rethrow it after readline has finished processing
| @@ -351,7 +351,6 @@ bool NixRepl::getLine(std::string & input, const std::string & prompt) | |||
| }; | |||
|
|
|||
| setupSignals(); | |||
| Finally resetTerminal([&]() { rl_deprep_terminal(); }); | |||
There was a problem hiding this comment.
alternatively :)
Finally resetTerminal([&]() { if (isatty(STDIN_FILENO) rl_deprep_terminal(); });
Fix "Failed tcsetattr(TCSADRAIN)" when `nix repl` is not a TTY (cherry picked from commit 864fc85) Change-Id: I8198674b935fabd741a349cc74544e61c53ea7b3
Fixes #9941.
Before:
After: