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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Mon, 2015-08-03 at 17:59 +0100, Stefano Stabellini wrote:
> 
> > > +    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.

This is not an optimisation, it is a maintainability improvement, having
multiple places which operate in an open coded way on a list of things
which are all supposed to remain in sync just means they will get out of
sync.

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

Perl is really not _so_ obscure or hard that this is much of an excuse,
even if you don't want to write it in a Perl-ish way you could get this
done just using constructs common to most languages, i.e. hashes, arrays
and loops.

Nevertheless, since this seems to be going nowhere I'll take your series
and fix this up for you.

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

The fact we have made it to v8 without a piece of review commentary being
implemented is not a reason to continue to ignore it, nor to just accept
the patch as is IMHO.

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

As above, I'll fix it.

Ian.


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