Hi,

thank you for the review of this patch as well.  Comments below.

Maxim Cournoyer <maxim.cournoyer@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.