Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Less fails to read pipe from dict when executed in a urxvt subshell. #372

Closed
tzcrawford opened this issue May 30, 2023 · 36 comments
Closed

Comments

@tzcrawford
Copy link

tzcrawford commented May 30, 2023

The command
urxvt -e sh -c "dict -d english 'blue' | less "
produces an empty less instance with no content or an empty string. Although
urxvt -e sh -c "dict -d english 'blue' | more "
works as expected, and so does
dict -d english 'blue' | less and urxvt -e sh -c "echo -e 'foo foo\n bar' | less".
So just running the three programs in conjuction is the issue, running either dict or urxvt alone with less is working. With xterm 380-1 subshell, I get the following command to execute as it should
xterm -e "dict -d english 'blue' | less".

Right now I am using:

less 1:633-1
rxvt-unicode 9.31-2 (9.26-3 appears to be the same)
rxvt-unicode-terminfo 9.31-2
dictd 1.13.1-4
linux 6.3.4-arch1-1 (Arch Linux build)
bash 5.1.16
perl 5.36.1-1 (under urxvt)
on an Acer Aspire E5-521 V1.03 laptop, AMD E2-6110 APU

This bug started occuring with v633 on the Arch Linux build, although v608 was fine. I can confirm at commit c8df315 the bug is still occuring.

@gwsw
Copy link
Owner

gwsw commented Jun 1, 2023

I am unable to reproduce with rxvt-unicode 9.26, dict 1.12.1/rf, bash 5.1.0(1), perl 5.32.1, on a Fedora 5.12.8 system. Running the first command produces an rxvt window with the dict output displayed.

I'm wondering about the version number "less 1:633-1". The "-1" appended to the version number makes me think this came from somewhere other than a direct compile of the less-633 source. But if you've reproduced this with a direct compile of c8df315 I guess that doesn't matter.

@barnzen
Copy link

barnzen commented Jun 22, 2023

There is indeed an issue with reading from pipe on version 633. Version 590 on another machine is fine though. Please see this URL https://codeberg.org/nsxiv/nsxiv/issues/451

@gwsw
Copy link
Owner

gwsw commented Jun 22, 2023

Can you explain how to reproduce the problem? I see a fragment of a script quoted on that page as part of a "key-handler file", but I don't know what that is nor how to invoke it. Also I don't have a program named "identify" on my system, and dnf install identify did not find such a program.

@tzcrawford
Copy link
Author

Can you explain how to reproduce the problem? I see a fragment of a script quoted on that page as part of a "key-handler file", but I don't know what that is nor how to invoke it. Also I don't have a program named "identify" on my system, and dnf install identify did not find such a program.

I believe identify is referring to a component of imagemagick, see https://imagemagick.org/script/identify.php

@barnzen
Copy link

barnzen commented Jun 22, 2023

Can you explain how to reproduce the problem? I see a fragment of a script quoted on that page as part of a "key-handler file", but I don't know what that is nor how to invoke it. Also I don't have a program named "identify" on my system, and dnf install identify did not find such a program.

I believe identify is referring to a component of imagemagick, see https://imagemagick.org/script/identify.php

Yes

$ file $(type -P identify)
/usr/bin/identify: symbolic link to magick

$ rpm -qf /usr/bin/identify
ImageMagick-7.1.1.11-1.fc38.x86_64

key-handler is the nsxiv config file for key bindings. It is executable and located at $XDG_CONFIG_HOME/nsxiv/exec/key-handler:

#!/usr/bin/sh

case $1 in
  i)
    tr '\n' '\0' | xargs -0 xterm -e sh -c 'identify -verbose "$@" | less' sh & ;;
esac

Key bindings require ^X as the prefix, so invoke the command on the current or selected images with ^X i.

@gwsw
Copy link
Owner

gwsw commented Jun 22, 2023

Ok, I installed ImageMagick and copied the script into my key-handler file. Now what? Do I need to install nsxiv too? And then open a photo with ImageMagick or something? Do I need to invoke nsxiv somehow?

@barnzen
Copy link

barnzen commented Jun 22, 2023

Now what?

$ dnf copr enable mamg22/nsxiv; dnf install nsxiv
$ nxiv img.jpg

Ctrl-x i to invoke the command on the viewed image.

Can also use sxiv instead:

$ dnf install sxiv
$ sxiv img.jpg

Everything is same for sxiv except key-handler need to be located at $XDG_CONFIG_HOME/sxiv/exec/key-handler.

@gwsw
Copy link
Owner

gwsw commented Jun 22, 2023

On the first step I get:

$ sudo dnf copr enable mamg22/nsxiv
Enabling a Copr repository. Please note that this repository is not part
of the main distribution, and quality may vary.

