external commands launched from config.fish don't have the correct tty modes #2980

Open
andrex-e-schulman opened this Issue Apr 27, 2016 · 21 comments

Projects

None yet

5 participants

@andrex-e-schulman
Contributor
andrex-e-schulman commented Apr 27, 2016 edited

When I run /bin/rm -i in fish 2.3b1 in Cygwin, the command waits for me to answer y or n, but it doesn't receive any keyboard input. I have to press Ctrl-C to abort the command.

Reproduction Steps:

  1. Run touch tstfile
  2. Run /bin/rm -i tstfile
  3. Command responds with /bin/rm: remove regular empty file 'tstfile'?

Expected behavior:

Command accepts y or n as an answer.

Observed behavior:

No keyboard input is accepted. The command hangs until I interrupt it with Ctrl-C.

Additional information:

rm -i worked fine in fish 2.2.0 in Cygwin.

fish-2.3b1-1 is available in Cygwin setup as a test release (or it will be shortly) (click on "Exp" at the top right to see test releases).


Fish version: 2.3b1

Operating system: Cygwin x86_64

Terminal or terminal emulator: mintty

@krader1961
Member

What happens if you press "y" or "n" followed by [ctrl-J] rather than [enter]? Do any other commands that read from stdin ignore their input? For example, what happens if you run cat and enter a line of text followed by [ctrl-D]?

@andrex-e-schulman
Contributor

What happens if you press "y" or "n" followed by [ctrl-J] rather than [enter]?

Fascinating. Yes, if I do that the rm command removes the file and returns, all without echoing any input:

andrex@selenium ~> touch tstfile
andrex@selenium ~> /bin/rm -i tstfile
/bin/rm: remove regular empty file 'tstfile'?
andrex@selenium ~> ls tstfile
ls: cannot access 'tstfile': No such file or directory
andrex@selenium ~>

Dunno what that means, but I guess you do.

Do any other commands that read from stdin ignore their input? For example, what happens if you run cat and enter a line of text followed by [ctrl-D]?

cat works normally:

andrex@selenium ~> cat
abcd[Enter]
abcd
[Ctrl-d]
andrex@selenium ~>

So does sort:

andrex@selenium ~> sort
ab[Enter]
aa[Ctrl-d]
aa
ab
andrex@selenium ~>
@krader1961
Member

It suggests the tty modes (e.g., what stty -a shows) aren't being set correctly. Normally icrnl mode is enabled on the tty to map \cM (carriage-return, what [enter] sends) to \cJ (newline). If icrnl mode is disabled you get the behavior you're observing. What isn't clear is why it is affecting just the /bin/rm -i prompt given that tty input to other commands is being correctly handled. I wonder if /bin/rm is opening /dev/tty to issue that prompt and read the response. That shouldn't make any difference but it's the only thing I can think of which might be relevant.

Please run stty -a so we can confirm the tty modes are set in a sane fashion for external commands.

@andrex-e-schulman
Contributor
andrex@selenium ~> stty -a
speed 38400 baud; rows 70; columns 110; line = 0;
intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = <undef>; eol2 = <undef>; swtch = ^Z; start = ^Q;
stop = ^S; susp = ^Z; rprnt = ^R; werase = ^W; lnext = ^V; discard = ^O; min = 1; time = 0;
-parenb -parodd cs8 -hupcl -cstopb cread -clocal -crtscts
-ignbrk brkint -ignpar -parmrk -inpck -istrip -inlcr -igncr -icrnl -ixon -ixoff -iuclc ixany imaxbel
opost -olcuc -ocrnl onlcr -onocr -onlret -ofill -ofdel nl0 cr0 tab0 bs0 vt0 ff0
isig -icanon iexten -echo echoe echok -echonl -noflsh -tostop echoctl echoke -flusho
@krader1961
Member

The -icrnl is the problem. I'm betting it's not affecting the cat and sort commands because they're reading from stdin in "text" mode and the Cygwin shims are mapping the carriage-return to newline. Whereas rm -i is probably opening /dev/tty in binary mode thus causing Cygwin to not do that mapping.

