GNOME Bugzilla – Bug 728449
Add a journald entry with MESSAGE_ID when Gnome Shell is started/loaded
Last modified: 2014-05-03 21:55:55 UTC
Smoketest on gnome-continuous now waits for gnome-session's message 'Entering running state' with specific MESSAGE_ID, but GNOME Shell doesn't write any log entry after it has loaded. This would solve 'black/garbled final screenshot on gnome-continuous' issue.
Created attachment 274715 [details] [review] Add a journald log entry with MESSAGE_ID if systemd is enabled
Review of attachment 274715 [details] [review]: ::: js/ui/main.js @@ +77,3 @@ let dynamicWorkspacesSchema = null; +let gnomeShellStartedMessageID = 'f3ea493c22934e26811cd62abe8e203a'; should be const STARTED_MESSAGE_ID = '...'; @@ +194,3 @@ + if (LoginManager.haveSystemd()) { + GSystem.log_structured_print('GNOME Shell started at ' + _startDate, + ["MESSAGE_ID=" + gnomeShellStartedMessageID]); i think there needs to be two different message ids, one for greeter and one for user session otherwise you'll pick up the message id from the login screen and take your screenshot too early.
Created attachment 274947 [details] [review] Make sure we send the message only when we're in user mode
Created attachment 274949 [details] [review] Make sure we send the message only when we're in user mode Right, using 'const' now
Created attachment 274954 [details] [review] Send a message after startup-complete signal As Rui has suggested, we should call it after startup-complete signal. We're waiting for 10 seconds after the message is found, so race condition will not hurt the screenshot
Review of attachment 274954 [details] [review]: wrong patch? It doesn't seem to log after startup-complete like you said it would
Created attachment 275128 [details] [review] Add a journald log entry with MESSAGE_ID if systemd is enabled (In reply to comment #6) > Review of attachment 274954 [details] [review]: > > wrong patch? It doesn't seem to log after startup-complete like you said it > would Oops, my fault, this one should work now
Created attachment 275151 [details] [review] Add a journald log entry with MESSAGE_ID if systemd is enabled Fixed a nasty typo
Review of attachment 275151 [details] [review]: ::: js/ui/main.js @@ +45,3 @@ const A11Y_SCHEMA = 'org.gnome.desktop.a11y.keyboard'; const STICKY_KEYS_ENABLE = 'stickykeys-enable'; +const gnomeShellStartedMessageID = 'f3ea493c22934e26811cd62abe8e203a'; should be all caps and separated by underscores @@ +219,3 @@ + GSystem.log_structured_print('GNOME Shell started at ' + _startDate, + ["MESSAGE_ID=" + gnomeShellStartedMessageID]); + } spacing is wrong on this brace, shouldn't use double quotes
Created attachment 275162 [details] [review] Add a journald log entry with MESSAGE_ID if systemd is enabled
Review of attachment 275162 [details] [review]: ::: configure.ac @@ +107,3 @@ polkit-agent-1 >= $POLKIT_MIN_VERSION + gcr-base-3 >= $GCR_MIN_VERSION + libgsystem" libgsystem is not a compile time dependency; we don't link to runtime deps ::: js/ui/main.js @@ +218,3 @@ + if (LoginManager.haveSystemd() && sessionMode.currentMode === 'user') { + GSystem.log_structured_print('GNOME Shell started at ' + _startDate, + ['MESSAGE_ID=' + GNOMESHELL_STARTED_MESSAGEID]); Nitpick: MESSAGE_ID vs. MESSAGEID - consistency would be nice Also: we log() the same message in _initializeUI() (which should end up in the journal as well), so probably a good idea to move that into an else block here.
Created attachment 275364 [details] [review] Add a journald log entry with MESSAGE_ID if systemd is enabled (In reply to comment #11) > libgsystem is not a compile time dependency; we don't link to runtime deps Okay, removed it in next patch (or should I put it somewhere else? Please advice) > Nitpick: MESSAGE_ID vs. MESSAGEID - consistency would be nice Fixed > Also: we log() the same message in _initializeUI() (which should end up in the > journal as well), so probably a good idea to move that into an else block here. Done, though GNOME Shell used to log two messages - one for gdm mode and one for user mode. Now nothing will be logged for gdm mode - I hope it will not break anybody's code
(In reply to comment #12) > Created an attachment (id=275364) [details] [review] > Add a journald log entry with MESSAGE_ID if systemd is enabled > > (In reply to comment #11) > > libgsystem is not a compile time dependency; we don't link to runtime deps > Okay, removed it in next patch (or should I put it somewhere else? Please > advice) We don't put that anywhere that would should be pulled in at runtime by distro deps. > > Nitpick: MESSAGE_ID vs. MESSAGEID - consistency would be nice > Fixed > > > Also: we log() the same message in _initializeUI() (which should end up in the > > journal as well), so probably a good idea to move that into an else block here. > Done, though GNOME Shell used to log two messages - one for gdm mode and one > for user mode. Now nothing will be logged for gdm mode - I hope it will not > break anybody's code I doubt any code uses that that's not really a documented API (or any API at all) so don't worry about that.
Review of attachment 275364 [details] [review]: ::: js/ui/main.js @@ +217,3 @@ + GSystem.log_structured_print('GNOME Shell started at ' + _startDate, + ['MESSAGE_ID=' + GNOMESHELL_STARTED_MESSAGE_ID]); + log('GNOME Shell started at ' + _startDate); Well he said "in an else block" you are still logging it twice.
(In reply to comment #14) > Well he said "in an else block" you are still logging it twice. Fixed and pushed as https://git.gnome.org/browse/gnome-shell/commit/?id=1c8036b8633b8de4f6d68127cb2fcbc64eef355b
(In reply to comment #12) > Done, though GNOME Shell used to log two messages - one for gdm mode and one > for user mode. No, those are two messages because two different processes log one message each. > Now nothing will be logged for gdm mode Not that I think it matters, but why? The gnome-shell process in the gdm session should still run this code on startup-complete as far as I can see.
libgsystem does not build or work on non-Linux system. Can we just copy necessary files (gsystem-log.[ch]) from libgsystem instead?
(In reply to comment #17) > libgsystem does not build or work on non-Linux system. Can we just copy > necessary files (gsystem-log.[ch]) from libgsystem instead? But that wouldn't work on non-Linux systems either, no? Given that GSystem is only needed when using systemd, we could just remove the unconditional import.
(In reply to comment #18) > (In reply to comment #17) > > libgsystem does not build or work on non-Linux system. Can we just copy > > necessary files (gsystem-log.[ch]) from libgsystem instead? > > But that wouldn't work on non-Linux systems either, no? Given that GSystem is > only needed when using systemd, we could just remove the unconditional import. What you means is that we should check whether GSystem exists when importing it?
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > libgsystem does not build or work on non-Linux system. Can we just copy > > > necessary files (gsystem-log.[ch]) from libgsystem instead? > > > > But that wouldn't work on non-Linux systems either, no? Given that GSystem is > > only needed when using systemd, we could just remove the unconditional import. > > What you means is that we should check whether GSystem exists when importing > it? Yes (check if systemd exists to be exact).
(In reply to comment #19) > What you means is that we should check whether GSystem exists when importing > it? Either that, or just don't import it before we reach the one place where we use it (in which case we are running under systemd).
(In reply to comment #21) > (In reply to comment #19) > > What you means is that we should check whether GSystem exists when importing > > it? > > Either that, or just don't import it before we reach the one place where we use > it (in which case we are running under systemd). That sounds good to me.
Comment on attachment 275364 [details] [review] Add a journald log entry with MESSAGE_ID if systemd is enabled Attachment 275364 [details] was pushed as 1c8036b8633b a while ago, marking as such.
Created attachment 275758 [details] [review] main: Don't depend on GSystem unconditionally We only need GSystem when running under systemd. As libgsystem itself has a hard dependency on systemd, only import it when actually needed to keep working on systems where systemd is not available.
Review of attachment 275758 [details] [review]: LG.
Attachment 275758 [details] pushed as 8c45e6f - main: Don't depend on GSystem unconditionally