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

[Mathieu Othacehe <m.othacehe@gmail.com>]

Hello Danny,

> Will Guix do the derivation (especially the invocation of depmod) for the
> intended system and target?

Yes, "profile-derivation" should use the current system or target if the
#:system and #:target arguments are #f.

> Apparently, module-init-tools are supposed to be cross-platform anyway and work
> when invoking depmod for files of an other architecture than the architecture
> depmod is invoked on (and was compiled for).  So maybe we can also just ignore
> the entire system/target propagation in this case.

In that case, you should use #+kmod instead of #$kmod. This way, when
cross-compiling, the native kmod would be used.

> Then I tried
>
>   ./pre-inst-env guix build --target=xxx -m etc/system-tests.scm
>
> and that seems to ignore target entirely.

I'm not sure this has ever been tested. Support of cross-compilation for
Guix System is still wip, even if since a few days, core-updates is in a
good shape.

Anyway, if you're willing to wait a few days, I can test your patch does
not break system cross-compilation on core-updates.

Regarding --system, producing disk-images is currently broken on all
branches[1], so it will be harder to test it for now.

Also, here are a few remarks about your patch.

+(define (depmod! kmod version output)
+  "Given an (existing) OUTPUT directory, invoke KMOD's depmod on it for
+kernel version VERSION."

"OUTPUT" is maybe not the best naming as you read multiple "input" files
from it. Maybe just "DIRECTORY"?

+  (let ((destination-directory (string-append output "/lib/modules/" version))
+        ;; Note: "System.map" is an input file.
+        (maps-file (string-append output "/System.map"))
+        ;; Note: "Module.symvers" is an input file.
+        (symvers-file (string-append output "/Module.symvers")))
+    (for-each (lambda (basename)
+                (when (and (string-prefix? "modules." basename)
+                           ;; Note: "modules.builtin" is an input file.
+                           (not (string=? "modules.builtin" basename))
+                           ;; Note: "modules.order" is an input file.
+                           (not (string=? "modules.order" basename)))
+                  (delete-file (string-append destination-directory "/"
+                                              basename))))

You can maybe add a comment explaining what's the point of this
operation.

+              (scandir destination-directory))
+    (invoke (string-append kmod "/bin/depmod")
+            "-e" ; Report symbols that aren't supplied
+            ;"-w" ; Warn on duplicates
+            "-b" output
+            "-F" maps-file
+            "-E" symvers-file

The man page of depmod says that '-F' and '-E' options are mutually
exclusive.

+               (let* ((versions (filter (lambda (name)
+                                          (not (string-prefix? "." name)))
+                                        (scandir moddir)))
+                      (version (match versions
+                                ((x) x))))

If versions only contains one element, then you can use find instead of
filtering and matching.

+                          ;; TODO: system, target.
+                          (profile-derivation
+                           (packages->manifest
+                            (cons kernel modules))
+                           #:hooks (list linux-module-database)
+                           #:locales? #f
+                           #:allow-collisions? #f
+                           #:relative-symlinks? #t))
+                         (initrd -> (operating-system-initrd-file os))
+                         (params    (operating-system-boot-parameters-file os)))

As stated above, I think you are fine removing the TODO.

+(define (input-files inputs path)
+  "Given a list of directories INPUTS, return all entries with PATH in it."
+  ;; TODO: Use filter-map.
+  #~(begin
+      (use-modules (srfi srfi-1))
+      (filter file-exists?
+        (map (lambda (x)
+               (string-append x #$path))
+             '#$inputs))))
+

This TODO can be resolved I think :)

+(define (linux-module-database manifest)
+  "Return a derivation that unions all the kernel modules in the manifest
+and creates the dependency graph for all these kernel modules."
+  (mlet %store-monad ((kmod (manifest-lookup-package manifest "kmod")))
+    (define build
+      (with-imported-modules (source-module-closure '((guix build utils) (gnu build linux-modules)))
+        #~(begin
+            (use-modules (ice-9 ftw))
+            (use-modules (ice-9 match))
+            (use-modules (srfi srfi-1)) ; append-map
+            (use-modules (guix build utils)) ; mkdir-p
+            (use-modules (gnu build linux-modules)) ; make-linux-module-directory
+            (let* ((inputs '#$(manifest-inputs manifest))
+                   (module-directories #$(input-files (manifest-inputs manifest) "/lib/modules"))
+                   (directory-entries
+                    (lambda (directory-name)
+                      (scandir directory-name (lambda (basename)
+                                                (not (string-prefix? "." basename))))))
+                   ;; Note: Should usually result in one entry.
+                   (versions (delete-duplicates
+                              (append-map directory-entries
+                                          module-directories))))

This part is over the column limit.

+                (match versions
+                 ((version)
+                  (make-linux-module-directory #$kmod inputs version #$output)))

If depmod output is system agnostic, then we should use
#+kmod. If that's not the case, this will be an issue as running #$kmod
won't work when cross-compiling.

Thanks,

Mathieu