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

["Ludovic Courtès" <ludo@gnu.org>]

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’.