<div dir="ltr">Hi,<div class="gmail_extra"><br><div class="gmail_quote">On 24 October 2014 11:18, Pekka Paalanen <span dir="ltr"><<a href="mailto:pekka.paalanen@collabora.co.uk" target="_blank">pekka.paalanen@collabora.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Fri, 24 Oct 2014 10:28:57 +0100<br>
Daniel Stone <<a href="mailto:daniel@fooishbar.org">daniel@fooishbar.org</a>> wrote:<br>><br>
> Hmm. Can we please use weston_client_start here instead of open-coding it?<br>
<br>
</span>weston_client_start() does not support passing in environment<br>
explicitly.</blockquote><div><br></div><div>Entirely fixable.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It also automatically sets up WAYLAND_SOCKET environment<br>
variable and creates a connection (wl_client) before the program even<br>
starts.<br>
<br>
I do not think it makes sense to use weston_client_start() here,<br>
because whatever we launch here, might not be a single-shot Wayland<br>
client. Especially in the script case mentioned below, it would<br>
fail all restart attempts in the script as WAYLAND_SOCKET would be set<br>
to a disconnected/invalid fd.</blockquote><div><br></div><div>Well sure, but the script case is indeed something which just restarts in a loop - which would be 100% unnecessary if autostart did that for us. Allowing multiple autostart entries would eliminate another reason to write scripts, in that you could launch multiple clients. And if you really, really, really need it: unset WAYLAND_SOCKET? Obviously that would preclude use of autorestart.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I'm not sure that's feasible. Watching the process exit is not used to<br>
trigger respawn for weston-desktop-shell, but losing the wl_client is.<br>
These two events happen in arbitrary order, and we recently fixed a<br>
related bug by exactly not respawning based on process exit. We want<br>
weston-desktop-shell to respawn if the compositor disconnects it, even<br>
if the actual process gets left behind due to malfunction.<br>
<br>
Restart for autolaunch OTOH would be based on process exit rather than<br>
losing the connection.<br></blockquote><div><br></div><div>I can see where you're going with this, but again, this could be solved with documentation: if you have a client which terminates the WAYLAND_SOCKET before it's actually exited, either don't enable autorestart, or handle it yourself.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
IOW, weston_client_start() and the existing respawn mechanism are<br>
intended for special purpose known clients, while the panel<br>
launchers and the autolaunch feature are meant for running arbitrary<br>
programs (that might not even be clients themselves/directly or at all).<br>
<br>
I'm not sure if adding restart to autolaunch is in scope... there are<br>
many aspects to configure for restart (delays, when to give up)<br>
so I'd rather maybe keep with the script. Or systemd user session.<br>
After all, the autolaunch is just a quick'n'useful hack when no session<br>
manager (systemd or other) is available.<br></blockquote><div><br></div><div>Sure, but having everyone just code the same while true script is necessarily that great either. Having Weston more usable OOTB as a kiosk mode is a pretty good thing, and having it be more robust doubly so.</div><div><br></div><div>I mean, I don't disagree with you, it's just that I don't see these issues as showstoppers - if people want to write random weird scripts or clients which don't fit in with passing sockets or autorestart, then don't use those features?</div><div><br></div><div>Cheers,</div><div>Daniel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks,<br>
pq<br>
<div class="HOEnZb"><div class="h5"><br>
> Aside from that, and splitting refactor / autorestart code move / autorun<br>
> feature into separate patches, this looks good to me.<br>
><br>
> Cheers,<br>
> Daniel<br>
<br>
</div></div></blockquote></div><br></div></div>