Re: [Xen-devel] [PATCH v2 6/9] ts-xen-build: Build the livepatch test-cases

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

On Tue, Dec 13, 2016 at 04:49:25PM +0000, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("[PATCH v2 6/9] ts-xen-build: Build the 
> livepatch test-cases"):
> > +    buildcmd_stamped_logged(600, 'xen', 'xenlpt-build', '',<<END,'') if 
> > $dokconfig;
> > +        if test -d xen/test; then
> > +            $make_prefix make -C xen tests
> > +        fi
> 
> Is $dokconfig really the right test for whether the livepatch build
> should be attempted ?  It seems like a rather arbitrary connection.

The earlier patch (ts-xen-build: Enable livepatch.) enables the
correct .config option to make this work. Without that you wouldn't
be able to enable livepatching.

And it looks like dokconfig gets changed to zero if --no-kconfig is
supplied which I presume happens to older Xen versions.
> 
> > +    buildcmd_stamped_logged(600, 'xen', 'xenlpt-install', '',<<END,'') if 
> > $dokconfig;
> > +        if test -d xen/test; then
> > +           mkdir -p dist/xenlptinstall/usr/lib/debug
> > +           livepatch_files=`find xen/test/livepatch -name '*.livepatch' 
> > -print`
> > +           cp \$livepatch_files dist/xenlptinstall/usr/lib/debug
> > +        fi
> 
> As I say, I don't much like this.  There's a conversation ongoing
> about it.


[tries to recall it]
It was about the make install stanza in the top root Makefile. And Jan
was not too thrilled about 'make install' installing the test-cases.

But I wonder, what if we had 'make -C xen/tests install' or such?
That _may_ work? (Depending on whether the xen/tests Makefile can pick
up the proper variables and such from the 'xen', this may require also
an -f Rules.mk or such?)

Something like this invocation:

DESTDIR=`pwd`/dist/xenlptinstall/usr/lib/debug
mkdir -p $DESTDIR
BASEDIR=`pwd`/xen XEN_ROOT=`pwd` make -C xen/test -f `pwd`/xen/Rules.mk install

And this diff to Xen:

diff --git a/xen/test/Makefile b/xen/test/Makefile
index d91b319..f9d90da 100644
--- a/xen/test/Makefile
+++ b/xen/test/Makefile
@@ -5,3 +5,8 @@ tests:
 .PHONY: clean
 clean::
        $(MAKE) -f $(BASEDIR)/Rules.mk -C livepatch clean
+
+.PHONY: install
+install:
+       $(MAKE) -f $(BASEDIR)/Rules.mk -C livepatch install


seems to work.

> 
> >  sub stash () {
> > -    foreach my $part ('', 'xen') {
> > +    foreach my $part ('', 'xen', 'xenlpt') {
> >     if (target_dir_exists($ho, "$builddir/xen/dist/${part}install")) {
> >               built_stash($ho, $builddir,
> 
> I don't much like this approach.  It might result in deferring certain
> failures undesirably.
> 
> Also, I don't know why it is necessary to look on the build box for
> this information.  ts-xen-build ought to know whether it has run `make
> xenlpt-tests-install' (or whatever it is), so it ought to simply know
> whether to do the build_stash.

And when you say 'xenlpt-tests-install' you mean 'xenlpt-install' (see
above).

So .. the one thing I am having a hard time is that certain versions
of Xen would not be able to build livepatches.

So how I determine that? If I do 'make -C xen tests' on older versions
it would return a failure. But I don't see how buildcmd_stamped_logged
reports that? Oh wait, it gives 'echo ok' so I should just do
something like:

    my $ok = buildcmd_stamped_logged(600, 'xen', 'xenlpt-build', '',<<END,'') 
if $dokconfig;
        $make_prefix make -C xen tests
END

    if ($ok =~ m/ok/) {
            store_runvar("livepatch", "built");

        buildcmd_stamped_logged(600, 'xen', 'xenlpt-install', <<END,<<END,'')
            XEN_ROOT=$builddir
            DESTDIR=$builddir/dist/xenlptinstall/usr/lib/debug
            BASEDIR=$builddir/xen
            mkdir -p \${DESTDIR}
END
            $make_prefix -C xen/test -f $builddir/xen/Rules.mk install
END

    }

And then I get get_runvar("livepatch") to figure out whether it was
actually built.

> 
> You could instead do something like
> 
>     our %skip_stash_part;
>     ...
>     if (some condition) {
>         make xenlpt-install
>     } else {
>         $skip_stash_part{xenlpttest}= 1;
>     }
>     ...
>     next if $skip_stash_part{$part}

Or have an
$stash_livepatch=0

And set it to 1 if the built worked?
> 
> or an ad-hoc variable, giving
> 
>     next if $part eq $xenlpttest && !$do_xenlpt;
> 
> or something ?
> 
> Thanks
> Ian.

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