[PATCH] linux-libre: Enable module compression.

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Mathieu Othacehe
  • Mathieu Othacehe
Owner
unassigned
Submitted by
Mathieu Othacehe
Severity
normal
M
M
Mathieu Othacehe wrote on 29 Jun 2020 16:24
(address . guix-patches@gnu.org)(name . Mathieu Othacehe)(address . othacehe@gnu.org)
20200629142434.21308-1-othacehe@gnu.org
This commit enables GZIP compression for linux-libre kernel modules, reducing
the size of linux-libre by 63% (165MB). The initrd modules are kept
uncompressed as the initrd is already compressed as a whole.

The linux-libre kernel also supports XZ compression, but as Guix does not have
any available bindings for now, and the compression time is far more
significant, GZIP seems to be a better option.

* gnu/packages/aux-files/linux-libre/5.4-arm.conf: Enable GZ compression.
* gnu/packages/aux-files/linux-libre/5.4-arm64.conf: Ditto.
* gnu/packages/aux-files/linux-libre/5.4-i686.conf: Ditto.
* gnu/packages/aux-files/linux-libre/5.4-x86_64.conf: Ditto.
* gnu/build/linux-modules.scm (modinfo-section-contents): Use
'call-with-gzip-input-port' to read from a module file using '.gz' extension,
(strip-extension): new procedure,
(dot-ko): adapt to support compression,
(ensure-dot-ko): ditto,
(file-name->module-name): ditto,
(find-module-file): ditto,
(load-linux-module*): ditto,
(module-name->file-name/guess): ditto,
(module-name-lookup): ditto,
(write-module-name-database): ditto,
(write-module-alias-database): ditto,
(write-module-device-database): ditto.
* gnu/system/linux-initrd.scm (flat-linux-module-directory): Make sure that
zlib bindings are available because they may be used in
'write-module-device-database'. Also make sure that the initrd only contains
uncompressed module files.
---
Hello,

I think the commit message pretty much describes this change. Just wanted to
add that it passes the loadable-kernel-modules-X tests.

Thanks,

Mathieu

gnu/build/linux-modules.scm | 102 ++++++++++++------
.../aux-files/linux-libre/5.4-arm.conf | 4 +-
.../aux-files/linux-libre/5.4-arm64.conf | 4 +-
.../aux-files/linux-libre/5.4-i686.conf | 4 +-
.../aux-files/linux-libre/5.4-x86_64.conf | 4 +-
gnu/system/linux-initrd.scm | 38 +++++--
6 files changed, 111 insertions(+), 45 deletions(-)