The Fedora Project does not exercise any power over the contents of
this repository beyond the rules outlined in the Copr FAQ at
<https://docs.pagure.org/copr.copr/user_documentation.html#what-i-can-build-in-copr>,
and packages are not held to any quality or security level.

Please do not file bug reports about these packages in Fedora
Bugzilla. In case of problems, contact the owner of this repository.

Do you really want to enable copr.fedorainfracloud.org/mamg22/nsxiv? [y/N]: y
Error: This repository does not have any builds yet so you cannot enable it now.

@barnzen
Copy link

barnzen commented Jun 22, 2023

On the first step

Test with sxiv instead, it's in the official Fedora repo. Don't forget to change the path of key-handler.

@gwsw
Copy link
Owner

gwsw commented Jun 22, 2023

Unfortunately I don't seem to be able to reproduce this. I tried about 25 times with 7 different jpgs, and it always opens an xterm window with less running and the identify output visible. Does it always fail when you do it?

@barnzen
Copy link

barnzen commented Jun 22, 2023

That's interesting.

Does it always fail when you do it?

Yes, always fails with identify, but works with exiv2, and identify never fails with most.

If it consistently worked for you with sxiv and less, obivously I got something. Thanks.

@gwsw
Copy link
Owner

gwsw commented Jun 22, 2023

I wonder if there is some timing issue. I notice that identify takes about 2 seconds to produce output on some jpg files (but is faster on others). Does it take a long time to produce output on the files you are using?

$ time identify -verbose p.jpg >/dev/null

real	0m2.053s
user	0m2.324s
sys	0m0.083s

@barnzen
Copy link

barnzen commented Jun 22, 2023

Looks likeMay be a timing issue, and identify is acting weird, but most is displaying output consistently on my system:

$ time identify -verbose tiny.jpg >/dev/null
real    0m0.020s
user    0m0.012s
sys     0m0.007s

$ time identify -verbose big.jpg >/dev/null
real    0m18.262s
user    0m19.073s
sys     0m0.521s

@barnzen
Copy link

barnzen commented Jun 22, 2023

When mediainfo is piped to less, sometimes there is output, and sometimes there isn't, so it looks like a synchronization issue, but again, most never fails :/

@gwsw
Copy link
Owner

gwsw commented Jun 22, 2023

I tried changing the config file line to

   tr '\n' '\0' | xargs -0 xterm -e sh -c '( sleep 15; identify -verbose "$@" ) | less' sh & ;;

to introduce an artificial delay. In this case I see a blank xterm for 15 seconds, and then the identify output appears. So two questions:

  1. Is it true that you never see the output appear, no matter how long you wait?
  2. Do you see output when you use the tiny.jpg file that your identify can process quickly?

Oh, one more thing, can you try this with less-635 or less-636? I think you're probably using less-633 which has a known issue involving startup timing.

@barnzen
Copy link

barnzen commented Jun 23, 2023

  1. Is it true that you never see the output appear, no matter how long you wait?

Yes

  1. Do you see output when you use the tiny.jpg file that your identify can process quickly?

No, consistently no output when identify is piped to less.

can you try this with less-635

Unforunately, there is still no output with either 635 or 636.

636 was missing the configure script, failed to autoconf and autoreconf as well, had to make less-636-beta.tar.gz myself with make -f Makefile.aut dist.

@gwsw
Copy link
Owner

gwsw commented Jun 24, 2023

@barnzen can you upload or link to one of the jpgs that you are using? It seems unlikely that the specific jpg would matter, but I want to eliminate all possible differences between our tests to try to determine why I am not reproducing what you see.

@barnzen
Copy link

barnzen commented Jun 25, 2023

Can't be a specific img because the problem exists for all imgs I tried, and also because identify -verbose works fine at the shell prompt with or without less.

The below commands consistently work at the shell prompt for <ver> 590 or 608, and don't work for <ver> 633, 635 and 636-beta:

identify -verbose img1.jpg img2.jpg | ~/<ver>/less
tmux new sh -c 'identify -verbose img1.jpg img2.jpg | ~/<ver>/less' sh
xterm -e sh -c 'identify -verbose img1.jpg img2.jpg | ~/<ver>/less' sh &
xterm -e tmux new sh -c 'identify -verbose img1.jpg img2.jpg | ~/<ver>/less' sh &

And strangely the above commands work for all <ver> consistently in vanilla Fedora 38 booted from USB running Wayland.

cp ~/608/less /usr/bin/less solves whatever problem I have.

@gwsw
Copy link
Owner

gwsw commented Jun 25, 2023

Ok, this is new information. I had thought that xterm was required to reproduce the problem, but you're saying that a simple pipe of identify into less is failing (as in the first command). Can you confirm whether this works?

