* [PATCH] vm: Have qemu-image generate derivations instead.
@ 2016-02-11 11:41 Jookia
2016-03-29 19:46 ` Ludovic Courtès
0 siblings, 1 reply; 6+ messages in thread
From: Jookia @ 2016-02-11 11:41 UTC (permalink / raw)
To: guix-devel
This small refactor should simplify some duplicated effort across functions and
allow smarter qemu-image to do smarter things based on the operating system
configuration rather than having each function that uses qemu-image pass
selective parameters whenever new information is needed.
* gnu/system/vm.scm (qemu-image): Replace os-derivation, grub-configuration and
inputs parameters with os-configuration, base-inputs and extra-inputs.
(qemu-image): Based on base-inputs, generate grub.cfg and os-drv.
(system-disk-image, system-qemu-image, system-qemu-image/shared-store):
Pass in the operating system configuration and base-inputs to qemu-image
instead of derivations.
---
gnu/system/vm.scm | 177 ++++++++++++++++++++++++++----------------------------
1 file changed, 85 insertions(+), 92 deletions(-)
diff --git a/gnu/system/vm.scm b/gnu/system/vm.scm
index a7c03bd..4c3fd87 100644
--- a/gnu/system/vm.scm
+++ b/gnu/system/vm.scm
@@ -191,68 +191,78 @@ made available under the /xchg CIFS share."
(disk-image-format "qcow2")
(file-system-type "ext4")
file-system-label
- os-derivation
- grub-configuration
+ os-configuration
(register-closures? #t)
- (inputs '())
+ (base-inputs (list 'grub.cfg 'system))
+ (extra-inputs '())
copy-inputs?)
"Return a bootable, stand-alone QEMU image of type DISK-IMAGE-FORMAT (e.g.,
'qcow2' or 'raw'), with a root partition of type FILE-SYSTEM-TYPE.
Optionally, FILE-SYSTEM-LABEL can be specified as the volume name for the root
-partition. The returned image is a full disk image that runs OS-DERIVATION,
-with a GRUB installation that uses GRUB-CONFIGURATION as its configuration
-file (GRUB-CONFIGURATION must be the name of a file in the VM.)
+partition. The returned image is a full disk image that runs OS-CONFIGURATION,
+with a GRUB installation that uses its associated GRUB-CONFIGURATION.
-INPUTS is a list of inputs (as for packages). When COPY-INPUTS? is true, copy
+BASE-INPUTS is a list of inputs to be generated by qemu-image. By default it
+contains 'grub.cfg which includes the GRUB bootloader configuration file and
+'system which includes the derivation of the operating system configuration.
+EXTRA-INPUTS is a list of inputs (as for packages). When COPY-INPUTS? is true, copy
all of INPUTS into the image being built. When REGISTER-CLOSURES? is true,
register INPUTS in the store database of the image so that Guix can be used in
the image."
- (expression->derivation-in-linux-vm
- name
- #~(begin
- (use-modules (gnu build vm)
- (guix build utils))
-
- (let ((inputs
- '#$(append (list qemu parted grub e2fsprogs)
- (map canonical-package
- (list sed grep coreutils findutils gawk))
- (if register-closures? (list guix) '())))
-
- ;; 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)
-
- (let* ((graphs '#$(match inputs
- (((names . _) ...)
- names)))
- (initialize (root-partition-initializer
- #:closures graphs
- #:copy-closures? #$copy-inputs?
- #:register-closures? #$register-closures?
- #:system-directory #$os-derivation))
- (partitions (list (partition
- (size #$(- disk-image-size
- (* 10 (expt 2 20))))
- (label #$file-system-label)
- (file-system #$file-system-type)
- (bootable? #t)
- (initializer initialize)))))
- (initialize-hard-disk "/dev/vda"
- #:partitions partitions
- #:grub.cfg #$grub-configuration)
- (reboot))))
- #:system system
- #:make-disk-image? #t
- #:disk-image-size disk-image-size
- #:disk-image-format disk-image-format
- #:references-graphs inputs))
+ (mlet* %store-monad ((os-drv (operating-system-derivation os-configuration))
+ (grub.cfg (operating-system-grub.cfg os-configuration))
+ (inputs -> (append
+ (if (member 'grub.cfg base-inputs)
+ `(("grub.cfg" ,grub.cfg)) '())
+ (if (member 'system base-inputs)
+ `(("system" ,os-drv)) '())
+ extra-inputs)))
+ (expression->derivation-in-linux-vm
+ name
+ #~(begin
+ (use-modules (gnu build vm)
+ (guix build utils))
+
+ (let ((inputs
+ '#$(append (list qemu parted grub e2fsprogs)
+ (map canonical-package
+ (list sed grep coreutils findutils gawk))
+ (if register-closures? (list guix) '())))
+
+ ;; 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)
+
+ (let* ((graphs '#$(match inputs
+ (((names . _) ...)
+ names)))
+ (initialize (root-partition-initializer
+ #:closures graphs
+ #:copy-closures? #$copy-inputs?
+ #:register-closures? #$register-closures?
+ #:system-directory #$os-drv))
+ (partitions (list (partition
+ (size #$(- disk-image-size
+ (* 10 (expt 2 20))))
+ (label #$file-system-label)
+ (file-system #$file-system-type)
+ (bootable? #t)
+ (initializer initialize)))))
+ (initialize-hard-disk "/dev/vda"
+ #:partitions partitions
+ #:grub.cfg #$grub.cfg)
+ (reboot))))
+ #:system system
+ #:make-disk-image? #t
+ #:disk-image-size disk-image-size
+ #:disk-image-format disk-image-format
+ #:references-graphs inputs)))
\f
;;;
@@ -297,19 +307,14 @@ to USB sticks meant to be read-only."
(type file-system-type))
file-systems-to-keep)))))
- (mlet* %store-monad ((os-drv (operating-system-derivation os))
- (grub.cfg (operating-system-grub.cfg os)))
- (qemu-image #:name name
- #:os-derivation os-drv
- #:grub-configuration grub.cfg
- #:disk-image-size disk-image-size
- #:disk-image-format "raw"
- #:file-system-type file-system-type
- #:file-system-label root-label
- #:copy-inputs? #t
- #:register-closures? #t
- #:inputs `(("system" ,os-drv)
- ("grub.cfg" ,grub.cfg))))))
+ (qemu-image #:name name
+ #:os-configuration os
+ #:disk-image-size disk-image-size
+ #:disk-image-format "raw"
+ #:file-system-type file-system-type
+ #:file-system-label root-label
+ #:copy-inputs? #t
+ #:register-closures? #t)))
(define* (system-qemu-image os
#:key
@@ -341,16 +346,10 @@ of the GNU system as described by OS."
(device "/dev/sda1")
(type file-system-type))
file-systems-to-keep)))))
- (mlet* %store-monad
- ((os-drv (operating-system-derivation os))
- (grub.cfg (operating-system-grub.cfg os)))
- (qemu-image #:os-derivation os-drv
- #:grub-configuration grub.cfg
- #:disk-image-size disk-image-size
- #:file-system-type file-system-type
- #:inputs `(("system" ,os-drv)
- ("grub.cfg" ,grub.cfg))
- #:copy-inputs? #t))))
+ (qemu-image #:os-configuration os
+ #:disk-image-size disk-image-size
+ #:file-system-type file-system-type
+ #:copy-inputs? #t)))
\f
;;;
@@ -430,22 +429,16 @@ with the host.
When FULL-BOOT? is true, return an image that does a complete boot sequence,
bootloaded included; thus, make a disk image that contains everything the
bootloader refers to: OS kernel, initrd, bootloader data, etc."
- (mlet* %store-monad ((os-drv (operating-system-derivation os))
- (grub.cfg (operating-system-grub.cfg os)))
- ;; XXX: When FULL-BOOT? is true, we end up creating an image that contains
- ;; GRUB.CFG and all its dependencies, including the output of OS-DRV.
- ;; This is more than needed (we only need the kernel, initrd, GRUB for its
- ;; font, and the background image), but it's hard to filter that.
- (qemu-image #:os-derivation os-drv
- #:grub-configuration grub.cfg
- #:disk-image-size disk-image-size
- #:inputs (if full-boot?
- `(("grub.cfg" ,grub.cfg))
- '())
-
- ;; XXX: Passing #t here is too slow, so let it off by default.
- #:register-closures? #f
- #:copy-inputs? full-boot?)))
+ ;; XXX: When FULL-BOOT? is true, we end up creating an image that contains
+ ;; GRUB.CFG and all its dependencies, including the output of OS-DRV.
+ ;; This is more than needed (we only need the kernel, initrd, GRUB for its
+ ;; font, and the background image), but it's hard to filter that.
+ (qemu-image #:os-configuration os
+ #:disk-image-size disk-image-size
+ #:base-inputs (if full-boot? (list 'grub.cfg) '())
+ ;; XXX: Passing #t here is too slow, so let it off by default.
+ #:register-closures? #f
+ #:copy-inputs? full-boot?))
(define* (common-qemu-options image shared-fs)
"Return the a string-value gexp with the common QEMU options to boot IMAGE,
--
2.7.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] vm: Have qemu-image generate derivations instead.
2016-02-11 11:41 [PATCH] vm: Have qemu-image generate derivations instead Jookia
@ 2016-03-29 19:46 ` Ludovic Courtès
2016-03-29 23:13 ` Jookia
0 siblings, 1 reply; 6+ messages in thread
From: Ludovic Courtès @ 2016-03-29 19:46 UTC (permalink / raw)
To: Jookia; +Cc: guix-devel
Jookia <166291@gmail.com> skribis:
> This small refactor should simplify some duplicated effort across functions and
> allow smarter qemu-image to do smarter things based on the operating system
> configuration rather than having each function that uses qemu-image pass
> selective parameters whenever new information is needed.
Damn, it’s been more than a month, please accept my apologies!
Overall this sounds like a good idea. However…
> * gnu/system/vm.scm (qemu-image): Replace os-derivation, grub-configuration and
> inputs parameters with os-configuration, base-inputs and extra-inputs.
> (qemu-image): Based on base-inputs, generate grub.cfg and os-drv.
> (system-disk-image, system-qemu-image, system-qemu-image/shared-store):
> Pass in the operating system configuration and base-inputs to qemu-image
> instead of derivations.
One of the reasons ‘os-drv’ and ‘grub.cfg’ are passed around is that
recomputing them is relatively costly.
There’s a solution to that in 4c2ade20c65e94c41dc8c65db73dd128343a0ad5
(in ‘wip-build-systems-gexp’; it memoizes ‘operating-system-derivation’
and others), so we could almost consider it solved.
Nevertheless it’d be nice to make sure performance remains reasonable
even in the absence of the above commit.
> + (base-inputs (list 'grub.cfg 'system))
[...]
> + (mlet* %store-monad ((os-drv (operating-system-derivation os-configuration))
> + (grub.cfg (operating-system-grub.cfg os-configuration))
> + (inputs -> (append
> + (if (member 'grub.cfg base-inputs)
> + `(("grub.cfg" ,grub.cfg)) '())
> + (if (member 'system base-inputs)
> + `(("system" ,os-drv)) '())
Use of “magic” values like 'grub.cfg here is undesirable IMO, because it
introduces singularities in the API, and generally makes the interface
non-obvious.
So I think I’d leave it up to the caller to pass
#:inputs `(("grub.cfg" ,grub.cfg))
Same for "system".
All in all, I sympathize with the desire to avoid passing OS-DRV and
GRUB.CFG around, but I’m not convinced that this approach can improve
the situation.
WDYT?
Thank you!
Ludo’.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vm: Have qemu-image generate derivations instead.
2016-03-29 19:46 ` Ludovic Courtès
@ 2016-03-29 23:13 ` Jookia
2016-04-11 22:21 ` Jookia
0 siblings, 1 reply; 6+ messages in thread
From: Jookia @ 2016-03-29 23:13 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: guix-devel
On Tue, Mar 29, 2016 at 09:46:14PM +0200, Ludovic Courtès wrote:
> Damn, it’s been more than a month, please accept my apologies!
No problem!
> One of the reasons ‘os-drv’ and ‘grub.cfg’ are passed around is that
> recomputing them is relatively costly.
>
> There’s a solution to that in 4c2ade20c65e94c41dc8c65db73dd128343a0ad5
> (in ‘wip-build-systems-gexp’; it memoizes ‘operating-system-derivation’
> and others), so we could almost consider it solved.
>
> Nevertheless it’d be nice to make sure performance remains reasonable
> even in the absence of the above commit.
I thought it was only recomputed and used in once place now?
> > + (base-inputs (list 'grub.cfg 'system))
>
> [...]
>
> > + (mlet* %store-monad ((os-drv (operating-system-derivation os-configuration))
> > + (grub.cfg (operating-system-grub.cfg os-configuration))
> > + (inputs -> (append
> > + (if (member 'grub.cfg base-inputs)
> > + `(("grub.cfg" ,grub.cfg)) '())
> > + (if (member 'system base-inputs)
> > + `(("system" ,os-drv)) '())
>
> Use of “magic” values like 'grub.cfg here is undesirable IMO, because it
> introduces singularities in the API, and generally makes the interface
> non-obvious.
How much more non-obvious than how it is now with having to pass certain inputs?
But yes, you're right. If I was doing a bigger refactor I would've added them as
flags to the function.
> So I think I’d leave it up to the caller to pass
>
> #:inputs `(("grub.cfg" ,grub.cfg))
>
> Same for "system".
>
> All in all, I sympathize with the desire to avoid passing OS-DRV and
> GRUB.CFG around, but I’m not convinced that this approach can improve
> the situation.
>
> WDYT?
Well, I'm working here on the assumption that callers shouldn't have to know
about the contents of their os-configuration, specifically in this situation
which bootloader is being used. This is part of a bigger effort to organize
things a bit better so different bootloaders could be switched out. Having to
pass these things violate this assumption.
> Thank you!
>
> Ludo’.
Thank you,
Jookia.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vm: Have qemu-image generate derivations instead.
2016-03-29 23:13 ` Jookia
@ 2016-04-11 22:21 ` Jookia
2016-04-12 0:01 ` Mathieu Lirzin
2016-04-26 9:23 ` Ludovic Courtès
0 siblings, 2 replies; 6+ messages in thread
From: Jookia @ 2016-04-11 22:21 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: guix-devel
I'm hesitant to write this email so I'll make it quick:
I'm withdrawing from submitting this patch and all others of mine such as proxy
support and Libreboot support. The process of getting changes reviewed and
incorporated in to upstream isn't suited for me, the tooling just isn't there
yet and participating in IRC makes me sad. I'm going to just create a downstream
fork of Guix somewhere with those features and put my patches there without
worrying too much. If something's really needed, someone else can upstream it.
Apologies,
Jookia.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vm: Have qemu-image generate derivations instead.
2016-04-11 22:21 ` Jookia
@ 2016-04-12 0:01 ` Mathieu Lirzin
2016-04-26 9:23 ` Ludovic Courtès
1 sibling, 0 replies; 6+ messages in thread
From: Mathieu Lirzin @ 2016-04-12 0:01 UTC (permalink / raw)
To: Jookia; +Cc: guix-devel
Hello,
Jookia <166291@gmail.com> writes:
> I'm hesitant to write this email so I'll make it quick:
>
> I'm withdrawing from submitting this patch and all others of mine such as proxy
> support and Libreboot support.
Too bad. :(
> The process of getting changes reviewed and incorporated in to
> upstream isn't suited for me, the tooling just isn't there yet and
> participating in IRC makes me sad.
What doesn't suits you? IME Guix is a welcoming community, so I think
you can speak freely about things that you think are not working well
without expecting aggressive reaction. So if you feel up to, please
don't make it "quick".
Taking a look at this thread, I have seen that you didn't get quick
answers. If this is the main problem then pinging the thread regularly
(once a week for example) is not considered bad behavior.
> I'm going to just create a downstream fork of Guix somewhere with
> those features and put my patches there without worrying too much. If
> something's really needed, someone else can upstream it.
if waiting for patch reviews blocks you in your workflow, this could
perhaps work better. However if you choose this path please take some
time to follow Guix coding style (indentation + changelog style commits)
and notify the list when you achieve something. It would help things to
get merged then.
--
Mathieu Lirzin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vm: Have qemu-image generate derivations instead.
2016-04-11 22:21 ` Jookia
2016-04-12 0:01 ` Mathieu Lirzin
@ 2016-04-26 9:23 ` Ludovic Courtès
1 sibling, 0 replies; 6+ messages in thread
From: Ludovic Courtès @ 2016-04-26 9:23 UTC (permalink / raw)
To: Jookia; +Cc: guix-devel
[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]
Hi Jookia,
Jookia <166291@gmail.com> skribis:
> I'm withdrawing from submitting this patch and all others of mine such as proxy
> support and Libreboot support. The process of getting changes reviewed and
> incorporated in to upstream isn't suited for me, the tooling just isn't there
> yet and participating in IRC makes me sad. I'm going to just create a downstream
> fork of Guix somewhere with those features and put my patches there without
> worrying too much. If something's really needed, someone else can upstream it.
As you can see, it took me a while to reply to even this simple message,
and I apologize for that.
It takes a lot of time to review “difficult” patches like this one. A
patch tracker would help, but not with the review work itself.
It saddens me to see you leave. I realize that the slowness and
tediousness of our process is at fault, and that we are losing valuable
contributions in an area that really needs them.
I think that more and more people are getting a handle on more parts of
the code, and that is what will help the project scale better.
I hope we can keep in touch and work together again when you deem
appropriate.
Thank you for your contributions,
Ludo’.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-04-26 9:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-11 11:41 [PATCH] vm: Have qemu-image generate derivations instead Jookia
2016-03-29 19:46 ` Ludovic Courtès
2016-03-29 23:13 ` Jookia
2016-04-11 22:21 ` Jookia
2016-04-12 0:01 ` Mathieu Lirzin
2016-04-26 9:23 ` Ludovic Courtès
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).