Re: [Xen-devel] [PATCH v5] OSSTEST: introduce a raisin build test

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-05-12 at 19:04 +0100, Stefano Stabellini wrote:
> On Tue, 12 May 2015, Ian Jackson wrote:
> > Stefano Stabellini writes ("[PATCH v5] OSSTEST: introduce a raisin build 
> > test"):
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ...
> > > +    echo >>config XEN_URL=\\"$r{tree_xen}\\"
> > > +    echo >>config XEN_REVISION=\\"$r{revision_xen}\\"
> > 
> > This is very repetitive.  In ts-xen-build, the names of the variables
> > are irregular, but here they are regular.  I think you should refactor
> > this accordingly.
> 
> I think that the cure here would be worse than the disease. In bash
> would be fragile and difficult to read, but you probably would do it in
> perl, right? In that case I don't know how it would look like; in fact I
> wouldn't know how to write it. However if you are keen on it, feel free
> to provide a snippet of code and I'll try to include it in the patch.

Something like:

foreach (qw(xen foo bar)) {
       (nonempty($r{tree_$_}) && nonempty($r{revision_$_}) ? <<END : '').
    echo >>config ".uc($_)."_URL=\\"$r{tree_$_}\\"
    echo >>config ".uc($_)."_REVISION=\\"$r{revision_$_}\\"
+END
}
I think.

> 
> 
> > I don't understand what the \\ are doing here.  Perhaps you should use
> > '' like in ts-xen-build ?
> 
> I need to retain the " in the output

If you use ' as your quote character then you do not need to escape uses
of ". However I think that would stop $r{tree_$_} being evaluated too,
so you would need to do like I did with uc above and use . to paste
strings together, which may not be an overall improvement.

> > > +sub divide () {
> > > +    # Only move hv to xeninstall, so that we can have
> > > +    # xenpolicy in tools tarball.
> > > +    #
> > > +    # The files inside boot/ after `make dist' are
> > > +    # xen-$XEN_VERSION: Xen binary
> > > +    # xen.gz/xen: symlink to xen-$XEN_VERSION
> > > +    # xen-$MAJOR: symlink to xen-$XEN_VERSION
> > > +    # xen-$MAJOR.$MINOR: symlink to xen-$XEN_VERSION
> > > +    # xen-sym-$XEN_VERSION: Xen symbol
> > > +    # xenpolicy-$XEN_VERSION: flask policy binary if xsm is enabled
> > > +    #
> > > +    # So the following snippet will leave xenpolicy* in
> > > +    # install/boot and get packaged to tools tarball.
> > > +    target_cmd_build($ho, 100, $builddir, <<END);
> > > +        cd raisin
> > > +        mkdir xendist
> > > +        for f in *dist; do
> > > +            mkdir -p \$f/lib
> > > +        done
> > > +        if test -d dist/boot; then
> > > +            if test -f dist/boot/xen.gz || test -f dist/boot/xen; then
> > > +                mkdir xendist/boot
> > > +                mvfiles=`find dist/boot -name 'xen[a-z]*' -prune -o 
> > > -name 'xen*' -print`
> > > +                mv \$mvfiles xendist/boot/.
> > 
> > This, and much of stash(), is a clone-and-hack of ts-xen-build.
> > 
> > > +our @probs;
> > > +
> > > +sub trapping ($) {
> > > +    my ($sub) = @_;
> > > +    my $tok= eval { $sub->(); 1; };
> > > +    if (!$tok) { push @probs, $@; print STDERR "failure (trapped): 
> > > $@\n"; }
> > > +}
> > 
> > Again, this is copied from ts-xen-build.
> 
> Yes, it is. If this is not a descriptive comment, what would you have me
> do? I could move trapping somewhere else common, but I don't think that
> generalizing divide and stash is a good idea.

Osstest/Testsupport.pm is a suitable home for any random shared code I
think, even if only shared between two ts-*. Or Osstest/Buildsupport.pm
might be a better choice in this case.

Thinking back to the issue about dividing the raisin build into per
components though, perhaps use_raisin should become a runvar which is
used in ts-*-build to either do the existing thing vs the new raisin
thing? Then stash+divide etc would only be used in the one place, just
consuming a potentially different dist dir output from the build phase.

Ian.


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