[PATCH 0/5] Add new image API.

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Mathieu Othacehe
  • Tobias Geerinckx-Rice
Owner
unassigned
Submitted by
Mathieu Othacehe
Severity
normal
M
M
Mathieu Othacehe wrote on 29 Apr 2020 10:38
(address . guix-patches@gnu.org)(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
20200429083814.23768-1-m.othacehe@gmail.com
Hello,

This has been discussed previously on the mailing-list and can be seen on the
wip-disk-image branch.

It introduces a new (gnu image) module. This module makes image creation more
modular and extensible.

Creation of raw disk-images and ISO9660 images is now handled in (gnu system
image), and based on the new image API. This does not involve the use of
virtual machines anymore.

Some bootloaders are not yet supported (MBR based grub, extlinux,
u-boot). Image creation with those bootloaders, fallback to original VM based
process.

See:

Thanks,

Mathieu

Mathieu Othacehe (5):
build: store-copy: Export file-size procedure.
build: install: Ignore chown exceptions.
build: bootloader: Add install-efi procedure.
image: Add a new API.
vm: Remove obsolete procedures.

gnu/build/bootloader.scm | 55 +++-
gnu/build/image.scm | 275 ++++++++++++++++++++
gnu/build/install.scm | 4 +-
gnu/build/vm.scm | 175 +------------
gnu/ci.scm | 45 ++--
gnu/image.scm | 77 ++++++
gnu/local.mk | 3 +
gnu/system/image.scm | 514 ++++++++++++++++++++++++++++++++++++++
gnu/system/vm.scm | 168 ++-----------
gnu/tests/install.scm | 22 +-
guix/build/store-copy.scm | 1 +
guix/scripts/system.scm | 13 +-
12 files changed, 996 insertions(+), 356 deletions(-)
create mode 100644 gnu/build/image.scm
create mode 100644 gnu/image.scm
create mode 100644 gnu/system/image.scm

--
2.26.0
M
M
Mathieu Othacehe wrote on 29 Apr 2020 10:47
[PATCH 1/5] build: store-copy: Export file-size procedure.
(address . 40955@debbugs.gnu.org)(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
20200429084756.25072-1-m.othacehe@gmail.com
* guix/build/store-copy.scm (file-size): Export it.
---
guix/build/store-copy.scm | 1 +
1 file changed, 1 insertion(+)

Toggle diff (14 lines)
diff --git a/guix/build/store-copy.scm b/guix/build/store-copy.scm
index 549aa4f28b..ad551bca98 100644
--- a/guix/build/store-copy.scm
+++ b/guix/build/store-copy.scm
@@ -35,6 +35,7 @@
read-reference-graph
+ file-size
closure-size
populate-store))
--
2.26.0
M
M
Mathieu Othacehe wrote on 29 Apr 2020 10:47
[PATCH 2/5] build: install: Ignore chown exceptions.
(address . 40955@debbugs.gnu.org)(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
20200429084756.25072-2-m.othacehe@gmail.com
Changing ownership may require root permissions. As image can now be generated
without root permissions (no VM involved), ignore those exceptions.

* gnu/build/install.scm (evaluate-populate-directive): Ignore chown
exceptions.
---
gnu/build/install.scm | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

Toggle diff (17 lines)
diff --git a/gnu/build/install.scm b/gnu/build/install.scm
index c0d4d44091..0b0d01cf86 100644
--- a/gnu/build/install.scm
+++ b/gnu/build/install.scm
@@ -63,7 +63,9 @@ directory TARGET."
(('directory name uid gid)
(let ((dir (string-append target name)))
(mkdir-p dir)
- (chown dir uid gid)))
+ ;; This will fail if this is not run from a VM, ignore those
+ ;; errors.
+ (false-if-exception (chown dir uid gid))))
(('directory name uid gid mode)
(loop `(directory ,name ,uid ,gid))
(chmod (string-append target name) mode))
--
2.26.0
M
M
Mathieu Othacehe wrote on 29 Apr 2020 10:47
[PATCH 3/5] build: bootloader: Add install-efi procedure.
(address . 40955@debbugs.gnu.org)(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
20200429084756.25072-3-m.othacehe@gmail.com
* gnu/build/bootloader.scm (install-efi-loader): New exported procedure. This
is based on install-efi from (gnu build vm) module.
* gnu/build/vm.scm (initialize-hard-disk): Adapt to use install-efi-loader.
---
gnu/build/bootloader.scm | 55 +++++++++++++++++++++++++++++++++++++++-
gnu/build/vm.scm | 19 +++-----------
2 files changed, 57 insertions(+), 17 deletions(-)

Toggle diff (118 lines)
diff --git a/gnu/build/bootloader.scm b/gnu/build/bootloader.scm
index 9570d6dd18..e15e7c0828 100644
--- a/gnu/build/bootloader.scm
+++ b/gnu/build/bootloader.scm
@@ -18,8 +18,12 @@
;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>.
(define-module (gnu build bootloader)
+ #:use-module (guix build utils)
+ #:use-module (guix utils)
#:use-module (ice-9 binary-ports)
- #:export (write-file-on-device))
+ #:use-module (ice-9 format)
+ #:export (write-file-on-device
+ install-efi-loader))
;;;
@@ -36,3 +40,52 @@
(seek output offset SEEK_SET)
(put-bytevector output bv))
#:binary #t)))))
+
+
+;;;
+;;; EFI bootloader.
+;;;
+
+(define (install-efi grub grub-config esp)
+ "Write a self-contained GRUB EFI loader to the mounted ESP using GRUB-CONFIG."
+ (let* ((system %host-type)
+ ;; Hard code the output location to a well-known path recognized by
+ ;; compliant firmware. See "3.5.1.1 Removable Media Boot Behaviour":
+ ;; http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_6.pdf
+ (grub-mkstandalone (string-append grub "/bin/grub-mkstandalone"))
+ (efi-directory (string-append esp "/EFI/BOOT"))
+ ;; Map grub target names to boot file names.
+ (efi-targets (cond ((string-prefix? "x86_64" system)
+ '("x86_64-efi" . "BOOTX64.EFI"))
+ ((string-prefix? "i686" system)
+ '("i386-efi" . "BOOTIA32.EFI"))
+ ((string-prefix? "armhf" system)
+ '("arm-efi" . "BOOTARM.EFI"))
+ ((string-prefix? "aarch64" system)
+ '("arm64-efi" . "BOOTAA64.EFI")))))
+ ;; grub-mkstandalone requires a TMPDIR to prepare the firmware image.
+ (setenv "TMPDIR" esp)
+
+ (mkdir-p efi-directory)
+ (invoke grub-mkstandalone "-O" (car efi-targets)
+ "-o" (string-append efi-directory "/"
+ (cdr efi-targets))
+ ;; Graft the configuration file onto the image.
+ (string-append "boot/grub/grub.cfg=" grub-config))))
+
+(define (install-efi-loader grub-efi esp)
+ ;; Create a tiny configuration file telling the embedded grub
+ ;; where to load the real thing.
+ ;; XXX This is quite fragile, and can prevent the image from booting
+ ;; when there's more than one volume with this label present.
+ ;; Reproducible almost-UUIDs could reduce the risk (not eliminate it).
+ (let ((grub-config "grub.cfg"))
+ (call-with-output-file grub-config
+ (lambda (port)
+ (format port
+ "insmod part_msdos~@
+ search --set=root --label Guix_image~@
+ configfile /boot/grub/grub.cfg~%")
+ (fsync port)))
+ (install-efi grub-efi grub-config esp)
+ (delete-file grub-config)))
diff --git a/gnu/build/vm.scm b/gnu/build/vm.scm
index 9caa110463..bc6071daa9 100644
--- a/gnu/build/vm.scm
+++ b/gnu/build/vm.scm
@@ -27,6 +27,7 @@
#:use-module (guix build store-copy)
#:use-module (guix build syscalls)
#:use-module (guix store database)
+ #:use-module (gnu build bootloader)
#:use-module (gnu build linux-boot)
#:use-module (gnu build install)
#:use-module (gnu system uuid)
@@ -610,30 +611,16 @@ passing it a directory name where it is mounted."
(when esp
;; Mount the ESP somewhere and install GRUB UEFI image.
- (let ((mount-point (string-append target "/boot/efi"))
- (grub-config (string-append target "/tmp/grub-standalone.cfg")))
+ (let ((mount-point (string-append target "/boot/efi")))
(display "mounting EFI system partition...\n")
(mkdir-p mount-point)
(mount (partition-device esp) mount-point
(partition-file-system esp))
- ;; Create a tiny configuration file telling the embedded grub
- ;; where to load the real thing.
- ;; XXX This is quite fragile, and can prevent the image from booting
- ;; when there's more than one volume with this label present.
- ;; Reproducible almost-UUIDs could reduce the risk (not eliminate it).
- (call-with-output-file grub-config
- (lambda (port)
- (format port
- "insmod part_msdos~@
- search --set=root --label Guix_image~@
- configfile /boot/grub/grub.cfg~%")))
-
(display "creating EFI firmware image...")
- (install-efi grub-efi mount-point grub-config)
+ (install-efi-loader grub-efi mount-point)
(display "done.\n")
- (delete-file grub-config)
(umount mount-point)))
;; Register BOOTCFG as a GC root.
--
2.26.0
M
M
Mathieu Othacehe wrote on 29 Apr 2020 10:47
[PATCH 4/5] image: Add a new API.
(address . 40955@debbugs.gnu.org)(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
20200429084756.25072-4-m.othacehe@gmail.com
Raw disk-images and ISO9660 images are created in a Qemu virtual machine. This
is quite fragile, very slow, and almost unusable without KVM.

For all these reasons, add support for host image generation. This implies the
use new image generation mechanisms.

- Raw disk images: images of partitions are created using tools such as mke2fs
and mkdosfs depending on the partition file-system type. The partition
images are then assembled into a final image using genimage.

- ISO9660 images: the ISO root directory is populated within the store. GNU
xorriso is then called on that directory, in the exact same way as this is
done in (gnu build vm) module.

Those mechanisms are built upon the new (gnu image) module.

* gnu/image.scm: New file.
* gnu/system/image.scm: New file.
* gnu/build/image: New file.
* gnu/local.mk: Add them.
* gnu/system/vm.scm (system-disk-image): Rename to system-disk-image-in-vm.
* gnu/ci.scm (qemu-jobs): Adapt to new API.
* gnu/tests/install.scm (run-install): Ditto.
* guix/scripts/system.scm (system-derivation-for-action): Ditto.
---
gnu/build/image.scm | 275 +++++++++++++++++++++
gnu/ci.scm | 45 ++--
gnu/image.scm | 77 ++++++
gnu/local.mk | 3 +
gnu/system/image.scm | 514 ++++++++++++++++++++++++++++++++++++++++
gnu/system/vm.scm | 17 +-
gnu/tests/install.scm | 22 +-
guix/scripts/system.scm | 13 +-
8 files changed, 917 insertions(+), 49 deletions(-)
create mode 100644 gnu/build/image.scm
create mode 100644 gnu/image.scm
create mode 100644 gnu/system/image.scm

Toggle diff (458 lines)
diff --git a/gnu/build/image.scm b/gnu/build/image.scm
new file mode 100644
index 0000000000..7c47bda344
--- /dev/null
+++ b/gnu/build/image.scm
@@ -0,0 +1,275 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2020 Mathieu Othacehe <m.othacehe@gmail.com>
+;;;
+;;; This file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>.
+
+(define-module (gnu build image)
+ #:use-module (guix build store-copy)
+ #:use-module (guix build syscalls)
+ #:use-module (guix build utils)
+ #:use-module (guix store database)
+ #:use-module (gnu build bootloader)
+ #:use-module (gnu build install)
+ #:use-module (gnu build linux-boot)
+ #:use-module (gnu image)
+ #:use-module (gnu system uuid)
+ #:use-module (ice-9 ftw)
+ #:use-module (ice-9 match)
+ #:use-module (srfi srfi-19)
+ #:use-module (srfi srfi-34)
+ #:use-module (srfi srfi-35)
+ #:export (make-partition-image
+ genimage
+ initialize-efi-partition
+ initialize-root-partition
+
+ make-iso9660-image))
+
+(define (sexp->partition sexp)
+ "Take SEXP, a tuple as returned by 'partition->gexp', and turn it into a
+<partition> record."
+ (match sexp
+ ((size file-system label uuid)
+ (partition (size size)
+ (file-system file-system)
+ (label label)
+ (uuid uuid)))))
+
+(define (size-in-kib size)
+ "Convert SIZE expressed in bytes, to kilobytes and return it as a string."
+ (number->string
+ (inexact->exact (ceiling (/ size 1024)))))
+
+(define (root-size root)
+ "Given the ROOT directory, evalute and return its size. As this doesn't take
+the partition metadata size into account, take a 25% margin."
+ (* 1.25 (file-size root)))
+
+(define* (make-ext4-image partition target root
+ #:key (owner 0))
+ "Handle the creation of EXT4 partition images. See 'make-partition-image'."
+ (let ((size (partition-size partition))
+ (label (partition-label partition))
+ (uuid (partition-uuid partition))
+ (options "lazy_itable_init=1,lazy_journal_init=1"))
+ (invoke "mke2fs" "-t" "ext4" "-d" root
+ "-L" label "-U" (uuid->string uuid)
+ "-E" (format #f "root_owner=~a:~a,~a"
+ owner owner options)
+ target
+ (format #f "~ak"
+ (size-in-kib
+ (if (eq? size 'guess)
+ (root-size root)
+ size))))))
+
+(define* (make-vfat-image partition target root)
+ "Handle the creation of VFAT partition images. See 'make-partition-image'."
+ (let ((size (partition-size partition))
+ (label (partition-label partition)))
+ (invoke "mkdosfs" "-n" label "-C" target "-F" "16" "-S" "1024"
+ (size-in-kib
+ (if (eq? size 'guess)
+ (root-size root)
+ size)))
+ (for-each (lambda (file)
+ (unless (member file '("." ".."))
+ (invoke "mcopy" "-bsp" "-i" target
+ (string-append root "/" file)
+ (string-append "::" file))))
+ (scandir root))))
+
+(define* (make-partition-image partition-sexp target root)
+ "Create and return the image of PARTITION-SEXP as TARGET. Use the given ROOT
+directory to populate the image."
+ (let* ((partition (sexp->partition partition-sexp))
+ (type (partition-file-system partition)))
+ (cond
+ ((string=? type "ext4")
+ (make-ext4-image partition target root))
+ ((string=? type "vfat")
+ (make-vfat-image partition target root))
+ (else
+ (format (current-error-port)
+ "Unsupported partition type~%.")))))
+
+(define* (genimage config target)
+ "Use genimage to generate in TARGET directory, the image described in the
+given CONFIG file."
+ ;; genimage needs a 'root' directory.
+ (mkdir "root")
+ (invoke "genimage" "--config" config
+ "--outputpath" target))
+
+(define* (initialize-efi-partition root
+ #:key
+ bootloader-package
+ #:allow-other-keys)
+ "Install in ROOT directory, an EFI loader using BOOTLOADER-PACKAGE."
+ (install-efi-loader bootloader-package root))
+
+(define (register-bootcfg-root target bootcfg)
+ "On file system TARGET, register BOOTCFG as a GC root."
+ (let ((directory (string-append target "/var/guix/gcroots")))
+ (mkdir-p directory)
+ (symlink bootcfg (string-append directory "/bootcfg"))))
+
+(define* (register-closure prefix closure
+ #:key
+ (deduplicate? #t) (reset-timestamps? #t)
+ (schema (sql-schema)))
+ "Register CLOSURE in PREFIX, where PREFIX is the directory name of the
+target store and CLOSURE is the name of a file containing a reference graph as
+produced by #:references-graphs.. As a side effect, if RESET-TIMESTAMPS? is
+true, reset timestamps on store files and, if DEDUPLICATE? is true,
+deduplicates files common to CLOSURE and the rest of PREFIX."
+ (let ((items (call-with-input-file closure read-reference-graph)))
+ (register-items items
+ #:prefix prefix
+ #:deduplicate? deduplicate?
+ #:reset-timestamps? reset-timestamps?
+ #:registration-time %epoch
+ #:schema schema)))
+
+(define* (initialize-root-partition root
+ #:key
+ bootcfg
+ bootcfg-location
+ (deduplicate? #t)
+ references-graphs
+ (register-closures? #t)
+ system-directory
+ #:allow-other-keys)
+ "Initialize the given ROOT directory. Use BOOTCFG and BOOTCFG-LOCATION to
+install the bootloader configuration.
+
+If REGISTER-CLOSURES? is true, register REFERENCES-GRAPHS in the store. If
+DEDUPLICATE? is true, then also deduplicate files common to CLOSURES and the
+rest of the store when registering the closures. SYSTEM-DIRECTORY is the name
+of the directory of the 'system' derivation."
+ (populate-root-file-system system-directory root)
+ (populate-store references-graphs root)
+
+ (when register-closures?
+ (for-each (lambda (closure)
+ (register-closure root
+ closure
+ #:reset-timestamps? #t
+ #:deduplicate? deduplicate?))
+ references-graphs))
+
+ (when bootcfg
+ (install-boot-config bootcfg bootcfg-location root)
+
+ ;; Register BOOTCFG as a GC root.
+ (register-bootcfg-root root bootcfg)))
+
+(define* (make-iso9660-image xorriso grub-mkrescue-environment
+ grub bootcfg system-directory root target
+ #:key (volume-id "Guix_image") (volume-uuid #f)
+ register-closures? (references-graphs '())
+ (compression? #t))
+ "Given a GRUB package, creates an iso image as TARGET, using BOOTCFG as
+GRUB configuration and OS-DRV as the stuff in it."
+ (define grub-mkrescue
+ (string-append grub "/bin/grub-mkrescue"))
+
+ (define grub-mkrescue-sed.sh
+ (string-append (getcwd) "/" "grub-mkrescue-sed.sh"))
+
+ ;; Use a modified version of grub-mkrescue-sed.sh, see below.
+ (copy-file (string-append xorriso
+ "/bin/grub-mkrescue-sed.sh")
+ grub-mkrescue-sed.sh)
+
+ ;; Force grub-mkrescue-sed.sh to use the build directory instead of /tmp
+ ;; that is read-only inside the build container.
+ (substitute* grub-mkrescue-sed.sh
+ (("/tmp/") (string-append (getcwd) "/"))
+ (("MKRESCUE_SED_XORRISO_ARGS \\$x")
+ (format #f "MKRESCUE_SED_XORRISO_ARGS $(echo $x | sed \"s|/tmp|~a|\")"
+ (getcwd))))
+
+ ;; 'grub-mkrescue' calls out to mtools programs to create 'efi.img', a FAT
+ ;; file system image, and mtools honors SOURCE_DATE_EPOCH for the mtime of
+ ;; those files. The epoch for FAT is Jan. 1st 1980, not 1970, so choose
+ ;; that.
+ (setenv "SOURCE_DATE_EPOCH"
+ (number->string
+ (time-second
+ (date->time-utc (make-date 0 0 0 0 1 1 1980 0)))))
+
+ ;; Our patched 'grub-mkrescue' honors this environment variable and passes
+ ;; it to 'mformat', which makes it the serial number of 'efi.img'. This
+ ;; allows for deterministic builds.
+ (setenv "GRUB_FAT_SERIAL_NUMBER"
+ (number->string (if volume-uuid
+
+ ;; On 32-bit systems the 2nd argument must be
+ ;; lower than 2^32.
+ (string-hash (iso9660-uuid->string volume-uuid)
+ (- (expt 2 32) 1))
+
+ #x77777777)
+ 16))
+
+ (setenv "MKRESCUE_SED_MODE" "original")
+ (setenv "MKRESCUE_SED_XORRISO" (string-append xorriso "/bin/xorriso"))
+ (setenv "MKRESCUE_SED_IN_EFI_NO_PT" "yes")
+
+ (for-each (match-lambda
+ ((name . value) (setenv name value)))
+ grub-mkrescue-environment)
+
+ (apply invoke grub-mkrescue
+ (string-append "--xorriso=" grub-mkrescue-sed.sh)
+ "-o" target
+ (string-append "boot/grub/grub.cfg=" bootcfg)
+ root
+ "--"
+ ;; Set all timestamps to 1.
+ "-volume_date" "all_file_dates" "=1"
+
+ `(,@(if compression?
+ '(;; ‘zisofs’ compression reduces the total image size by
+ ;; ~60%.
+ "-zisofs" "level=9:block_size=128k" ; highest compression
+ ;; It's transparent to our Linux-Libre kernel but not to
+ ;; GRUB. Don't compress the kernel, initrd, and other
+ ;; files read by grub.cfg, as well as common
+ ;; already-compressed file names.
+ "-find" "/" "-type" "f"
+ ;; XXX Even after "--" above, and despite documentation
+ ;; claiming otherwise, "-or" is stolen by grub-mkrescue
+ ;; which then chokes on it (as ‘-o …’) and dies. Don't use
+ ;; "-or".
+ "-not" "-wholename" "/boot/*"
+ "-not" "-wholename" "/System/*"
+ "-not" "-name" "unicode.pf2"
+ "-not" "-name" "bzImage"
+ "-not" "-name" "*.gz" ; initrd & all man pages
+ "-not" "-name" "*.png" ; includes grub-image.png
+ "-exec" "set_filter" "--zisofs"
+ "--")
+ '())
+ "-volid" ,(string-upcase volume-id)
+ ,@(if volume-uuid
+ `("-volume_date" "uuid"
+ ,(string-filter (lambda (value)
+ (not (char=? #\- value)))
+ (iso9660-uuid->string
+ volume-uuid)))
+ '()))))
diff --git a/gnu/ci.scm b/gnu/ci.scm
index fb2596c809..0430cf594b 100644
--- a/gnu/ci.scm
+++ b/gnu/ci.scm
@@ -38,6 +38,7 @@
#:select (lookup-compressor self-contained-tarball))
#:use-module (gnu bootloader)
#:use-module (gnu bootloader u-boot)
+ #:use-module (gnu image)
#:use-module (gnu packages)
#:use-module (gnu packages gcc)
#:use-module (gnu packages base)
@@ -49,6 +50,7 @@
#:use-module (gnu packages make-bootstrap)
#:use-module (gnu packages package-management)
#:use-module (gnu system)
+ #:use-module (gnu system image)
#:use-module (gnu system vm)
#:use-module (gnu system install)
#:use-module (gnu tests)
@@ -209,32 +211,23 @@ system.")
(expt 2 20))
(if (member system %guixsd-supported-systems)
- (if (member system %u-boot-systems)
- (list (->job 'flash-image
- (run-with-store store
- (mbegin %store-monad
- (set-guile-for-build (default-guile))
- (system-disk-image
- (operating-system (inherit installation-os)
- (bootloader (bootloader-configuration
- (bootloader u-boot-bootloader)
- (target #f))))
- #:disk-image-size
- (* 1500 MiB))))))
- (list (->job 'usb-image
- (run-with-store store
- (mbegin %store-monad
- (set-guile-for-build (default-guile))
- (system-disk-image installation-os
- #:disk-image-size
- (* 1500 MiB)))))
- (->job 'iso9660-image
- (run-with-store store
- (mbegin %store-monad
- (set-guile-for-build (default-guile))
- (system-disk-image installation-os
- #:file-system-type
- "iso9660"))))))
+ (list (->job 'usb-image
+ (run-with-store store
+ (mbegin %store-monad
+ (set-guile-for-build (default-guile))
+ (system-image
+ (image
+ (inherit efi-disk-image)
+ (size (* 1500 MiB))
+ (operating-system installation-os))))))
+ (->job 'iso9660-image
+ (run-with-store store
+ (mbegin %store-monad
+ (set-guile-for-build (default-guile))
+ (system-image
+ (image
+ (inherit iso9660-image)
+ (operating-system installation-os)))))))
'()))
(define channel-build-system
diff --git a/gnu/image.scm b/gnu/image.scm
new file mode 100644
index 0000000000..e250741c98
--- /dev/null
+++ b/gnu/image.scm
@@ -0,0 +1,77 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2020 Mathieu Othacehe <m.othacehe@gmail.com>
+;;;
+;;; This file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>.
+
+(define-module (gnu image)
+ #:use-module (guix records)
+ #:use-module (ice-9 match)
+ #:export (partition
+ partition?
+ partition-device
+ partition-size
+ partition-file-system
+ partition-label
+ partition-uuid
+ partition-flags
+ partition-initializer
+
+ image
+ image-name
+ image-format
+ image-size
+ image-operating-system
+ image-partitions
+ image-compression?
+ image-volatile-root?
+ image-substitutable?))
+
+
+;;;
+;;; Partition record.
+;;;
+
+(define-record-type* <partition> partition make-partition
+ partition?
+ (device partition-device (default #f))
+ (size partition-size)
+ (file-system partition-file-system (default "ext4"))
+ (label partition-label (default #f))
+ (uuid partition-uuid (default #f))
+ (flags partition-flags (default '()))
+ (initializer partition-initializer (default #f)))
+
+
+;;;
+;;; Image record.
+;;;
+
+(define-record-type* <image>
+ image make-image
+ image?
+ (format image-format) ;symbol
+ (size image-size ;size in bytes as integer
+ (default 'guess))
+ (operating-system image-operating-system ;<operating-system>
+ (default #f))
+ (partitions image-partitions ;list of <partition>
+ (default '()))
+ (compression? image-compression? ;boolean
+ (default #t))
+ (volatile-root? image-volatile-root? ;boolean
+ (default #t))
+ (substitutable? image-substitutable? ;boolean
+ (default #t)))
diff --git a/gnu/local.mk b/gnu/local.mk
index 9f212434a9..408063a36e 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -61,6 +61,7 @@ GNU_SYSTEM_MODULES = \
%D%/bootloader/u-boot.scm \
%D%/bootloader/depthcharge.scm \
%D%/ci.scm \
+ %D%/image.scm \
%D%/packages.scm \
%D%/packages/abduco.scm \
%D%/packages/abiword.scm \
@@ -602,6 +603,7 @@ GNU_SYSTEM_MODULES = \
%D%/system.scm \
%D%/system/accounts.scm \
%D%/system/file-systems.scm \
+ %D%/system/image.scm \
%D%/system/install.scm \
%D%/system/keyboard.scm \
%D%/system/linux-container.scm \
@@ -622,6 +624,7 @@ GNU_SYSTEM_MODULES = \
%D%/build/activation.scm \
%D%/build/bootloader.scm \
%D%
This message was truncated. Download the full message here.
M
M
Mathieu Othacehe wrote on 29 Apr 2020 10:47
[PATCH 5/5] vm: Remove obsolete procedures.
(address . 40955@debbugs.gnu.org)(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
20200429084756.25072-5-m.othacehe@gmail.com
* gnu/build/vm.scm (install-efi, make-iso9660-image): Remove those procedures
that are now implemented in (gnu build image) module,
(initialize-hard-disk): remove efi support.
* gnu/system/vm.scm (iso9660-image): Remove it,
(qemu-image): adapt it to remove ISO9660 support.
---
gnu/build/vm.scm | 156 +---------------------------------------------
gnu/system/vm.scm | 151 +++++---------------------------------------
2 files changed, 18 insertions(+), 289 deletions(-)

Toggle diff (365 lines)
diff --git a/gnu/build/vm.scm b/gnu/build/vm.scm
index bc6071daa9..1a888b1a51 100644
--- a/gnu/build/vm.scm
+++ b/gnu/build/vm.scm
@@ -57,8 +57,7 @@
estimated-partition-size
root-partition-initializer
initialize-partition-table
- initialize-hard-disk
- make-iso9660-image))
+ initialize-hard-disk))
;;; Commentary:
;;;
@@ -417,159 +416,6 @@ SYSTEM-DIRECTORY is the name of the directory of the 'system' derivation."
(mkdir-p directory)
(symlink bootcfg (string-append directory "/bootcfg"))))
-(define (install-efi grub esp config-file)
- "Write a self-contained GRUB EFI loader to the mounted ESP using CONFIG-FILE."
- (let* ((system %host-type)
- ;; Hard code the output location to a well-known path recognized by
- ;; compliant firmware. See "3.5.1.1 Removable Media Boot Behaviour":
- ;; http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_6.pdf
- (grub-mkstandalone (string-append grub "/bin/grub-mkstandalone"))
- (efi-directory (string-append esp "/EFI/BOOT"))
- ;; Map grub target names to boot file names.
- (efi-targets (cond ((string-prefix? "x86_64" system)
- '("x86_64-efi" . "BOOTX64.EFI"))
- ((string-prefix? "i686" system)
- '("i386-efi" . "BOOTIA32.EFI"))
- ((string-prefix? "armhf" system)
- '("arm-efi" . "BOOTARM.EFI"))
- ((string-prefix? "aarch64" system)
- '("arm64-efi" . "BOOTAA64.EFI")))))
- ;; grub-mkstandalone requires a TMPDIR to prepare the firmware image.
- (setenv "TMPDIR" esp)
-
- (mkdir-p efi-directory)
- (invoke grub-mkstandalone "-O" (car efi-targets)
- "-o" (string-append efi-directory "/"
- (cdr efi-targets))
- ;; Graft the configuration file onto the image.
- (string-append "boot/grub/grub.cfg=" config-file))))
-
-(define* (make-iso9660-image xorriso grub-mkrescue-environment
- grub config-file os-drv target
- #:key (volume-id "Guix_image") (volume-uuid #f)
- register-closures? (closures '()))
- "Given a GRUB package, creates an iso image as TARGET, using CONFIG-FILE as
-GRUB configuration and OS-DRV as the stuff in it."
- (define grub-mkrescue
- (string-append grub "/bin/grub-mkrescue"))
-
- (define grub-mkrescue-sed.sh
- (string-append xorriso "/bin/grub-mkrescue-sed.sh"))
-
- (define target-store
- (string-append "/tmp/root" (%store-directory)))
-
- (define items
- ;; The store items to add to the image.
- (delete-duplicates
- (append-map (lambda (closure)
- (map store-info-item
- (call-with-input-file (string-append "/xchg/" closure)
- read-reference-graph)))
- closures)))
-
- (populate-root-file-system os-drv "/tmp/root")
- (mount (%store-directory) target-store "" MS_BIND)
-
- (when register-closures?
- (display "registering closures...\n")
- (for-each (lambda (closure)
- (register-closure
- "/tmp/root"
- (string-append "/xchg/" closure)
-
- ;; TARGET-STORE is a read-only bind-mount so we shouldn't try
- ;; to modify it.
- #:deduplicate? #f
- #:reset-timestamps? #f))
- closures)
- (register-bootcfg-root "/tmp/root" config-file))
-
- ;; 'grub-mkrescue' calls out to mtools programs to create 'efi.img', a FAT
- ;; file system image, and mtools honors SOURCE_DATE_EPOCH for the mtime of
- ;; those files. The epoch for FAT is Jan. 1st 1980, not 1970, so choose
- ;; that.
- (setenv "SOURCE_DATE_EPOCH"
- (number->string
- (time-second
- (date->time-utc (make-date 0 0 0 0 1 1 1980 0)))))
-
- ;; Our patched 'grub-mkrescue' honors this environment variable and passes
- ;; it to 'mformat', which makes it the serial number of 'efi.img'. This
- ;; allows for deterministic builds.
- (setenv "GRUB_FAT_SERIAL_NUMBER"
- (number->string (if volume-uuid
-
- ;; On 32-bit systems the 2nd argument must be
- ;; lower than 2^32.
- (string-hash (iso9660-uuid->string volume-uuid)
- (- (expt 2 32) 1))
-
- #x77777777)
- 16))
-
- (setenv "MKRESCUE_SED_MODE" "original")
- (setenv "MKRESCUE_SED_XORRISO" (string-append xorriso
- "/bin/xorriso"))
- (setenv "MKRESCUE_SED_IN_EFI_NO_PT" "yes")
- (for-each (match-lambda
- ((name . value) (setenv name value)))
- grub-mkrescue-environment)
-
- (let ((pipe
- (apply open-pipe* OPEN_WRITE
- grub-mkrescue
- (string-append "--xorriso=" grub-mkrescue-sed.sh)
- "-o" target
- (string-append "boot/grub/grub.cfg=" config-file)
- "etc=/tmp/root/etc"
- "var=/tmp/root/var"
- "run=/tmp/root/run"
- ;; /mnt is used as part of the installation
- ;; process, as the mount point for the target
- ;; file system, so create it.
- "mnt=/tmp/root/mnt"
- "-path-list" "-"
- "--"
-
- ;; Set all timestamps to 1.
- "-volume_date" "all_file_dates" "=1"
-
- ;; ‘zisofs’ compression reduces the total image size by ~60%.
- "-zisofs" "level=9:block_size=128k" ; highest compression
- ;; It's transparent to our Linux-Libre kernel but not to GRUB.
- ;; Don't compress the kernel, initrd, and other files read by
- ;; grub.cfg, as well as common already-compressed file names.
- "-find" "/" "-type" "f"
- ;; XXX Even after "--" above, and despite documentation claiming
- ;; otherwise, "-or" is stolen by grub-mkrescue which then chokes
- ;; on it (as ‘-o …’) and dies. Don't use "-or".
- "-not" "-wholename" "/boot/*"
- "-not" "-wholename" "/System/*"
- "-not" "-name" "unicode.pf2"
- "-not" "-name" "bzImage"
- "-not" "-name" "*.gz" ; initrd & all man pages
- "-not" "-name" "*.png" ; includes grub-image.png
- "-exec" "set_filter" "--zisofs"
- "--"
-
- "-volid" (string-upcase volume-id)
- (if volume-uuid
- `("-volume_date" "uuid"
- ,(string-filter (lambda (value)
- (not (char=? #\- value)))
- (iso9660-uuid->string
- volume-uuid)))
- `()))))
- ;; Pass lines like 'gnu/store/…-x=/gnu/store/…-x' corresponding to the
- ;; '-path-list -' option.
- (for-each (lambda (item)
- (format pipe "~a=~a~%"
- (string-drop item 1) item))
- items)
- (unless (zero? (close-pipe pipe))
- (error "oh, my! grub-mkrescue failed" grub-mkrescue))))
-
(define* (initialize-hard-disk device
#:key
bootloader-package
diff --git a/gnu/system/vm.scm b/gnu/system/vm.scm
index 37840ce355..1cab8997b4 100644
--- a/gnu/system/vm.scm
+++ b/gnu/system/vm.scm
@@ -269,95 +269,6 @@ substitutable."
(eq? (service-kind service) guix-service-type))
(operating-system-services os)))))
-(define* (iso9660-image #:key
- (name "iso9660-image")
- file-system-label
- file-system-uuid
- (system (%current-system))
- (target (%current-target-system))
- (qemu qemu-minimal)
- os
- bootcfg-drv
- bootloader
- (register-closures? (has-guix-service-type? os))
- (inputs '())
- (grub-mkrescue-environment '())
- (substitutable? #t))
- "Return a bootable, stand-alone iso9660 image.
-
-INPUTS is a list of inputs (as for packages)."
- (define schema
- (and register-closures?
- (local-file (search-path %load-path
- "guix/store/schema.sql"))))
-
- (expression->derivation-in-linux-vm
- name
- (with-extensions gcrypt-sqlite3&co
- (with-imported-modules `(,@(source-module-closure '((gnu build vm)
- (guix store database)
- (guix build utils))
- #:select? not-config?)
- ((guix config) => ,(make-config.scm)))
- #~(begin
- (use-modules (gnu build vm)
- (guix store database)
- (guix build utils))
-
- (sql-schema #$schema)
-
- ;; Allow non-ASCII file names--e.g., 'nss-certs'--to be decoded.
- (setenv "GUIX_LOCPATH"
- #+(file-append glibc-utf8-locales "/lib/locale"))
- (setlocale LC_ALL "en_US.utf8")
-
- (let ((inputs
- '#$(append (list parted e2fsprogs dosfstools xorriso)
- (map canonical-package
- (list sed grep coreutils findutils gawk))))
-
-
- (graphs '#$(match inputs
- (((names . _) ...)
- names)))
- ;; This variable is unused but allows us to add INPUTS-TO-COPY
- ;; as inputs.
- (to-register
- '#$(map (match-lambda
- ((name thing) thing)
- ((name thing output) `(,thing ,output)))
- inputs)))
-
- (set-path-environment-variable "PATH" '("bin" "sbin") inputs)
- (make-iso9660-image #$xorriso
- '#$grub-mkrescue-environment
- #$(bootloader-package bootloader)
- #$bootcfg-drv
- #$os
- "/xchg/guixsd.iso"
- #:register-closures? #$register-closures?
- #:closures graphs
- #:volume-id #$file-system-label
- #:volume-uuid #$(and=> file-system-uuid
- uuid-bytevector))))))
- #:system system
- #:target target
-
- ;; Keep a local file system for /tmp so that we can populate it directly as
- ;; root and have files owned by root. See <https://bugs.gnu.org/31752>.
- #:file-systems (remove (lambda (file-system)
- (string=? (file-system-mount-point file-system)
- "/tmp"))
- %linux-vm-file-systems)
-
- #:make-disk-image? #f
- #:single-file-output? #t
- #:references-graphs inputs
- #:substitutable? substitutable?
-
- ;; Xorriso seems to be quite memory-hungry, so increase the VM's RAM size.
- #:memory-size 512))
-
(define* (qemu-image #:key
(name "qemu-image")
(system (%current-system))
@@ -618,25 +529,14 @@ to USB sticks meant to be read-only.
SUBSTITUTABLE? determines whether the returned derivation should be marked as
substitutable."
- (define normalize-label
- ;; ISO labels are all-caps (case-insensitive), but since
- ;; 'find-partition-by-label' is case-sensitive, make it all-caps here.
- (if (string=? "iso9660" file-system-type)
- string-upcase
- identity))
-
(define root-label
- ;; Volume name of the root file system.
- (normalize-label "Guix_image"))
+ "Guix_image")
(define (root-uuid os)
;; UUID of the root file system, computed in a deterministic fashion.
;; This is what we use to locate the root file system so it has to be
;; different from the user's own file system UUIDs.
- (operating-system-uuid os
- (if (string=? file-system-type "iso9660")
- 'iso9660
- 'dce)))
+ (operating-system-uuid os 'dce))
(define file-systems-to-keep
(remove (lambda (fs)
@@ -653,11 +553,7 @@ substitutable."
#:volatile-root? volatile?
rest)))
- (bootloader (if (string=? "iso9660" file-system-type)
- (bootloader-configuration
- (inherit (operating-system-bootloader os))
- (bootloader grub-mkrescue-bootloader))
- (operating-system-bootloader os)))
+ (bootloader (operating-system-bootloader os))
;; Force our own root file system. (We need a "/" file system
;; to call 'root-uuid'.)
@@ -675,33 +571,20 @@ substitutable."
(type file-system-type))
file-systems-to-keep))))
(bootcfg (operating-system-bootcfg os)))
- (if (string=? "iso9660" file-system-type)
- (iso9660-image #:name name
- #:file-system-label root-label
- #:file-system-uuid uuid
- #:os os
- #:bootcfg-drv bootcfg
- #:bootloader (bootloader-configuration-bootloader
- (operating-system-bootloader os))
- #:inputs `(("system" ,os)
- ("bootcfg" ,bootcfg))
- #:grub-mkrescue-environment
- '(("MKRESCUE_SED_MODE" . "mbr_hfs"))
- #:substitutable? substitutable?)
- (qemu-image #:name name
- #:os os
- #:bootcfg-drv bootcfg
- #:bootloader (bootloader-configuration-bootloader
- (operating-system-bootloader os))
- #:disk-image-size disk-image-size
- #:disk-image-format "raw"
- #:file-system-type file-system-type
- #:file-system-label root-label
- #:file-system-uuid uuid
- #:copy-inputs? #t
- #:inputs `(("system" ,os)
- ("bootcfg" ,bootcfg))
- #:substitutable? substitutable?))))
+ (qemu-image #:name name
+ #:os os
+ #:bootcfg-drv bootcfg
+ #:bootloader (bootloader-configuration-bootloader
+ (operating-system-bootloader os))
+ #:disk-image-size disk-image-size
+ #:disk-image-format "raw"
+ #:file-system-type file-system-type
+ #:file-system-label root-label
+ #:file-system-uuid uuid
+ #:copy-inputs? #t
+ #:inputs `(("system" ,os)
+ ("bootcfg" ,bootcfg))
+ #:substitutable? substitutable?)))
(define* (system-qemu-image os
#:key
--
2.26.0
L
L
Ludovic Courtès wrote on 2 May 2020 13:09
Re: [bug#40955] [PATCH 2/5] build: install: Ignore chown exceptions.
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 40955@debbugs.gnu.org)
87wo5ua8zs.fsf@gnu.org
Hey!

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

Toggle quote (22 lines)
> Changing ownership may require root permissions. As image can now be generated
> without root permissions (no VM involved), ignore those exceptions.
>
> * gnu/build/install.scm (evaluate-populate-directive): Ignore chown
> exceptions.
> ---
> gnu/build/install.scm | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/gnu/build/install.scm b/gnu/build/install.scm
> index c0d4d44091..0b0d01cf86 100644
> --- a/gnu/build/install.scm
> +++ b/gnu/build/install.scm
> @@ -63,7 +63,9 @@ directory TARGET."
> (('directory name uid gid)
> (let ((dir (string-append target name)))
> (mkdir-p dir)
> - (chown dir uid gid)))
> + ;; This will fail if this is not run from a VM, ignore those
> + ;; errors.
> + (false-if-exception (chown dir uid gid))))

We still want the directives to be honored though.

How about having a procedure like:

(define (ensure-ownership file uid gid)
(catch 'system-error
(lambda ()
(chown file uid gid))
(lambda args
(if (= EPERM (system-error-errno args))
(let ((st (lstat file)))
(or (and (= uid (stat:uid st)) (= gid (stat:gid st)))
(apply throw args)))
(apply throw args)))))

and use that?

Or perhaps that’s still not helpful but we instead need a guarantee
elsewhere that the UID/GID is going to be honored, perhaps by calling:

(evaluate-populate-directive … #:default-gid 0 #:default-uid 0)

and have it ignore chown when it matches #:default-uid and
#:default-gid.

Thoughts?

Ludo’.
L
L
Ludovic Courtès wrote on 2 May 2020 14:16
Re: [bug#40955] [PATCH 3/5] build: bootloader: Add install-efi procedure.
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 40955@debbugs.gnu.org)
87r1w2a5wy.fsf@gnu.org
Mathieu Othacehe <m.othacehe@gmail.com> skribis:

Toggle quote (3 lines)
> * gnu/build/bootloader.scm (install-efi-loader): New exported procedure. This
> is based on install-efi from (gnu build vm) module.

Please mention the two new procedures.

Toggle quote (2 lines)
> * gnu/build/vm.scm (initialize-hard-disk): Adapt to use install-efi-loader.

[…]

Toggle quote (4 lines)
> +(define (install-efi-loader grub-efi esp)
> + ;; Create a tiny configuration file telling the embedded grub
> + ;; where to load the real thing.

Could you turn that into a docstring mentioning GRUB-EFI and ESP?

Toggle quote (6 lines)
> + ;; XXX This is quite fragile, and can prevent the image from booting
> + ;; when there's more than one volume with this label present.
> + ;; Reproducible almost-UUIDs could reduce the risk (not eliminate it).
> + (let ((grub-config "grub.cfg"))
> + (call-with-output-file grub-config
> + (lambda (port)
^
Indentation is off. :-)

Toggle quote (6 lines)
> + (format port
> + "insmod part_msdos~@
> + search --set=root --label Guix_image~@
> + configfile /boot/grub/grub.cfg~%")
> + (fsync port)))

You sure we need an ‘fsync’ call here? Perhaps add a comment explaining
why.

Otherwise LGTM!

Ludo’.
L
L
Ludovic Courtès wrote on 2 May 2020 14:50
Re: [bug#40955] [PATCH 4/5] image: Add a new API.
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 40955@debbugs.gnu.org)
87wo5u8prf.fsf@gnu.org
Mathieu Othacehe <m.othacehe@gmail.com> skribis:

Toggle quote (25 lines)
> Raw disk-images and ISO9660 images are created in a Qemu virtual machine. This
> is quite fragile, very slow, and almost unusable without KVM.
>
> For all these reasons, add support for host image generation. This implies the
> use new image generation mechanisms.
>
> - Raw disk images: images of partitions are created using tools such as mke2fs
> and mkdosfs depending on the partition file-system type. The partition
> images are then assembled into a final image using genimage.
>
> - ISO9660 images: the ISO root directory is populated within the store. GNU
> xorriso is then called on that directory, in the exact same way as this is
> done in (gnu build vm) module.
>
> Those mechanisms are built upon the new (gnu image) module.
>
> * gnu/image.scm: New file.
> * gnu/system/image.scm: New file.
> * gnu/build/image: New file.
> * gnu/local.mk: Add them.
> * gnu/system/vm.scm (system-disk-image): Rename to system-disk-image-in-vm.
> * gnu/ci.scm (qemu-jobs): Adapt to new API.
> * gnu/tests/install.scm (run-install): Ditto.
> * guix/scripts/system.scm (system-derivation-for-action): Ditto.

Yay!

Toggle quote (2 lines)
> +++ b/gnu/build/image.scm

Maybe we need to preserve some of the copyright lines of (gnu build vm).

Toggle quote (5 lines)
> +(define (root-size root)
> + "Given the ROOT directory, evalute and return its size. As this doesn't take
> +the partition metadata size into account, take a 25% margin."
> + (* 1.25 (file-size root)))

Perhaps ‘estimated-partition-size’ would be a better name?

Nitpick: two spaces after end-of-sentence periods. :-)

Toggle quote (3 lines)
> +(define* (make-ext4-image partition target root
> + #:key (owner 0))

Would it make sense to separate #:owner-uid and #:owner-gid?

It does mean that we can only create images where all the files have the
same UID/GID.

Looking at (gnu build install), there’s one case where it might be
problematic: the store’s GID is supposed to match the ‘guixbuilder’
group. But the good news is that the daemon does this:

if (chown(chrootStoreDir.c_str(), 0, buildUser.getGID()) == -1)
throw SysError(format("cannot change ownership of ‘%1%’") % chrootStoreDir);

So we can just remove the UID/GID from the directives that are in (gnu
build install).

Toggle quote (8 lines)
> +(define* (genimage config target)
> + "Use genimage to generate in TARGET directory, the image described in the
> +given CONFIG file."
> + ;; genimage needs a 'root' directory.
> + (mkdir "root")
> + (invoke "genimage" "--config" config
> + "--outputpath" target))

I had missed that bit, so we still need genimage in the end?

Toggle quote (6 lines)
> +(define (register-bootcfg-root target bootcfg)
> + "On file system TARGET, register BOOTCFG as a GC root."
> + (let ((directory (string-append target "/var/guix/gcroots")))
> + (mkdir-p directory)
> + (symlink bootcfg (string-append directory "/bootcfg"))))

Maybe just ‘register-gc-root’?

Toggle quote (17 lines)
> +(define* (register-closure prefix closure
> + #:key
> + (deduplicate? #t) (reset-timestamps? #t)
> + (schema (sql-schema)))
> + "Register CLOSURE in PREFIX, where PREFIX is the directory name of the
> +target store and CLOSURE is the name of a file containing a reference graph as
> +produced by #:references-graphs.. As a side effect, if RESET-TIMESTAMPS? is
> +true, reset timestamps on store files and, if DEDUPLICATE? is true,
> +deduplicates files common to CLOSURE and the rest of PREFIX."
> + (let ((items (call-with-input-file closure read-reference-graph)))
> + (register-items items
> + #:prefix prefix
> + #:deduplicate? deduplicate?
> + #:reset-timestamps? reset-timestamps?
> + #:registration-time %epoch
> + #:schema schema)))

This is duplicated from (guix build vm). Should we instead factorize it
in (guix build store-copy)?

Toggle quote (4 lines)
> +(define-module (gnu image)
> + #:use-module (guix records)
> + #:use-module (ice-9 match)

(ice-9 match) can be removed.

Toggle quote (2 lines)
> + #:use-module ((srfi srfi-1) #:prefix scm:)

I’d suggest either ‘srfi-1:’ as the prefix or, better, hide whichever
binding is causing a name clash.

Toggle quote (15 lines)
> +(define-syntax-rule (with-imported-modules* exp ...)
> + (with-extensions gcrypt-sqlite3&co
> + (with-imported-modules `(,@(source-module-closure
> + '((gnu build vm)
> + (gnu build image)
> + (guix store database))
> + #:select? not-config?)
> + ((guix config) => ,(make-config.scm)))
> + #~(begin
> + (use-modules (gnu build vm)
> + (gnu build image)
> + (guix store database)
> + (guix build utils))
> + exp ...))))

Probably a better name would be ‘gexp*’ (or ‘image-gexp’) to make it
clear that it builds a gexp.

Toggle quote (8 lines)
> +(define* (system-disk-image image
> + #:key
> + (name "disk-image")
> + bootcfg
> + bootloader
> + register-closures?
> + (inputs '()))

[...]

Toggle quote (7 lines)
> + (define (image->genimage-cfg image)
> + "Return as a file-like object, the genimage configuration file describing
> +the given IMAGE."
> + (define (format->image-type format)
> + "Return the genimage format corresponding to FORMAT. For now, only the
> +hdimage format (raw disk-image) is supported."

Use comments instead of docstrings for inner procedures in all this file
(docstrings could not be accessed anyway). Also, two spaces after
end-of-sentence periods. :-)

Toggle quote (6 lines)
> + (case format
> + ((disk-image) "hdimage")
> + (else
> + (error
> + (format #f "Unsupported image type ~a~%." format)))))

For host-side code, better use “proper” error reporting that can be
gracefully dealt with. So at least:

(raise (condition (&message (message (format #f (G_ …) …)))))

and/or a specific error condition type (well, we can leave that for
later).

Toggle quote (6 lines)
> + (gexp->derivation name
> + #~(symlink
> + (string-append #$image-dir "/" #$genimage-name)
> + #$output)
> + #:substitutable? substitutable?)))

Can we use ‘computed-file’ as well instead of ‘gexp->derivation’? That
way, the API is entirely non-monadic and hopefully easier to use.

It does mean that we need to adjust (gnu ci), (gnu tests …), and (guix
scripts system), but maybe that’s OK.

Anyhow, the switch to non-monadic style could be made later, but not too
late so we can still consider the API as not-quite-public. :-)

Toggle quote (4 lines)
> + (gexp->derivation name builder
> + #:references-graphs inputs
> + #:substitutable? substitutable?)))

Same here.

Toggle quote (14 lines)
> +(define* (make-system-image image)
> + "Return the derivation of IMAGE. It can be a raw disk-image or an ISO9660
> +image, depending on IMAGE format."
> + (let* ((image-os (image-operating-system image))
> + (format (image-format image))
> + (file-systems-to-keep
> + (scm:remove
> + (lambda (fs)
> + (string=? (file-system-mount-point fs) "/"))
> + (operating-system-file-systems image-os)))
> + (root-file-system-type (image->root-file-system image))
> + (substitutable? (image-substitutable? image))
> + (volatile-root? (image-volatile-root? image))

To improve readability, maybe you can use inner ‘define’ for these
things.

Toggle quote (19 lines)
> + (os (operating-system
> + (inherit image-os)
> + (initrd (lambda (file-systems . rest)
> + (apply (operating-system-initrd image-os)
> + file-systems
> + #:volatile-root? volatile-root?
> + rest)))
> + (bootloader (if (eq? format 'iso9660)
> + (bootloader-configuration
> + (inherit
> + (operating-system-bootloader image-os))
> + (bootloader grub-mkrescue-bootloader))
> + (operating-system-bootloader image-os)))
> + (file-systems (cons (file-system
> + (mount-point "/")
> + (device "/dev/placeholder")
> + (type root-file-system-type))
> + file-systems-to-keep))))

Perhaps define an auxiliary ‘operating-system-for-image’ procedure, just
like we have ‘virtualized-operating-system’ & co.

That’s all!

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 2 May 2020 14:52
Re: [bug#40955] [PATCH 5/5] vm: Remove obsolete procedures.
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 40955@debbugs.gnu.org)
87r1w28po1.fsf@gnu.org
Mathieu Othacehe <m.othacehe@gmail.com> skribis:

Toggle quote (6 lines)
> * gnu/build/vm.scm (install-efi, make-iso9660-image): Remove those procedures
> that are now implemented in (gnu build image) module,
> (initialize-hard-disk): remove efi support.
> * gnu/system/vm.scm (iso9660-image): Remove it,
> (qemu-image): adapt it to remove ISO9660 support.

[...]

Toggle quote (8 lines)
> --- a/gnu/system/vm.scm
> +++ b/gnu/system/vm.scm
> @@ -269,95 +269,6 @@ substitutable."
> (eq? (service-kind service) guix-service-type))
> (operating-system-services os)))))
>
> -(define* (iso9660-image #:key

Instead of removing it wholesale, can we use ‘define-deprecated’ and
make it an alias for the new one?

Toggle quote (4 lines)
> (define* (qemu-image #:key
> (name "qemu-image")
> (system (%current-system))

Oh, we keep this one?

Thanks for all this!

Ludo’.
T
T
Tobias Geerinckx-Rice wrote on 2 May 2020 15:02
Re: [bug#40955] [PATCH 4/5] image: Add a new API.
87wo5upk2f.fsf@nckx
Mathieu,

This is great. Thank you so much.

Mathieu Othacehe ???
Toggle quote (6 lines)
> - ISO9660 images: the ISO root directory is populated within the
> store. GNU
> xorriso is then called on that directory, in the exact same
> way as this is
> done in (gnu build vm) module.

This made me do a double-take, but looking at the code it's still
s/xorriso/grub-mkrescue/ (which calls Xorriso, sure, but performs
arcane magicks in between that lets us actually boot on real
hardware). Right? Any cases where that changes?

Kind regards,

T G-R
-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQT12iAyS4c9C3o4dnINsP+IT1VteQUCXq1vaQAKCRANsP+IT1Vt
eTOxAP0eY4pWKNfO4p8kucrfYuXAU+mRAQ/9eT/vdrpNji+Y3wEAzhQSO7MEIR4c
iWbWSD9xyIfVmFpK+QttgBOW8fiG3gw=
=y46m
-----END PGP SIGNATURE-----

M
M
Mathieu Othacehe wrote on 5 May 2020 15:42
Re: [bug#40955] [PATCH 2/5] build: install: Ignore chown exceptions.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40955@debbugs.gnu.org)
87y2q6cxbq.fsf@gmail.com
Hello Ludo,

Thanks for reviewing :)

Toggle quote (8 lines)
> Or perhaps that’s still not helpful but we instead need a guarantee
> elsewhere that the UID/GID is going to be honored, perhaps by calling:
>
> (evaluate-populate-directive … #:default-gid 0 #:default-uid 0)
>
> and have it ignore chown when it matches #:default-uid and
> #:default-gid.

This seems like a good idea. This way, 'chown' is only applied if the
requested uid/gid doesn't match the default one.

Thanks,

Mathieu
M
M
Mathieu Othacehe wrote on 5 May 2020 15:43
Re: [bug#40955] [PATCH 3/5] build: bootloader: Add install-efi procedure.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40955@debbugs.gnu.org)
87tv0ucxam.fsf@gmail.com
Toggle quote (3 lines)
> You sure we need an ‘fsync’ call here? Perhaps add a comment explaining
> why.

Turns out this is useless as the port is closed, hence everything should
be flushed.

Thanks,

Mathieu
M
M
Mathieu Othacehe wrote on 5 May 2020 15:52
Re: [bug#40955] [PATCH 4/5] image: Add a new API.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40955@debbugs.gnu.org)
87pnbicwv5.fsf@gmail.com
Toggle quote (2 lines)
> Would it make sense to separate #:owner-uid and #:owner-gid?

Yes, fixed.

Toggle quote (14 lines)
>
> It does mean that we can only create images where all the files have the
> same UID/GID.
>
> Looking at (gnu build install), there’s one case where it might be
> problematic: the store’s GID is supposed to match the ‘guixbuilder’
> group. But the good news is that the daemon does this:
>
> if (chown(chrootStoreDir.c_str(), 0, buildUser.getGID()) == -1)
> throw SysError(format("cannot change ownership of ‘%1%’") % chrootStoreDir);
>
> So we can just remove the UID/GID from the directives that are in (gnu
> build install).

Done in a separate commit.

Toggle quote (11 lines)
>
>> +(define* (genimage config target)
>> + "Use genimage to generate in TARGET directory, the image described in the
>> +given CONFIG file."
>> + ;; genimage needs a 'root' directory.
>> + (mkdir "root")
>> + (invoke "genimage" "--config" config
>> + "--outputpath" target))
>
> I had missed that bit, so we still need genimage in the end?

genimage is used to assemble the disk-image together. It's a matter of
doing some 'dd' with the right offset. Once this part is implemented in
(gnu build image), we can get rid of genimage.

Toggle quote (8 lines)
>> +(define (register-bootcfg-root target bootcfg)
>> + "On file system TARGET, register BOOTCFG as a GC root."
>> + (let ((directory (string-append target "/var/guix/gcroots")))
>> + (mkdir-p directory)
>> + (symlink bootcfg (string-append directory "/bootcfg"))))
>
> Maybe just ‘register-gc-root’?

Turns out, I don't think this is useful anymore, so I removed it.

Toggle quote (21 lines)
>
>> +(define* (register-closure prefix closure
>> + #:key
>> + (deduplicate? #t) (reset-timestamps? #t)
>> + (schema (sql-schema)))
>> + "Register CLOSURE in PREFIX, where PREFIX is the directory name of the
>> +target store and CLOSURE is the name of a file containing a reference graph as
>> +produced by #:references-graphs.. As a side effect, if RESET-TIMESTAMPS? is
>> +true, reset timestamps on store files and, if DEDUPLICATE? is true,
>> +deduplicates files common to CLOSURE and the rest of PREFIX."
>> + (let ((items (call-with-input-file closure read-reference-graph)))
>> + (register-items items
>> + #:prefix prefix
>> + #:deduplicate? deduplicate?
>> + #:reset-timestamps? reset-timestamps?
>> + #:registration-time %epoch
>> + #:schema schema)))
>
> This is duplicated from (guix build vm). Should we instead factorize it
> in (guix build store-copy)?

I tried it. The problem is that it introduces a dependency to
guile-sqlite3, so a bunch of with-extensions needs to be added in
various places. I kept the copy, as (gnu build vm) will be
deprecated/removed anyways.

Toggle quote (3 lines)
> I’d suggest either ‘srfi-1:’ as the prefix or, better, hide whichever
> binding is causing a name clash.

I need partition from srfi-1 and partition from (gnu image). That's not
great :( I opted for the srfi-1 prefix as you suggested.

Toggle quote (3 lines)
> Can we use ‘computed-file’ as well instead of ‘gexp->derivation’? That
> way, the API is entirely non-monadic and hopefully easier to use.

Ok, so I used computed-file as suggested. However, the "system-image"
procedure is calling the monadic system-disk-image-in-vm. So I had to
keep this one monadic.

The good news is once, system-disk-image-in-vm is no longer needed, this
whole file will be entirely non-monadic.

Toggle quote (2 lines)
> That’s all!

Thanks a lot for going though all of this.

Mathieu
M
M
Mathieu Othacehe wrote on 5 May 2020 16:02
Re: [bug#40955] [PATCH 5/5] vm: Remove obsolete procedures.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40955@debbugs.gnu.org)
87lfm6cwel.fsf@gmail.com
Toggle quote (5 lines)
>> -(define* (iso9660-image #:key
>
> Instead of removing it wholesale, can we use ‘define-deprecated’ and
> make it an alias for the new one?

As make-iso9660-image is moved from '(gnu build vm)' to '(gnu build
image)', I think it is safe to remove iso9660-image from '(guix system
vm)'.

Toggle quote (7 lines)
>
>> (define* (qemu-image #:key
>> (name "qemu-image")
>> (system (%current-system))
>
> Oh, we keep this one?

Yes, because '(gnu system image)' does not yet support producing
disk-images when the bootloader is Grub non-EFI or extlinux.

The "system-image" procedure in '(gnu system image)' is taking care of
the transition. Once support for those two bootloaders is added, we will
be able to get rid of "qemu-image", I guess.

Thanks,

Mathieu
M
M
Mathieu Othacehe wrote on 5 May 2020 16:12
Re: [bug#40955] [PATCH 4/5] image: Add a new API.
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)(address . 40955@debbugs.gnu.org)
87h7wucvyf.fsf@gmail.com
Toggle quote (2 lines)
> This is great. Thank you so much.

Thanks Tobias :)

Toggle quote (5 lines)
> This made me do a double-take, but looking at the code it's still
> s/xorriso/grub-mkrescue/ (which calls Xorriso, sure, but performs arcane
> magicks in between that lets us actually boot on real hardware). Right? Any
> cases where that changes?

No, for ISO creation, the process is still the same. The only change is
that grub-mkrescue is run from the host and not inside a VM.

Mathieu
M
M
Mathieu Othacehe wrote on 5 May 2020 16:16
Re: [bug#40955] [PATCH 5/5] vm: Remove obsolete procedures.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 40955-done@debbugs.gnu.org)
87d07icvrf.fsf@gmail.com
I pushed this serie to master.

Thanks,

Mathieu
Closed
?