From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:470:142:3::10]:60595) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1j5uFb-0005UQ-14 for guix-patches@gnu.org; Sun, 23 Feb 2020 11:37:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1j5uFZ-0002mE-QY for guix-patches@gnu.org; Sun, 23 Feb 2020 11:37:02 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:45564) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1j5uFZ-0002lz-Mg for guix-patches@gnu.org; Sun, 23 Feb 2020 11:37:01 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1j5uFZ-00008b-LD for guix-patches@gnu.org; Sun, 23 Feb 2020 11:37:01 -0500 Subject: [bug#37868] [PATCH v2 2/2] system: Add kernel-module-packages to operating-system. Resent-Message-ID: From: Ludovic =?UTF-8?Q?Court=C3=A8s?= References: <20191112172048.61ba69eb@scratchpost.org> <20200218094207.6196-1-dannym@scratchpost.org> <20200218094207.6196-3-dannym@scratchpost.org> Date: Sun, 23 Feb 2020 17:36:40 +0100 In-Reply-To: <20200218094207.6196-3-dannym@scratchpost.org> (Danny Milosavljevic's message of "Tue, 18 Feb 2020 10:42:07 +0100") Message-ID: <875zfxs1k7.fsf@gnu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Danny Milosavljevic Cc: Mark H Weaver , 37868@debbugs.gnu.org Danny Milosavljevic skribis: > * gnu/system.scm (): 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=E2=80=99t require them to be objects, right? = Any lowerable object, like , would work? Thus, I=E2=80=99d be tempted to remove =E2=80=9Cpackages=E2=80=9D from the = field name. =E2=80=98kernel-modules=E2=80=99 is not a good idea because one may assume = it=E2=80=99s a list of .ko file names. Perhaps =E2=80=98kernel-loadable-modules=E2=80=99? 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; =E2=80=98modules=E2=80=99 is en= ough here. > + (kernel* s/kernel*/kernel/ since there=E2=80=99s 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 =E2=80=98null?=E2=80=99 case. Also, rather leave out #:sy= stem 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 =E2=80=98mlet=E2=80=99 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) input= s)))) s/path/file/ + use of =E2=80=98filter-map=E2=80=99 > + (module-directories (input-files "/lib/modules")) > + (System.maps (input-files "/System.map")) > + (Module.symverss (input-files "/Module.symvers")) ^ Typo. Also perhaps just =E2=80=98maps-file=E2=80=99 and =E2=80=98symvers-file=E2= =80=99. > + (directory-entries (lambda (directory-name) > + (filter (lambda (basename) > + (not (string-prefix? "." > + ba= sename))) > + (scandir directory-name))= )) > + ;; Note: Should result in one entry. > + (versions (append-map directory-entries module-director= ies))) > + ;; TODO: if len(module-directories) =3D=3D 1: return modul= e-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-directo= ry)) ; unique > + (union-build destination-directory > + ;; All directories with the s= ame version as us. > + (filter-map (lambda (director= y-name) > + (if (member ver= sion > + (di= rectory-entries directory-name)) > + (string-app= end directory-name "/" version) > + #f)) > + module-directorie= s) > + #:create-all-directories? #t) > + ;; Delete generated files (they will be re= created 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-d= irectory)) > + (unless (zero? (system* (string-append #$k= mod "/bin/depmod") > + "-e" ; Report symb= ols that aren't supplied > + "-w" ; Warn on dup= licates > + "-b" #$output ; de= stination-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 (=E2=80=98for-each=E2=80=99, =E2= =80=98when=E2=80=99, =E2=80=98unless=E2=80=99) happening at the very end. I wonder what=E2=80=99s missing form (gnu build linux-modules) to do the =E2=80=9Cdepmod=E2=80=9D bit entirely in Scheme. It would be nice for seve= ral reasons, one of which is that we wouldn=E2=80=99t need the =E2=80=98manifest-lookup-= package=E2=80=99 hack, which in turn would allow us to keep this procedure out of (guix profiles). Thoughts? Ludo=E2=80=99.