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.
View this message in rfc822 format
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: [bug#37868] [PATCH v2 2/2] system: Add kernel-module-packages to operating-system. Date: Sun, 23 Feb 2020 17:36:40 +0100
Danny Milosavljevic <dannym <at> 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’.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.