[bug#47979,v3] installer: Recommend 'ntp-service-type' for non-graphical systems.

On Fri, Apr 23, 2021 at 06:45:59PM -0400, Leo Famulari wrote:
> ice-9/eval.scm:159:9: In procedure struct-vtable: Wrong type argument in position 1 (expecting struct): (#<<system-service> name: "Network time service, to set the clock automatically" type: administration recommended?: #t snippet: ((service ntp-service-type)) packages: ()>)

With Leo Prikler's help on #guix [0], I got it to work :)

I've attached the working patch, as well as a patch to offer the GPM
"console mouse" service on non-graphical systems.

[0] http://logs.guix.gnu.org/guix/2021-04-24.log#012921
From 6f2f131551247aa20794007c43ae61b82f6e34d6 Mon Sep 17 00:00:00 2001
From: Leo Famulari <leo@famulari.name>
Date: Fri, 23 Apr 2021 14:50:15 -0400
Subject: [PATCH 1/2] installer: Recommend 'ntp-service-type' for non-graphical
 systems.

We had several bug reports with a root cause of "the clock was
incorrect" from users who used the installer to install a non-graphical
Guix System.

* gnu/installer/services.scm (%system-services): Add the ntp-service-type.
* gnu/installer/newt/services.scm (run-system-administration-cbt-page): New
variable.
(run-services-page): Use run-system-administration-cbt-page when not
installing a desktop.
---
 gnu/installer/newt/services.scm | 23 ++++++++++++++++++++++-
 gnu/installer/services.scm      |  8 ++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

On Fri, Apr 23, 2021 at 08:56:43PM -0400, Leo Famulari wrote:
> With Leo Prikler's help on #guix [0], I got it to work :)
> 
> I've attached the working patch, as well as a patch to offer the GPM
> "console mouse" service on non-graphical systems.
> 
> [0] http://logs.guix.gnu.org/guix/2021-04-24.log#012921

> From 6f2f131551247aa20794007c43ae61b82f6e34d6 Mon Sep 17 00:00:00 2001
> From: Leo Famulari <leo@famulari.name>
> Date: Fri, 23 Apr 2021 14:50:15 -0400
> Subject: [PATCH 1/2] installer: Recommend 'ntp-service-type' for non-graphical
>  systems.
> 
> We had several bug reports with a root cause of "the clock was
> incorrect" from users who used the installer to install a non-graphical
> Guix System.
> 
> * gnu/installer/services.scm (%system-services): Add the ntp-service-type.
> * gnu/installer/newt/services.scm (run-system-administration-cbt-page): New
> variable.
> (run-services-page): Use run-system-administration-cbt-page when not
> installing a desktop.

Ping!

I suppose it's too late for 1.3.0, due to the string freeze.

But I am asking for review anyways.

Hey Leo,

> I suppose it's too late for 1.3.0, due to the string freeze.

It looks rather nice, but I'll test it more thoroughly after the
release. Did you make sure that the installer tests are passing with:

make check-system TESTS="gui-installed-os gui-uefi-installed-os
gui-installed-os-encrypted gui-installed-desktop-os-encrypted"

Thanks,

Mathieu

On Sun, May 02, 2021 at 11:25:05AM +0200, Mathieu Othacehe wrote:
> 
> Hey Leo,
> 
> > I suppose it's too late for 1.3.0, due to the string freeze.
> 
> It looks rather nice, but I'll test it more thoroughly after the
> release. Did you make sure that the installer tests are passing with:
> 
> make check-system TESTS="gui-installed-os gui-uefi-installed-os
> gui-installed-os-encrypted gui-installed-desktop-os-encrypted"

I tested it on "bare metal", but I will run the systems tests, too.

Hey Leo,

> +(define (run-system-administration-cbt-page)
> +  "Run a page to select various system adminstration services."
> +  (let ((items (filter (lambda (service)
> +                         (eq? 'administration
> +                              (system-service-type service)))
> +                       %system-services)))
> +    (run-checkbox-tree-page
> +      #:title (G_ "Miscellaneous services")
> +      #:info-text (G_ "Select miscellaneous services to run on your system.")
> +      #:items items
> +      #:selection (map system-service-recommended? items)
> +      #:item->text (compose G_ system-service-name)
> +      #:checkbox-tree-height 5
> +      #:exit-button-callback-procedure
> +      (lambda ()
> +        (raise
> +          (condition
> +            (&installer-step-abort)))))))

Indentation is off here, 

> +     ;; Miscellaneous system administration services.
> +     (system-service
> +       (name (G_ "Network time service, to set the clock automatically"))
> +       (type 'administration)
> +       (recommended? #t)
> +       (snippet '((service ntp-service-type))))
> +

here, 

> +     (system-service
> +       (name (G_ "GPM mouse daemon, to use the mouse in the console"))
> +       (type 'administration)
> +       (snippet '((service gpm-service-type))))

and here. Those patches are breaking the installer tests because they
introduce a new dialog. You need to add something like:

--8<---------------cut here---------------start------------->8---
    ((checkbox-list (title "Miscellaneous services") (text _)
                    (items ,services))
     (filter choose-misc-service? services))
--8<---------------cut here---------------end--------------->8---

in the (gnu installer tests) module, to fix this error:

--8<---------------cut here---------------start------------->8---
conversation expecting pattern ((quote list-selection) ((quote title) "Partitioning method") ((quote multiple-choices?) #f) ((quote items) (not-encrypted encrypted _ ...)))
/gnu/store/h38i2hvaqy9bd0sbn9isdfzl7m56mngr-shepherd-marionette.scm:1:1718: ERROR:
  1. &pattern-not-matched:
      pattern: ((quote list-selection) ((quote title) "Partitioning method") ((quote multiple-choices?) #f) ((quote items) (not-encrypted encrypted _ ...)))
      sexp: (checkbox-list (title "Miscellaneous services") (text "Select miscellaneous services to run on your system.") (items ("Network time service, to set the clock automatically" "GPM mouse daemon, to use the mouse in the console")))
May 21 14:53:11 localhost instaBacktrace:
ller[180]: running form #<newt-form 79dc20> ("Miscellaneous services") with 1 clients 
           2 (primitive-load "/gnu/store/9vzfy688gawwn2p06nn75kiqqz3?")
In ice-9/eval.scm:
   191:35  1 (_ #f)
    619:8  0 (_ #(#<directory (guile-user) 7ffff3bb3f00> #<variabl?>))
--8<---------------cut here---------------end--------------->8---

Otherwise, it looks fine :)

Thanks,

Mathieu

Thanks for the review.

On Fri, May 21, 2021 at 02:58:48PM +0200, Mathieu Othacehe wrote:
> > +(define (run-system-administration-cbt-page)
> > +  "Run a page to select various system adminstration services."
> > +  (let ((items (filter (lambda (service)
> > +                         (eq? 'administration
> > +                              (system-service-type service)))
> > +                       %system-services)))
> > +    (run-checkbox-tree-page
> > +      #:title (G_ "Miscellaneous services")
> > +      #:info-text (G_ "Select miscellaneous services to run on your system.")
> > +      #:items items
> > +      #:selection (map system-service-recommended? items)
> > +      #:item->text (compose G_ system-service-name)
> > +      #:checkbox-tree-height 5
> > +      #:exit-button-callback-procedure
> > +      (lambda ()
> > +        (raise
> > +          (condition
> > +            (&installer-step-abort)))))))
> 
> Indentation is off here, 
> 
> > +     ;; Miscellaneous system administration services.
> > +     (system-service
> > +       (name (G_ "Network time service, to set the clock automatically"))
> > +       (type 'administration)
> > +       (recommended? #t)
> > +       (snippet '((service ntp-service-type))))
> > +
> 
> here, 
> 
> > +     (system-service
> > +       (name (G_ "GPM mouse daemon, to use the mouse in the console"))
> > +       (type 'administration)
> > +       (snippet '((service gpm-service-type))))
> 
> and here.

I don't see the problems. I copied from existing pieces of similar code
and I don't really have a sense of what's right and wrong for indenting
this kind of thing.

> Those patches are breaking the installer tests because they
> introduce a new dialog. You need to add something like:
> 
> --8<---------------cut here---------------start------------->8---
>     ((checkbox-list (title "Miscellaneous services") (text _)
>                     (items ,services))
>      (filter choose-misc-service? services))
> --8<---------------cut here---------------end--------------->8---
> 
> in the (gnu installer tests) module, to fix this error:

Okay, I'll work on that.

On Fri, May 21, 2021 at 02:58:48PM +0200, Mathieu Othacehe wrote:
> Those patches are breaking the installer tests because they
> introduce a new dialog. You need to add something like:
> 
> --8<---------------cut here---------------start------------->8---
>     ((checkbox-list (title "Miscellaneous services") (text _)
>                     (items ,services))
>      (filter choose-misc-service? services))
> --8<---------------cut here---------------end--------------->8---
> 
> in the (gnu installer tests) module, to fix this error:

How do I run this test? I can never figure out the names of the tests
used by `make check-system`...

On Fri, May 21, 2021 at 10:21:50AM -0400, Leo Famulari wrote:
> On Fri, May 21, 2021 at 02:58:48PM +0200, Mathieu Othacehe wrote:
> > Those patches are breaking the installer tests because they
> > introduce a new dialog. You need to add something like:
> > 
> > --8<---------------cut here---------------start------------->8---
> >     ((checkbox-list (title "Miscellaneous services") (text _)
> >                     (items ,services))
> >      (filter choose-misc-service? services))
> > --8<---------------cut here---------------end--------------->8---
> > 
> > in the (gnu installer tests) module, to fix this error:
> 
> How do I run this test? I can never figure out the names of the tests
> used by `make check-system`...

With informed use of grep I found it:

`make check-system TESTS=iso-image-installer`.

> How do I run this test? I can never figure out the names of the tests
> used by `make check-system`...

The installer tests are defined in the (gnu tests install) module. You
can then search for "%test-gui" for all the graphical installation
tests.

You can run all of them by typing:

--8<---------------cut here---------------start------------->8---
make check-system TESTS="gui-installed-os gui-uefi-installed-os
gui-installed-os-encrypted gui-installed-desktop-os-encrypted"
--8<---------------cut here---------------end--------------->8---

Fixing the "gui-installed-os" test should be enough to fix all of them
though.

Thanks,

Mathieu