nixos/xserver: fix X.org session script logging #29401
Conversation
rnhmjoj
force-pushed
the
logx
branch
2 times, most recently
from
2d81dcc to
12845be
Compare
rnhmjoj
changed the title
|
That would result in a breaking change. I think it has to be documented anywhere in the release notes, but I don't know what the workflow for breaking changes is. |
|
Whats the state? |
| @@ -72,7 +72,7 @@ let | |||
| sessionType="$1" | |||
| if [ "$sessionType" = default ]; then sessionType=""; fi | |||
|
|
|||
| ${optionalString (!cfg.displayManager.job.logsXsession && !cfg.displayManager.logToJournal) '' | |||
| ${optionalString cfg.displayManager.job.logsXsession '' | |||
There was a problem hiding this comment.
Hmm, if they are not mutually exclusive then in the case where both logsXsession and logToJournal are true wouldn't you need to both redirect stdout/stderr to the file and pass them on to the journal? Something like
exec > >(tee -i ~/.xsession-errors) 2> >(tee -i /dev/stderr >> ~/.xsession-errors)Disclamer: I don't know if the above is correct.
There was a problem hiding this comment.
@rycee First, thank you for the review. Yes, you are correct.
I tried exec > >(tee -i ~/.xsession-errors) 2> >(tee -i ~/.xsession-errors >&2) and seems to work.
The problem is that the script is executed multiple times and worse, if both logXsession and logToJournal are false the script is not executed at all.
I'm not sure how to deal with this.
rnhmjoj
force-pushed
the
logx
branch
2 times, most recently
from
4b94819 to
bd00832
Compare
rnhmjoj
changed the title
|
Ok, it took a little more effort than I initially expected. I have also renamed the options to be more consistent. |
|
Hey, whats the state? Looks good to me |
| @@ -306,7 +307,7 @@ in | |||
| description = "Additional environment variables needed by the display manager."; | |||
| }; | |||
|
|
|||
| logsXsession = mkOption { | |||
| logToFile = mkOption { | |||
There was a problem hiding this comment.
I think you'll have to add something like
mkRenamedOptionModule [ "services" "xserver" "displayManager" "job" "logsXsession" ] [ "services" "xserver" "displayManager" "job" "logsToFile" ];
to rename.nix. Otherwise backwards compatibility is broken.
Same with logToJournal below.
There was a problem hiding this comment.
That isn't a simple rename of logsXsession because of the logic bug.
That is something you need to take care of concering backwards compatibilty.
If logsXsession is true, logToFile needs to be false and vice versa to preserve the current behaviour.
There was a problem hiding this comment.
@rycee Thank you. It's done.
@Gerschtli The current behavior is wrong. It may be mentioned in the release changes but it makes no sense to keep it that way for compatibility.
GrahamcOfBorg
added
10.rebuild-darwin: 0
|
@rnhmjoj As you've noticed in the Nix chat room I've been trying this out. Basically everything seems to work for me if I do these changes: diff --git a/nixos/modules/services/x11/display-managers/default.nix b/nixos/modules/services/x11/display-managers/default.nix
index 15f7a1cca21..bfce4be6542 100644
--- a/nixos/modules/services/x11/display-managers/default.nix
+++ b/nixos/modules/services/x11/display-managers/default.nix
@@ -59,17 +59,6 @@ let
# Now it should be safe to assume that the script was called with the
# expected parameters.
- ${optionalString cfg.displayManager.job.logToJournal ''
- if [ -z "$_DID_SYSTEMD_CAT" ]; then
- export _DID_SYSTEMD_CAT=1
- exec &> >(tee >(${config.systemd.package}/bin/systemd-cat -t xsession))
- fi
- ''}
-
- ${optionalString cfg.displayManager.job.logToFile ''
- exec &> >(tee ~/.xsession-errors)
- ''}
-
. /etc/profile
cd "$HOME"
@@ -83,6 +72,17 @@ let
fi
''}
+ ${optionalString cfg.displayManager.job.logToJournal ''
+ if [ -z "$_DID_SYSTEMD_CAT" ]; then
+ export _DID_SYSTEMD_CAT=1
+ exec ${config.systemd.package}/bin/systemd-cat -t xsession "$0" "$sessionType"
+ fi
+ ''}
+
+ ${optionalString cfg.displayManager.job.logToFile ''
+ exec &> >(tee -a ~/.xsession-errors)
+ ''}
+
# Start PulseAudio if enabled.
${optionalString (config.hardware.pulseaudio.enable) ''
${optionalString (!config.hardware.pulseaudio.systemWide)The main difference is that the dbus-launch happens before the output redirection. Note, I also made two unrelated changes, one to remove the need for |
|
@rycee Ok, I added your changes. |
| ''} | ||
|
|
||
| ${optionalString cfg.displayManager.job.logToFile '' | ||
| exec &> >(tee -a ~/.xsession-errors) |
There was a problem hiding this comment.
Looking at the old code for logging to file it seems to me like it actually didn't append. Perhaps it is best to not have -a here? Yes, I realize I added it, sorry!
|
Looking good :-) I've now rebased this into master in 13bb5ff. Thanks for your hard work and patience! |
|
Thank you! |
Motivation for this change
Fix issue #29106
Things done