identify -verbose img1.jpg img2.jpg > out; cat out | ~/<ver>/less

And regarding your second to last comment, are you saying that less-635 and less-636 work when you boot from USB? Or only less-608 works in that case?

@barnzen
Copy link

barnzen commented Jun 25, 2023

Can you confirm whether this works?

It works.

are you saying

All versions consistently work in vanilla Fedora 38 live image booted from USB, which runs Wayland instead of Xorg.

@gwsw
Copy link
Owner

gwsw commented Jun 25, 2023

So identify fails but cat of the same data works, but the failure goes away if you use wayland. Very strange. When it fails, do you get a prompt from less, or is it hung and needs to be killed?

Could you try this, using less-636, and upload the strace.out file?

identify -verbose img1.jpg img2.jpg | strace -o strace.out ~/636/less

@barnzen
Copy link

barnzen commented Jun 25, 2023

Sorry I gave wrong info above, this may be an xterm bug.

The commands without xterm actually work with all versions of less.

xterm -e sh -c 'identify -verbose img1.jpg img2.jpg | ~/633/less' sh & does not work for version 633 or later.

But the same command works with all versions of less on the gnome terminal:

gnome-terminal -- sh -c 'identify -verbose img1.jpg img2.jpg | ~/633/less' sh &

@gwsw
Copy link
Owner

gwsw commented Jun 26, 2023

Ok. Three questions:

  1. When it fails, you said you see a blank window. Is there a less prompt at the bottom of the window, or is it completely blank?
  2. Can you run this command (using less-636) and upload the strace.out file?
xterm -e sh -c 'identify -verbose img1.jpg img2.jpg | strace -o strace.out ~/636/less' sh &
  1. Does it work with urxvt? The original report in this thread said that urxvt didn't work, although with a different shell command.

@barnzen
Copy link

barnzen commented Jun 26, 2023

Okay the problem was setting the xterm's vertical size to a value slightly larger than the screen size, the difference is less than a cell actually.

With the command xterm -e sh -c 'identify -verbose img1.jpg img2.jpg | less' sh &, less versions 633 and later display the output with XTerm*geometry: 150x59 in .Xresources, but they display an empty buffer (less prompt line is still visible) with XTerm*geometry: 150x60 in .Xresources.

The same problem occurs with less verions 633 and later with both urxvt and gnome-terminal if the vertical size of the terminal is set to a value larger than the screen size. Again, only the less versions 633 and later display the problem, 608 is working well even if the the vertical size of the terminal exceeds the screen size.

@gwsw
Copy link
Owner

gwsw commented Jun 26, 2023

Thanks, this is very useful information to help track down where the problem is. Unfortunately I still can't reproduce it. I set my screen size (in my VM) to just barely hold a 59-line xterm, add XTerm*geometry: 150x60 in .Xresources, then run your xterm command, and it produces valid output from identify in a 59-line xterm. I guess X sizes the terminal to fit the screen. Am I missing anything in how to reproduce?

@barnzen
Copy link

barnzen commented Jun 26, 2023

Something may be masking the SIGWINCH. Try without virtualization and in a bash shell if you can. I can consistently reproduce it with less version 633 or later in xterm, urxvt, mate-terminal, and gnome-terminal on Fedora 37 & 38 as the host OS. Less displays the output at first, but it very quickly disappears, so it's like it never showed anything, and it isn't redrawn after the window resize. Thanks.

@gwsw
Copy link
Owner

gwsw commented Jun 26, 2023

Hm, I didn't realize that the file is briefly visible. It would be interesting to see the output from running with LESS_TERMCAP_DEBUG set to 1.

@gwsw
Copy link
Owner

gwsw commented Jun 26, 2023

Are you using a LESSOPEN script?

@barnzen
Copy link

barnzen commented Jun 26, 2023

Are you using a LESSOPEN script?

No

LESS_TERMCAP_DEBUG=1 xterm -e sh -c 'identify -verbose img1.jpg img2.jpg | less' sh just gives:

<md>~<me>
...
<md>~<me>
<so>Standard input [?-?/?]<se><ce>

XTerm*geometry: 150x59 works:
strace59.txt

XTerm*geometry: 150x60 doesn't work:
strace60.txt

@gwsw
Copy link
Owner

gwsw commented Jun 27, 2023

Can you apply this patch to one of the broken versions (less-633 or less-635 or less-636) and see if it fixes it for you?

diff --git a/os.c b/os.c
index 35eb153..7cc6f08 100644
--- a/os.c
+++ b/os.c
@@ -183,7 +183,7 @@ start:
    }
 #endif
 #endif
