Re: [Xen-devel] [PATCH v8 1/2] OSSTEST: introduce a raisin build test

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

(Please disregard my previous incomplete mail about this patch.  This
reply has all of my comments.  Thanks.)

Stefano Stabellini writes ("[PATCH v8 1/2] OSSTEST: introduce a raisin build 
test"):
> Make divide_xen_build common between ts-raisin-build and ts-xen-build.
> It is used to split the build output in two archives: one with the
> hypervisor binary and another with the userspace tools.

This refactoring should be in its own patch.

> +    case "$xenbranch" in
> +    xen-3.*-testing) enable_raisin=false;;
> +    xen-4.0-testing) enable_raisin=false;;
> +    xen-4.1-testing) enable_raisin=false;;
> +    xen-4.2-testing) enable_raisin=false;;
> +    xen-4.3-testing) enable_raisin=false;;
> +    xen-4.4-testing) enable_raisin=false;;
> +    xen-4.5-testing) enable_raisin=false;;

How about

> +    xen-4.[0-5]-testing) enable_raisin=false;;

?

> +    echo >>config ENABLED_COMPONENTS=\\"seabios ovmf xen qemu 
> qemu_traditional libvirt\\"
> +END
> +               (nonempty($r{tree_xen}) && nonempty($r{revision_xen}) ? <<END 
> : '').
> +    echo >>config XEN_URL=\\"$r{tree_xen}\\"
> +    echo >>config XEN_REVISION=\\"$r{revision_xen}\\"
> +END

This URL/REVISION setting is too repetetive.  If you improved it you
could combine it with your setting of ENABLED_COMPONENTS.

It's true that qemuu/QEMU_URL and qemu/QEMU_TRADITIONAL_URL etc. need
special handling but their specialness should be minimised.  Maybe an
anonymous subref for generating each stanza, which you call with
matching arguments in a loop for most components, and explicitly with
nonmatching arguments for the two qemus.

> +sub checkoutput () {
> +    target_cmd_build($ho, 100, $builddir, <<END);
> +            cd $raisindir/dist
> +            ls boot/xen.gz
> +            ls usr/sbin/xenstored
> +            ls usr/sbin/xenconsoled
> +            ls usr/lib/libxenlight.so
> +            ls usr/sbin/xl
> +            ls usr/lib/xen/boot/hvmloader
> +            ls usr/lib/xen/bin/qemu-dm
> +            ls usr/lib/xen/bin/qemu-system-i386
> +            ls usr/sbin/libvirtd
> +END

I'm not sure why not `ls -l'.

> +sub collectversions () {
> +    store_revision($ho, 'xen', "$raisindir/xen-dir", 1);
> +    store_revision($ho, 'qemu', "$raisindir/qemu-traditional-dir", 1);
> +    store_revision($ho, 'qemuu', "$raisindir/qemu-dir", 1);
> +    store_revision($ho, 'seabios', "$raisindir/seabios-dir", 1);
> +    store_revision($ho, 'ovmf', "$raisindir/ovmf-dir", 1);
> +    store_revision($ho, 'libvirt', "$raisindir/libvirt-dir", 1);

This list of revisions should come from the same place as the list of
config settings, above.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel