Hi,
thank you for the review of this patch as well. Comments below.
Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
Toggle quote (17 lines)
> 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.
Toggle quote (4 lines)
> 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.
Toggle quote (8 lines)
>
>> * 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.
Toggle quote (37 lines)
>
>> (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:
Toggle snippet (8 lines)
$ISSUE
$HOST login: $USER
Password: $PASSWORD
$MOTD
So in my QEMU test system the "logging screen" looks (roughly) like
this:
Toggle snippet (9 lines)
GNU/Linux test-system x86_64 <--- /etc/issue
login: tester (automatic login)
GuixSD test VM. Welcome. <--- /gnu/store/...-motd
$
Toggle quote (3 lines)
> (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:
Toggle snippet (3 lines)
(extra-special-file "/etc/motd" motd)
That should make the message of the day visible even for SSH logins.
Toggle quote (30 lines)
>
>> +@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.
Toggle quote (89 lines)
>
>> + 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.
