| Message ID | 20171023165633.26011-1-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 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.
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
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?
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.
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.
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.
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.
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.
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.
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
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.
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