[bug#37868,v2,2/2] system: Add kernel-module-packages to operating-system.

* gnu/system.scm (<operating-system>): Add kernel-module-packages.
(operating-system-directory-base-entries): Use it.
* guix/profiles.scm (linux-module-database): New procedure.  Export it.
---
 gnu/system.scm    | 26 +++++++++++++---
 guix/profiles.scm | 76 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 97 insertions(+), 5 deletions(-)

Hello Danny,

Thanks for this patch! A few remarks below.

> +                               ; TODO: system, target.
> +                               #:system #f
> +                               #:target #f)))

We need to figure out what #:system and #:target to pass, otherwise it
will break system compilation with --system and --target. This is
somehow linked to this thread[1].

> +(define (linux-module-database manifest)

This is a rather long and over 80 columns procedure.  Maybe you should
consider split it into several functions.

> +                                (display "FAILED\n" (current-error-port))

This could be more specific and would need to be translated.

Mathieu

[1]: https://lists.gnu.org/archive/html/guix-patches/2019-12/msg00416.html

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> * gnu/system.scm (<operating-system>): Add kernel-module-packages.
> (operating-system-directory-base-entries): Use it.
> * guix/profiles.scm (linux-module-database): New procedure.  Export it.

[...]

> +  (kernel-module-packages operating-system-kernel-module-packages
> +                    (default '()))                ; list of packages

Technically we don’t require them to be <package> objects, right?  Any
lowerable object, like <computed-file>, would work?

Thus, I’d be tempted to remove “packages” from the field name.

‘kernel-modules’ is not a good idea because one may assume it’s a list
of .ko file names.  Perhaps ‘kernel-loadable-modules’?

Could you also add an entry in guix.texi?

> +    (mlet* %store-monad ((kernel -> (operating-system-kernel os))
> +                         (kernel-module-packages ->
> +                          (operating-system-kernel-module-packages os))

Please use short names for local variables; ‘modules’ is enough here.

> +                         (kernel*

s/kernel*/kernel/ since there’s no ambiguity.

> +                          (if (null? kernel-module-packages)
> +                              kernel
> +                              (profile-derivation
> +                               (packages->manifest
> +                                (cons kernel kernel-module-packages))
> +                               #:hooks (list linux-module-database)
> +                               #:locales? #f
> +                               #:allow-collisions? #f
> +                               #:relative-symlinks? #t
> +                               ; TODO: system, target.
> +                               #:system #f
> +                               #:target #f)))

You can omit the ‘null?’ case.  Also, rather leave out #:system and
#:target so that they take their default value.

> +(define (linux-module-database manifest)
> +  (mlet %store-monad
> +    ((kmod (manifest-lookup-package manifest "kmod")))

Please add a docstring and make the ‘mlet’ a single line.

> +    (define build
> +      (with-imported-modules '((guix build utils)
> +                               (guix build union))
> +       #~(begin
> +          (use-modules (srfi srfi-1)
> +                       (srfi srfi-26)
> +                       (guix build utils)
> +                       (guix build union)
> +                       (ice-9 ftw)
> +                       (ice-9 match))
> +          (let* ((inputs '#$(manifest-inputs manifest))
> +                 (input-files (lambda (path)
> +                                (filter file-exists?
> +                                  (map (cut string-append <> path) inputs))))

s/path/file/ + use of ‘filter-map’

> +                 (module-directories (input-files "/lib/modules"))
> +                 (System.maps (input-files "/System.map"))
> +                 (Module.symverss (input-files "/Module.symvers"))
                                   ^
Typo.
Also perhaps just ‘maps-file’ and ‘symvers-file’.

> +                 (directory-entries (lambda (directory-name)
> +                                       (filter (lambda (basename)
> +                                                 (not (string-prefix? "."
> +                                                                      basename)))
> +                                               (scandir directory-name))))
> +                 ;; Note: Should result in one entry.
> +                 (versions (append-map directory-entries module-directories)))
> +              ;; TODO: if len(module-directories) == 1: return module-directories[0]
> +              (mkdir-p (string-append #$output "/lib/modules"))
> +              ;; Iterate over each kernel version directory (usually one).
> +              (for-each (lambda (version)
> +                          (let ((destination-directory (string-append #$output "/lib/modules/" version)))
> +                            (when (not (file-exists? destination-directory)) ; unique
> +                              (union-build destination-directory
> +                                           ;; All directories with the same version as us.
> +                                           (filter-map (lambda (directory-name)
> +                                                         (if (member version
> +                                                                     (directory-entries directory-name))
> +                                                             (string-append directory-name "/" version)
> +                                                             #f))
> +                                                       module-directories)
> +                                           #:create-all-directories? #t)
> +                              ;; Delete generated files (they will be recreated shortly).
> +                              (for-each (lambda (basename)
> +                                          (when (string-prefix? "modules." basename)
> +                                            (false-if-file-not-found
> +                                              (delete-file
> +                                               (string-append
> +                                                destination-directory "/"
> +                                                basename)))))
> +                                        (directory-entries destination-directory))
> +                              (unless (zero? (system* (string-append #$kmod "/bin/depmod")
> +                                                      "-e" ; Report symbols that aren't supplied
> +                                                      "-w" ; Warn on duplicates
> +                                                      "-b" #$output ; destination-directory
> +                                                      "-F" (match System.maps
> +                                                            ((x) x))
> +                                                      "-E" (match Module.symverss
> +                                                            ((x) x))
> +                                                      version))
> +                                (display "FAILED\n" (current-error-port))
> +                                (exit #f)))))

Like Mathieu wrote, I think this should be shortened and/or decomposed
in several functions, with all the effects (‘for-each’, ‘when’,
‘unless’) happening at the very end.

I wonder what’s missing form (gnu build linux-modules) to do the
“depmod” bit entirely in Scheme.  It would be nice for several reasons,
one of which is that we wouldn’t need the ‘manifest-lookup-package’
hack, which in turn would allow us to keep this procedure out of (guix
profiles).

Thoughts?

Ludo’.

Hi Ludo,

On Sun, 23 Feb 2020 17:36:40 +0100
Ludovic Courtès <ludo@gnu.org> wrote:

> Could you also add an entry in guix.texi?

OK!

> > +                 (module-directories (input-files "/lib/modules"))
> > +                 (System.maps (input-files "/System.map"))
> > +                 (Module.symverss (input-files "/Module.symvers"))  
>                                    ^
> Typo.

Not really.  The file is called "Module.symvers" and those are multiple
"Module.symvers"s.  It's my naming convention for lists.  If we don't
want that then I can change it here.

> I wonder what’s missing form (gnu build linux-modules) to do the
> “depmod” bit entirely in Scheme.

Probably not a lot, but there are quite a few binary cache files (.bin)
generated by depmod and not by us--not sure whether we want to replicate
that complexity given the problems we had even with the initrd stuff.

I'm not sure whether those bin files are mandatory or optional to have.

>  It would be nice for several reasons,
> one of which is that we wouldn’t need the ‘manifest-lookup-package’
> hack, which in turn would allow us to keep this procedure out of (guix
> profiles).

Yeah.