Toggle diff (341 lines)
diff --git a/gnu/build/linux-modules.scm b/gnu/build/linux-modules.scm
index aa1c7cfeae..7c6945a881 100644
--- a/gnu/build/linux-modules.scm
+++ b/gnu/build/linux-modules.scm
@@ -21,6 +21,7 @@
(define-module (gnu build linux-modules)
#:use-module (guix elf)
#:use-module (guix glob)
+ #:use-module (guix zlib)
#:use-module (guix build syscalls)
#:use-module ((guix build utils) #:select (find-files invoke))
#:use-module (guix build union)
@@ -97,7 +98,16 @@ string list."
(define (modinfo-section-contents file)
"Return the contents of the '.modinfo' section of FILE as a list of
key/value pairs.."
- (let* ((bv (call-with-input-file file get-bytevector-all))
+ (define (get-bytevector file)
+ (cond
+ ((string-contains file ".ko.gz")
+ (call-with-input-file file
+ (lambda (port)
+ (call-with-gzip-input-port port get-bytevector-all))))
+ (else
+ (call-with-input-file file get-bytevector-all))))
+
+ (let* ((bv (get-bytevector file))
(elf (parse-elf bv))
(section (elf-section-by-name elf ".modinfo"))
(modinfo (section-contents elf section)))
@@ -110,7 +120,7 @@ key/value pairs.."
(define (module-formal-name file)
"Return the module name of FILE as it appears in its info section. Usually
the module name is the same as the base name of FILE, modulo hyphens and minus
-the \".ko\" extension."
+the \".ko[.gz|.xz]\" extension."
(match (assq 'name (modinfo-section-contents file))
(('name . name) name)
(#f #f)))
@@ -171,14 +181,25 @@ modules that can be postloaded, of the soft dependencies of module FILE."
(_ #f))
(modinfo-section-contents file))))
-(define dot-ko
- (cut string-append <> ".ko"))
-
-(define (ensure-dot-ko name)
- "Return NAME with a '.ko' prefix appended, unless it already has it."
- (if (string-suffix? ".ko" name)
+(define (strip-extension filename)
+ (let ((extension (string-index filename #\.)))
+ (if extension
+ (string-take filename extension)
+ filename)))
+
+(define (dot-ko name compression)
+ (let ((suffix (match compression
+ ('xz ".ko.xz")
+ ('gzip ".ko.gz")
+ (else ".ko"))))
+ (string-append name suffix)))
+
+(define (ensure-dot-ko name compression)
+ "Return NAME with a '.ko[.gz|.xz]' suffix appended, unless it already has
+it."
+ (if (string-contains name ".ko")
name
- (dot-ko name)))
+ (dot-ko name compression)))
(define (normalize-module-name module)
"Return the \"canonical\" name for MODULE, replacing hyphens with
@@ -191,9 +212,9 @@ underscores."
module))
(define (file-name->module-name file)
- "Return the module name corresponding to FILE, stripping the trailing '.ko'
-and normalizing it."
- (normalize-module-name (basename file ".ko")))
+ "Return the module name corresponding to FILE, stripping the trailing
+'.ko[.gz|.xz]' and normalizing it."
+ (normalize-module-name (strip-extension (basename file))))
(define (find-module-file directory module)
"Lookup module NAME under DIRECTORY, and return its absolute file name.
@@ -208,19 +229,19 @@ whereas file names often, but not always, use hyphens. Examples:
;; List of possible file names. XXX: It would of course be cleaner to
;; have a database that maps module names to file names and vice versa,
;; but everyone seems to be doing hacks like this one. Oh well!
- (map ensure-dot-ko
- (delete-duplicates
- (list module
- (normalize-module-name module)
- (string-map (lambda (chr) ;converse of 'normalize-module-name'
- (case chr
- ((#\_) #\-)
- (else chr)))
- module)))))
+ (delete-duplicates
+ (list module
+ (normalize-module-name module)
+ (string-map (lambda (chr) ;converse of 'normalize-module-name'
+ (case chr
+ ((#\_) #\-)
+ (else chr)))
+ module))))
(match (find-files directory
(lambda (file stat)
- (member (basename file) names)))
+ (member (strip-extension
+ (basename file)) names)))
((file)
file)
(()
@@ -290,8 +311,8 @@ not a file name."
(recursive? #t)
(lookup-module dot-ko)
(black-list (module-black-list)))
- "Load Linux module from FILE, the name of a '.ko' file; return true on
-success, false otherwise. When RECURSIVE? is true, load its dependencies
+ "Load Linux module from FILE, the name of a '.ko[.gz|.xz]' file; return true
+on success, false otherwise. When RECURSIVE? is true, load its dependencies
first (à la 'modprobe'.) The actual files containing modules depended on are
obtained by calling LOOKUP-MODULE with the module name. Modules whose name
appears in BLACK-LIST are not loaded."
@@ -523,16 +544,27 @@ are required to access DEVICE."
;;; Module databases.
;;;
-(define (module-name->file-name/guess directory name)
+(define* (module-name->file-name/guess directory name
+ #:key compression)
"Guess the file name corresponding to NAME, a module name. That doesn't
always work because sometimes underscores in NAME map to hyphens (e.g.,
\"input-leds.ko\"), sometimes not (e.g., \"mac_hid.ko\")."
- (string-append directory "/" (ensure-dot-ko name)))
+ (string-append directory "/" (ensure-dot-ko name compression)))
(define (module-name-lookup directory)
"Return a one argument procedure that takes a module name (e.g.,
\"input_leds\") and returns its absolute file name (e.g.,
\"/.../input-leds.ko\")."
+ (define (guess-file-name name)
+ (let ((names (list
+ (module-name->file-name/guess directory name)
+ (module-name->file-name/guess directory name
+ #:compression 'xz)
+ (module-name->file-name/guess directory name
+ #:compression 'gzip))))
+ (or (find file-exists? names)
+ (first names))))
+
(catch 'system-error
(lambda ()
(define mapping
@@ -541,23 +573,23 @@ always work because sometimes underscores in NAME map to hyphens (e.g.,
(lambda (name)
(or (assoc-ref mapping name)
- (module-name->file-name/guess directory name))))
+ (guess-file-name name))))
(lambda args
(if (= ENOENT (system-error-errno args))
- (cut module-name->file-name/guess directory <>)
+ (cut guess-file-name <>)
(apply throw args)))))
(define (write-module-name-database directory)
"Write a database that maps \"module names\" as they appear in the relevant
-ELF section of '.ko' files, to actual file names. This format is
+ELF section of '.ko[.gz|.xz]' files, to actual file names. This format is
Guix-specific. It aims to deal with inconsistent naming, in particular
hyphens vs. underscores."
(define mapping
(map (lambda (file)
(match (module-formal-name file)
- (#f (cons (basename file ".ko") file))
+ (#f (cons (strip-extension (basename file)) file))
(name (cons name file))))
- (find-files directory "\\.ko$")))
+ (find-files directory "\\.ko.*$")))
(call-with-output-file (string-append directory "/modules.name")
(lambda (port)
@@ -569,12 +601,12 @@ hyphens vs. underscores."
(pretty-print mapping port))))
(define (write-module-alias-database directory)
- "Traverse the '.ko' files in DIRECTORY and create the corresponding
+ "Traverse the '.ko[.gz|.xz]' files in DIRECTORY and create the corresponding
'modules.alias' file."
(define aliases
(map (lambda (file)
(cons (file-name->module-name file) (module-aliases file)))
- (find-files directory "\\.ko$")))
+ (find-files directory "\\.ko.*$")))
(call-with-output-file (string-append directory "/modules.alias")
(lambda (port)
@@ -616,7 +648,7 @@ are found, return a tuple (DEVNAME TYPE MAJOR MINOR), otherwise return #f."
(char-set-complement (char-set #\-)))
(define (write-module-device-database directory)
- "Traverse the '.ko' files in DIRECTORY and create the corresponding
+ "Traverse the '.ko[.gz|.xz]' files in DIRECTORY and create the corresponding
'modules.devname' file. This file contains information about modules that can
be loaded on-demand, such as file system modules."
(define aliases
@@ -624,7 +656,7 @@ be loaded on-demand, such as file system modules."
(match (aliases->device-tuple (module-aliases file))
(#f #f)
(tuple (cons (file-name->module-name file) tuple))))
- (find-files directory "\\.ko$")))
+ (find-files directory "\\.ko.*$")))
(call-with-output-file (string-append directory "/modules.devname")
(lambda (port)
diff --git a/gnu/packages/aux-files/linux-libre/5.4-arm.conf b/gnu/packages/aux-files/linux-libre/5.4-arm.conf
index a54228643b..7c9ab94719 100644
--- a/gnu/packages/aux-files/linux-libre/5.4-arm.conf
+++ b/gnu/packages/aux-files/linux-libre/5.4-arm.conf
@@ -880,7 +880,9 @@ CONFIG_MODULE_FORCE_UNLOAD=y
CONFIG_MODVERSIONS=y
# CONFIG_MODULE_SRCVERSION_ALL is not set
# CONFIG_MODULE_SIG is not set
-# CONFIG_MODULE_COMPRESS is not set
+CONFIG_MODULE_COMPRESS=y
+CONFIG_MODULE_COMPRESS_GZIP=y
+# CONFIG_MODULE_COMPRESS_XZ is not set
# CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is not set
# CONFIG_UNUSED_SYMBOLS is not set
# CONFIG_TRIM_UNUSED_KSYMS is not set
diff --git a/gnu/packages/aux-files/linux-libre/5.4-arm64.conf b/gnu/packages/aux-files/linux-libre/5.4-arm64.conf
index fabd25c6a4..6520d1ddf2 100644
--- a/gnu/packages/aux-files/linux-libre/5.4-arm64.conf
+++ b/gnu/packages/aux-files/linux-libre/5.4-arm64.conf
@@ -781,7 +781,9 @@ CONFIG_MODVERSIONS=y
CONFIG_ASM_MODVERSIONS=y
# CONFIG_MODULE_SRCVERSION_ALL is not set
# CONFIG_MODULE_SIG is not set
-# CONFIG_MODULE_COMPRESS is not set
+CONFIG_MODULE_COMPRESS=y
+CONFIG_MODULE_COMPRESS_GZIP=y
+# CONFIG_MODULE_COMPRESS_XZ is not set
# CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is not set
# CONFIG_UNUSED_SYMBOLS is not set
# CONFIG_TRIM_UNUSED_KSYMS is not set
diff --git a/gnu/packages/aux-files/linux-libre/5.4-i686.conf b/gnu/packages/aux-files/linux-libre/5.4-i686.conf
index 50a41f6cc9..3727f9d486 100644
--- a/gnu/packages/aux-files/linux-libre/5.4-i686.conf
+++ b/gnu/packages/aux-files/linux-libre/5.4-i686.conf
@@ -850,7 +850,9 @@ CONFIG_MODVERSIONS=y
CONFIG_ASM_MODVERSIONS=y
CONFIG_MODULE_SRCVERSION_ALL=y
# CONFIG_MODULE_SIG is not set
-# CONFIG_MODULE_COMPRESS is not set
+CONFIG_MODULE_COMPRESS=y
+CONFIG_MODULE_COMPRESS_GZIP=y
+# CONFIG_MODULE_COMPRESS_XZ is not set
# CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is not set
CONFIG_UNUSED_SYMBOLS=y
CONFIG_MODULES_TREE_LOOKUP=y
diff --git a/gnu/packages/aux-files/linux-libre/5.4-x86_64.conf b/gnu/packages/aux-files/linux-libre/5.4-x86_64.conf
index 30fdc4e86a..be7a603af1 100644
--- a/gnu/packages/aux-files/linux-libre/5.4-x86_64.conf
+++ b/gnu/packages/aux-files/linux-libre/5.4-x86_64.conf
@@ -849,7 +849,9 @@ CONFIG_MODVERSIONS=y
CONFIG_ASM_MODVERSIONS=y
CONFIG_MODULE_SRCVERSION_ALL=y
# CONFIG_MODULE_SIG is not set
-# CONFIG_MODULE_COMPRESS is not set
+CONFIG_MODULE_COMPRESS=y
+CONFIG_MODULE_COMPRESS_GZIP=y
+# CONFIG_MODULE_COMPRESS_XZ is not set
# CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is not set
CONFIG_UNUSED_SYMBOLS=y
CONFIG_MODULES_TREE_LOOKUP=y
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index 0971ec29e2..99ec82246b 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -111,11 +111,29 @@ the derivations referenced by EXP are automatically copied to the initrd."
(define (flat-linux-module-directory linux modules)
"Return a flat directory containing the Linux kernel modules listed in
MODULES and taken from LINUX."
+ (define zlib
+ (module-ref (resolve-interface '(gnu packages compression)) 'zlib))
+
+ (define config.scm
+ (scheme-file "config.scm"
+ #~(begin
+ (define-module (guix config)
+ #:export (%libz))
+
+ (define %libz
+ #+(file-append zlib "/lib/libz")))))
+
+ (define imported-modules
+ (cons `((guix config) => ,config.scm)
+ (delete '(guix config)
+ (source-module-closure '((gnu build linux-modules)
+ (guix build utils))))))
+
(define build-exp
- (with-imported-modules (source-module-closure
- '((gnu build linux-modules)))
+ (with-imported-modules imported-modules
#~(begin
(use-modules (gnu build linux-modules)
+ (guix build utils)
(srfi srfi-1)
(srfi srfi-26))
@@ -129,12 +147,20 @@ MODULES and taken from LINUX."
(recursive-module-dependencies modules
#:lookup-module lookup))))
+ (define (maybe-uncompress file)
+ ;; If FILE is a compressed module, uncompress it, as the initrd is
+ ;; already gzipped as a whole.
+ (cond
+ ((string-contains file ".ko.gz")
+ (invoke #+(file-append gzip "/bin/gunzip") file))))
+
(mkdir #$output)
(for-each (lambda (module)
- (format #t "copying '~a'...~%" module)
- (copy-file module
- (string-append #$output "/"
- (basename module))))
+ (let ((out-module
+ (string-append #$output "/" (basename module))))
+ (format #t "copying '~a'...~%" module)
+ (copy-file module out-module)
+ (maybe-uncompress out-module)))
(delete-duplicates modules))
;; Hyphen or underscore? This database tells us.
--
2.26.2
M
M
Mathieu Othacehe wrote on 30 Jun 2020 09:31
(address . 42123@debbugs.gnu.org)
87a70l103p.fsf@gnu.org
Hello,

There's at least one issue with this one. When running the
"installation-os" test, I have this backtrace:

Toggle snippet (32 lines)
+ guix system init /mnt/etc/config.scm /mnt --no-substitutes
The following derivations will be built:
/gnu/store/rlq93i0ahllyvmjxk14w3snmqsv7bsvg-system.drv
/gnu/store/28cfjshgch8jyixmwv3q0x1ybniwlvf5-parameters.drv
/gnu/store/vk3rw7qwm7p0ldy34cadzgczxx2hc5xf-raw-initrd.drv
/gnu/store/w219h1ia74bkplvr622f0b8qxyi9xw7y-provenance.drv
/gnu/store/z56pp4bw5hif9mh856cckqvfnvykaybh-profile.drv
/gnu/store/xalv7fm73cpyyi3zyvy1zlb26nfi1fb6-module-import-compiled.drv
/gnu/store/rmpwh65ypvv1805zwmcir8gk9b53c4gz-module-import-compiled.drv
/gnu/store/l9ixrby1zjqvdc719lyvfp9n9n4hb5pv-grub.cfg.drv
The following profile hook will be built:
/gnu/store/92giwj2f8jyq90l20q19vcvds18v9xf4-linux-module-database.drv
building /gnu/store/w219h1ia74bkplvr622f0b8qxyi9xw7y-provenance.drv...
building /gnu/store/k4m2pj7sadz4vv5aaz40f68gaa4mw36b-Python-3.5.9.tar.xz.drv...
\builder for `/gnu/store/k4m2pj7sadz4vv5aaz40f68gaa4mw36b-Python-3.5.9.tar.xz.drv' failed to produce output path `/gnu/store/f99fblkzb6ip268sg096shhs7wzjyp55-Python-3.5.9.tar.xz'
build of /gnu/store/k4m2pj7sadz4vv5aaz40f68gaa4mw36b-Python-3.5.9.tar.xz.drv failed
View build log at '/var/log/guix/drvs/k4/m2pj7sadz4vv5aaz40f68gaa4mw36b-Python-3.5.9.tar.xz.drv.bz2'.
cannot build derivation `/gnu/store/9l09n8d6ick1nsjvchysys3hdgq7ynfr-Python-3.5.9.tar.xz.drv': 1 dependencies couldn't be built
building /gnu/store/wd9giqyfcfm1wgc6rscl3m9i30hg6rcs-bash-2.05b.tar.gz.drv...
cannot build derivation `/gnu/store/66q0r5cr3j1cbwckx5zvi0wv4cp3kxgl-python-minimal-3.5.9.drv': 1 dependencies couldn't be built
cannot build derivation `/gnu/store/c2ly303cbslks8hx3811wa91wahqr295-glibc-2.31.drv': 1 dependencies couldn't be built
building /gnu/store/h1p3962y3bmv1zgwsng1gb4c7caryj82-config.scm.drv...
cannot build derivation `/gnu/store/97vna9jgn19hyfx24s2kd6c3wywg22wl-e2fsprogs-1.45.6.drv': 1 dependencies couldn't be built
cannot build derivation `/gnu/store/r4dg2iypidr067kyddg90z07arrxp3h6-e2fsck-static-1.45.6.drv': 1 dependencies couldn't be built
cannot build derivation `/gnu/store/64f1y8njbd8bppl61fm6dhljnlgmh3h2-init.drv': 1 dependencies couldn't be built
building /gnu/store/6v5f1ry0pbhm2a1v5v8b773qpncnf6rr-module-import-compiled.drv...
cannot build derivation `/gnu/store/vk3rw7qwm7p0ldy34cadzgczxx2hc5xf-raw-initrd.drv': 1 dependencies couldn't be built
cannot build derivation `/gnu/store/l9ixrby1zjqvdc719lyvfp9n9n4hb5pv-grub.cfg.drv': 1 dependencies couldn't be built
guix system: error: build of `/gnu/store/l9ixrby1zjqvdc719lyvfp9n9n4hb5pv-grub.cfg.drv' failed
environment variable `PATH' set to `/gnu/store/a2b10x6h8j8qgsrcqip06xhnbssa0k25-qemu-minimal-5.0.0/bin'

For some reason, something triggers a build of the initrd (instead of using
the gcrooted one). I guess it's related to the addition of "zlib" and
"gzip" to the initrd inputs, but I don't understand why.

Someone?

Thanks,

Mathieu
L
L
Ludovic Courtès wrote on 2 Jul 2020 12:23
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
87366atdve.fsf@gnu.org
Hi!

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

Toggle quote (30 lines)
> This commit enables GZIP compression for linux-libre kernel modules, reducing
> the size of linux-libre by 63% (165MB). The initrd modules are kept
> uncompressed as the initrd is already compressed as a whole.
>
> The linux-libre kernel also supports XZ compression, but as Guix does not have
> any available bindings for now, and the compression time is far more
> significant, GZIP seems to be a better option.
>
> * gnu/packages/aux-files/linux-libre/5.4-arm.conf: Enable GZ compression.
> * gnu/packages/aux-files/linux-libre/5.4-arm64.conf: Ditto.
> * gnu/packages/aux-files/linux-libre/5.4-i686.conf: Ditto.
> * gnu/packages/aux-files/linux-libre/5.4-x86_64.conf: Ditto.
> * gnu/build/linux-modules.scm (modinfo-section-contents): Use
> 'call-with-gzip-input-port' to read from a module file using '.gz' extension,
> (strip-extension): new procedure,
> (dot-ko): adapt to support compression,
> (ensure-dot-ko): ditto,
> (file-name->module-name): ditto,
> (find-module-file): ditto,
> (load-linux-module*): ditto,
> (module-name->file-name/guess): ditto,
> (module-name-lookup): ditto,
> (write-module-name-database): ditto,
> (write-module-alias-database): ditto,
> (write-module-device-database): ditto.
> * gnu/system/linux-initrd.scm (flat-linux-module-directory): Make sure that
> zlib bindings are available because they may be used in
> 'write-module-device-database'. Also make sure that the initrd only contains
> uncompressed module files.

Nice!

I do think that gzip is more appropriate than xz here, also in terms of
memory requirements.

Perhaps you can do this in two patches: first the linux-initrd bits, and
2nd the linux-libre changes.

Toggle quote (21 lines)
> diff --git a/gnu/build/linux-modules.scm b/gnu/build/linux-modules.scm
> index aa1c7cfeae..7c6945a881 100644
> --- a/gnu/build/linux-modules.scm
> +++ b/gnu/build/linux-modules.scm
> @@ -21,6 +21,7 @@
> (define-module (gnu build linux-modules)
> #:use-module (guix elf)
> #:use-module (guix glob)
> + #:use-module (guix zlib)
> #:use-module (guix build syscalls)
> #:use-module ((guix build utils) #:select (find-files invoke))
> #:use-module (guix build union)
> @@ -97,7 +98,16 @@ string list."
> (define (modinfo-section-contents file)
> "Return the contents of the '.modinfo' section of FILE as a list of
> key/value pairs.."
> - (let* ((bv (call-with-input-file file get-bytevector-all))
> + (define (get-bytevector file)
> + (cond
> + ((string-contains file ".ko.gz")

‘string-suffix?’ would be more accurate.

Toggle quote (4 lines)
> + (call-with-input-file file
> + (lambda (port)
> + (call-with-gzip-input-port port get-bytevector-all))))

‘call-with-input-file’ creates a buffered port, which could be
problematic, although ‘make-gzip-input-port’ checks that.

To be safe, you’d do (open-file file "r0") with a dynwind.

Toggle quote (6 lines)
> +(define* (module-name->file-name/guess directory name
> + #:key compression)
> "Guess the file name corresponding to NAME, a module name. That doesn't
> always work because sometimes underscores in NAME map to hyphens (e.g.,
> \"input-leds.ko\"), sometimes not (e.g., \"mac_hid.ko\")."

Please mention COMPRESSION in the docstring.

Toggle quote (10 lines)
> (define (write-module-alias-database directory)
> - "Traverse the '.ko' files in DIRECTORY and create the corresponding
> + "Traverse the '.ko[.gz|.xz]' files in DIRECTORY and create the corresponding
> 'modules.alias' file."
> (define aliases
> (map (lambda (file)
> (cons (file-name->module-name file) (module-aliases file)))
> - (find-files directory "\\.ko$")))
> + (find-files directory "\\.ko.*$")))

Should we refine this regexp (there are a couple of places like this)?

There other Scheme bits LGTM!

(I don’t really know about Linux-libre but you do!)

Ludo’.
M
M
Mathieu Othacehe wrote on 6 Jul 2020 10:48
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 42123@debbugs.gnu.org)
87lfjx6nb9.fsf@gnu.org
Hey Ludo,

Toggle quote (4 lines)
> There other Scheme bits LGTM!
>
> (I don’t really know about Linux-libre but you do!)

Thanks a lot for reviewing :) I took all your remarks into
account. Regarding the test issue I'm describing in this thread, I now
have an understanding of the situation.

Adding (guix zlib) as a dependency of (gnu build linux-modules) also
drags (guix config). That's an issue because there are a lot of
derivations including (gnu build linux-modules) without deleting/mocking
(guix config).

That's for instance the case with "expression->initrd", and it explains
why the installation test is trying to rebuild the initrd.

I see two solutions here:

* Delete/mock (guix config) in every derivation including (gnu build
linux-modules) or any other build module that includes it. Not ideal
because there are quite numerous.

* Do not use (guix zlib) but a raw "gzip -cd" pipe in (gnu build
linux-modules). It works but it way slower.
A third solution would be nice here :)

WDYT?

Thanks,

Mathieu
L
L
Ludovic Courtès wrote on 6 Jul 2020 14:20
(name . Mathieu Othacehe)(address . othacehe@gnu.org)(address . 42123@debbugs.gnu.org)
873664ltqt.fsf@gnu.org
Hello!

Mathieu Othacehe <othacehe@gnu.org> skribis:

Toggle quote (17 lines)
> Adding (guix zlib) as a dependency of (gnu build linux-modules) also
> drags (guix config). That's an issue because there are a lot of
> derivations including (gnu build linux-modules) without deleting/mocking
> (guix config).
>
> That's for instance the case with "expression->initrd", and it explains
> why the installation test is trying to rebuild the initrd.
>
> I see two solutions here:
>
> * Delete/mock (guix config) in every derivation including (gnu build
> linux-modules) or any other build module that includes it. Not ideal
> because there are quite numerous.
>
> * Do not use (guix zlib) but a raw "gzip -cd" pipe in (gnu build
> linux-modules). It works but it way slower.

I don’t have other ideas, but both solutions sound good to me. Using
(guix zlib) is slightly more “elegant” IMO, but no big deal. I don’t
expect any significant difference from the use of in-process
decompression, unless we really have to go and decompress many modules
in a row.

Thanks,
Ludo’.
M
M
Mathieu Othacehe wrote on 6 Jul 2020 16:23
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 42123@debbugs.gnu.org)
878sfw7mec.fsf@gnu.org
Hey,

Toggle quote (6 lines)
> I don’t have other ideas, but both solutions sound good to me. Using
> (guix zlib) is slightly more “elegant” IMO, but no big deal. I don’t
> expect any significant difference from the use of in-process
> decompression, unless we really have to go and decompress many modules
> in a row.

Creating the initrd implies to create the module name database, and it
ends-up decompressing every single module. Using the in-process method
it takes 2 seconds, using the second method 30 seconds.

So, I opted for the first solution as you suggested. Here's an attached
patch that fixes the situation.

Thanks,

Mathieu
Attachment: import.patch
L
L
Ludovic Courtès wrote on 6 Jul 2020 22:13
(name . Mathieu Othacehe)(address . othacehe@gnu.org)(address . 42123@debbugs.gnu.org)
871rloiept.fsf@gnu.org
Hi,

Mathieu Othacehe <othacehe@gnu.org> skribis:

Toggle quote (10 lines)
>> I don’t have other ideas, but both solutions sound good to me. Using
>> (guix zlib) is slightly more “elegant” IMO, but no big deal. I don’t
>> expect any significant difference from the use of in-process
>> decompression, unless we really have to go and decompress many modules
>> in a row.
>
> Creating the initrd implies to create the module name database, and it
> ends-up decompressing every single module. Using the in-process method
> it takes 2 seconds, using the second method 30 seconds.

Woow, interesting.

Toggle quote (3 lines)
> So, I opted for the first solution as you suggested. Here's an attached
> patch that fixes the situation.

[...]

Toggle quote (20 lines)
> From 8bbf343510091fad4a08758e0115a70410c1c8d7 Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <othacehe@gnu.org>
> Date: Mon, 6 Jul 2020 16:04:21 +0200
> Subject: [PATCH] self: Add with-imported-modules+config and use it.
>
> Introduce "with-imported-modules+config" and use it to replace every call to
> "with-imported-modules" that would trigger an import of (guix config) module.
>
> * guix/self.scm (not-config?): New procedure,
> (with-imported-modules+config): new macro.
> * guix/profiles.scm (linux-module-database): Replace with-imported-modules by
> with-imported-modules+config.
> * gnu/system/shadow.scm (account-shepherd-service): Ditto.
> * gnu/system/linux-initrd.scm (raw-initrd): Ditto.
> * gnu/services/base.scm (default-serial-port): Ditto,
> (file-system-shepherd-service): ditto,
> (udev-shepherd-service): ditto.
> * gnu/services.scm (activation-script): Ditto.
> * gnu/machine/ssh.scm (machine-check-initrd-modules): Ditto.

[...]

Toggle quote (35 lines)
> diff --git a/guix/self.scm b/guix/self.scm
> index e1350a7403..82bb55f8e7 100644
> --- a/guix/self.scm
> +++ b/guix/self.scm
> @@ -33,6 +33,8 @@
> #:use-module (srfi srfi-35)
> #:use-module (ice-9 match)
> #:export (make-config.scm
> + not-config?
> + with-imported-modules+config
> whole-package ;for internal use in 'guix pull'
> compiled-guix
> guix-derivation))
> @@ -1063,6 +1065,24 @@ Info manual."
> ;; made relative to a nonexistent anonymous module.
> #:splice? #t))
>
> +(define not-config?
> + ;; Select (guix …) and (gnu …) modules, except (guix config).
> + (match-lambda
> + (('guix 'config) #f)
> + (('guix rest ...) #t)
> + (('gnu rest ...) #t)
> + (rest #f)))
> +
> +(define-syntax-rule (with-imported-modules+config modules exp ...)
> + "Import the closure of MODULES and evaluate EXP within this context. If the
> +(guix config) module is part of the closure, it is not selected. This module
> +is always replaced by a mocked-one, created by MAKE-CONFIG.SCM pocedure."
> + (with-imported-modules `(,@(source-module-closure
> + modules
> + #:select? not-config?)
> + ((guix config) => ,(make-config.scm)))
> + exp ...))

Two remarks: I feel that this is not the best place for it, and I think
we should add (guix config) if and only if it’s actually part of the
closure.

For the name I’m tempted by a simpler but less descriptive option.

That would give us:

(define-syntax-rule (with-imported-modules* modules exp ...)
(let ((closure (source-module-closure modules #:select? not-config?)))
(with-imported-modules (map (match-lambda
(('guix 'config) …)
(module module))
closure)
exp ...)))

WDYT?

As for the location… I think the reason things are like this is to avoid
having everything depend on (guix self), but maybe that’s OK after all?

If the zlib bindings were an external package, things would be easier
because we wouldn’t have to do this (guix config) dance.

More generally, we should look at all the uses of (guix config) and see
whether/how we can get rid of them.

But I digress… Thoughts?

Thank you,
Ludo’.
M
M
Mathieu Othacehe wrote on 7 Jul 2020 09:32
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 42123@debbugs.gnu.org)
874kqjn5k4.fsf@gnu.org
Hey Ludo!

Toggle quote (11 lines)
> As for the location… I think the reason things are like this is to avoid
> having everything depend on (guix self), but maybe that’s OK after all?
>
> If the zlib bindings were an external package, things would be easier
> because we wouldn’t have to do this (guix config) dance.
>
> More generally, we should look at all the uses of (guix config) and see
> whether/how we can get rid of them.
>
> But I digress… Thoughts?

Thanks for your quick feedback. Given the time I spent debugging this
issue, I agree that this (guix config) module thing is hard to deal
with.

If it works for you, I could create three new Guile libraries:

* Guile-libz
* Guile-liblz
* Guile-xz

or maybe just,

* Guile-compression

It would help us getting rid of (guix config) as you proposed.

WDYT?

Thanks,

Mathieu
L
L
Ludovic Courtès wrote on 9 Jul 2020 09:56
(name . Mathieu Othacehe)(address . othacehe@gnu.org)(address . 42123@debbugs.gnu.org)
874kqhcea1.fsf@gnu.org
Hi,

Mathieu Othacehe <othacehe@gnu.org> skribis:

Toggle quote (6 lines)
> If it works for you, I could create three new Guile libraries:
>
> * Guile-libz
> * Guile-liblz
> * Guile-xz

If I may :-), it should be guile-zlib and guile-lzlib to match upstream
names.

(We don’t have bindings to XZ though.)

Toggle quote (4 lines)
> or maybe just,
>
> * Guile-compression

Yeah it’s tempting, though OTOH users would be forced to pull everything
even if they only care about one of these. Dunno.

Hall should come in handy to build those libs! I guess people will be
happy to have them available outside Guix.

Toggle quote (2 lines)
> It would help us getting rid of (guix config) as you proposed.

Yeah.

Thank you!

Ludo’.
M
M
Mathieu Othacehe wrote on 27 Jul 2020 18:24
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 42123@debbugs.gnu.org)
87v9i9exjq.fsf@gnu.org
Hey hey,

Toggle quote (5 lines)
> Hall should come in handy to build those libs! I guess people will be
> happy to have them available outside Guix.
>
>> It would help us getting rid of (guix config) as you proposed.

So as you suggested I created "guile-zlib" and "guile-lzlib" which was
the easy part. The other part is the hard to digest, attached patch.

I tested it running "make check", "make as-derivation", "./pre-inst-env
guix build guix". Hope I didn't miss something.

Please, tell me what you think :)

Thanks,

Mathieu
L
L
Ludovic Courtès wrote on 29 Jul 2020 00:16
(name . Mathieu Othacehe)(address . othacehe@gnu.org)(address . 42123@debbugs.gnu.org)
87tuxrqo9k.fsf@gnu.org
Hi!

Mathieu Othacehe <othacehe@gnu.org> skribis:

Toggle quote (3 lines)
> So as you suggested I created "guile-zlib" and "guile-lzlib" which was
> the easy part. The other part is the hard to digest, attached patch.

Thanks for doing that!

(I figured David Thompson had one wrapping the low-level bits of zlib:
incorporate that.)

Toggle quote (3 lines)
> I tested it running "make check", "make as-derivation", "./pre-inst-env
> guix build guix". Hope I didn't miss something.

Nice!

Toggle quote (2 lines)
> Please, tell me what you think :)

Just a quick review because I’m headed for a break. :-)

Toggle quote (53 lines)
> From d0f23078d1963f9aa8facda0fb3ae40e1e8c3cf2 Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <othacehe@gnu.org>
> Date: Mon, 27 Jul 2020 16:36:39 +0200
> Subject: [PATCH] Use "guile-zlib" and "guile-lzlib" instead of (guix config).
>
> * Makefile.am (MODULES): Remove guix/zlib.scm and guix/lzlib.scm,
> (SCM_TESTS): remove tests/zlib.scm, tests/lzlib.scm.
> * build-aux/build-self.scm (make-config.scm): Remove unused %libz variable.
> * configure.ac: Remove LIBZ and LIBLZ variables and check instead for
> Guile-zlib and Guile-lzlib.
> * doc/guix.texi ("Requirements"): Remove zlib requirement and add Guile-zlib
> and Guile-lzlib instead.
> * gnu/packages/package-management.scm (guix)[native-inputs]: Add "guile-zlib"
> and "guile-lzlib",
> [inputs]: remove "zlib" and "lzlib",
> [propagated-inputs]: ditto,
> [arguments]: add "guile-zlib" and "guile-lzlib" to Guile load path.
> * guix/build/download-nar.scm: Use (zlib) instead of (guix zlib).
> * guix/config.scm.in (%libz, %liblz): Remove them.
> * guix/cvs-download.scm (cvs-fetch): Do not stub (guix config) in imported
> modules list, instead add "guile-zlib" to the extension list.
> * guix/git-download.scm (git-fetch): Ditto.
> * guix/gnu-maintenance.scm: Use (zlib) instead of (guix zlib).
> * guix/hg-download.scm (hg-fetch): Do not stub (guix config) in imported
> modules list, instead add "guile-zlib" to the extension list.
> * guix/lzlib.scm: Remove it.
> * guix/man-db.scm: Use (zlib) instead of (guix zlib).
> * guix/profiles.scm (manual-database): Do not stub (guix config) in imported
> modules list, instead add "guile-zlib" to the extension list.
> * guix/scripts/publish.scm: Use (zlib) instead of (guix zlib) and (lzlib)
> instead of (guix lzlib),
> (string->compression-type, effective-compression): do not check for zlib and
> lzlib availability.
> * guix/scripts/substitute.scm (%compression-methods): Do not check for lzlib
> availability.
> * guix/self.scm (specification->package): Add "guile-zlib" and "guile-lzlib"
> and remove "zlib" and "lzlib",
> (compiled-guix): remove "zlib" and "lzlib" arguments and add guile-zlib and
> guile-lzlib to the dependencies, also do not pass "zlib" and "lzlib" to
> "make-config.scm" procedure,
> (make-config.scm): remove "zlib" and "lzlib" arguments as well as %libz and
> %liblz variables.
> * guix/utils.scm (lzip-port): Use (lzlib) instead of (guix lzlib) and do not
> check for lzlib availability.
> * guix/zlib.scm: Remove it.
> * m4/guix.m4 (GUIX_LIBZ_LIBDIR, GUIX_LIBLZ_FILE_NAME): Remove them.
> * tests/lzlib.scm: Use (zlib) instead of (guix zlib) and (lzlib)
> instead of (guix lzlib), and do not check for zlib and lzlib availability.
> * tests/publish.scm: Ditto.
> * tests/substitute.scm: Do not check for lzlib availability.
> * tests/utils.scm: Ditto.
> * tests/zlib.scm: Remove it.

This can be decomposed in several steps:

1. We can start using ‘guile-zlib’ as extensions for gexps: in (guix
scripts pack), (guix download), etc. Easy, no risk.

2. Use guile-zlib & co. in Guix itself: (guix scripts substitute),
(guix scripts publish), etc. Keep (guix zlib) and (guix lzlib) in
parallel.

3. Update ‘guix’ package with these two new dependencies.

4. Remove uses (guix zlib) and (guix lzlib), adjust build machinery
and doc.

5. Adjust (guix self) and related code. This is a bit touchy.

Perhaps there’s a couple of steps that can be merged, but having a split
along these lines would be clearer and better for our peace of mind.

Toggle quote (12 lines)
> --- a/build-aux/build-self.scm
> +++ b/build-aux/build-self.scm
> @@ -71,7 +71,7 @@
> (variables rest ...))))))
> (variables %localstatedir %storedir %sysconfdir %system)))
>
> -(define* (make-config.scm #:key zlib gzip xz bzip2
> +(define* (make-config.scm #:key gzip xz bzip2
> (package-name "GNU Guix")
> (package-version "0")
> (bug-report-address "bug-guix@gnu.org")

This is OK.

Toggle quote (11 lines)
> @@ -133,11 +133,7 @@
> (define %bzip2
> #+(and bzip2 (file-append bzip2 "/bin/bzip2")))
> (define %xz
> - #+(and xz (file-append xz "/bin/xz")))
> -
> - (define %libz
> - #+(and zlib
> - (file-append zlib "/lib/libz")))))))
> + #+(and xz (file-append xz "/bin/xz")))))))

I think it’s OK too, but we have to keep in mind that this code can be
run by a past Guix that expects ‘%libz’. Normally it’s OK because the
modules needed by ‘compute-guix-derivation’ do not rely on ‘%libz’ and
(guix zlib).

One test would be something like:

guix time-machine --commit=6298c3ffd9654d3231a6f25390b056483e8f407c -- \
pull -p /tmp/test --url=/path/to/local/repo --branch=the-branch

This will check that 1.0.0 can indeed pull this new repo.

The rest LGTM. Thanks a lot for all the work!

Ludo’, who’s going to be away the coming weeks.
M
M
Mathieu Othacehe wrote on 6 Aug 2020 15:44
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 42123@debbugs.gnu.org)
871rkjvqgf.fsf@gnu.org
Hey Ludo!

Toggle quote (5 lines)
> This can be decomposed in several steps:
>
> 1. We can start using ‘guile-zlib’ as extensions for gexps: in (guix
> scripts pack), (guix download), etc. Easy, no risk.

There's an attached patch that should cover this first step. An issue
here is that (guix build download-nar) is built in both "make" and "guix
pull" commands, so I cannot use a bare:

Toggle snippet (3 lines)
#:use-module (zlib)

so, I used:

Toggle snippet (3 lines)
#:autoload (zlib) (call-with-gzip-input-port)

that seems to work but produces a lot of warnings when running
"make". Would it be acceptable as a first step?

Toggle quote (5 lines)
>
> 2. Use guile-zlib & co. in Guix itself: (guix scripts substitute),
> (guix scripts publish), etc. Keep (guix zlib) and (guix lzlib) in
> parallel.

I'm not sure how it can work without step 4. For me, including (zlib) in
Guix itself requires that build machinery and (guix self) are updated,
but maybe I'm missing something.

Toggle quote (3 lines)
>
> 3. Update ‘guix’ package with these two new dependencies.

Seems fair!

Toggle quote (2 lines)
> The rest LGTM. Thanks a lot for all the work!

Thanks for having a look :)

Mathieu
L
L
Ludovic Courtès wrote on 23 Aug 2020 18:27
(name . Mathieu Othacehe)(address . othacehe@gnu.org)(address . 42123@debbugs.gnu.org)
87ft8dba4e.fsf@gnu.org
Hello!

Apologies for the holiday-induced delay!

Mathieu Othacehe <othacehe@gnu.org> skribis:

Toggle quote (19 lines)
>> This can be decomposed in several steps:
>>
>> 1. We can start using ‘guile-zlib’ as extensions for gexps: in (guix
>> scripts pack), (guix download), etc. Easy, no risk.
>
> There's an attached patch that should cover this first step. An issue
> here is that (guix build download-nar) is built in both "make" and "guix
> pull" commands, so I cannot use a bare:
>
> #:use-module (zlib)
>
>
> so, I used:
>
> #:autoload (zlib) (call-with-gzip-input-port)
>
> that seems to work but produces a lot of warnings when running
> "make". Would it be acceptable as a first step?

Yeah, sounds good to me.

Toggle quote (8 lines)
>> 2. Use guile-zlib & co. in Guix itself: (guix scripts substitute),
>> (guix scripts publish), etc. Keep (guix zlib) and (guix lzlib) in
>> parallel.
>
> I'm not sure how it can work without step 4. For me, including (zlib) in
> Guix itself requires that build machinery and (guix self) are updated,
> but maybe I'm missing something.

Hmm you must be right. Well in that case you can do these in lockstep.
Sorry for the confusion!

Toggle quote (14 lines)
> From 680e19137d22204f34b00336a3cb98a02397b0f9 Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <othacehe@gnu.org>
> Date: Thu, 6 Aug 2020 15:00:01 +0200
> Subject: [PATCH] Use guile-zlib extension in build-side code.
>
> * Makefile.am (MODULES): Move guix/build/download-nar.scm to ...
> (MODULES_NOT_COMPILED): ... here.
> * guix/build/download-nar.scm: Use (zlib) instead of (guix zlib).
> * guix/cvs-download.scm (cvs-fetch): Do not stub (guix config) in imported
> modules list, instead add "guile-zlib" to the extension list.
> * guix/git-download.scm (git-fetch): Ditto.
> * guix/hg-download.scm (hg-fetch): Do not stub (guix config) in imported
> modules list, instead add "guile-zlib" to the extension list.

LGTM! Glad we’re making progress on this front, thanks a lot!

Ludo’.
M
M
Mathieu Othacehe wrote on 24 Aug 2020 13:38
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 42123@debbugs.gnu.org)
871rjwe0kg.fsf@gnu.org
Hey,

Toggle quote (2 lines)
> Apologies for the holiday-induced delay!

Glad to see you back!

Toggle quote (3 lines)
> Hmm you must be right. Well in that case you can do these in lockstep.
> Sorry for the confusion!

So, I did proceed in two steps:

* Pushed the build-side patch as e9f8a7e21579fd2833dfca6830e21f886a79a9ca.
* Pushed the rest as 4c0c65acfade63ce0549115d19db4b639c1e9992.

As you suggested before, I tested the "guix pull" from 1.0.0 revision,
which seems to work fine.

Thanks,

Mathieu
L
L
Ludovic Courtès wrote on 24 Aug 2020 16:03
(name . Mathieu Othacehe)(address . othacehe@gnu.org)(address . 42123@debbugs.gnu.org)
87364c5efw.fsf@gnu.org
Hi!

Mathieu Othacehe <othacehe@gnu.org> skribis:

Toggle quote (8 lines)
> So, I did proceed in two steps:
>
> * Pushed the build-side patch as e9f8a7e21579fd2833dfca6830e21f886a79a9ca.
> * Pushed the rest as 4c0c65acfade63ce0549115d19db4b639c1e9992.
>
> As you suggested before, I tested the "guix pull" from 1.0.0 revision,
> which seems to work fine.

Awesome, thanks a lot!

Ludo’.
M
M
Mathieu Othacehe wrote on 25 Aug 2020 12:30
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 42123-done@debbugs.gnu.org)
87tuwrt3v7.fsf@gnu.org
Hey,

Toggle quote (5 lines)
>> As you suggested before, I tested the "guix pull" from 1.0.0 revision,
>> which seems to work fine.
>
> Awesome, thanks a lot!

I just pushed Linux module compression support with
755f365b02b42a5d1e8ef3000dadef069553a478 and
5fe12be0dd03d1a316343549f8c131d931f21a9a.

The build-side module (gnu build linux-modules) now uses (zlib)
module. This means that derivations that effectively rely on reading
compressed modules (such as "linux-modules"), need to add "guile-zlib"
as an extension.

Thanks,

Mathieu
Closed
?