GNOME Bugzilla – Bug 756324
RFC: rework session initialization order to fix environment variable problems
Last modified: 2015-11-13 00:18:05 UTC
See the discussion in bug 738205. I opened a new bug to avoid cluttering the other with the patches, but the discussion should still happen there, and we can move to this one once we reach an agreement and we start talking code.
Created attachment 312993 [details] [review] Support PreDisplayServer phase To run gnome-keyring before gnome-shell is set up, so that gnome-shell can pick up it's own variables.
Created attachment 312994 [details] [review] Read and apply LC_* environment variables from GSettings Before anything else runs
Created attachment 312995 [details] [review] Make sure we use the local gio vfs in gnome-settings So that we don't accidentally start gvfsd too early
Created attachment 312996 [details] [review] manager: forbid Setenv in Initialization phase With LC_* env vars moved to gnome-session, and gnome-keyring moved to PreDisplayServer, we don't need to Setenv from random apps in the Initialization phase, and doing so can lead to subtle bugs and races.
Created attachment 312997 [details] [review] daemon: move autostart to PreDisplayServer phase Which, as the name implies, runs before the DisplayServer, ie, before gnome-shell-wayland, and makes sure gnome-shell and its descendants see the right environment variable.
Created attachment 312998 [details] [review] daemon: reduce the amount of environment to send to the master process Let's keep the environment vars that the daemon can depend upon to a minimum, so that we reduce the dependency to the initialization order and we avoid inverse dependencies with gnome-shell/XWayland. The daemon was already ignoring most of these.
Created attachment 312999 [details] [review] Remove egg-spawn In looking for places where gnome-keyring-daemon potentially spawns subprocesses, I found this piece of unused code. Remove it.
Attachment 312994 [details] pushed as 39f146e - Read and apply LC_* environment variables from GSettings Attachment 312995 [details] pushed as 6b8d94a - Make sure we use the local gio vfs in gnome-settings
There are a number of typos in the environment variable names in the patch that was pushed...
sigh will fix it
Created attachment 313319 [details] [review] main: fix typos I introduced I reworked some of Giovanni's code and introduced a few typos in commit 6b8d94ae13b2bec1db1bcff4ab4cc67bf63fda9f
Comment on attachment 313319 [details] [review] main: fix typos I introduced Attachment 313319 [details] pushed as c9eec29 - main: fix typos I introduced
(In reply to Giovanni Campagna from comment #2) > Created attachment 312994 [details] [review] [review] > Read and apply LC_* environment variables from GSettings > > Before anything else runs This needs to run before any threads are started: https://git.gnome.org/browse/gnome-settings-daemon/commit/?id=5cb44f6387de341ef254d33f76ec26e164a5ee0d
(In reply to Bastien Nocera from comment #13) > (In reply to Giovanni Campagna from comment #2) > > Created attachment 312994 [details] [review] [review] [review] > > Read and apply LC_* environment variables from GSettings > > > > Before anything else runs > > This needs to run before any threads are started: > https://git.gnome.org/browse/gnome-settings-daemon/commit/ > ?id=5cb44f6387de341ef254d33f76ec26e164a5ee0d Which is what the patch does - it uses a helper script like g-s-d
(In reply to Giovanni Campagna from comment #14) > (In reply to Bastien Nocera from comment #13) > > (In reply to Giovanni Campagna from comment #2) > > > Created attachment 312994 [details] [review] [review] [review] [review] > > > Read and apply LC_* environment variables from GSettings > > > > > > Before anything else runs > > > > This needs to run before any threads are started: > > https://git.gnome.org/browse/gnome-settings-daemon/commit/ > > ?id=5cb44f6387de341ef254d33f76ec26e164a5ee0d > > Which is what the patch does - it uses a helper script like g-s-d It's still called in gsm_util_setenv() though.
Created attachment 314846 [details] [review] gsm-util: avoid overwriting a env variable with itself This avoids a potentially thread-safety issue in our use of gsm_util_setenv() to push locale variables to DBus. Oh I see what you mean now. This should address your concern.
Attachment 312993 [details] pushed as 1526e96 - Support PreDisplayServer phase Attachment 314846 [details] pushed as 771dc6c - gsm-util: avoid overwriting a env variable with itself (not entirely sure about attachment 314846 [details] [review] but pushing for now)
Comment on attachment 312996 [details] [review] manager: forbid Setenv in Initialization phase I think, for now, i'd like to keep SetEnv working in Initialization
moving to keyring for the remaining patches
Review of attachment 312998 [details] [review]: Looks good. Please commit to master only. gnome-keyring used to spawn processes for prompting. It now does this via DBus activation, so you're correct that display server variables should be irrelevant.
Review of attachment 312999 [details] [review]: This needs just one more tweak: make[2]: *** No rule to make target '../../egg/egg-spawn.c', needed by 'gnome-keyring.pot'. Stop. After a fix for that, the patch looks good.
Review of attachment 312997 [details] [review]: Looks good. Since this part of the desktop files are GNOME specific, we can assume that there are no compatibility issues with other desktops that use gnome-keyring here.
Attachment 312997 [details] pushed as 7a475c6 - daemon: move autostart to PreDisplayServer phase Attachment 312998 [details] pushed as 607855d - daemon: reduce the amount of environment to send to the master process Attachment 312999 [details] pushed as eb16c03 - Remove egg-spawn