GNOME Bugzilla – Bug 772370
Split up .desktop target into multiple daemons
Last modified: 2016-10-11 09:35:07 UTC
Using patches from bug 741688 we could: - build stand-alone daemons - add .desktop files for every stand-alone daemons - change the gnome-session definitions to add every one of those daemons[1] - then remove the stand-alone daemon [1]: do we need to re-add a dummy target for optional ones like the wacom tablet support?
Created attachment 336916 [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 336917 [details] [review] main: Remove GSettings override for plugin priority There's a default value already available in the plugin file itself.
Created attachment 336918 [details] [review] plugins: Remove "test" from the stand-alone program names
Created attachment 336919 [details] [review] common: Add verbose option to test-plugin.h
Created attachment 336920 [details] [review] plugins: Rename sources of all test applications
Created attachment 336921 [details] [review] sound: Rename the sound plugin for PulseAudio org.gnome.SettingsDaemon.Sound not simply org.gnome.SettingsDaemon
Created attachment 336923 [details] [review] plugins: Add desktop files for all the plugins So that the plugins can be started independently.
Created attachment 336924 [details] [review] sharing: Add missing generated file to the daemon sources
Created attachment 336925 [details] [review] plugins: Stop building helpers as plugins
Created attachment 336926 [details] [review] main: Remove gnome-settings-daemon binary
Created attachment 336927 [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 336928 [details] [review] common: Remove use of SCHEMA_NAME
Created attachment 336929 [details] [review] common: Report errors on startup in the helper skeleton
Created attachment 336930 [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 336931 [details] [review] build: Remove separate check for gnome-desktop
Created attachment 336932 [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 336933 [details] [review] plugins: Use LIBM macro instead of hard-coding it And fix gsd-color linkage by the same token.
Created attachment 336934 [details] [review] sharing: Install stand-alone gsd-sharing
Created attachment 336935 [details] [review] power: Install stand-alone gsd-power
This nearly works. It's missing some code to register with gnome-session, and using the dummy plugin to launch fake daemons for the non-Linux cases (to replace the Wacom plugin mentioned in the gnome-session session file).
Created attachment 336949 [details] [review] common: Make daemons register with gnome-session
Created attachment 336952 [details] [review] dummy: Add support for stub daemons So that gnome-session can keep a list of hard-coded gsd daemons for now, we'll need to replace some of the daemons with dummy ones. For each of the daemons that can be disabled, create a .desktop file anyway, and launch a gsd-dummy.
Ray, this is ready to go as far as I'm concerned. I tested it by building RPMs on my machine, and I can start sessions without a problem. Power management and media keys work. This requires the single line patch I made to gnome-session. The only thing blocking it right now is: how do we get gdm to only start the daemons it needs?
So maybe the plugins themselves should check the session class of the logind session and disable or degrade ? I think Allison always thought we should go that way over the whitelists anyway.
(In reply to Ray Strode [halfline] from comment #24) > So maybe the plugins themselves should check the session class of the logind > session and disable or degrade ? I think Allison always thought we should go > that way over the whitelists anyway. How would I get the session class? When we move to systemd, it might be better to avoid running the daemons altogether. In the meantime, they'll need to stay running as stubs.
Created attachment 336975 [details] [review] dummy: Add support for stub daemons So that gnome-session can keep a list of hard-coded gsd daemons for now, we'll need to replace some of the daemons with dummy ones. For each of the daemons that can be disabled, create a .desktop file anyway, and launch a gsd-dummy.
Created attachment 336977 [details] [review] common: Make daemons register with gnome-session
Created attachment 336978 [details] [review] common: Whitelist the plugins that should run under gdm As we don't have a way to restrict which plugins get enabled from the gdm side, do it ourselves.
This should do it. Please test and report. I'll commit all this tomorrow after testing if I don't get negative feedback.
Review of attachment 336978 [details] [review]: ::: plugins/common/daemon-skeleton-gtk.h @@ +55,3 @@ + guint i; + + session_mode = g_getenv ("GNOME_SHELL_SESSION_MODE"); This is probably fine, but I think it would be better to blacklist certain plugins if sd_session_get_class() returns anything but "user". One complication is knowing what session to pass, gnome-settings-daemon itself isn't running in a session anymore, so you can't use the current session. You have to iterate over the available sessions for the user and hope there's only one graphical one I guess. Of course this all sort of falls over with multi-seat. Probably what you're doing is the easiest answer for now.
Review of attachment 336916 [details] [review]: okay but this should probably go in after the gdm detection to black list certain ones
Review of attachment 336917 [details] [review]: ++
Review of attachment 336918 [details] [review]: maybe just add in the description "This is going to be the primary way they are started now".
Review of attachment 336919 [details] [review]: ++
Review of attachment 336920 [details] [review]: feels like this commit should come before the commit that adds verbose. or maybe the commit that adds --verbose should come before the commit that renamed the binaries.
Review of attachment 336921 [details] [review]: ok
Review of attachment 336923 [details] [review]: so the days desktop files are supposed to follow reverse dns notation.
Review of attachment 336924 [details] [review]: ok
Review of attachment 336925 [details] [review]: nice, and mitosis is complete
Review of attachment 336926 [details] [review]: ++
Review of attachment 336927 [details] [review]: maybe we should ultimately move them to gsettings-desktop-schemas or something so you can drop the file entirely.
Review of attachment 336928 [details] [review]: this should maybe be earlier in the set.
Review of attachment 336929 [details] [review]: ++
Review of attachment 336930 [details] [review]: nice cleanup. ::: configure.ac @@ +73,3 @@ AC_PATH_PROG([GLIB_MKENUMS],[glib-mkenums]) +AC_CHECK_LIBM interesting, didn't know about that. ::: plugins/common/daemon-skeleton-gtk.h @@ +19,3 @@ + +#ifndef PLUGIN_NAME +#error Include PLUGIN_CFLAGS in the test application s CFLAGS probably shouldn't have the word test here (but i guess it should get stripped from the file this was clone from before this file gets cloned) ::: plugins/common/daemon-skeleton.h @@ +44,2 @@ + context = g_option_context_new (NULL); + g_option_context_add_main_entries (context, *(&entries), GETTEXT_PACKAGE); *(&entries) is weird, just put entries (unless i'm missing something?)
Review of attachment 336931 [details] [review]: yay
Review of attachment 336932 [details] [review]: maybe squash this with the "stop building helpers as plugins" commit?
Review of attachment 336933 [details] [review]: makes sense to me, though could do all the LIBM stuff up front before doing any of the modularization work I guess.
Review of attachment 336934 [details] [review]: should definitely be before the commit that adds the desktop file.
Review of attachment 336935 [details] [review]: also should be before the commit that adds the desktop file
(In reply to Ray Strode [halfline] from comment #30) > Review of attachment 336978 [details] [review] [review]: > > ::: plugins/common/daemon-skeleton-gtk.h > @@ +55,3 @@ > + guint i; > + > + session_mode = g_getenv ("GNOME_SHELL_SESSION_MODE"); > > This is probably fine, but I think it would be better to blacklist certain > plugins if sd_session_get_class() returns anything but "user". > > One complication is knowing what session to pass, gnome-settings-daemon > itself isn't running in a session anymore, so you can't use the current > session. You have to iterate over the available sessions for the user and > hope there's only one graphical one I guess. Of course this all sort of > falls over with multi-seat. > > Probably what you're doing is the easiest answer for now. Isn't it easier to have a gdm.session file in gnome-session (or in gdm itself), and then let gdm start gnome-session with the right --session argument?
Review of attachment 336975 [details] [review]: ugh. if we just let these guys get dbus activated the first time they're queried by gnome-shell or whatever would things start malfunctioning?
Review of attachment 336977 [details] [review]: ok
(In reply to Ray Strode [halfline] from comment #30) > Review of attachment 336978 [details] [review] [review]: > > ::: plugins/common/daemon-skeleton-gtk.h > @@ +55,3 @@ > + guint i; > + > + session_mode = g_getenv ("GNOME_SHELL_SESSION_MODE"); > > This is probably fine, but I think it would be better to blacklist certain > plugins if sd_session_get_class() returns anything but "user". > > One complication is knowing what session to pass, gnome-settings-daemon > itself isn't running in a session anymore, so you can't use the current > session. You have to iterate over the available sessions for the user and > hope there's only one graphical one I guess. Of course this all sort of > falls over with multi-seat. > > Probably what you're doing is the easiest answer for now. I'll make a note that we should change this, but it would add a library dependency on libsystemd, which is probably not something we want to do for now. (In reply to Ray Strode [halfline] from comment #33) > Review of attachment 336918 [details] [review] [review]: > > maybe just add in the description "This is going to be the primary way they > are started now". Done. (In reply to Ray Strode [halfline] from comment #37) > Review of attachment 336923 [details] [review] [review]: > > so the days desktop files are supposed to follow reverse dns notation. Done. (In reply to Ray Strode [halfline] from comment #41) > Review of attachment 336927 [details] [review] [review]: > > maybe we should ultimately move them to gsettings-desktop-schemas or > something so you can drop the file entirely. Maybe, but most of those relate to settings that are in g-s-d itself. This is used by gnome-control-center and I think dconf/dconf-editor. Will do the rest tomorrow. (I'm at comment 44 right now).
(In reply to Ray Strode [halfline] from comment #44) > ::: plugins/common/daemon-skeleton-gtk.h > @@ +19,3 @@ > + > +#ifndef PLUGIN_NAME > +#error Include PLUGIN_CFLAGS in the test application s CFLAGS > > probably shouldn't have the word test here (but i guess it should get > stripped from the file this was clone from before this file gets cloned) Fixed. > ::: plugins/common/daemon-skeleton.h > @@ +44,2 @@ > + context = g_option_context_new (NULL); > + g_option_context_add_main_entries (context, *(&entries), > GETTEXT_PACKAGE); > > *(&entries) is weird, just put entries (unless i'm missing something?) Fixed (In reply to Ray Strode [halfline] from comment #48) > Review of attachment 336934 [details] [review] [review]: > > should definitely be before the commit that adds the desktop file. Done. (In reply to Ray Strode [halfline] from comment #49) > Review of attachment 336935 [details] [review] [review]: > > also should be before the commit that adds the desktop file Done.
I didn't do any commits reordering, as rebasing is really costly in time. I however made sure that distcheck worked after every single commit. Out to fix a couple of bugs now.
Newer versions merged and released in 3.23.2.