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 #20 received at 74508 <at> debbugs.gnu.org (full text, mbox):
From: Tomas Volf <~@wolfsden.cz> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> 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 16:37:32 +0100
[Message part 1 (text/plain, inline)]
Hi,
thank you for the review of this patch as well. Comments below.
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:
> 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.
I assume there might be historical and/or technical reasons for that,
but that is probably topic for guile-devel. :) I personally am bit torn,
while I can imagine situations when some symbol is by design left
unbound, I had no need for that yet.
> 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).
Yeah, I agree, will split it.
>
>> * 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?
Sure, why not? I can for example imagine having Alpine Linux installed
in chroot in /opt/alpine, and tty2 would login you into Alpine instead
of GuixSD.
Do I expect the `chroot' to be used often? No. But it *is* a valid
argument to mingetty, so I think it makes sense to add them all and
leave it to the user creativity whether it will be used or not.
>
>> (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
I do not think so. Notice the default value (#t). The issue is printed
by default. When you see a tty login prompt, it looks (by default) like
this:
--8<---------------cut here---------------start------------->8---
$ISSUE
$HOST login: $USER
Password: $PASSWORD
$MOTD
--8<---------------cut here---------------end--------------->8---
So in my QEMU test system the "logging screen" looks (roughly) like
this:
--8<---------------cut here---------------start------------->8---
GNU/Linux test-system x86_64 <--- /etc/issue
login: tester (automatic login)
GuixSD test VM. Welcome. <--- /gnu/store/...-motd
$
--8<---------------cut here---------------end--------------->8---
> (and doesn't currently seem to do anything, at least on berlin, for
> unknown reason).
I think this is expected no? I would assume that most people log via
SSH (only Ricardo has physical access?) and there is no `/etc/motd' file
created by `frontend-services'. Consider adding something like this to
the list of services returned by the `frontend-services' procedure:
--8<---------------cut here---------------start------------->8---
(extra-special-file "/etc/motd" motd)
--8<---------------cut here---------------end--------------->8---
That should make the message of the day visible even for SSH logins.
>
>> +@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?
Good catch, that was not intentional.
>
>> + 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.
Will polish the commit messages and send v2, thank you once more for the
review.
Have a nice day,
Tomas
--
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.
[signature.asc (application/pgp-signature, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.