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

From: Jan Nieuwenhuizen <[email-masked]>
Date: Fri, 12 Jun 2020 23:33:38 +0200

Mathieu Othacehe writes:

Hello Mathieu,

>> +  (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.

Okay, sure.  I've made that change for now.

>> +@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:
>
> (service hurd-vm-service-type
>          (hurd-vm-configuration
>           (disk-size (* 5 GiB)
>           (memory-size (* 1024 MiB)))

I like it!

> 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:
>
> (define (image os #:key (image-size 'guess))
>   (image
>    (inherit hurd-disk-image)
>    (size image-size)
>    (operating-system os)))

Oh yes...that's great.  It seems that until we have that, as Ludo
suggested with this:

--8<---------------cut here---------------start------------->8---
    (with-parameters ((%current-target-system target))
      (system-image
       (image (inherit base-image)
              (size disk-size)
              (operating-system os))))))
--8<---------------cut here---------------end--------------->8---

we have now something "that works" for the Hurd.

>> +  (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 :)

Thank you.  I'll be working to prepare a v3 series that works on master
right now; then we can see how to proceed.

Thanks!
Janneke

-- 
Jan Nieuwenhuizen <[email-masked]> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com