[v2,6/9] ts-xen-build: Build the livepatch test-cases

Commit Message

Konrad Rzeszutek Wilk May 18, 2017, 12:07 a.m. UTC

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:


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.

Comments

Konrad Rzeszutek Wilk May 17, 2017, 8:30 p.m. UTC | #1

On Wed, May 17, 2017 at 08:07:58PM -0400, Konrad Rzeszutek Wilk wrote:
> 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.


<sighs>

If I do:

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

On older Xen versions (Xen 4.4) I get this:

*** something failed:                                                           
                                                                                
status 256 at Osstest/TestSupport.pm line 444.                                  
                                                                                
** something failed at ./ts-xen-build line 284.                                 

So I feel like the only way to figure out whether the livepatch tests cases
can be built is if I check either the version of Xen (4.9 or above say)
or if an file exists (xen/xen/test/Makefile).

Similar to how ovm_enable or xsm_enable is constructed.
Let me do that.

Ian Jackson May 18, 2017, 4:41 p.m. UTC | #2

Konrad Rzeszutek Wilk writes ("Re: [PATCH v2 6/9] ts-xen-build: Build the livepatch test-cases"):
> So I feel like the only way to figure out whether the livepatch tests cases
> can be built is if I check either the version of Xen (4.9 or above say)
> or if an file exists (xen/xen/test/Makefile).

> Similar to how ovm_enable or xsm_enable is constructed.
> Let me do that.

Yes, this is the right approach.  I would test for a file.

Ian.

Patch

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