Make display detection more robust. #916
Conversation
I am not certain whether this change makes the detection logic much more readable but I do think it makes it slightly more robust while also spreading it out a little.
5 seconds is probably on the long side but if 5s is not long enough then my hypothesis that the issue is startup time may be incorrect.
Substitutions don't print by default when print suppression is on.
| # Strip the path and leading X from the X11 socket | ||
| # but check that the resulting string is numeric and | ||
| # non-empty before exporting. | ||
| DISPLAY=:$(basename $i | sed -n -E 's/^X([0-9]+)/:\1/p') |
There was a problem hiding this comment.
$i is no longer just the value of the DISPLAY variable so once confirming a likely active X11 server we need to get the value of DISPLAY for export. Compared with the prior substitution this one suppresses values that do not match X[0-9]+.
| if [ -n $DISPLAY ]; then | ||
| export DISPLAY | ||
| fi |
There was a problem hiding this comment.
Since the variable value could be empty, but an earlier socket file may have set a DISPLAY, only export the current value if it is not empty.
| # Wait for lightdm service to restart X11. | ||
| sleep 5 |
There was a problem hiding this comment.
5 seconds is possibly way too long, but if this does not work as expected we likely need a different hypothesis.
There was a problem hiding this comment.
Trying to figure out how we can inspect systemd activations, maybe a loop with a timeout on systemctl is-active can work here.
There was a problem hiding this comment.
Yeah I'm not certain that the watching lightdm activation / status would be sufficient to resolve the race since I don't know if lightdm uses systemd's notification API for actually declaring readiness or if it's a simple/exec type service which is considered active as soon as the process runs https://www.freedesktop.org/software/systemd/man/systemd.service.html in which case we'd still need to wait ourselves for X11 to start.
| # but check that the resulting string is numeric and | ||
| # non-empty before exporting. | ||
| DISPLAY=:$(basename $i | sed -n -E 's/^X([0-9]+)/:\1/p') | ||
| if [ -n $DISPLAY ]; then |
There was a problem hiding this comment.
| if [ -n $DISPLAY ]; then | |
| if [[ -n $DISPLAY ]]; then |
dragons are in [ ] single brackets https://www.shellcheck.net/wiki/SC2070
There was a problem hiding this comment.
Good catch, I added quotes in 73ba462 I prefer POSIX compatible shell to Bash-isms where possible, although since this is an explicitly bash script I don't mind being overridden on this point.
| # Wait for lightdm service to restart X11. | ||
| sleep 5 |
There was a problem hiding this comment.
Trying to figure out how we can inspect systemd activations, maybe a loop with a timeout on systemctl is-active can work here.
Attempting to improve the display detection for the Linux GPU agents to resolve #906.