GNOME Bugzilla – Bug 766330
server: Add a way for Vino to be started via user systemd
Last modified: 2016-09-05 10:52:01 UTC
.
Created attachment 327713 [details] [review] server: Add a way for Vino to be started via user systemd This will allow better tracking of whether vino is running, ensuring that even if the manager to enable/disable its startup (gnome-settings-daemon) crashes and needs to restart, its status is still available by querying the user systemd.
Review of attachment 327713 [details] [review]: Seems fine to me.
Review of attachment 327713 [details] [review]: ::: server/vino-server.service.in @@ +5,3 @@ +Type=dbus +BusName=org.gnome.Vino +ExecStart=@libexecdir@/rygel This does not seem right
Created attachment 327723 [details] [review] server: Add a way for Vino to be started via user systemd This will allow better tracking of whether vino is running, ensuring that even if the manager to enable/disable its startup (gnome-settings-daemon) crashes and needs to restart, its status is still available by querying the user systemd.
(In reply to Giovanni Campagna from comment #3) > Review of attachment 327713 [details] [review] [review]: > > ::: server/vino-server.service.in > @@ +5,3 @@ > +Type=dbus > +BusName=org.gnome.Vino > +ExecStart=@libexecdir@/rygel > > This does not seem right Indeed. Shows that 1) I didn't test all those changes yet, but am planning to before committing 2) Where the cut'n'paste came from ;)
Review of attachment 327723 [details] [review]: Seems fine.
Created attachment 327765 [details] [review] server: Add a way for Vino to be started via user systemd This will allow better tracking of whether vino is running, ensuring that even if the manager to enable/disable its startup (gnome-settings-daemon) crashes and needs to restart, its status is still available by querying the user systemd.
Review of attachment 327765 [details] [review]: ::: Makefile.am @@ +316,3 @@ MAINTAINERCLEANFILES = $(gsettings_SCHEMAS:.xml=.valid) +DISTCHECK_CONFIGURE_FLAGS = --with-systemduserunitdir='$${libdir}/systemd/user-distcheck' If this is necessary for distcheck to succeed, it should be AM_DISTCHECK_CONFIGURE_FLAGS (a developer/maintainer option), not DISTCHECK_CONFIGURE_FLAGS (which is reserved for the consumer): https://www.gnu.org/software/automake/manual/automake.html
Review of attachment 327765 [details] [review]: ::: configure.ac @@ +270,3 @@ + AS_HELP_STRING([--with-systemduserunitdir=DIR], [Directory for systemd user service files]), + [], + [with_systemduserunitdir=$($PKG_CONFIG --variable=systemduserunitdir systemd)]) maybe before calling $PKG_CONFIG directly there should be a check first like PKG_CHECK_EXISTS (systemd, , AC_MSG_ERROR ([systemd development headers not found])) so it errors out more cleanly ? ::: server/vino-server.service.in @@ +1,2 @@ +[Unit] +Description=Vino VNC server So there were some recent changes to how systemd --user targets are laid out. see https://github.com/systemd/systemd/commit/c92fcc4f4375b0aebc5919311bbf703138b21918 I wonder if we should put PartOf=graphical-session.target so vino is automatically killed if the session ends. Downside is it ties us to an unreleased systemd, so maybe skip for now. just food for thought. @@ +3,3 @@ + +[Service] +Type=dbus if you're going to make this type dbus then you need to change the dbus service file to map to the systemd service file using SystemdService=vino=server.service . Also, by convention the Exec line is normally changed to /bin/false in the dbus service file to make it clear its no longer used.
(In reply to Ray Strode [halfline] from comment #9) > Review of attachment 327765 [details] [review] [review]: > > ::: configure.ac > @@ +270,3 @@ > + AS_HELP_STRING([--with-systemduserunitdir=DIR], [Directory for systemd > user service files]), > + [], > + [with_systemduserunitdir=$($PKG_CONFIG --variable=systemduserunitdir > systemd)]) > > maybe before calling $PKG_CONFIG directly there should be a check first like > PKG_CHECK_EXISTS (systemd, , AC_MSG_ERROR ([systemd development headers not > found])) so it errors out more cleanly ? Fixed. We can make this better in a separate patch. > ::: server/vino-server.service.in > @@ +1,2 @@ > +[Unit] > +Description=Vino VNC server > > So there were some recent changes to how systemd --user targets are laid > out. see > https://github.com/systemd/systemd/commit/ > c92fcc4f4375b0aebc5919311bbf703138b21918 > > I wonder if we should put PartOf=graphical-session.target so vino is > automatically killed if the session ends. Downside is it ties us to an > unreleased systemd, so maybe skip for now. just food for thought. Made a note, and will file bugs for that as well. > @@ +3,3 @@ > + > +[Service] > +Type=dbus > > if you're going to make this type dbus then you need to change the dbus > service file to map to the systemd service file using > SystemdService=vino=server.service . Also, by convention the Exec line is > normally changed to /bin/false in the dbus service file to make it clear its > no longer used. There's no D-Bus service file for vino, it's not autostartable. I also fixed the duplicated DISTCHECK_CONFIGURE_FLAGS.
Created attachment 334428 [details] [review] server: Add a way for Vino to be started via user systemd This will allow better tracking of whether vino is running, ensuring that even if the manager to enable/disable its startup (gnome-settings-daemon) crashes and needs to restart, its status is still available by querying the user systemd.
Created attachment 334429 [details] [review] build: Use AM_DISTCHECK_CONFIGURE_FLAGS And not DISTCHECK_CONFIGURE_FLAGS, as it is a developer/maintainer option, not a user option.
Attachment 334428 [details] pushed as 27785a2 - server: Add a way for Vino to be started via user systemd Attachment 334429 [details] pushed as b1d968e - build: Use AM_DISTCHECK_CONFIGURE_FLAGS
Created attachment 334524 [details] [review] build: fix syntax error (vino) Patch to fix this error: ./configure: line 15051: syntax error near unexpected token `(' ./configure: line 15051: `fi (systemd,, as_fn_error $? "" "$LINENO" 5 (systemd development headers not found))'
Created attachment 334525 [details] [review] build: fix syntax error (gnome-user-share) Patch for same error.
Configure fails with syntax error after 27785a2 commit.
This causes vino to fail to configure when systemd is not available. Can we add an option to allow building vino on non-Linux systems? configure: error: systemd development headers not found
Already covered by bug 770759.