So now the question is why the tty modes aren't being reset to sane values by fish when it spawns an external command. I don't currently have Cygwin environment setup so I can't investigate this. For anyone who can build a debug version to run on Cygwin I would start by instrumenting restore_term_mode() to report whether the tcsetattr() call it does is failing and what the errno is.

P.S., The fact that we don't report unexpected syscall failures is itself a problem. We should at least be doing so if the debug level is greater than one.

@zanchey
Member
zanchey commented Apr 28, 2016

Did this work in 2.2.0?

@zanchey zanchey modified the milestone: fish-future, 2.3.0 Apr 28, 2016
@andrex-e-schulman
Contributor

Yes, it works as expected in fish 2.2.0 in Cygwin.

@zanchey
Member
zanchey commented Apr 28, 2016

Annoyingly, I can't reproduce this - either with the current source, or with the 2.3b1 build from Cygwin.

How are you starting fish within mintty?

@andrex-e-schulman
Contributor

OK. In fact, I run fish within screen, in mintty. I didn't check it before, but the input to rm -i works fine in mintty without screen:

andrex@selenium ~> echo $TERM
xterm-256color
andrex@selenium ~> touch tstfile
andrex@selenium ~> /bin/rm -i tstfile
/bin/rm: remove regular empty file 'tstfile'? y
andrex@selenium ~>

But it has the problem inside screen:

andrex@selenium ~> echo $TERM
screen-256color
andrex@selenium ~> touch tstfile
andrex@selenium ~> /bin/rm -i tstfile
/bin/rm: remove regular empty file 'tstfile'?
andrex@selenium ~> ls tstfile
ls: cannot access 'tstfile': No such file or directory
andrex@selenium ~>

Sorry for not checking that before.

So is that screen's fault?

@zanchey
Member
zanchey commented Apr 29, 2016

It certainly suggests that there's something in screen that doesn't cooperate with the modes that fish expects, but I'm not sure why. Do you have anything in your .screenrc?

@andrex-e-schulman
Contributor
andrex-e-schulman commented Apr 29, 2016 edited

In Cygwin, /etc/screenrc has:

