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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 31 Jul 2015, Ian Jackson wrote:
> (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.

OK


> > +    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;;
> 
> ?

OK


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

I disagree on this: some things are not worth optimizing.

Nonetheless I would make the changes you are requesting, but as
explained in the past, what you are asking is beyond my knowledge of the
language. Furthermore the snippet of code that was provided to me on the
13th of May did not actually work.

In conclusion, given that this is v8, I'll leave this code as is. Feel
free to change it the way you think is best.



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

OK


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

I guess you mean the list of revisions to store? As in xen, qemu, qemuu,
seabios, ovmf, libvirt? The list is currently hardcoded and names of the
corresponding repositories are not regular, so for similar reasons as
above I would prefer to keep it as is.

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