franciscolourenco / done

A fish-shell package to automatically receive notifications when long processes finish.
MIT License
779 stars 71 forks source link

allow xprop to be annyoingly picky about how it is asked for help #78

Closed jriddy closed 4 years ago

jriddy commented 4 years ago

Fixes the requested re-opening of #77 which seems tied to a mis-behaving xprop config.

This just allows us to consider xprop as working if it responds correctly to either the -help or -grammar flags.

jriddy commented 4 years ago

Tested manually on my Ubuntu 18.04 install with sleep 6

ammgws commented 4 years ago

Formatting looks off - try running fish_indent

jriddy commented 4 years ago

@ammgws Oh i did that intentionally, I didn't realize that there was an auto format command. Can't say I agree with it lining up the continuation of an if statement with the body of the if...it makes it visually hard to parse where the body of the "then" section of the if begins.

That said, I'll reformat

franciscolourenco commented 4 years ago

@jriddy thank you for the contribution. If grammar works consistently, should we drop -help all together?

jriddy commented 4 years ago

@franciscolourenco I don't know if -grammar works consistently, and I don't really have a good way of testing that across versions of xprop. I think a command responding with a zero exit code to a help invocation is a pretty reasonable expectation of all CLI programs and @ammgws has already made a PR for xprop upstream.

So I think we should leave this trying both invocations, especially if it worked before with -help

ammgws commented 4 years ago

-grammar looks like it has been around since the original commit for xprop, so it seems like a safe bet to me.

However, one thing we should check is, does xprop -help or xprop -grammar actually return 1 when "when DISPLAY is set, but no X server is running." as mentioned in #73? If it doesn't then all this work is for nothing. @rstacruz looks like the only one with an environment that can confirm this.

jriddy commented 4 years ago

-grammar looks like it has been around since the original commit for xprop, so it seems like a safe bet to me.

However, one thing we should check is, does xprop -help or xprop -grammar actually return 1 when "when DISPLAY is set, but no X server is running." as mentioned in #73? If it doesn't then all this work is for nothing. @rstacruz looks like the only one with an environment that can confirm this.

That should actually be pretty easy to check in a docker image.

jriddy commented 4 years ago

Okay my results are here...

from a really simple dockerfile like this:

FROM debian:buster

ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update -y \
 && apt-get install -y --no-install-recommends \
        x11-utils \
        fish
# too lazy to clean up the apt-cache here, this is just for testing

ENTRYPOINT [ "/usr/bin/fish" ]

I get this...

root@48e27001d789 /# set -x DISPLAY :0
root@48e27001d789 /# xprop -grammar
xprop:  unable to open display ':0'
root@48e27001d789 /# xprop -grammar; echo $status
xprop:  unable to open display ':0'
1

Seems like it fails when there's no display. I would argue that the correct behavior of a -help command shouldn't require a display (although in my experience a lot of X-related tools will fail for even trivial help and version commands if X isn't running :smile: :gun: ), and since -grammar has been around forever, i'll just switch it to that.

Perhaps a better test for X running is xset q, as per https://stackoverflow.com/a/11965765/199007 but I'll leave that choice up to you guys.

franciscolourenco commented 4 years ago

Thanks!

ammgws commented 4 years ago

Hopefully this is the end of this saga