Package: guix-patches;
Reported by: Tomas Volf <~@wolfsden.cz>
Date: Sun, 24 Nov 2024 14:29:02 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
Message #14 received at 74508 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Tomas Volf <~@wolfsden.cz> Cc: 74508 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [bug#74508] [PATCH 1/2] services: mingetty: Add additional configuration options. Date: Mon, 25 Nov 2024 10:45:24 +0900
Hello,
Tomas Volf <~@wolfsden.cz> writes:
> Not all aspects of mingetty were configurable, so this commit adds the
> additional configuration fields to support that.
>
> It also renames the accessors for all fields except `tty'. They were named
> just `mingetty-$FIELD', instead of `mingetty-configuration-$FIELD'. However
> the renaming *is* backwards compatible, since in the define-module's #:export
> argument the correct (`mingetty-configuration-$FIELD') were used already.
> Hence, until now, there was no way to access anything except the `tty' field.
Fun, good catch! I find it odd that Guile doesn't warn or error when
exporting a symbol that isn't defined; I was bit by that recently
myself. Personally I'd have effected that correction in a distinct
commit, to keep your new functionality diff to its bare minimum
(clearer/easier to review).
> * gnu/services/base.scm (<mingetty-configuration>): Add delay, print-issue,
> print-hostname, nice, chdir, chroot fields. Rename accessors for auto-login,
> login-program, login-pause?, clear-on-logout?.
Are all these expected to be functional on Guix, e.g., the 'chroot' option?
> (mingetty-shepherd-service): Use the new fields.
> (define-module)<#:export>: Export the new accessors.
> * doc/guix.texi (Base Services)<mingetty-configuration>: Document the
> additional field.
>
> Change-Id: I4557a82498805ade0b341feda9d33eccc305690f
> ---
> doc/guix.texi | 27 ++++++++++++++++++-
> gnu/services/base.scm | 62 ++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 79 insertions(+), 10 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 1c39628ffa..d689711e80 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -19285,7 +19285,32 @@ Base Services
> will have to press a key before the log-in shell is launched.
>
> @item @code{clear-on-logout?} (default: @code{#t})
> -When set to @code{#t}, the screen will be cleared after logout.
> +When set to @code{#t}, the screen will be cleared before showing the
> +login prompt. The field name is bit unfortunate, since it controls
> +clearing also before the initial login, not just after a logout.
> +
> +@item @code{delay} (default: @code{#f})
> +When set to a number, sleep that many seconds after startup.
> +
> +@item @code{print-issue} (default: @code{#t})
> +When set to @code{#t}, write out a new line and the content of
> +@file{/etc/issue}. Value of @code{'no-nl} can be used to suppress the
> +new line.
This sounds useful. Could it be used in place of a 'motd', such as in
our hydra/berlin.scm conf (from the Guix maintenance repo), which is
currently relying on PAM to work (and doesn't currently seem to do
anything, at least on berlin, for unknown reason).
> +@item @code{print-hostname} (default: @code{#t})
> +When set to @code{#t}, print the host name before the login prompt. The
> +host name is printed up to the first dot. Can be set to @code{'long} to
> +print the full host name.
> +
> +@item @code{nice} (default: @code{#f})
> +When set to a number, change the process priority using @code{nice}.
> +
> +@item @code{chdir} (default: @code{#f})
> +When set to a string, change into that directory before calling the
> +login program.
> +
> +@item @code{chroot} (default: @code{#f})
> +When set to a string, call @code{chroot} with that directory.
>
> @item @code{mingetty} (default: @var{mingetty})
> The Mingetty package to use.
> diff --git a/gnu/services/base.scm b/gnu/services/base.scm
> index 6473e1a91a..b3d3553091 100644
> --- a/gnu/services/base.scm
> +++ b/gnu/services/base.scm
> @@ -186,7 +186,12 @@ (define-module (gnu services base)
> mingetty-configuration-login-program
> mingetty-configuration-login-pause?
> mingetty-configuration-clear-on-logout?
> - mingetty-configuration-mingetty
Why is the above accessor removed?
> + mingetty-configuration-delay
> + mingetty-configuration-print-issue
> + mingetty-configuration-print-hostname
> + mingetty-configuration-nice
> + mingetty-configuration-chdir
> + mingetty-configuration-chroot
> mingetty-configuration?
> mingetty-service ; deprecated
> mingetty-service-type
> @@ -1242,20 +1247,33 @@ (define-record-type* <mingetty-configuration>
> mingetty-configuration?
> (mingetty mingetty-configuration-mingetty ;file-like
> (default mingetty))
> - (tty mingetty-configuration-tty) ;string
> - (auto-login mingetty-auto-login ;string | #f
> + (tty mingetty-configuration-tty) ;string
> + (auto-login mingetty-configuration-auto-login ;string | #f
> (default #f))
> - (login-program mingetty-login-program ;gexp
> + (login-program mingetty-configuration-login-program ;gexp
> (default #f))
> - (login-pause? mingetty-login-pause? ;Boolean
> + (login-pause? mingetty-configuration-login-pause? ;Boolean
> (default #f))
> - (clear-on-logout? mingetty-clear-on-logout? ;Boolean
> - (default #t)))
> + (clear-on-logout? mingetty-configuration-clear-on-logout? ;Boolean
> + (default #t))
> + (delay mingetty-configuration-delay ;Integer | #f
> + (default #f))
> + (print-issue mingetty-configuration-print-issue ;Boolean | Symbol
> + (default #t))
> + (print-hostname mingetty-configuration-print-hostname ;Boolean | Symbol
> + (default #t))
> + (nice mingetty-configuration-nice ;Integer | #f
> + (default #f))
> + (chdir mingetty-configuration-chdir ;String | #f
> + (default #f))
> + (chroot mingetty-configuration-chroot ;String | #f
> + (default #f)))
>
> (define (mingetty-shepherd-service config)
> (match-record config <mingetty-configuration>
> - (mingetty tty auto-login login-program
> - login-pause? clear-on-logout?)
> + (mingetty tty auto-login login-program
> + login-pause? clear-on-logout? delay
> + print-issue print-hostname nice chdir chroot)
> (list
> (shepherd-service
> (documentation "Run mingetty on an tty.")
> @@ -1286,6 +1304,32 @@ (define (mingetty-shepherd-service config)
> #~())
> #$@(if login-pause?
> #~("--loginpause")
> + #~())
> + #$@(if delay
> + #~("--delay" #$(number->string delay))
> + #~())
> + #$@(match print-issue
> + (#t
> + #~())
> + ('no-nl
> + #~("--nonewline"))
> + (#f
> + #~("--noissue")))
> + #$@(match print-hostname
> + (#t
> + #~())
> + ('long
> + #~("--long-hostname"))
> + (#f
> + #~("--nohostname")))
> + #$@(if nice
> + #~("--nice" #$(number->string nice))
> + #~())
> + #$@(if chdir
> + #~("--chdir" #$chdir)
> + #~())
> + #$@(if chroot
> + #~("--chroot" #$chroot)
> #~()))))
> (stop #~(make-kill-destructor))))))
Neatly crafted. Could you please send a v2 with my small comments above
addressed, such as splitting in two commits, and not removing the
'mingetty-configuration-mingetty' accessor? Other than that, LGTM.
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.