From debbugs-submit-bounces@debbugs.gnu.org Sun Mar 15 17:00:28 2020 Received: (at 37868) by debbugs.gnu.org; 15 Mar 2020 21:00:28 +0000 Received: from localhost ([127.0.0.1]:35685 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jDaN1-0008Pp-O3 for submit@debbugs.gnu.org; Sun, 15 Mar 2020 17:00:28 -0400 Received: from eggs.gnu.org ([209.51.188.92]:59487) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jDaMw-0008PZ-QG for 37868@debbugs.gnu.org; Sun, 15 Mar 2020 17:00:26 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]:59710) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1jDaMi-0000RL-P9; Sun, 15 Mar 2020 17:00:15 -0400 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=41596 helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jDaMg-0003vJ-HU; Sun, 15 Mar 2020 17:00:08 -0400 From: =?utf-8?Q?Ludovic_Court=C3=A8s?= To: Danny Milosavljevic Subject: Re: [PATCH v6] system: Add kernel-module-packages to operating-system. References: <20200226195929.3615-1-dannym@scratchpost.org> <20200227122519.3226-1-dannym@scratchpost.org> X-URL: http://www.fdn.fr/~lcourtes/ X-Revolutionary-Date: 26 =?utf-8?Q?Vent=C3=B4se?= an 228 de la =?utf-8?Q?R?= =?utf-8?Q?=C3=A9volution?= X-PGP-Key-ID: 0x090B11993D9AEBB5 X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc X-PGP-Fingerprint: 3CE4 6455 8A84 FDC6 9DB4 0CFB 090B 1199 3D9A EBB5 X-OS: x86_64-pc-linux-gnu Date: Sun, 15 Mar 2020 22:00:04 +0100 In-Reply-To: <20200227122519.3226-1-dannym@scratchpost.org> (Danny Milosavljevic's message of "Thu, 27 Feb 2020 13:25:19 +0100") Message-ID: <877dzlibaj.fsf@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Spam-Score: -0.7 (/) X-Debbugs-Envelope-To: 37868 Cc: Mark H Weaver , 37868@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.7 (-) Hi! Danny Milosavljevic skribis: > * gnu/system.scm (): Add kernel-module-packages. > (operating-system-directory-base-entries): Use it. > * doc/guix.texi (operating-system Reference): Document KERNEL-LOADABLE-MO= DULES. > * 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 i= t." > + ;; TODO: Use filter-map. > + (filter file-exists? > + (map (lambda (x) > + (string-append x file)) > + inputs))) =E2=80=9CInput=E2=80=9D in Guix is usually used to describe association lis= ts. 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=E2=80=99s shouldn=E2=80=99t be a bang, by convention. Also please ad= d 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=3D? "modules.builtin" basename)) > + (not (string=3D? "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=E2=80=99d 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=E2=80=99s creating =E2=80= =98System.maps=E2=80=99 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-fil= e 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 =E2=80=98guix system build -s =E2=80=A6 -d=E2=80=99 = and checking the relevant .drv. :-) I don=E2=80=99t think #:allow-collisions?, #:locales? and #:relative-symlin= ks? are needed, so I=E2=80=99d 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 i= t." > + ;; 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 =E2=80=9CThis is meant to be used as a profile hoo= k.=E2=80=9D 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 =E2=80=98use-modules=E2=80=99 form. > + (let* ((inputs '#$(manifest-inputs manifest)) > + (module-directories #$(input-files (manifest-inputs m= anifest) "/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) =3D=3D 1: return mod= ule-directories[0] > + (mkdir-p (string-append #$output "/lib")) > + (match versions > + ((version) > + (make-linux-module-directory #$kmod inputs version #$o= utput))) > + (exit #t))))) No need for =E2=80=98exit=E2=80=99, but perhaps and =E2=80=98error=E2=80=99= call in the unmatched case? Thanks, and apologies for the delay! Ludo=E2=80=99.