Subject: Re: [bug#41785] [PATCH] DRAFT services: Add 'hurd-in-vm service-type'.

From: Mathieu Othacehe <[email-masked]>
Date: Fri, 12 Jun 2020 17:04:58 +0200

Hey janneke,

> +  (let ((target (%current-target-system)))
> +    (match file-system-type
> +      ("iso9660" iso9660-image)
> +      (_ (cond
> +          ((and target
> +                (hurd-triplet? target))
> +           hurd-disk-image)
> +          (else
> +           efi-disk-image))))))

I think it would be safe to pass a "target" argument. Then the two
actual callers could pass (current-target-system) as target
argument. This is guaranteed to return a correct value, whereas
%current-target-system is not here.

> +@lisp
> +(service hurd-vm-service-type
> +         (hurd-vm-configuration
> +          (disk-size (* 5000 (expt 2 20))) ;5G
> +          (memory-size 1024)))             ;1024MiB

That's really nice! We could really use a (guix units) module or so
where we would put those definitions: "(define MiB (expt 2 20))". Then
we could use:

--8<---------------cut here---------------start------------->8---
(service hurd-vm-service-type
         (hurd-vm-configuration
          (disk-size (* 5 GiB)
          (memory-size (* 1024 MiB)))
--8<---------------cut here---------------end--------------->8---

Well, this is really not a blocking thing.

> +(define* (disk-image os #:key (image-size 'guess) target)
> +  "Return a disk-image for OS with size IMAGE-SIZE, built for TARGET."
> +  (let ((base-image (find-image "ext2")))
> +    (system-image
> +     (image (inherit base-image)
> +            (size image-size)
> +            (operating-system
> +              (with-parameters ((%current-target-system target))
> +                os))))))

Yeah then again, I think it would be nice to use something like the
"hurd-disk-image" as I proposed here:
https://lists.gnu.org/archive/html/guix-devel/2020-05/msg00417.html.

This way, no need to call "find-image". You could just write:

--8<---------------cut here---------------start------------->8---
(define (image os #:key (image-size 'guess))
  (image
   (inherit hurd-disk-image)
   (size image-size)
   (operating-system os)))
--8<---------------cut here---------------end--------------->8---

> +  (image       hurd-vm-configuration-image              ;string
> +               (thunked)

Then the thunked field wouldn't be required.

Mostly comments related to the fact that (gnu image) needs some
polishing, your patches are really nice here :)

Thanks,

Mathieu