Created attachment 292944 [details] [review]
Don't use install an autostart file

Wasn't it the case at some point that some GNOME apps were using gnome-settings-daemon's DBus names (maybe Rhythmbox for media keys), which caused g-s-d to start outside of GNOME, prompting to remove DBus activation?

(In reply to comment #2)
> Wasn't it the case at some point that some GNOME apps were using
> gnome-settings-daemon's DBus names (maybe Rhythmbox for media keys), which
> caused g-s-d to start outside of GNOME, prompting to remove DBus activation?

Yep, see https://bugzilla.gnome.org/show_bug.cgi?id=673586

I think we can have our cake and eat it too.  If the session is activated with a org.gtk.Application.Activate() call then we can start it up in a full blown all things go sort of way.  If it's activated when using some api provided by a module, we can either exit with an error or load just that module.

I really don't want to do that, because what I would actually want to do in gnome-settings-daemon, is split each plugin into a systemd service, so we don't have I/O blockers, we can start the housekeeping plugin through timers instead of keeping it always there, etc.

well that fixes the problem too.

Patch set coming. A couple of things aren't particularly set though:
- how to we make the start of the daemons conditional on gnome-session being present? Should we exit as soon as possible if we're not in a GNOME session?
- how do we mark the .service files as to be auto-started (is that a post-install script/trigger? Do we need to print instructions after "make install")

Additional changes could include:
- make xrandr plugin exit when unused
- simplify plugin start (do we need separate init/finalize and start/stop)
- make housekeeping use a timer
- merge gsd-printer and gsd-print-notifications

Created attachment 336326 [details] [review]
main: Remove ability to start/stop individual plugins

All the plugins are now enabled by default, until we use another way to
start them individually.

Created attachment 336327 [details] [review]
main: Remove GSettings override for plugin priority

There's a default value already available in the plugin file itself.

Created attachment 336328 [details] [review]
dummy: Remove dummy plugin

We soon won't need an example plugin.

Created attachment 336329 [details] [review]
clipboard: Add test application

Created attachment 336330 [details] [review]
color: Add test application

Created attachment 336331 [details] [review]
plugins: Remove "test" from the stand-alone program names

Created attachment 336332 [details] [review]
common: Add verbose option to test-plugin.h

Created attachment 336333 [details] [review]
plugins: Rename sources of all test applications

Created attachment 336334 [details] [review]
sound: Rename the sound plugin for PulseAudio

org.gnome.SettingsDaemon.Sound not simply org.gnome.SettingsDaemon

Created attachment 336335 [details] [review]
housekeeping: Fix typo in DISTCLEANFILES

Created attachment 336336 [details] [review]
plugins: Add systemd user service files for all the plugins

Most of the plugins are either D-Bus services, or simple background
services. Special cases are:
- the media-keys and sound plugins waiting for PulseAudio to start
- the xrandr plugin is not started on boot, but auto-started when
  the D-Bus service is requested

Created attachment 336337 [details] [review]
sharing: Add missing generated file to the daemon sources

Created attachment 336338 [details] [review]
plugins: Stop building helpers as plugins

Created attachment 336339 [details] [review]
power: Add missing source to stand-alone program

Created attachment 336340 [details] [review]
main: Remove gnome-settings-daemon binary

Created attachment 336341 [details] [review]
data: Remove pkg-config files

We don't have an API anymore, plugins are now stand-alone daemons.

Created attachment 336342 [details] [review]
common: Remove use of SCHEMA_NAME

And a small list of the benefits:
- don't start plugins that aren't needed at a particular point (housekeeping, xrandr)
- avoid side-effects with some X11 APIs (see bug 760288)
- avoid crashes restarting all of gnome-settings-daemon (including loss of theme setting, with fonts and themes changing)
- avoid side-effects of hangs or synchronous I/O (like the recent CUPS problems)
- easier to debug crashes and hangs, better logging

Created attachment 336631 [details] [review]
build: Bump version to current development version

Created attachment 336632 [details] [review]
main: Remove ability to start/stop individual plugins

All the plugins are now enabled by default, until we use another way to
start them individually.

Created attachment 336633 [details] [review]
main: Remove GSettings override for plugin priority

There's a default value already available in the plugin file itself.

Created attachment 336634 [details] [review]
dummy: Remove dummy plugin

We soon won't need an example plugin.

Created attachment 336635 [details] [review]
clipboard: Add test application

Created attachment 336636 [details] [review]
color: Add test application

Created attachment 336637 [details] [review]
plugins: Remove "test" from the stand-alone program names

Created attachment 336638 [details] [review]
common: Add verbose option to test-plugin.h

Created attachment 336639 [details] [review]
plugins: Rename sources of all test applications

Created attachment 336640 [details] [review]
sound: Rename the sound plugin for PulseAudio

org.gnome.SettingsDaemon.Sound not simply org.gnome.SettingsDaemon

Created attachment 336641 [details] [review]
housekeeping: Fix typo in DISTCLEANFILES

Created attachment 336642 [details] [review]
plugins: Add systemd user service files for all the plugins

Most of the plugins are either D-Bus services, or simple background
services. Special cases are:
- the media-keys and sound plugins waiting for PulseAudio to start
- the xrandr plugin is not started on boot, but auto-started when
  the D-Bus service is requested

Created attachment 336643 [details] [review]
sharing: Add missing generated file to the daemon sources

Created attachment 336644 [details] [review]
plugins: Stop building helpers as plugins

Created attachment 336645 [details] [review]
power: Add missing source to stand-alone program

Created attachment 336646 [details] [review]
main: Remove gnome-settings-daemon binary

Created attachment 336647 [details] [review]
data: Adapt pkg-config file for API removal

We don't have an API anymore, plugins are now stand-alone daemons, but
we still need to install enumeration headers.

Created attachment 336648 [details] [review]
common: Remove use of SCHEMA_NAME

Created attachment 336649 [details] [review]
common: Report errors on startup in the helper skeleton

Created attachment 336650 [details] [review]
common: Remove obsolete key parsing test program

Created attachment 336651 [details] [review]
build: Add separate GTK+ skeleton

And simplify library requirements for each helper. This should allow
us to trim the number of dependencies for each one of the helpers. This
also moves the libnotify initialisation directly into the plugins that
need it.

Created attachment 336652 [details] [review]
build: Require lcms2 version 2.2 minimum

Created attachment 336653 [details] [review]
build: Remove separate check for gnome-desktop

Created attachment 336654 [details] [review]
common: Don't run when not under GNOME

Created attachment 336655 [details] [review]
main: Remove gnome-settings-plugin.h

The plugin system is not used any more, and wasn't dist'ed, which broke
building from tarballs.

Created attachment 336656 [details] [review]
power: Re-add "mock" support to gpm-common.c

It was removed when we removed support for building plugins, with the
stand-alone programs taking over.

This adds runtime mock support, rather than the compile-time mock
support we used before.

Created attachment 336657 [details] [review]
color: Fix self-test

The data now comes from hwdb via gnome-desktop, and the name of the
vendor changed.

Created attachment 336658 [details] [review]
plugins: Use LIBM macro instead of hard-coding it

And fix gsd-color linkage by the same token.

Created attachment 336659 [details] [review]
sharing: Install stand-alone gsd-sharing

Created attachment 336660 [details] [review]
power: Install stand-alone gsd-power

I ran into a bunch of problems trying to make use of this:
- we'll need to install a preset file, and run "systemctl --user preset ..." to set it up to run by default
- attachment 336654 [details] [review] doesn't work because we're started so early that 1) XDG_CURRENT_DESKTOP isn't set 2) XDG_SESSION_DESKTOP isn't set either even though pam_systemd is supposed to set it up when logging in!
- if we revert attachment 336654 [details] [review], we are being run too early, and X isn't available yet.
- I added a gnome-session.service file, but systemd cannot attach it to the running gnome-session (which isn't started via D-Bus)

Created attachment 336662 [details] [review]
data: Adapt pkg-config file for API removal

We don't have an API anymore, plugins are now stand-alone daemons, but
we still need to install enumeration headers.

So I want to dive in and try these soon and do some review, but let me just chime in on this first:

(In reply to Bastien Nocera from comment #56)
> - we'll need to install a preset file, and run "systemctl --user preset ..."
> to set it up to run by default
So Martin Pitt is actually doing some work in this area, and he added some new targets we should make use of.  See

https://github.com/systemd/systemd/pull/3678
https://github.com/systemd/systemd/pull/3848

Basically we need to put in each of the service files:

PartOf=graphical-session.target

Then the gnome-session.service should have:

BindsTo=graphical-session.target

Finally, we'll need to change how gnome-session is started.  Right now /usr/bin/gnome-session is a shell script that starts /usr/libexec/gnome-session-binary.  We'll need to change it to instead:

systemctl --user start graphical-session-pre.target
systemctl --user start gnome-session.target

> - attachment 336654 [details] [review] [review] doesn't work because we're started so
> early that 1) XDG_CURRENT_DESKTOP isn't set 2) XDG_SESSION_DESKTOP isn't set
> either even though pam_systemd is supposed to set it up when logging in!
Those things are actually set by GDM.  The problem is they are set in the session environment, not the systemd --user environment. But /usr/bin/gnome-session pushes them up to the systemd --user environment with its dbus-update-activation-environment call, so if the session is kicked off like I outlined above, they should be set.

> - if we revert attachment 336654 [details] [review] [review], we are being run too
> early, and X isn't available yet.
So I think the right way to associate the target with the gnome session would be to do:

systemctl --user add-wants gnome-session.target foo.service

(of course we'll have to have a gnome-classic-session.target too)

> - I added a gnome-session.service file, but systemd cannot attach it to the
> running gnome-session (which isn't started via D-Bus)
Yea, we just need to change how we're starting gnome-session.

Comment 58 along with https://www.youtube.com/watch?v=hq18daxTkLA tells me that we're not quite ready for using systemd to start gnome-settings-daemon bits.

But, what we can do, is commit only some of those patches, and start X different .desktop targets from gnome-session instead of a single one. I've filed bug 772370 about this.

(In reply to Ray Strode [halfline] from comment #58)
> So Martin Pitt is actually doing some work in this area, and he added some
> new targets we should make use of.  See
> 
> https://github.com/systemd/systemd/pull/3678
> https://github.com/systemd/systemd/pull/3848

I haven't announced this widely yet as currently the required startup script is still quite horrible, and this should be worked out first before we start upstreaming this at a large scale. After systemd.conf I actually cleaned that up quite a bit, and the only major outstanding thing is 
https://github.com/systemd/systemd/issues/3750 -- however, this has a relatively easy workaround (just add After=graphical-session-pre.target to every session service), and that workaround is now done in our run-systemd-session wrapper in Ubuntu.

> Basically we need to put in each of the service files:
> PartOf=graphical-session.target

Yes.

> Then the gnome-session.service should have:
> 
> BindsTo=graphical-session.target

No, I think gnome-session.target should have that -- i. e. the top-level thing (the GNOME session target) should define what its process leader is, services should IMHO not declare themselves to be that.


> We'll need to change it to instead:
> 
> systemctl --user start graphical-session-pre.target
> systemctl --user start gnome-session.target

Explicitly starting -pre.target is now not actually required any more (that was one of the workarounds). However, you need to add --wait so that the systemctl command waits until the .target is stopped again (the Exec= line is the process leader, if it exits the whole  session is stopped and gdm tosses you back to the login screen).

Rebased versions of those patches are now in.

Attachment 336631 [details] pushed as f95dc22 - build: Bump version to current development version
Attachment 336641 [details] pushed as 61f8c51 - housekeeping: Fix typo in DISTCLEANFILES
Attachment 336645 [details] pushed as a41174d - power: Add missing source to stand-alone program
Attachment 336650 [details] pushed as 8fdc5b1 - common: Remove obsolete key parsing test program
Attachment 336652 [details] pushed as c4ae361 - build: Require lcms2 version 2.2 minimum
Attachment 336657 [details] pushed as dd0dba2 - color: Fix self-test

Attachment 336635 [details] pushed as 32b8dd0 - clipboard: Add test application
Attachment 336636 [details] pushed as e29a17f - color: Add test application

(In reply to Martin Pitt from comment #60)
> > Then the gnome-session.service should have:
> > 
> > BindsTo=graphical-session.target
> 
> No, I think gnome-session.target should have that
oops, that's actually what I meant. I was transcribing from the systemd.special man page, and somehow introduced a thinko.

> Explicitly starting -pre.target is now not actually required any more (that
> was one of the workarounds). However, you need to add --wait so that the
> systemctl command waits until the .target is stopped again (the Exec= line
> is the process leader, if it exits the whole  session is stopped and gdm
> tosses you back to the login screen).
Oh, good.  So since this is turning into one command, we should probably augment the /usr/share/xsession file format to allow specifying a systemd target, then GDM can just do it itself, and we can ditch the wrapper script altogether.  We'll still need to do environment setup, but we can do that during graphical-session-pre instead of from the gnome-session wrapper. 

or maybe pam_systemd could set the $XDG_RUNTIME_DIR/systemd/user/default.target symlink itself based on the XDG_SESSION_DESKTOP that GDM passes to it?

Hi, n00b jhbuild user here.

These commits broke my build.  I manually applied the LIBM and dummy patches, then the build succeeded.

(In reply to Alan Jenkins from comment #64)
> Hi, n00b jhbuild user here.
> 
> These commits broke my build.  I manually applied the LIBM and dummy
> patches, then the build succeeded.

They worked fine here, so you might want to attach a build log. I'd probably have got yelled at if it broke the automated builds as well.

Making all in color
make[3]: Entering directory '/mnt/wd20ezrx-00dc0b0/JHBuild/3.22/checkout/gnome-settings-daemon/plugins/color'
  CC       libcolor_la-gcm-edid.lo
  CC       libcolor_la-gsd-color-calibrate.lo
  CC       libcolor_la-gsd-color-manager.lo
  CC       libcolor_la-gsd-color-profiles.lo
  CC       gsd_test_color-test-color.o
  CC       gsd_test_color-gcm-edid.o
  CC       libcolor_la-gsd-color-state.lo
  CC       libcolor_la-gsd-color-plugin.lo
  CC       gsd_test_color-gsd-color-calibrate.o
  CC       gsd_test_color-gsd-color-manager.o
  CC       gsd_test_color-gsd-color-profiles.o
  CC       gsd_test_color-gsd-color-state.o
LC_ALL=C /usr/bin/intltool-merge -d -u -c ../../po/.intltool-merge-cache ../../po color.gnome-settings-plugin.in color.gnome-settings-plugin
Found cached translation database
Merging translations into color.gnome-settings-plugin.
  CCLD     gsd-test-color
  CCLD     libcolor.la
/usr/bin/ld: gsd_test_color-gcm-edid.o: undefined reference to symbol 'pow@@GLIBC_2.2.5'
//lib/x86_64-linux-gnu/libm.so.6: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
Makefile:876: recipe for target 'gsd-test-color' failed
make[3]: *** [gsd-test-color] Error 1
make[3]: *** Waiting for unfinished jobs....
ar: `u' modifier ignored since `D' is the default (see `U')
make[3]: Leaving directory '/mnt/wd20ezrx-00dc0b0/JHBuild/3.22/checkout/gnome-settings-daemon/plugins/color'

Making all in dummy
make[3]: Entering directory '/mnt/wd20ezrx-00dc0b0/JHBuild/3.22/checkout/gnome-settings-daemon/plugins/dummy'
make[3]: *** No rule to make target 'test-dummy.c', needed by 'gsd_test_dummy-test-dummy.o'.  Stop.
make[3]: *** Waiting for unfinished jobs....
  CC       libdummy_la-gsd-dummy-manager.lo
  CC       libdummy_la-gsd-dummy-plugin.lo
make[3]: Leaving directory '/mnt/wd20ezrx-00dc0b0/JHBuild/3.22/checkout/gnome-settings-daemon/plugins/dummy'

Both of those are fixed in master:
commit 818a6e99a51a18c01b50a661a641bd009136922d
Author: Bastien Nocera <hadess@hadess.net>
Date:   Wed Oct 5 14:29:54 2016 +0200

    color: Fix gsd-test-color linkage
    
    https://bugzilla.gnome.org/show_bug.cgi?id=741688#c66

commit 70e90f817bff29eedd3d649460048404cfcf66f1
Author: Bastien Nocera <hadess@hadess.net>
Date:   Wed Oct 5 14:26:09 2016 +0200

    dummy: Add missing test-dummy.c source
    
    https://bugzilla.gnome.org/show_bug.cgi?id=741688#c67

lol, apparently I'm also a n00b at gnome bugzilla.  There was a conflict when I commented, & I chose a resolution which left me off the bug CC list.

To confirm, the above commits fix the problem I was having.  Thanks all.

Created attachment 337620 [details] [review]
dummy: fix build

/usr/bin/ld: gsd_dummy-main.o: undefined reference to symbol 'g_dbus_proxy_call'
/mnt/wd20ezrx-00dc0b0/JHBuild/3.24/install/lib/libgio-2.0.so.0: error adding symbols: DSO missing from command line

Also gsd-color segfaults...

Problem comes from gcm_session_get_output_edid in gsd-color-state.c. When edid is found in cache NULL is returned not edid. That leads to segfault when trying to access error->message.

(In reply to Alberts Muktupāvels from comment #71)
> Also gsd-color segfaults...
> 
> Problem comes from gcm_session_get_output_edid in gsd-color-state.c. When
> edid is found in cache NULL is returned not edid. That leads to segfault
> when trying to access error->message.

Please file new bugs. This bug is about using systemd to handle the g-s-d daemons.

Comment on attachment 337620 [details] [review]
dummy: fix build

Already fixed in bug 772900

Just doing some first research.

(In reply to Ray Strode [halfline] from comment #58)
> (In reply to Bastien Nocera from comment #56)
> > - attachment 336654 [details] [review] [review] [review] doesn't work because we're started so
> > early that 1) XDG_CURRENT_DESKTOP isn't set 2) XDG_SESSION_DESKTOP isn't set
> > either even though pam_systemd is supposed to set it up when logging in!
> Those things are actually set by GDM.  The problem is they are set in the
> session environment, not the systemd --user environment. But
> /usr/bin/gnome-session pushes them up to the systemd --user environment with
> its dbus-update-activation-environment call, so if the session is kicked off
> like I outlined above, they should be set.

Since 52a3c15a1d756c559402cc9505926a9b9d6cf3a7 this comment isn't true any more. /usr/bin/gnome-session-binary is now uploading the environment into systemd itself.

I think maybe split this out into a separate binary that's called early (Before=graphical-session-pre.target or from /usr/bin/gnome-session again)?

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/262.