Re: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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
Re: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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: [Xen-devel] [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.
> > > +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: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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 [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: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
On Mon, Oct 23, 2017 at 05:56:33PM +0100, Wei Liu wrote: > + > +if test $# -lt 2 ; then > +echo "Usage: $0 [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: [Xen-devel] [PATCH v2] scripts: introduce a script for build test
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
[Xen-devel] [PATCH v2] scripts: introduce a script for build test
Signed-off-by: Ian Jackson Signed-off-by: Wei Liu --- Cc: Andrew Cooper Cc: George Dunlap Cc: Ian Jackson Cc: Jan Beulich Cc: Konrad Rzeszutek Wilk Cc: Stefano Stabellini Cc: Tim Deegan Cc: Wei Liu Cc: Julien Grall Cc: Anthony PERARD --- 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 00..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 [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
