firecat53 / urlscan

Mutt and terminal url selector (similar to urlview)
GNU General Public License v2.0
214 stars 38 forks source link

urlscan should read from stderr #122

Closed b0o closed 2 years ago

b0o commented 2 years ago

I use a shell script as my $BROWSER to launch my actual browser. My script writes to stderr; when invoked through urlscan, it seems stderr is not writable:

browser.sh:

#!/usr/bin/env bash
ls -la /proc/self/fd/ > /tmp/browser-out
$ BROWSER=/tmp/browser.sh urlscan urls.txt 
... open a URL and quit urlscan ...
$ cat /tmp/browser-out
total 0
dr-x------ 2 maddy maddy  0 Feb 23 03:02 .
dr-xr-xr-x 9 maddy maddy  0 Feb 23 03:02 ..
lrwx------ 1 maddy maddy 64 Feb 23 03:02 0 -> /dev/pts/222
lrwx------ 1 maddy maddy 64 Feb 23 03:02 1 -> /dev/pts/222
lr-x------ 1 maddy maddy 64 Feb 23 03:02 2 -> /home/maddy/bin/browser.sh
lr-x------ 1 maddy maddy 64 Feb 23 03:02 255 -> /home/maddy/bin/browser.sh

Writing to stderr returns a non-zero exit code, and since my script uses bash's -e (errexit) option, it crashes.

Reproduction:

$ cat browser.sh
#!/usr/bin/env bash
set -euo pipefail

echo 1 >/tmp/browser-out
echo stdout
echo 2 >>/tmp/browser-out
echo stderr >&2
echo 3 >>/tmp/browser-out
echo stdout
echo 4 >>/tmp/browser-out
echo stderr >&2
echo 5 >>/tmp/browser-out

$ BROWSER="$PWD/browser.sh" urlscan urls.txt
... open a URL and quit urlscan ...

If you look at /tmp/browser-out, the expected contents would be...

1
2
3
4
5

...but the actual contents are...

1
2

If I just don't write to stderr, everything works fine, but this seems like a bug and it took me a while to figure out what was going on. I think urlscan should either pipe stderr to the terminal or to /dev/null.

firecat53 commented 2 years ago

In commit 01e4cfbf I basically turned off stderr so the extraneous terminal (stderr) output from browsers or xdg-open would not disturb the urlscan display. This happens in urlscan/urlchoose.py (line 754):

savout = os.dup(2)
os.close(2)
os.open(os.devnull, os.O_RDWR)
....
os.dup2(savout, 2)
if thread is False:
    self.draw_screen()

I'm interested in keeping stderr suppressed from a usability standpoint. It's typically annoying to have to hit Ctrl-L every time after you open a URL.

I actually don't understand how your browser.sh script works! What functionality do you lose if you don't have stderr for this use case?

Thanks for your interest!

b0o commented 2 years ago

Thanks for the quick response. It seems like that code is trying to redirect stderr to /dev/null, am I correct? I’m not sure if it’s actually working because my script sees stderr as not writable at all.

The browser.sh scripts above are just for demonstration purposes. The first example I’m listing the contents of /proc/self/fd which shows that fd 2 (stderr) is not writable (it has permissions lr-x------).

The second example is just demonstrating that my script fails after the echo 2 >>/tmp/browser-out, which implies that it’s failing on the next line when attempting to write to stderr.

It’s not that I lose functionality, but that it was a rather unexpected failure and took a while to debug. I am fine with stderr going to /dev/null, but attempting to write to it should not fail. I imagine this could trip someone up in the future.

firecat53 commented 2 years ago

Please give the develop branch a try. I changed how the redirection of stdout/stderr works and it seems to work with your test case. Let me know if it breaks anything else!

b0o commented 2 years ago

Fantastic, that works. Will let you know if anything breaks.

I also have to say that I love this tool and I appreciate your work on it. Thank you!

I'll let you decide if this should be closed now or when the change is merged into main.

firecat53 commented 2 years ago

Thanks for the kind words!