GNOME Bugzilla – Bug 755897
evince: add a systemd user service for the daemon D-Bus session service
Last modified: 2016-04-13 07:15:33 UTC
Created attachment 312446 [details] [review] Add a systemd user unit corresponding to the D-Bus session service This lets systemd put Evince in its own cgroup, instead of treating it as part of dbus.service. --- See https://bugzilla.gnome.org/show_bug.cgi?id=755735#c3 for a more detailed explanation of why this is desirable. As with Bug #755735, I could make the location of the systemd file a configure-time option if desired, but in this patch I've kept it simple, similar to Bug #755760.
Review of attachment 312446 [details] [review]: Thanks for the patch. I agree with Milan that this should be optional, so that distros cant just disable it. I don't think we need an option to change the destination dir, though, just enabled/disable it would be enough, leaving it enabled by default. So, feel free to push the patch if you add the configure option.
Review of attachment 312446 [details] [review]: ::: data/Makefile.am @@ +41,3 @@ $(AM_V_GEN) $(SED) -e "s|\@libexecdir\@|$(libexecdir)|" $< > $@ + +systemd_userdir = $(prefix)/lib/systemd/user Oh, use $libdir here as well, please.
(In reply to Carlos Garcia Campos from comment #2) > Oh, use $libdir here as well, please. No, $libdir is specifically not correct here. systemd services are kept in /usr/lib/systemd (or /lib/systemd if the distribution has split /usr), even if $libdir is something like /usr/lib64 (Red Hat etc.) or /usr/lib/x86_64-linux-gnu (Debian etc.)
(In reply to Simon McVittie from comment #3) > (In reply to Carlos Garcia Campos from comment #2) > > Oh, use $libdir here as well, please. > > No, $libdir is specifically not correct here. systemd services are kept in > /usr/lib/systemd (or /lib/systemd if the distribution has split /usr), even > if $libdir is something like /usr/lib64 (Red Hat etc.) or > /usr/lib/x86_64-linux-gnu (Debian etc.) Ah, it's ok then.
(In reply to Simon McVittie from comment #3) > systemd services are kept in > /usr/lib/systemd (or /lib/systemd if the distribution has split /usr) Slight correction there: the split-/usr thing only applies to system services. User services like this one are always in /usr/lib, even on split-/usr distributions like Debian. (That doesn't invalidate my point though.)
This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.
(In reply to Carlos Garcia Campos from comment #1) > Review of attachment 312446 [details] [review] [review]: > > Thanks for the patch. I agree with Milan that this should be optional, so > that distros cant just disable it. I don't think we need an option to change > the destination dir, though, just enabled/disable it would be enough, > leaving it enabled by default. So, feel free to push the patch if you add > the configure option. I don't see the configure option. In what way is installing "lib/systemd/user/evince.service" optional?
(In reply to Patrick Welche from comment #7) > I don't see the configure option. In what way is installing > "lib/systemd/user/evince.service" optional? I'm not the one who committed it, but I can add a configure option if you feel strongly about installing one less text file.
(In reply to Carlos Garcia Campos from comment #1) > I don't think we need an option to change > the destination dir, though, just enabled/disable it would be enough, > leaving it enabled by default. So, feel free to push the patch if you add > the configure option. If I'm adding a configure option at all, I'd prefer it to be --with[out]-systemduserunitdir, because that seems to be what's conventional in other projects that are optionally user services. If we're going to pay the complexity cost of making it configurable at all, I might as well go all the way. Testing a patch now.
Created attachment 325497 [details] [review] Make the systemd user unit conditional I've implemented this by adding a --with[out]-systemduserunitdir configure option, because that seems to be what's conventional in projects where user units are configurable at all. If a user or distribution wants to disable these units, it's easier if they can pass --without-systemduserunitdir to everything, rather than checking whether it's spelled --disable-systemd-user-units in this particular project. Also, a binary enable/disable option wouldn't be noticeably less code. If the systemd user unit is disabled at configure time, the SystemdService line in the installed D-Bus service file is commented out. This addresses the corner-case situation where a user configures Evince --without-systemduserunitdir, but enables systemd activation for their session dbus-daemon (perhaps later). In that situation, we presumably want Evince to continue to use traditional activation, rather than trying to launch a nonexistent systemd unit.
(In reply to Simon McVittie from comment #10) > I've implemented this by adding a --with[out]-systemduserunitdir > configure option, because that seems to be what's conventional > in projects where user units are configurable at all. As I think I've mentioned on some of the similar bugs, this name wasn't my idea; it would have had at least one more "-" if it had been up to me :-)
Comment on attachment 325497 [details] [review] Make the systemd user unit conditional Ok.
Comment on attachment 325497 [details] [review] Make the systemd user unit conditional Committed as 3e76c45a for 3.21.0. I haven't cherry-picked it into the 3.20 branch, but you could do so if you want.