Package: guix-patches;
Reported by: muradm <mail <at> muradm.net>
Date: Wed, 1 Jan 2025 22:49:02 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: muradm <mail <at> muradm.net> Cc: ludo <at> gnu.org, 75270 <at> debbugs.gnu.org, pelzflorian <at> pelzflorian.de Subject: [bug#75270] [PATCH v4 1/3] services: greetd: Improve greeter configurations. Date: Wed, 29 Jan 2025 00:38:59 +0900
Hi,
muradm <mail <at> muradm.net> writes:
> This improvement focuses on providing common user session scripts
> for use by multiple greeters. Now user session entry point is
> factored out into `<greetd-user-session>`, which can be reused
> as is with different greeters. By default it uses `bash` as
> first user process. Then user normally starts additional programs
> with `.profile` or `.bashrc`. Using `command`, `command-args` and
> `extra-env` one can specify something else, which could be
> `dbus-session` wrapped process, some desktop environment or else.
> While its above is possible, one is still encouraged to use
While *the* above
> `.bashrc`, `.profile` or similar.
> It also fixes incorrect use of `XDG_RUNTIME_DIR` for `wlgreet`.
> `wlgreet` requires compositor to run. We provide common sway based
requires *a* compositor
> greeter script, which can be shared by other graphical greeters.
>
> * gnu/services/base.scm (<greetd-user-session>): Common user session
> factored-out, for shared use by multiple greeters.
factored out (no hyphen)
> (<greetd-agreety-session>): Switch to common user session.
> (<greetd-wlgreet-color>): New record, `wlgreet` color holder.
> (<greetd-wlgreet-configuration>): Refactor `wlgreet` configuration
> (<greetd-wlgreet-sway-session>): Switch to common user session.
> * gnu/tests/desktop.scm (%minimal-services): Reflect configuration
> changes.
> * doc/guix.texi (Base Services): Document refactoring changes.
>
> Change-Id: I7d79f07e9aaac21673a7c84ee0f989a474d4749d
> ---
> doc/guix.texi | 115 +++++++++++------
> gnu/services/base.scm | 278 +++++++++++++++++++++++-------------------
> gnu/tests/desktop.scm | 14 ++-
> 3 files changed, 242 insertions(+), 165 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 9a53bdcd374..d74a8938a6d 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -20512,13 +20512,21 @@ Base Services
> (terminal-vt "2")
> (default-session-command
> (greetd-agreety-session
> - (extra-env '(("MY_VAR" . "1")))
> - (xdg-env? #f))))
> + (command
> + (greetd-user-session
> + (extra-env '(("MY_VAR" . "1")))
> + (xdg-env? #f))))))
> ;; we can use different shell instead of default bash
> (greetd-terminal-configuration
> (terminal-vt "3")
> (default-session-command
> - (greetd-agreety-session (command (file-append zsh "/bin/zsh")))))
> + (greetd-agreety-session
> + (command
> + (greetd-user-session
> + (command (file-append zsh "/bin/zsh"))
> + (command-args '())
> + (extra-env '(("MY_VAR" . "1")))
> + (xdg-env? #f))))))
You are changing the public API rather drastically. You need to come up
with a mechanism that will warn & automatically rewrite the legacy form
to the new form in the meantime, which I'm afraid won't be trivial given
the disruptive overhaul. There are examples among other services; it
typically involves defining a man-in-the-middle field sanitizer that
takes the old value, updates it to the new expected one while emitting a
deprecation warning.
> ;; we can use any other executable command as greeter
> (greetd-terminal-configuration
> (terminal-vt "4")
> @@ -20586,19 +20594,20 @@ Base Services
> The user to use for running the greeter.
>
> @item @code{default-session-command} (default: @code{(greetd-agreety-session)})
> -Can be either instance of @code{greetd-agreety-session} configuration or
> +Can be either @code{greetd-agreety-session}, @code{greetd-wlgreet-sway-session} or
> @code{gexp->script} like object to use as greeter.
>
> @end table
> @end deftp
>
> -@deftp {Data Type} greetd-agreety-session
> -Configuration record for the agreety greetd greeter.
> +@deftp {Data Type} greetd-user-session
> +Configuration record for the user session command. Greeters require user command
> +to be specified in some or another way. @code{greetd-user-session} provides a
I was wondering why 'greetd-agreety-session' was moved around; it makes
your change more confusing to review. Also, please use double space
between sentences, which is conventional in Guix.
> +common command for that. User should prefer stable shell command like @code{bash},
s/user/users/, s/command/commands/, or 'a stable shell command'. I'm
not sure what 'stable' means in this context.
> +which can start actual user terminal shell, window manager or desktop environment
*an* actual user terminal shell, [...]
> +with its own mechanism, which would be @code{~/.bashrc} in case of @code{bash}.
@file{~/.bashrc} (not @code)
also, "in case of the @command{bash} command."
>
> @table @asis
> -@item @code{agreety} (default: @code{greetd})
> -The package with @command{/bin/agreety} command.
> -
> @item @code{command} (default: @code{(file-append bash "/bin/bash")})
> Command to be started by @command{/bin/agreety} on successful login.
>
> @@ -20608,6 +20617,10 @@ Base Services
> @item @code{extra-env} (default: @code{'()})
> Extra environment variables to set on login.
>
> +@item @code{xdg-session-type} (default: @code{"tty"})
> +Specify the value of @code{XDG_SESSION_TYPE}. User environment may
*The* user environment [...]
> +adapt depending on its value (normaly by @code{.bashrc} or similar).
s/normaly/normally/
also, @file{~/.bashrc} to be consistent with above
> +
> @item @code{xdg-env?} (default: @code{#t})
> If true @code{XDG_RUNTIME_DIR} and @code{XDG_SESSION_TYPE} will be set
> before starting command. One should note that, @code{extra-env} variables
> @@ -20616,60 +20629,86 @@ Base Services
> @end table
> @end deftp
>
> -@deftp {Data Type} greetd-wlgreet-session
> -Generic configuration record for the wlgreet greetd greeter.
> +@deftp {Data Type} greetd-agreety-session
> +Configuration record for the agreety greetd greeter.
>
> @table @asis
>
> -@item @code{wlgreet} (default: @code{wlgreet})
> -The package with the @command{/bin/wlgreet} command.
> +@item @code{agreety} (default: @code{greetd})
> +The package with @command{/bin/agreety} command.
Note that @command is not intended for path, but for command *names*,
so, ideally we'd mention simply @command{agreety} command, though I see
this is a mistake from the past so could be done in prior commit.
>
> -@item @code{command} (default: @code{(file-append sway "/bin/sway")})
> -Command to be started by @command{/bin/wlgreet} on successful login.
> +@item @code{command} (default: @code{(greetd-user-session)})
> +Command to be started by @command{/bin/agreety} on successful login.
@command{agreety}
> +Normally should be a variation of @code{greetd-user-session}, but could
I'd reword to: Typically, a <greetd-user-session> record instance is
used, but file-like objects are also valid.
>
> -@item @code{command-args} (default: @code{'()})
> -Command arguments to pass to command.
> +@end table
> +@end deftp
> +
> +@deftp {Data Type} greetd-wlgreet-color
> +
> +@table @asis
> +@item @code{red}
> +Value of red.
> +
> +@item @code{green}
> +Value of green.
> +
> +@item @code{blue}
> +Value of blue.
> +
> +@item @code{opacity}
> +Value of opacity.
The descriptions are not too helpful. Are these supposed to be given as
hex value? Something else? A string? A number? Please expound.
> +@end table
> +@end deftp
> +
> +@deftp {Data Type} greetd-wlgreet-configuration
>
> +@table @asis
@table @code (and simplify below).
> @item @code{output-mode} (default: @code{"all"})
> Option to use for @code{outputMode} in the TOML configuration file.
>
> @item @code{scale} (default: @code{1})
> Option to use for @code{scale} in the TOML configuration file.
>
> -@item @code{background} (default: @code{'(0 0 0 0.9)})
> +@item @code{background} (default: @code{(greetd-wlgreet-color (red 0) (green 0) (blue 0) (opacity 0.9))})
You can break these overly long lines via '@' to escape the newline, I
think.
> RGBA list to use as the background colour of the login prompt.
>
> -@item @code{headline} (default: @code{'(1 1 1 1)})
> +@item @code{headline} (default: @code{(greetd-wlgreet-color (red 1) (green 1) (blue 1) (opacity 1))})
> RGBA list to use as the headline colour of the UI popup.
>
> -@item @code{prompt} (default: @code{'(1 1 1 1)})
> +@item @code{prompt} (default: @code{(greetd-wlgreet-color (red 1) (green 1) (blue 1) (opacity 1))})
> RGBA list to use as the prompt colour of the UI popup.
>
> -@item @code{prompt-error} (default: @code{'(1 1 1 1)})
> +@item @code{prompt-error} (default: @code{(greetd-wlgreet-color (red 1) (green 1) (blue 1) (opacity 1))})
> RGBA list to use as the error colour of the UI popup.
>
> -@item @code{border} (default: @code{'(1 1 1 1)})
> +@item @code{border} (default: @code{(greetd-wlgreet-color (red 1) (green 1) (blue 1) (opacity 1))})
> RGBA list to use as the border colour of the UI popup.
I'm not convinced the greetd-wlgreet-color API change is worth the
trouble (especially given you need to make it backward-compatible with a
custom record field accessor that checks what it gets and adapts it to
the new expected format, emitting a deprecation warning in the process).
> -@item @code{extra-env} (default: @code{'()})
> -Extra environment variables to set on login.
> -
> @end table
> @end deftp
>
> @deftp {Data Type} greetd-wlgreet-sway-session
> -Sway-specific configuration record for the wlgreet greetd greeter.
> +Configuration record for the in sway wlgreet greetd greeter.
>
> @table @asis
> -@item @code{wlgreet-session} (default: @code{(greetd-wlgreet-session)})
> -A @code{greetd-wlgreet-session} record for generic wlgreet configuration,
> -on top of the Sway-specific @code{greetd-wlgreet-sway-session}.
> -
> @item @code{sway} (default: @code{sway})
> -The package providing the @command{/bin/sway} command.
> +The package with @command{/bin/sway} and @command{/bin/swaymsg} commands.
>
> -@item @code{sway-configuration} (default: #f)
> -File-like object providing an additional Sway configuration file to be
> -prepended to the mandatory part of the configuration.
> +@item @code{sway-configuration} (default: @code{(plain-file "greetd-wlgreet-sway-config" "")})
> +Extra configuration for sway to be included before executing greeter.
Is that new default worth it/better? It'll create a not so useful empty
file in the store.
> +@item @code{wlgreet} (default: @code{wlgreet})
> +The package with the @command{/bin/wlgreet} command.
> +
> +@item @code{wlgreet-configuration} (default: @code{(greetd-wlgreet-configuration)})
> +Configuration of @code{wlgreet} represented by @code{greetd-wlgreet-configuration}.
> +
> +@item @code{command} (default: @code{(greetd-user-session)})
> +Command to be started by @command{/bin/agreety} on successful login.
> +Normally should be a variation of @code{greetd-user-session}, but could
> +be any @code{gexp->script} like object.
>
> @end table
Looks like all the tables can use '@table @code' instead of @asis, then
you can remove all the @code decorators for each items. This could be
done in a prior commit.
> @@ -20677,8 +20716,7 @@ Base Services
>
> @lisp
> (greetd-configuration
> - ;; We need to give the greeter user these permissions, otherwise
> - ;; Sway will crash on launch.
> + ;; Graphical greeter require additional group membership.
"*The* graphical greeter requireS additional group membership."
> (greeter-supplementary-groups (list "video" "input" "seat"))
> (terminals
> (list (greetd-terminal-configuration
> @@ -20687,7 +20725,10 @@ Base Services
> (default-session-command
> (greetd-wlgreet-sway-session
> (sway-configuration
> - (local-file "sway-greetd.conf"))))))))
> + (local-file "sway-greetd.conf")) ;; optional extra sway configuration
This line is a bit too long (80 columns max). Pleasae place the comment
on the line above, and punctuate it correctly as a line comment (note
also that margin comments are conventionally written like: ';no space
after semicolon', e.g. one ';' and no space between it and the comment).
[...]
> (define-record-type* <greetd-wlgreet-sway-session>
> greetd-wlgreet-sway-session make-greetd-wlgreet-sway-session
> greetd-wlgreet-sway-session?
> - (wlgreet-session greetd-wlgreet-sway-session-wlgreet-session ;<greetd-wlgreet-session>
> - (default (greetd-wlgreet-session)))
> - (sway greetd-wlgreet-sway-session-sway (default sway)) ;<package>
> - (sway-configuration greetd-wlgreet-sway-session-sway-configuration ;file-like
> - (default (plain-file "wlgreet-sway-config" ""))))
> -
> -(define (make-wlgreet-sway-configuration-file session)
> - (let* ((wlgreet-session (greetd-wlgreet-sway-session-wlgreet-session session))
> - (wlgreet-config (make-wlgreet-configuration-file wlgreet-session))
> - (wlgreet (file-append (greetd-wlgreet wlgreet-session) "/bin/wlgreet"))
> - (sway-config (greetd-wlgreet-sway-session-sway-configuration session))
> - (swaymsg (file-append (greetd-wlgreet-sway-session-sway session)
> - "/bin/swaymsg")))
> - (mixed-text-file "wlgreet-sway.conf"
> - "include " sway-config "\n"
> - "xwayland disable\n"
> - "exec \"" wlgreet " --config " wlgreet-config "; "
> - swaymsg " exit\"\n")))
> + (sway greetd-wlgreet-sway-session-sway (default sway))
> + (sway-configuration greetd-wlgreet-sway-session-sway-configuration
> + (default (plain-file "greetd-wlgreet-sway-config" "")))
> + (wlgreet greetd-wlgreet-sway-session-wlgreet (default wlgreet))
> + (wlgreet-configuration greetd-wlgreet-sway-session-wlgreet-configuration
> + (default (greetd-wlgreet-configuration)))
> + (command greetd-wlgreet-sway-session-command (default (greetd-user-session))))
> +
> +(define make-greetd-wlgreet-sway-session-sway-config
> + (match-lambda
> + (($ <greetd-wlgreet-sway-session> sway sway-config wlgreet wlgreet-config command)
> + (let ((wlgreet-bin (file-append wlgreet "/bin/wlgreet"))
> + (wlgreet-config-file
> + (make-greetd-wlgreet-config command wlgreet-config))
> + (swaymsg-bin (file-append sway "/bin/swaymsg")))
> + (mixed-text-file
> + "wlgreet-sway-config"
> + "include " sway-config "\n"
> + "xwayland disable\n"
> + "exec \"" wlgreet-bin " --config " wlgreet-config-file "; " swaymsg-bin " exit\"\n")))))
Please break long lines so they fit in 80 chars.
>
> (define-gexp-compiler (greetd-wlgreet-sway-session-compiler
> (session <greetd-wlgreet-sway-session>)
> system target)
> - (let ((sway (file-append (greetd-wlgreet-sway-session-sway session)
> - "/bin/sway"))
> - (config (make-wlgreet-sway-configuration-file session)))
> + (match-record session <greetd-wlgreet-sway-session>
> + (sway)
> (lower-object
> - (program-file "wlgreet-sway-session-command"
> - #~(let* ((log-file (open-output-file
> - (string-append "/tmp/sway-greeter."
> - (number->string (getpid))
> - ".log")))
> - (username (getenv "USER"))
> - (useruid (number->string (passwd:uid (getpwuid username)))))
> - ;; redirect stdout/err to log-file
> - (dup2 (fileno log-file) 1)
> - (dup2 1 2)
> - (sleep 1) ;give seatd/logind some time to start up
That's a suspicious line which already existed. It looks fragile. Is
it really necessary?
> - (setenv "XDG_RUNTIME_DIR" (string-append "/run/user/" useruid))
> - (execl #$sway #$sway "-d" "-c" #$config))))))
> + (make-greetd-sway-greeter-command
> + sway
> + (make-greetd-wlgreet-sway-session-sway-config session)))))
>
> (define-record-type* <greetd-terminal-configuration>
> greetd-terminal-configuration make-greetd-terminal-configuration
> @@ -3625,7 +3648,8 @@ (define (greetd-accounts config)
> (name "greeter")
> (group "greeter")
> (supplementary-groups (greetd-greeter-supplementary-groups config))
> - (system? #t))))
> + (system? #t)
> + (create-home-directory? #f))))
>
> (define (make-greetd-pam-mount-conf-file config)
> (computed-file
> @@ -3675,6 +3699,9 @@ (define (greetd-pam-service config)
> (list optional-pam-mount))))
> pam))))))
>
> +(define (greetd-run-user-activation config)
> + #~(let ((d "/run/user")) (mkdir d #o755) (chmod d #o755)))
> +
> (define (greetd-shepherd-services config)
> (map
> (lambda (tc)
> @@ -3706,6 +3733,7 @@ (define greetd-service-type
> (list
> (service-extension account-service-type greetd-accounts)
> (service-extension file-system-service-type (const %greetd-file-systems))
> + (service-extension activation-service-type greetd-run-user-activation)
> (service-extension etc-service-type greetd-etc-service)
> (service-extension pam-root-service-type greetd-pam-service)
> (service-extension shepherd-root-service-type greetd-shepherd-services)))
> diff --git a/gnu/tests/desktop.scm b/gnu/tests/desktop.scm
> index 1c32076ccb2..923c71a7f89 100644
> --- a/gnu/tests/desktop.scm
> +++ b/gnu/tests/desktop.scm
> @@ -141,13 +141,21 @@ (define %minimal-services
> (terminal-vt "2")
> (default-session-command
> (greetd-agreety-session
> - (extra-env '(("MY_VAR" . "1")))
> - (xdg-env? #f))))
> + (command
> + (greetd-user-session
> + (extra-env '(("MY_VAR" . "1")))
> + (xdg-env? #f))))))
> ;; we can use different shell instead of default bash
> (greetd-terminal-configuration
> (terminal-vt "3")
> (default-session-command
> - (greetd-agreety-session (command (file-append zsh "/bin/zsh")))))
> + (greetd-agreety-session
> + (command
> + (greetd-user-session
> + (command (file-append zsh "/bin/zsh"))
> + (command-args '())
> + (extra-env '(("MY_VAR" . "1")))
> + (xdg-env? #f))))))
> ;; we can use any other executable command as greeter
> (greetd-terminal-configuration
> (terminal-vt "4")
I lost focus a bit in this file given the large hunks of changes, but it
looks reasonable. The main problem I see is the breakage of the old API
in the current form. I've given hints at how this can be overcome.
Could you attempt to do so in a v5?
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.