-   if (!reading && SET_JUMP(read_label))
+   if (SET_JUMP(read_label))
    {
        /*
         * We jumped here from intread.

@barnzen
Copy link

barnzen commented Jun 27, 2023

see if it fixes

Doesn't fix 633 or master but thanks for the effort

for you

I'd say try to reproduce the problem for you before patching. It is reproducable for me on different machines on different terminal emulators.

Linux 6.3.8-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Jun 15 02:15:40 UTC 2023 x86_64 GNU/Linux
GNU bash, version 5.2.15(1)-release (x86_64-redhat-linux-gnu)
xorg-x11-server-Xorg-1.20.14-23.fc38.x86_64
GNOME Shell 44.2
Fedora Linux 38 (Workstation Edition)
XTerm(380)
rxvt-unicode (urxvt) v9.31
GNOME Terminal 3.48.1 using VTE 0.72.2 +BIDI +GNUTLS +ICU +SYSTEMD
MATE Terminal 1.26.1

All I do for xterm is change 59 in the line XTerm*geometry: 150x59 in .Xresources to a larger value and

$ xrdb -merge ~/.Xresources
$ xterm -e sh -c 'identify -verbose img1.jpg img2.jpg | ~/633/less' sh

Strangely, other commands like exiv2, file, stat, even identify without the -verbose option produce output.

@barnzen
Copy link

barnzen commented Jun 28, 2023

Okay, sleeping actually solves the problem lol In the first attempt, you slept before identify, and also in a subshell, so my mind must have just skipped it (I mean I tried your sleep command, but didn't think about sleeping after pipe and before less, where it would actually be needed), sorry. I thought about trying to sleep again only after realizing that identify worked without -verbose.

xterm -e sh -c 'identify -verbose img1.jpg img2.jpg | ~/633/less' sh does not work.
xterm -e sh -c 'identify -verbose img1.jpg img2.jpg | { sleep 1; ~/633/less; }' sh does work.

How come it fails in all terminals I tried, and only with the -verbose option of identify, and how come changing the window size affects it, I have no idea.

Still versions before 633 work in all cases, without requiring to sleep before the pipe.

@gwsw
Copy link
Owner

gwsw commented Jun 28, 2023

I can see in your strace output that in the failing case, less receives a SIGWINCH very early, during its first read of data from the input pipe. I guess this is X resizing the terminal from 60 to 59 lines. But somehow less is not responding appropriately to the SIGWINCH, since it doesn't redraw the screen. The patch I sent was one change that I noticed had been made to signal handling between less-608 and less-633, so I hoped that might be it, but I guess not.

The sleep you added in your last test probably delays less startup enough that the SIGWINCH occurs before less starts. Perhaps I'm not able to reproduce this because my computer is faster (or slower) than yours, which is changes the timing of the SIGWINCH. I will continue to look into this.

@gwsw
Copy link
Owner

gwsw commented Jun 28, 2023

A couple of interesting points:

I missed this before, but based on your strace it looks like less IS redrawing the screen after it receives SIGWINCH. Initially less detects the terminal as having 60 lines (line 66 in strace60.txt), then it receives SIGWINCH (line 99), then it detects that the terminal has been resized to 59 lines (line 104), then it sends what appears to be a full screen of data (in line 106 it writes 667 bytes). I don't know why this redraw data doesn't appear on the screen.

Running strace on my system, less detects the initial terminal size as 59 lines, not 60, and it never receives a SIGWINCH. Perhaps I have a different version of X and it chooses the correct terminal size at startup, rather than drawing once then resizing as yours seems to do. I have xorg-x11-server-Xorg-1.20.11-1.fc34.x86_64.

@gwsw
Copy link
Owner

gwsw commented Jun 28, 2023

Ok, I think I've figured this out. It should be fixed in 9d7be3c. Let me know whether that works for you.

@barnzen
Copy link

barnzen commented Jun 28, 2023

Let me know whether that works for you.

Nice. Thanks a lot.

I think I've figured this out

For any possible future problems on this, entering help with h and quiting help with q did actually cause less to read from pipe and write the right amount; a screenful of less buffer having the output of identify for those two images would be 1371 bytes on my end (and more than that with escape sequences emitted by less), and the whole output of identify for those two images would be 5285 bytes, the following lines from strace.txt in the attachment occurs after quiting help inside less:

read(0, "Image:\n  Filename: img1.jpg\n  Pe"..., 8192) = 5285
write(1, "Image:\33[m\n  Filename: img1.jpg\33["..., 1023) = 1023
write(1, "     entropy: 0.163925\33[m\n    Bl"..., 557) = 557
read(3, "q", 1)                         = 1

See the strace.txt for 633 strace.txt.

Also, -c/--clear-screen ("Repaint by clearing rather than scrolling") inside less didn't have any effect, the buffer was still empty until entering and exiting the help.

Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants