[PATCH v2] scripts: introduce a script for build test

[PATCH v2] scripts: introduce a script for build test

[Wei Liu]

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
 scripts/build-test.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100755 scripts/build-test.sh

diff --git a/scripts/build-test.sh b/scripts/build-test.sh
new file mode 100755
index 0000000000..316419d6b7
--- /dev/null
+++ b/scripts/build-test.sh
@@ -0,0 +1,53 @@
+#!/bin/sh
+
+# Run command on every commit within the range specified. If no command is
+# provided, use the default one to clean and build the whole tree.
+#
+# Cross-build is not yet supported.
+
+set -e
+
+if ! test -f xen/common/kernel.c; then
+    echo "Please run this script from top-level directory"
+    exit 1
+fi
+
+if test $# -lt 2 ; then
+    echo "Usage: $0 <BASE> <TIP> [CMD]"
+    exit 1
+fi
+
+status=`git status -s`
+if test -n "$status"; then
+    echo "Tree is dirty, aborted"
+    exit 1
+fi
+
+if git branch | grep -q '^\*.\+detached at'; then
+    echo "Detached HEAD, aborted"
+    exit 1
+fi
+
+BASE=$1; shift
+TIP=$1; shift
+ORIG_BRANCH=`git rev-parse --abbrev-ref HEAD`
+
+if ! git merge-base --is-ancestor $BASE $TIP; then
+    echo "$BASE is not an ancestor of $TIP, aborted"
+    exit 1
+fi
+
+git rev-list $BASE..$TIP | nl -ba | tac | \
+while read num rev; do
+    echo "Testing $num $rev"
+    git checkout $rev
+    if test $# -eq 0 ; then
+        make -j4 distclean && ./configure && make -j4
+    else
+        "$@"
+    fi
+    echo
+done
+
+echo "Restoring original HEAD"
+git checkout $ORIG_BRANCH
-- 
2.11.0


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

Re: [PATCH v2] scripts: introduce a script for build test

[Ian Jackson]

Wei Liu writes ("[PATCH v2] scripts: introduce a script for build test"):
...
> +if git branch | grep -q '^\*.\+detached at'; then

You mean some rune involving git-symbolic-ref.

git-symbolic-ref -q HEAD exits with status 1 if HEAD is detached, 0 if
HEAD is a branch, or some other status in case of trouble.

But you could combine this test with your ORIG_BRANCH thing, and just
say
  git-symbolic-ref HEAD
which exits 128 if HEAD is detached.

> +if ! git merge-base --is-ancestor $BASE $TIP; then
> +    echo "$BASE is not an ancestor of $TIP, aborted"
> +    exit 1
> +fi

I would remove this check.  There is nothing wrong with asking "does
this branch build everywhere" even if it hasn't rebased yet.

The rest LGTM.

Ian.

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

Re: [PATCH v2] scripts: introduce a script for build test

[Anthony PERARD]

On Mon, Oct 23, 2017 at 05:56:33PM +0100, Wei Liu wrote:
> +
> +if test $# -lt 2 ; then
> +    echo "Usage: $0 <BASE> <TIP> [CMD]"
> +    exit 1
> +fi
[...]
> +git rev-list $BASE..$TIP | nl -ba | tac | \
> +while read num rev; do
> +    echo "Testing $num $rev"
> +    git checkout $rev
> +    if test $# -eq 0 ; then
> +        make -j4 distclean && ./configure && make -j4
> +    else
> +        "$@"

That feels wrong. How do I run the same exact command at the default
one, but with -j8 instead of -j4?

I can see only two options, but I'm not sure if it is what you had in
mind:
- Option #1: a script
$ echo 'make -j8 distclean && ./configure && make -j8' > tmp-script.sh
$ ./script/build-test.sh master my-feature bash tmp-script.sh

- Option #2: with eval!
$ ./script/build-test.sh master my-feature eval make -j8 distclean '&&' ./configure '&&' make -j8
# notice the eval ................... here ^^^^ :-)

> +    fi
> +    echo
> +done
> +
> +echo "Restoring original HEAD"
> +git checkout $ORIG_BRANCH


Also, what a developper should do when the build fail?  She can't modify
the current code, because changes are going to be losts.  Maybe we could
trap failures, restore original HEAD and point out which commit fails to
build.


Another thing that can be done is do the build test in a temporary
checkout, but I'm not sure if it is a good idea.

(I'm still trying to find out how a script can do a better job than a
plain `git rebase --interactive --exec 'blah'`, it is maybe just because
I know what to do if there is an issue.)

-- 
Anthony PERARD

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

Re: [PATCH v2] scripts: introduce a script for build test

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
> That feels wrong. How do I run the same exact command at the default
> one, but with -j8 instead of -j4?

 .../build-test sh -ec make -j4 distclean && ./configure && make -j4

But I think Anthony has a point.  The clean should 1. be git-clean,
not make distclean 2. be run anyway.

> > +echo "Restoring original HEAD"
> > +git checkout $ORIG_BRANCH
> 
> Also, what a developper should do when the build fail?  She can't modify
> the current code, because changes are going to be losts.  Maybe we could
> trap failures, restore original HEAD and point out which commit fails to
> build.

IWBNI it would at least print the branch to checkout.  Tools like "git
bisect" record the information in .git and allow "git bisect reset".

Ian.

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

Re: [PATCH v2] scripts: introduce a script for build test

[Wei Liu]

On Tue, Oct 24, 2017 at 12:29:32PM +0100, Anthony PERARD wrote:
> On Mon, Oct 23, 2017 at 05:56:33PM +0100, Wei Liu wrote:
> > +
> > +if test $# -lt 2 ; then
> > +    echo "Usage: $0 <BASE> <TIP> [CMD]"
> > +    exit 1
> > +fi
> [...]
> > +git rev-list $BASE..$TIP | nl -ba | tac | \
> > +while read num rev; do
> > +    echo "Testing $num $rev"
> > +    git checkout $rev
> > +    if test $# -eq 0 ; then
> > +        make -j4 distclean && ./configure && make -j4
> > +    else
> > +        "$@"
> 
> That feels wrong. How do I run the same exact command at the default
> one, but with -j8 instead of -j4?
> 
> I can see only two options, but I'm not sure if it is what you had in
> mind:
> - Option #1: a script
> $ echo 'make -j8 distclean && ./configure && make -j8' > tmp-script.sh
> $ ./script/build-test.sh master my-feature bash tmp-script.sh

This is what I had in mind.

> 
> - Option #2: with eval!
> $ ./script/build-test.sh master my-feature eval make -j8 distclean '&&' ./configure '&&' make -j8
> # notice the eval ................... here ^^^^ :-)
> 
> > +    fi
> > +    echo
> > +done
> > +
> > +echo "Restoring original HEAD"
> > +git checkout $ORIG_BRANCH
> 
> 
> Also, what a developper should do when the build fail?  She can't modify
> the current code, because changes are going to be losts.  Maybe we could
> trap failures, restore original HEAD and point out which commit fails to
> build.
> 
> 
> Another thing that can be done is do the build test in a temporary
> checkout, but I'm not sure if it is a good idea.
> 
> (I'm still trying to find out how a script can do a better job than a
> plain `git rebase --interactive --exec 'blah'`, it is maybe just because
> I know what to do if there is an issue.)
> 

Frankly I myself would probably use git-rebase so that I can fix things
on the fly, but I want to point contributors to something safer. And I'm
tired of typing the same "ICYMI git-rebase can do this this this to
build test your branch".

> -- 
> Anthony PERARD

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

Re: [PATCH v2] scripts: introduce a script for build test

[Wei Liu]

On Tue, Oct 24, 2017 at 02:38:39PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
> > That feels wrong. How do I run the same exact command at the default
> > one, but with -j8 instead of -j4?
> 
>  .../build-test sh -ec make -j4 distclean && ./configure && make -j4
> 
> But I think Anthony has a point.  The clean should 1. be git-clean,
> not make distclean 2. be run anyway.
> 

I don't think we should call git-clean unconditionally -- imagine
someone knew for sure they only needed to build part of the tools or the
hypervisor.

> > > +echo "Restoring original HEAD"
> > > +git checkout $ORIG_BRANCH
> > 
> > Also, what a developper should do when the build fail?  She can't modify
> > the current code, because changes are going to be losts.  Maybe we could
> > trap failures, restore original HEAD and point out which commit fails to
> > build.
> 
> IWBNI it would at least print the branch to checkout.  Tools like "git
> bisect" record the information in .git and allow "git bisect reset".

Not sure I follow. Do you want the script to trap SIGCHLD, test the
return value and act accordingly?

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

Re: [PATCH v2] scripts: introduce a script for build test

[Anthony PERARD]

On Wed, Oct 25, 2017 at 04:17:26PM +0100, Wei Liu wrote:
> On Tue, Oct 24, 2017 at 02:38:39PM +0100, Ian Jackson wrote:
> > > Also, what a developper should do when the build fail?  She can't modify
> > > the current code, because changes are going to be losts.  Maybe we could
> > > trap failures, restore original HEAD and point out which commit fails to
> > > build.
> > 
> > IWBNI it would at least print the branch to checkout.  Tools like "git
> > bisect" record the information in .git and allow "git bisect reset".
> 
> Not sure I follow. Do you want the script to trap SIGCHLD, test the
> return value and act accordingly?

In scripts with `set -e`, `trap 'echo something failed' EXIT` is enough.
But that is probably not necessary here, you could just check the return
value of the build command, then start printing imformation about
what/where it went wrong, and how to go back.

-- 
Anthony PERARD

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

Re: [PATCH v2] scripts: introduce a script for build test

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
> On Tue, Oct 24, 2017 at 02:38:39PM +0100, Ian Jackson wrote:
> > Anthony PERARD writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
> > > That feels wrong. How do I run the same exact command at the default
> > > one, but with -j8 instead of -j4?
> > 
> >  .../build-test sh -ec make -j4 distclean && ./configure && make -j4
> > 
> > But I think Anthony has a point.  The clean should 1. be git-clean,
> > not make distclean 2. be run anyway.
> 
> I don't think we should call git-clean unconditionally -- imagine
> someone knew for sure they only needed to build part of the tools or the
> hypervisor.

If you are worried about this you should check that there are no
uncommitted files before starting.

I have a visceral loathing of clean targets.  They are often flaky,
and ours are no exception.

> > > > +echo "Restoring original HEAD"
> > > > +git checkout $ORIG_BRANCH
> > > 
> > > Also, what a developper should do when the build fail?  She can't modify
> > > the current code, because changes are going to be losts.  Maybe we could
> > > trap failures, restore original HEAD and point out which commit fails to
> > > build.
> > 
> > IWBNI it would at least print the branch to checkout.  Tools like "git
> > bisect" record the information in .git and allow "git bisect reset".
> 
> Not sure I follow. Do you want the script to trap SIGCHLD, test the
> return value and act accordingly?

I don't mean you should trap SIGCHLD.

But you should probably trap '' and use it to print a helpful message
containing ORIG_BRANCH.  On success you would disable the trap before
exiting.

Alternatively you could defuse `set -e' around the invocation of the
build command, and handle $? explicitly.

Ian.

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

Re: [PATCH v2] scripts: introduce a script for build test

[Wei Liu]

On Wed, Oct 25, 2017 at 04:25:21PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
> > On Tue, Oct 24, 2017 at 02:38:39PM +0100, Ian Jackson wrote:
> > > Anthony PERARD writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
> > > > That feels wrong. How do I run the same exact command at the default
> > > > one, but with -j8 instead of -j4?
> > > 
> > >  .../build-test sh -ec make -j4 distclean && ./configure && make -j4
> > > 
> > > But I think Anthony has a point.  The clean should 1. be git-clean,
> > > not make distclean 2. be run anyway.
> > 
> > I don't think we should call git-clean unconditionally -- imagine
> > someone knew for sure they only needed to build part of the tools or the
> > hypervisor.
> 
> If you are worried about this you should check that there are no
> uncommitted files before starting.

This is already done in this version.

I don't worry if there is uncommitted file, I just don't want to stop
developers from being smarter than the script when they know git-clean
is not necessary.

> 
> I have a visceral loathing of clean targets.  They are often flaky,
> and ours are no exception.
> 
> > > > > +echo "Restoring original HEAD"
> > > > > +git checkout $ORIG_BRANCH
> > > > 
> > > > Also, what a developper should do when the build fail?  She can't modify
> > > > the current code, because changes are going to be losts.  Maybe we could
> > > > trap failures, restore original HEAD and point out which commit fails to
> > > > build.
> > > 
> > > IWBNI it would at least print the branch to checkout.  Tools like "git
> > > bisect" record the information in .git and allow "git bisect reset".
> > 
> > Not sure I follow. Do you want the script to trap SIGCHLD, test the
> > return value and act accordingly?
> 
> I don't mean you should trap SIGCHLD.
> 
> But you should probably trap '' and use it to print a helpful message
> containing ORIG_BRANCH.  On success you would disable the trap before
> exiting.
> 
> Alternatively you could defuse `set -e' around the invocation of the
> build command, and handle $? explicitly.
> 

OK, that sounds easy enough.

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

Re: [PATCH v2] scripts: introduce a script for build test

[Wei Liu]

On Wed, Oct 25, 2017 at 04:23:35PM +0100, Anthony PERARD wrote:
> On Wed, Oct 25, 2017 at 04:17:26PM +0100, Wei Liu wrote:
> > On Tue, Oct 24, 2017 at 02:38:39PM +0100, Ian Jackson wrote:
> > > > Also, what a developper should do when the build fail?  She can't modify
> > > > the current code, because changes are going to be losts.  Maybe we could
> > > > trap failures, restore original HEAD and point out which commit fails to
> > > > build.
> > > 
> > > IWBNI it would at least print the branch to checkout.  Tools like "git
> > > bisect" record the information in .git and allow "git bisect reset".
> > 
> > Not sure I follow. Do you want the script to trap SIGCHLD, test the
> > return value and act accordingly?
> 
> In scripts with `set -e`, `trap 'echo something failed' EXIT` is enough.
> But that is probably not necessary here, you could just check the return
> value of the build command, then start printing imformation about
> what/where it went wrong, and how to go back.

Either is fine. Thanks for the suggestion.

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

Re: [PATCH v2] scripts: introduce a script for build test

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
> On Wed, Oct 25, 2017 at 04:25:21PM +0100, Ian Jackson wrote:
> > If you are worried about this you should check that there are no
> > uncommitted files before starting.
> 
> This is already done in this version.
> 
> I don't worry if there is uncommitted file, I just don't want to stop
> developers from being smarter than the script when they know git-clean
> is not necessary.

I don't understand your point.  If there are no uncommitted files at
the start of the run, then git clean is certainly safe later, since
everything that will be deleted was made by `make'.  Therefore doing
it unconditionally is fine.

Ian.

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

Re: [PATCH v2] scripts: introduce a script for build test

[Wei Liu]

On Wed, Oct 25, 2017 at 04:42:44PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
> > On Wed, Oct 25, 2017 at 04:25:21PM +0100, Ian Jackson wrote:
> > > If you are worried about this you should check that there are no
> > > uncommitted files before starting.
> > 
> > This is already done in this version.
> > 
> > I don't worry if there is uncommitted file, I just don't want to stop
> > developers from being smarter than the script when they know git-clean
> > is not necessary.
> 
> I don't understand your point.  If there are no uncommitted files at
> the start of the run, then git clean is certainly safe later, since
> everything that will be deleted was made by `make'.  Therefore doing
> it unconditionally is fine.

I mean I don't want git-clean to delete all the object files so that they
don't need to be rebuilt.

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

Re: [PATCH v2] scripts: introduce a script for build test

[George Dunlap]

On 10/25/2017 04:27 PM, Wei Liu wrote:
> On Wed, Oct 25, 2017 at 04:25:21PM +0100, Ian Jackson wrote:
>> Wei Liu writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
>>> On Tue, Oct 24, 2017 at 02:38:39PM +0100, Ian Jackson wrote:
>>>> Anthony PERARD writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
>>>>> That feels wrong. How do I run the same exact command at the default
>>>>> one, but with -j8 instead of -j4?
>>>>
>>>>  .../build-test sh -ec make -j4 distclean && ./configure && make -j4
>>>>
>>>> But I think Anthony has a point.  The clean should 1. be git-clean,
>>>> not make distclean 2. be run anyway.
>>>
>>> I don't think we should call git-clean unconditionally -- imagine
>>> someone knew for sure they only needed to build part of the tools or the
>>> hypervisor.
>>
>> If you are worried about this you should check that there are no
>> uncommitted files before starting.
> 
> This is already done in this version.
> 
> I don't worry if there is uncommitted file, I just don't want to stop
> developers from being smarter than the script when they know git-clean
> is not necessary.

What kind of "smarter" did you have in mind?

This script sounds like an aid to developers who don't have the
motivation / experience / whatever to write their own script (or do
something fancier, like git rebase --exec).  If people want to be
smarter they can write their own script, using this as a starting point.

FWIW in xsatool I use 'git clean' extensively.

 -George

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

Re: [PATCH v2] scripts: introduce a script for build test

[Wei Liu]

On Wed, Oct 25, 2017 at 04:47:33PM +0100, George Dunlap wrote:
> On 10/25/2017 04:27 PM, Wei Liu wrote:
> > On Wed, Oct 25, 2017 at 04:25:21PM +0100, Ian Jackson wrote:
> >> Wei Liu writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
> >>> On Tue, Oct 24, 2017 at 02:38:39PM +0100, Ian Jackson wrote:
> >>>> Anthony PERARD writes ("Re: [PATCH v2] scripts: introduce a script for build test"):
> >>>>> That feels wrong. How do I run the same exact command at the default
> >>>>> one, but with -j8 instead of -j4?
> >>>>
> >>>>  .../build-test sh -ec make -j4 distclean && ./configure && make -j4
> >>>>
> >>>> But I think Anthony has a point.  The clean should 1. be git-clean,
> >>>> not make distclean 2. be run anyway.
> >>>
> >>> I don't think we should call git-clean unconditionally -- imagine
> >>> someone knew for sure they only needed to build part of the tools or the
> >>> hypervisor.
> >>
> >> If you are worried about this you should check that there are no
> >> uncommitted files before starting.
> > 
> > This is already done in this version.
> > 
> > I don't worry if there is uncommitted file, I just don't want to stop
> > developers from being smarter than the script when they know git-clean
> > is not necessary.
> 

> What kind of "smarter" did you have in mind?
> 
> This script sounds like an aid to developers who don't have the
> motivation / experience / whatever to write their own script (or do
> something fancier, like git rebase --exec).  If people want to be
> smarter they can write their own script, using this as a starting point.

Exactly. If they want to supply their script, then fine. We shouldn't do
a git-clean for them.

I'm fine with using 'git clean' in the default rune but I don't want to
use it unconditionally.


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