$ grep -Ev '^\s*(#|$)' /etc/screenrc
defflow off # leave this off, so we can save in *emacs
deflogin on
vbell on
vbell_msg "   Wuff  ----  Wuff!!  "
termcap  facit|vt100|xterm LP:G0
terminfo facit|vt100|xterm LP:G0
termcap  vt100 dl=5\E[M
terminfo vt100 dl=5\E[M
termcap  facit al=\E[L\E[K:AL@:dl@:DL@:cs=\E[%i%d;%dr:ic@
terminfo facit al=\E[L\E[K:AL@:dl@:DL@:cs=\E[%i%p1%d;%p2%dr:ic@
termcap  sun 'up=^K:AL=\E[%dL:DL=\E[%dM:UP=\E[%dA:DO=\E[%dB:LE=\E[%dD:RI=\E[%dC:IC=\E[%d@:WS=1000\E[8;%d;%dt'
terminfo sun 'up=^K:AL=\E[%p1%dL:DL=\E[%p1%dM:UP=\E[%p1%dA:DO=\E[%p1%dB:LE=\E[%p1%dD:RI=\E[%p1%dC:IC=\E[%p1%d@:WS=\E[8;%p1%d;%p2%dt$<1000>'
termcap  xterm|fptwist hs@:cs=\E[%i%d;%dr:im=\E[4h:ei=\E[4l
terminfo xterm|fptwist hs@:cs=\E[%i%p1%d;%p2%dr:im=\E[4h:ei=\E[4l
termcap xterm 'is=\E[r\E[m\E[2J\E[H\E[?7h\E[?1;4;6l'
terminfo xterm 'is=\E[r\E[m\E[2J\E[H\E[?7h\E[?1;4;6l'
termcapinfo xterm*|rxvt*|kterm*|Eterm*|cygwin hs:ts=\E]0;:fs=\007:ds=\E]0;\007
termcap xterm|xterms|xs ti=\E7\E[?47l
terminfo xterm|xterms|xs ti=\E7\E[?47l
termcap  hp700 'Z0=\E[?3h:Z1=\E[?3l:hs:ts=\E[62"p\E[0$~\E[2$~\E[1$}:fs=\E[0}\E[61"p:ds=\E[62"p\E[1$~\E[61"p:ic@'
terminfo hp700 'Z0=\E[?3h:Z1=\E[?3l:hs:ts=\E[62"p\E[0$~\E[2$~\E[1$}:fs=\E[0}\E[61"p:ds=\E[62"p\E[1$~\E[61"p:ic@'
termcap wy75-42 nx:xo:Z0=\E[?3h\E[31h:Z1=\E[?3l\E[31h
terminfo wy75-42 nx:xo:Z0=\E[?3h\E[31h:Z1=\E[?3l\E[31h
bind ^k
bind ^\
bind \\ quit
bind K kill
bind I login on
bind O login off
bind } history

And my personal ~/.screenrc has:

$ grep -Ev '^\s*(#|$)' ~/.screenrc
escape `~
startup_message off
defflow off
deflogin off
defscrollback 5000
term screen-256color
bind ^k
bind ^\
bind x
bind \\ quit
bind K kill
bind } history
hardstatus on
hardstatus string "%h"
termcapinfo xterm*|rxvt*|kterm*|Eterm*|cygwin hs:ts=\E]0;:fs=\007:ds=\E]0;\007
caption always '%-Lw%{.kG} %n%f %t %{-}%+Lw'
markkeys ^b=^t:^f=^v:g=^T
@andrex-e-schulman andrex-e-schulman changed the title from rm -i doesn't accept input in fish 2.3b1 in Cygwin to rm -i doesn't accept input in fish 2.3b1 within screen in Cygwin Apr 29, 2016
@andrex-e-schulman
Contributor
andrex-e-schulman commented May 5, 2016 edited

Some more facts:

  1. /etc/screenrc and ~/.screenrc listed above don't matter. When I move them out of the way and restart screen, I still see the problem.
  2. The problem is still there in fish 2.3b2.
  3. Here's what does trigger the problem: At the end of my config.fish file, I have
if status --is-login
  screen
end

so when I start fish in a login shell, it takes me directly into a screen session. When this code runs and screen starts automatically, I have the problem. But if I comment out the above, then start a login shell and run screen manually by typing screen at the shell prompt, then rm -i works normally.

@krader1961 krader1961 self-assigned this May 5, 2016
@krader1961 krader1961 added the bug label May 5, 2016
@krader1961
Member

Very interesting. If I add

if status --is-interactive
  stty -a
end

to the bottom of my config.fish it reports -icrnl when it should report icrnl. That difference in how the tty driver is configured is why you're seeing that behavior. I'll take a look at the sequence of events before fish issues its first prompt.

@krader1961
Member

@andrex-e-schulman: You can workaround the problem by doing exec screen. Plus you really should do that regardless of this bug since the whole point is you want to replace the current shell with a screen shell.

Note that this bug has always been present. The problem is that fish doesn't restore the tty modes when running external commands from the startup scripts like ~/.config/fish/config.fish. This bug is simply more noticeable after my change last December to make it possible to bind \cM and \cJ independent of each other by disabling ICRNL mode.

@krader1961 krader1961 changed the title from rm -i doesn't accept input in fish 2.3b1 within screen in Cygwin to external commands launched from config.fish don't have the correct tty modes May 5, 2016
@andrex-e-schulman
Contributor

Thanks @krader1961. I confirm that if I start screen by exec screen within config.fish, rm -i works normally.

Unfortunately that solution doesn't work for me, because for me screen is a function that does some work - setting environment variables and so on - before running /usr/bin/screen. When I call that function within config.fish, a subshell is always started first, and it already has -icrnl set, which I then can't change.

So my workaround for now is not to run screen from config.fish. I can live with it.

@zanchey zanchey modified the milestone: fish-future, 2.3.0 May 6, 2016
@krader1961 krader1961 added a commit that referenced this issue May 15, 2016
@krader1961 krader1961 rename get_is_interactive and remove stupid test
I'm doing this as part of fixing issue #2980. The code for managing tty modes
and job control is a horrible mess. This is a very tiny step towards improving
the situation.
ff1d651
@krader1961
Member

This is closely related to issue #2619 which has already been fixed.

@faho
Member
faho commented May 25, 2016

This seems like something that would be nice for 2.3.1.

@faho faho modified the milestone: 2.3.1, fish-future May 25, 2016
@krader1961
Member

Gah! The fish code for managing process groups and the tty modes is a mess. The fix I was happy with three weeks ago turns out to have several failure modes. I've fixed those issues in a fashion that allows the unit tests to succeed and all of my ad-hoc interactive uses to succeed. Including running external commands from config.fish that read from the tty.

My change removes several bogosities in the code but introduces a couple of new ones. I'm going to create a pull-request for people to evaluate in a day or two after I've cleaned up the change by removing code I added to debug this issue. I am not happy with my fix since it is more of a bandaid than a perfect fix. Nonetheless it is clear that a perfect fix won't be possible in time for the 2.3.1 release so an imperfect fix is preferable to no fix at all.

@andrex-e-schulman
Contributor

Thanks for continuing to hammer at it. I wish I could help, but alas terminal I/O and tty modes are outside my ken.

@krader1961 krader1961 added a commit to krader1961/fish-shell that referenced this issue Jun 11, 2016
@krader1961 krader1961 fix tty modes for external commands
Fish doesn't set the tty modes correctly for external commands spawned from
startup scripts like config.fish.

This also corrects the use of is_forked_child() in common.cpp which I noticed
while debugging the core issue.

Fixes #2980
73e7e50
@krader1961 krader1961 added a commit to krader1961/fish-shell that referenced this issue Jun 11, 2016
@krader1961 krader1961 fix tty modes for external commands
Fish doesn't set the tty modes correctly for external commands spawned from
startup scripts like config.fish.

This also corrects the use of is_forked_child() in common.cpp which I noticed
while debugging the core issue.

Fixes #2980
ca95460
@krader1961 krader1961 modified the milestone: fish-future, 2.3.1 Jul 1, 2016
@floam floam modified the milestone: next-2.x, fish-future Jul 3, 2016
@andrex-e-schulman
Contributor

I thought this was fixed in 2.3.1, but now I find that it's not. To reiterate:

ASchulma@LZ77E1AASCHULMA ~> touch tstfile
ASchulma@LZ77E1AASCHULMA ~> /bin/rm -i tstfile
/bin/rm: remove regular empty file 'tstfile'? ⏎ASchulma@LZ77E1AASCHULMA ~>

The "y" that I typed at the prompt wasn't echoed, and pressing the Enter key didn't work - I had to type Ctrl-J.

This was in a shell within screen, when screen was started from ~/.config/fish/config.fish. If I wait instead for the initial fish prompt and then manually launch screen, this problem doesn't occur.

The problem is still there in 2.4b1.

@krader1961
Member

This is still unresolved. Which is why the issue is still open 😄

At the present time the only way to run an external command from your ~/.config/fish/config.fish if it depends on the tty modes is to prefix it with exec. Doing so means, obviously, that command will be the last command in your config that is run.

@krader1961 krader1961 added a commit to krader1961/fish-shell that referenced this issue Nov 7, 2016
@krader1961 krader1961 fix tty modes for external commands
Fish doesn't set the tty modes correctly for external commands spawned from
startup scripts like config.fish.

This also corrects the use of is_forked_child() in common.cpp which I noticed
while debugging the core issue.

Fixes #2980
dbbbe39
@zanchey zanchey modified the milestone: fish-future, fish 2.5.0 Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment