diff mbox series

[bug#57643,1/3] image: Make the operating-system field mandatory.

Message ID 20220907124633.17013-1-othacehe@gnu.org
State Accepted
Headers show
Series Document the image API. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git-branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

Mathieu Othacehe Sept. 7, 2022, 12:46 p.m. UTC 
Make the operating-system field mandatory as creating an image without it
makes no sense. Introduce a new macro, image-without-os for the specific cases
where the image is only created to be inherited from afterwards.

* gnu/image.scm (<image>)[operating-system]: Make it mandatory.
* gnu/system/image.scm (image-without-os): New macro.
(efi-disk-image, efi32-disk-image, iso9660-image, docker-image,
raw-with-offset-disk-image): Use it.
* gnu/system/images/hurd.scm (hurd-disk-image): Ditto.
---
 gnu/image.scm              |  3 +--
 gnu/system/image.scm       | 41 +++++++++++++++++++++++++++++++++-----
 gnu/system/images/hurd.scm |  2 +-
 3 files changed, 38 insertions(+), 8 deletions(-)

Comments

Liliana Marie Prikler Sept. 7, 2022, 6:34 p.m. UTC | #1 
Hi,

Am Mittwoch, dem 07.09.2022 um 14:46 +0200 schrieb Mathieu Othacehe:
> Make the operating-system field mandatory as creating an image
> without it
> makes no sense. Introduce a new macro, image-without-os for the
> specific cases
> where the image is only created to be inherited from afterwards.
> 
> * gnu/image.scm (<image>)[operating-system]: Make it mandatory.
> * gnu/system/image.scm (image-without-os): New macro.
> (efi-disk-image, efi32-disk-image, iso9660-image, docker-image,
> raw-with-offset-disk-image): Use it.
> * gnu/system/images/hurd.scm (hurd-disk-image): Ditto.
IMHO adding a new macro is a bit much.  Can't we simply add an empty
image from which the others are derived or use (operating-system #f)
literally?  (Alternatively to #f you might prefer "none" as syntactic
sugar.)

I do agree with making operating-system mandatory, though.

Cheers
Ludovic Courtès Sept. 24, 2022, 10:16 a.m. UTC | #2 
Hi,

Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:

> Am Mittwoch, dem 07.09.2022 um 14:46 +0200 schrieb Mathieu Othacehe:
>> Make the operating-system field mandatory as creating an image
>> without it
>> makes no sense. Introduce a new macro, image-without-os for the
>> specific cases
>> where the image is only created to be inherited from afterwards.
>> 
>> * gnu/image.scm (<image>)[operating-system]: Make it mandatory.
>> * gnu/system/image.scm (image-without-os): New macro.
>> (efi-disk-image, efi32-disk-image, iso9660-image, docker-image,
>> raw-with-offset-disk-image): Use it.
>> * gnu/system/images/hurd.scm (hurd-disk-image): Ditto.
> IMHO adding a new macro is a bit much.  Can't we simply add an empty
> image from which the others are derived or use (operating-system #f)
> literally?  (Alternatively to #f you might prefer "none" as syntactic
> sugar.)

I sympathize with what you write, though I don’t have a strong opinion.

> I do agree with making operating-system mandatory, though.

Same here!

Ludo’.
M Sept. 24, 2022, 10:55 a.m. UTC | #3 
On 07-09-2022 14:46, Mathieu Othacehe wrote:
> +(define-syntax image-without-os
> +  (lambda (x)
> +    "Return an image record with the mandatory operating-system field set to
> +#false.  This is useful when creating an image record that will serve as a
> +parent image record > +
> +    (define (maybe-cons field acc)
> +      ;; Return the given ACC list if FIELD is 'operating-system or the
> +      ;; concatenation of FIELD to ACC otherwise.
> +      (syntax-case field ()
> +        ((f v)
> +         (if (eq? (syntax->datum #'f) 'operating-system)
> +             acc
> +             (cons field acc)))) > +
> +    (syntax-case x (image)
> +      ;; Remove the operating-system field from the defined fields and then
> +      ;; force it to #false.
> +      ((_ fields ...)
> +       (let loop ((fields #'(fields ...))
> +                  (acc   '()))
> +         (syntax-case fields ()
> +           ((last)
> +            #`(image
> +               ;; Force it to #false.
> +               (operating-system #false)
> +               #,@(maybe-cons #'last acc)))
> +           ((field rest ...)
> +            (loop #'(rest ...) (maybe-cons #'field acc)))))))))

The complexity of this 'without os' macro seems to come from accepting 
an 'os' and then throwing it away.  However, when there is an 'os', you 
might as well use 'image' directly, without 'image-without-os', so I 
think this macro can be simplified to:

(define-syntax-rule (image-without-os . settings)
   "docstring"
   (image (operating-system #false) . settings))

(IIUC, '(guix records)' will detect duplicate definitions of fields.)

Greetings,
Maxime.
Mathieu Othacehe Sept. 24, 2022, 1:02 p.m. UTC | #4 
Hey,

> The complexity of this 'without os' macro seems to come from accepting an 'os'
> and then throwing it away.  However, when there is an 'os', you might as well
> use 'image' directly, without 'image-without-os', so I think this macro can be
> simplified to:

Yeah this was mostly a good pretext to have fun with macros, I agree
your alternative makes more sense. I used your proposed approach instead.

> (IIUC, '(guix records)' will detect duplicate definitions of fields.)

It does.

Thanks,

Mathieu
diff mbox series

Patch

diff --git a/gnu/image.scm b/gnu/image.scm
index 4a0068934e..68784deb12 100644
--- a/gnu/image.scm
+++ b/gnu/image.scm
@@ -170,8 +170,7 @@  (define-record-type* <image>
   (size               image-size  ;size in bytes as integer
                       (default 'guess)
                       (sanitize validate-size))
-  (operating-system   image-operating-system  ;<operating-system>
-                      (default #f))
+  (operating-system   image-operating-system)  ;<operating-system>
   (partition-table-type image-partition-table-type ; 'mbr or 'gpt
                       (default 'mbr)
                       (sanitize validate-partition-table-type))
diff --git a/gnu/system/image.scm b/gnu/system/image.scm
index a04363a130..709c3ab6ff 100644
--- a/gnu/system/image.scm
+++ b/gnu/system/image.scm
@@ -65,6 +65,7 @@  (define-module (gnu system image)
   #:use-module (ice-9 match)
   #:export (root-offset
             root-label
+            image-without-os
 
             esp-partition
             esp32-partition
@@ -102,6 +103,36 @@  (define root-offset (* 512 2048))
 ;; Generic root partition label.
 (define root-label "Guix_image")
 
+(define-syntax image-without-os
+  (lambda (x)
+    "Return an image record with the mandatory operating-system field set to
+#false.  This is useful when creating an image record that will serve as a
+parent image record."
+
+    (define (maybe-cons field acc)
+      ;; Return the given ACC list if FIELD is 'operating-system or the
+      ;; concatenation of FIELD to ACC otherwise.
+      (syntax-case field ()
+        ((f v)
+         (if (eq? (syntax->datum #'f) 'operating-system)
+             acc
+             (cons field acc)))))
+
+    (syntax-case x (image)
+      ;; Remove the operating-system field from the defined fields and then
+      ;; force it to #false.
+      ((_ fields ...)
+       (let loop ((fields #'(fields ...))
+                  (acc   '()))
+         (syntax-case fields ()
+           ((last)
+            #`(image
+               ;; Force it to #false.
+               (operating-system #false)
+               #,@(maybe-cons #'last acc)))
+           ((field rest ...)
+            (loop #'(rest ...) (maybe-cons #'field acc)))))))))
+
 (define esp-partition
   (partition
    (size (* 40 (expt 2 20)))
@@ -127,17 +158,17 @@  (define root-partition
    (initializer (gexp initialize-root-partition))))
 
 (define efi-disk-image
-  (image
+  (image-without-os
    (format 'disk-image)
    (partitions (list esp-partition root-partition))))
 
 (define efi32-disk-image
-  (image
+  (image-without-os
    (format 'disk-image)
    (partitions (list esp32-partition root-partition))))
 
 (define iso9660-image
-  (image
+  (image-without-os
    (format 'iso9660)
    (partitions
     (list (partition
@@ -146,11 +177,11 @@  (define iso9660-image
            (flags '(boot)))))))
 
 (define docker-image
-  (image
+  (image-without-os
    (format 'docker)))
 
 (define* (raw-with-offset-disk-image #:optional (offset root-offset))
-  (image
+  (image-without-os
    (format 'disk-image)
    (partitions
     (list (partition
diff --git a/gnu/system/images/hurd.scm b/gnu/system/images/hurd.scm
index 6da09b855a..2c64117c08 100644
--- a/gnu/system/images/hurd.scm
+++ b/gnu/system/images/hurd.scm
@@ -74,7 +74,7 @@  (define hurd-initialize-root-partition
                            #:wal-mode? #f)))))
 
 (define hurd-disk-image
-  (image
+  (image-without-os
    (format 'disk-image)
    (platform hurd)
    (partitions