Subject: [bug#75270] [PATCH v4 1/3] services: greetd: Improve greeter configurations.

From: Maxim Cournoyer <[email-masked]>
Date: Wed, 29 Jan 2025 00:38:59 +0900

Hi,

muradm <[email-masked]> 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