Package: guix-patches;
Reported by: Danny Milosavljevic <dannym <at> scratchpost.org>
Date: Tue, 22 Oct 2019 15:23:01 UTC
Severity: normal
Tags: patch
Done: Danny Milosavljevic <dannym <at> scratchpost.org>
Bug is archived. No further changes may be made.
Message #107 received at 37868 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Danny Milosavljevic <dannym <at> scratchpost.org> Cc: Mark H Weaver <mhw <at> netris.org>, 37868 <at> debbugs.gnu.org Subject: Re: [PATCH v6] system: Add kernel-module-packages to operating-system. Date: Sun, 15 Mar 2020 22:00:04 +0100
Hi!
Danny Milosavljevic <dannym <at> scratchpost.org> skribis:
> * gnu/system.scm (<operating-system>): Add kernel-module-packages.
> (operating-system-directory-base-entries): Use it.
> * doc/guix.texi (operating-system Reference): Document KERNEL-LOADABLE-MODULES.
> * gnu/build/linux-modules.scm (depmod!): New procedure.
> (make-linux-module-directory): New procedure. Export it.
> * guix/profiles.scm (linux-module-database): New procedure. Export it.
> * gnu/tests/linux-modules.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
> * gnu/packages/linux.scm (make-linux-libre*)[arguments]<#:phases>[install]:
> Disable depmod. Remove "build" and "source" symlinks.
[...]
> +@item @code{kernel-loadable-modules} (default: '())
> +A list of objects (usually packages) to collect loadable kernel modules from.
Perhaps you can add an example.
> +(define (input-files inputs file)
> + "Given a list of directories INPUTS, return all entries with FILE in it."
> + ;; TODO: Use filter-map.
> + (filter file-exists?
> + (map (lambda (x)
> + (string-append x file))
> + inputs)))
“Input” in Guix is usually used to describe association lists. To avoid
confusion, I propose:
(define (existing-files directories base)
"Return the absolute file name of every file named BASE under the
DIRECTORIES."
(filter-map (lambda (directory)
(let ((file (string-append directory "/" base)))
(and (file-exists? file) file)))
inputs)
> +(define (depmod! kmod inputs version destination-directory output)
There’s shouldn’t be a bang, by convention. Also please add a docstring.
> + (let ((maps-files (input-files inputs "/System.map"))
> + (symvers-files (input-files inputs "/Module.symvers")))
> + (for-each (lambda (basename)
> + (when (and (string-prefix? "modules." basename)
> + (not (string=? "modules.builtin" basename))
> + (not (string=? "modules.order" basename)))
> + (delete-file (string-append destination-directory "/"
> + basename))))
> + (scandir destination-directory))
> + (invoke (string-append kmod "/bin/depmod")
Generally, for this kind of utility function, we assume that the tool is
in $PATH, which allows us to avoid carrying its file name throughout the
API. I’d suggest doing the same here.
> +(define (make-linux-module-directory kmod inputs version output)
> + "Ensures that the directory OUTPUT...VERSION can be used by the Linux
> +kernel to load modules via KMOD. The modules to put into
> +OUTPUT are taken from INPUTS."
Perhaps be more specific as to the fact that it’s creating ‘System.maps’
etc. databases?
> (let ((locale (operating-system-locale-directory os)))
> - (mlet %store-monad ((kernel -> (operating-system-kernel os))
> - (initrd -> (operating-system-initrd-file os))
> - (params (operating-system-boot-parameters-file os)))
> + (mlet* %store-monad ((kernel -> (operating-system-kernel os))
> + (modules ->
> + (operating-system-kernel-loadable-modules os))
> + (kernel
> + ;; TODO: system, target.
> + (profile-derivation
> + (packages->manifest
> + (cons kernel modules))
> + #:hooks (list linux-module-database)
> + #:locales? #f
> + #:allow-collisions? #f
> + #:relative-symlinks? #t))
I think the system and target will be correct, but perhaps you can
double-check why doing ‘guix system build -s … -d’ and checking the
relevant .drv. :-)
I don’t think #:allow-collisions?, #:locales? and #:relative-symlinks?
are needed, so I’d recommend removing them.
> +++ b/gnu/tests/linux-modules.scm
Nice!
> +;; XXX: Dupe in gnu/build/linux-modules.scm .
> +(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))))
Same comment as above. :-)
> +(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."
Perhaps explicitly write “This is meant to be used as a profile hook.”
or similar.
> + (define build
> + (with-imported-modules (source-module-closure '((guix build utils) (gnu build linux-modules)))
80 chars please. :-)
> + #~(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))
Please make it only one ‘use-modules’ form.
> + (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))))))
80 chars please, and also one-word identifiers are preferred for local
variables.
> + ;; Note: Should usually result in one entry.
> + (versions (delete-duplicates
> + (append-map directory-entries
> + module-directories))))
> + ;; TODO: if len(module-directories) == 1: return module-directories[0]
> + (mkdir-p (string-append #$output "/lib"))
> + (match versions
> + ((version)
> + (make-linux-module-directory #$kmod inputs version #$output)))
> + (exit #t)))))
No need for ‘exit’, but perhaps and ‘error’ call in the unmatched case?
Thanks, and apologies for the delay!
Ludo’.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.