| Message ID | 20171020173255.31213-1-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 20.10.17 at 19:32, <wei.liu2@citrix.com> wrote: > --- /dev/null > +++ b/scripts/build-test.sh > @@ -0,0 +1,40 @@ > +#!/bin/sh > + > +# WARNING: Always backup the branch by creating another reference to it if > +# you're not familiar with git-rebase(1). > +# > +# Use `git rebase` to run command or script on every commit within the range > +# specified. If no command or script is provided, use the default one to clean > +# and build the whole tree. > +# > +# If something goes wrong, the script will stop at the commit that fails. Fix > +# the failure and run `git rebase --continue`. > +# > +# If for any reason the tree is screwed, use `git rebase --abort` to restore to > +# original state. > + > +if ! test -f xen/Kconfig; then > + echo "Please run this script from top-level directory" Wouldn't running this in one of the top-level sub-trees also be useful? E.g. why would one want a hypervisor only series not touching the public interface to have the tools tree rebuilt all the time? > + exit 1 > +fi > + > +if test $# -lt 2 ; then > + echo "Usage: $0 <BASE> <TIP> [CMD|SCRIPT]" Perhaps echo "Usage: $0 <BASE> <TIP> [<CMD>|<SCRIPT>]" ? Also I'm not clear why you make the distinction between command and script. > + exit 1 > +fi > + > +BASE=$1 > +TIP=$2 > +CMD=${3:-git clean -fdx && ./configure && make -j4} > + > +echo "Running command \"$CMD\" on every commit from $BASE to $TIP" > +echo -n "Starting in " > + > +for i in `seq 5 -1 1`; do > + echo -n "$i ... " > + sleep 1 > +done What is this startup delay intended for? > +echo > + > +git rebase $BASE $TIP -x "$CMD" Is this quoting on $CMD really going to work right no matter what the variable actually expands to? I.e. don't you either want to use "eval" or adjust script arguments such that you can use "$@" with its special quoting rules? Jan
Hi Wei, On 20/10/17 18:32, Wei Liu wrote: > 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> > > The risk for this is zero, hence the for-4.10 tag. Agree. > --- > scripts/build-test.sh | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 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..a08468e83b > --- /dev/null > +++ b/scripts/build-test.sh > @@ -0,0 +1,40 @@ > +#!/bin/sh > + > +# WARNING: Always backup the branch by creating another reference to it if > +# you're not familiar with git-rebase(1). > +# > +# Use `git rebase` to run command or script on every commit within the range > +# specified. If no command or script is provided, use the default one to clean > +# and build the whole tree. > +# > +# If something goes wrong, the script will stop at the commit that fails. Fix > +# the failure and run `git rebase --continue`. > +# > +# If for any reason the tree is screwed, use `git rebase --abort` to restore to > +# original state. > + > +if ! test -f xen/Kconfig; then > + echo "Please run this script from top-level directory" > + exit 1 > +fi > + > +if test $# -lt 2 ; then > + echo "Usage: $0 <BASE> <TIP> [CMD|SCRIPT]" > + exit 1 > +fi > + > +BASE=$1 > +TIP=$2 > +CMD=${3:-git clean -fdx && ./configure && make -j4} Can you document somewhere that there are no cross-compilation supported? > + > +echo "Running command \"$CMD\" on every commit from $BASE to $TIP" > +echo -n "Starting in " > + > +for i in `seq 5 -1 1`; do > + echo -n "$i ... " > + sleep 1 > +done > + > +echo > + > +git rebase $BASE $TIP -x "$CMD" > Cheers,
On Fri, Oct 20, 2017 at 06:32:55PM +0100, Wei Liu wrote: > +CMD=${3:-git clean -fdx && ./configure && make -j4} > + > +echo "Running command \"$CMD\" on every commit from $BASE to $TIP" > +echo -n "Starting in " > + > +for i in `seq 5 -1 1`; do > + echo -n "$i ... " > + sleep 1 > +done > + Instead of the count down, I would do: echo -n 'Continue ? (^C to quit) ' read OR something like: echo -n 'Continue ? [Yn] ' read answer [[ "$answer" =~ ^(|Y|y|yes)$ ]] || exit I don't like to wait.
On Mon, Oct 23, 2017 at 12:30:33PM +0100, Anthony PERARD wrote: > On Fri, Oct 20, 2017 at 06:32:55PM +0100, Wei Liu wrote: > > +CMD=${3:-git clean -fdx && ./configure && make -j4} > > + > > +echo "Running command \"$CMD\" on every commit from $BASE to $TIP" > > +echo -n "Starting in " > > + > > +for i in `seq 5 -1 1`; do > > + echo -n "$i ... " > > + sleep 1 > > +done > > + > > Instead of the count down, I would do: > echo -n 'Continue ? (^C to quit) ' > read > > > OR something like: > echo -n 'Continue ? [Yn] ' > read answer > [[ "$answer" =~ ^(|Y|y|yes)$ ]] || exit > > > I don't like to wait. And I don't like the presure to have to decide if I want to ^C within a limited time. I would probably kill the script if I did not know what it was going to do as soon as I see the count down.
On Mon, Oct 23, 2017 at 02:24:40AM -0600, Jan Beulich wrote: > >>> On 20.10.17 at 19:32, <wei.liu2@citrix.com> wrote: > > --- /dev/null > > +++ b/scripts/build-test.sh > > @@ -0,0 +1,40 @@ > > +#!/bin/sh > > + > > +# WARNING: Always backup the branch by creating another reference to it if > > +# you're not familiar with git-rebase(1). > > +# > > +# Use `git rebase` to run command or script on every commit within the range > > +# specified. If no command or script is provided, use the default one to clean > > +# and build the whole tree. > > +# > > +# If something goes wrong, the script will stop at the commit that fails. Fix > > +# the failure and run `git rebase --continue`. > > +# > > +# If for any reason the tree is screwed, use `git rebase --abort` to restore to > > +# original state. > > + > > +if ! test -f xen/Kconfig; then > > + echo "Please run this script from top-level directory" > > Wouldn't running this in one of the top-level sub-trees also be useful? > E.g. why would one want a hypervisor only series not touching the > public interface to have the tools tree rebuilt all the time? > You can do that by supplying your custom command. The script really aims to be an easy to use thing to point contributors to hence the checks, warning and restrictions, while at the same time allows some flexibility. For example, if you want to build hypervisor only: $ ./scripts/build-test.sh $BASE $TIP "make -C xen clean && make -C xen" > > + exit 1 > > +fi > > + > > +if test $# -lt 2 ; then > > + echo "Usage: $0 <BASE> <TIP> [CMD|SCRIPT]" > > Perhaps > > echo "Usage: $0 <BASE> <TIP> [<CMD>|<SCRIPT>]" > > ? Also I'm not clear why you make the distinction between command > and script. > I will just use CMD here. > > + exit 1 > > +fi > > + > > +BASE=$1 > > +TIP=$2 > > +CMD=${3:-git clean -fdx && ./configure && make -j4} > > + > > +echo "Running command \"$CMD\" on every commit from $BASE to $TIP" > > +echo -n "Starting in " > > + > > +for i in `seq 5 -1 1`; do > > + echo -n "$i ... " > > + sleep 1 > > +done > > What is this startup delay intended for? > To give user a chance to check the command -- git-rebase can be destructive after all. > > +echo > > + > > +git rebase $BASE $TIP -x "$CMD" > > Is this quoting on $CMD really going to work right no matter what > the variable actually expands to? I.e. don't you either want to use > "eval" or adjust script arguments such that you can use "$@" with > its special quoting rules? What sort of use cases you have in mind that involve complex quoting and expansion?
On Mon, Oct 23, 2017 at 12:30:33PM +0100, Anthony PERARD wrote: > On Fri, Oct 20, 2017 at 06:32:55PM +0100, Wei Liu wrote: > > +CMD=${3:-git clean -fdx && ./configure && make -j4} > > + > > +echo "Running command \"$CMD\" on every commit from $BASE to $TIP" > > +echo -n "Starting in " > > + > > +for i in `seq 5 -1 1`; do > > + echo -n "$i ... " > > + sleep 1 > > +done > > + > > Instead of the count down, I would do: > echo -n 'Continue ? (^C to quit) ' > read > > > OR something like: > echo -n 'Continue ? [Yn] ' > read answer > [[ "$answer" =~ ^(|Y|y|yes)$ ]] || exit > No objection from me. The latter is better.
Wei Liu writes ("Re: [PATCH for-4.10] scripts: add a script for build testing"):
> On Mon, Oct 23, 2017 at 02:24:40AM -0600, Jan Beulich wrote:
> > What is this startup delay intended for?
>
> To give user a chance to check the command -- git-rebase can be
> destructive after all.
I can't resist this bikeshed. This kind of thing is quite annoying.
If your command might be destructive, why not fix it so that it's not
destructive.
In particular, if you:
* check that the tree is not dirty
* detach HEAD
* reattach HEAD afterwards at least on success
then the risk of lossage is low and you can safely just go ahead.
Ian.
On Mon, Oct 23, 2017 at 01:02:00PM +0100, Ian Jackson wrote: > Wei Liu writes ("Re: [PATCH for-4.10] scripts: add a script for build testing"): > > On Mon, Oct 23, 2017 at 02:24:40AM -0600, Jan Beulich wrote: > > > What is this startup delay intended for? > > > > To give user a chance to check the command -- git-rebase can be > > destructive after all. > > I can't resist this bikeshed. This kind of thing is quite annoying. > If your command might be destructive, why not fix it so that it's not > destructive. > > In particular, if you: > * check that the tree is not dirty > * detach HEAD I think these two checks are good. > * reattach HEAD afterwards at least on success This is already the case for git-rebase on success.
Wei Liu writes ("Re: [PATCH for-4.10] scripts: add a script for build testing"):
> On Mon, Oct 23, 2017 at 01:02:00PM +0100, Ian Jackson wrote:
> > In particular, if you:
> > * check that the tree is not dirty
> > * detach HEAD
>
> I think these two checks are good.
>
> > * reattach HEAD afterwards at least on success
>
> This is already the case for git-rebase on success.
No. git-rebase _rewrites_ HEAD.
Your script should just check out the intermediate commits. You
probably don't in fact want git-rebase. In particular, you don't want
to risk merge conflicts.
I have a script I use for dgit testing that looks like this:
#!/bin/bash
#
# run git fetch main
# and then this
set -e
set -o pipefail
revspec=main/${STTM_TESTED-tested}..main/pretest
echo "testing $revspec ..."
git-rev-list $revspec | nl -ba | tac | \
while read num rev; do
echo >&2 ""
echo >&2 "testing $num $rev"
git checkout $rev
${0%/*}/sometest-to-tested
done
FAOD,
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
for inclusion of parts of this in the Xen build system.
Ian.
>>> On 23.10.17 at 13:41, <wei.liu2@citrix.com> wrote: > On Mon, Oct 23, 2017 at 02:24:40AM -0600, Jan Beulich wrote: >> >>> On 20.10.17 at 19:32, <wei.liu2@citrix.com> wrote: >> > --- /dev/null >> > +++ b/scripts/build-test.sh >> > @@ -0,0 +1,40 @@ >> > +#!/bin/sh >> > + >> > +# WARNING: Always backup the branch by creating another reference to it if >> > +# you're not familiar with git-rebase(1). >> > +# >> > +# Use `git rebase` to run command or script on every commit within the range >> > +# specified. If no command or script is provided, use the default one to clean >> > +# and build the whole tree. >> > +# >> > +# If something goes wrong, the script will stop at the commit that fails. Fix >> > +# the failure and run `git rebase --continue`. >> > +# >> > +# If for any reason the tree is screwed, use `git rebase --abort` to restore to >> > +# original state. >> > + >> > +if ! test -f xen/Kconfig; then >> > + echo "Please run this script from top-level directory" >> >> Wouldn't running this in one of the top-level sub-trees also be useful? >> E.g. why would one want a hypervisor only series not touching the >> public interface to have the tools tree rebuilt all the time? >> > > You can do that by supplying your custom command. Oh, of course - silly me. >> > +echo >> > + >> > +git rebase $BASE $TIP -x "$CMD" >> >> Is this quoting on $CMD really going to work right no matter what >> the variable actually expands to? I.e. don't you either want to use >> "eval" or adjust script arguments such that you can use "$@" with >> its special quoting rules? > > What sort of use cases you have in mind that involve complex quoting and > expansion? A typical cross build command line of mine looks like make -sC build/xen/$v {XEN_TARGET_ARCH,t}=x86_64 CC=gccx LD=ldx OBJCOPY=objcopyx NM=nmx -j32 xen which you can see leverages the fact that make allows variable settings on the command line. For other utilities this would require e.g. "CC=gccx my-script", and I'm not sure whether quoting you apply would work right (largely depends on what git does with the argument). Jan
Wei Liu writes ("Re: [PATCH for-4.10] scripts: add a script for build testing"):
> On Mon, Oct 23, 2017 at 02:24:40AM -0600, Jan Beulich wrote:
> > On 20.10.17 at 19:32, <wei.liu2@citrix.com> wrote:
> > > +git rebase $BASE $TIP -x "$CMD"
> >
> > Is this quoting on $CMD really going to work right no matter what
> > the variable actually expands to? I.e. don't you either want to use
> > "eval" or adjust script arguments such that you can use "$@" with
> > its special quoting rules?
Yes. Jan is completely right.
> What sort of use cases you have in mind that involve complex quoting and
> expansion?
There is really no excuse at all, in a script like this, for not using
`shift' to eat the main positional parameters, and then executing
"$@", faithfully reproducing the incoming parameters.
Of course there is a problem with getting this through git-rebase but
as I have just pointed out, git-rev-list and git-checkout are much
more suitable building blocks than git-rebase (which does a lot of
undesirable stuff that has to be suppressed, etc.)
Ian.
On Mon, Oct 23, 2017 at 01:07:36PM +0100, Ian Jackson wrote: > Wei Liu writes ("Re: [PATCH for-4.10] scripts: add a script for build testing"): > > On Mon, Oct 23, 2017 at 01:02:00PM +0100, Ian Jackson wrote: > > > In particular, if you: > > > * check that the tree is not dirty > > > * detach HEAD > > > > I think these two checks are good. > > > > > * reattach HEAD afterwards at least on success > > > > This is already the case for git-rebase on success. > > No. git-rebase _rewrites_ HEAD. > I see. I will steal bits from your snippet where appropriate.
On 10/20/2017 06:32 PM, Wei Liu wrote: > 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> > > The risk for this is zero, hence the for-4.10 tag. I'm not necessarily arguing against this, but in my estimation this isn't zero risk. It's a new feature (even if one only for developers). It's not *intended* to destroy anything, but a bug in it well could destroy data. -George
On Mon, Oct 23, 2017 at 02:02:53PM +0100, George Dunlap wrote: > On 10/20/2017 06:32 PM, Wei Liu wrote: > > 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> > > > > The risk for this is zero, hence the for-4.10 tag. > > I'm not necessarily arguing against this, but in my estimation this > isn't zero risk. It's a new feature (even if one only for developers). > It's not *intended* to destroy anything, but a bug in it well could > destroy data. There is a `git clean -dxf` in the script, this is destructif! I'm sure it's going to take some people by surprise. FIY, I do like to put script and other files in my checkouts, the git clean will remove them.
On Mon, Oct 23, 2017 at 03:50:31PM +0100, Anthony PERARD wrote: > On Mon, Oct 23, 2017 at 02:02:53PM +0100, George Dunlap wrote: > > On 10/20/2017 06:32 PM, Wei Liu wrote: > > > 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> > > > > > > The risk for this is zero, hence the for-4.10 tag. > > > > I'm not necessarily arguing against this, but in my estimation this > > isn't zero risk. It's a new feature (even if one only for developers). > > It's not *intended* to destroy anything, but a bug in it well could > > destroy data. > > There is a `git clean -dxf` in the script, this is destructif! I'm sure > it's going to take some people by surprise. > > FIY, I do like to put script and other files in my checkouts, the git > clean will remove them. I changed that to make distclean this morning.
Wei Liu writes ("Re: [Xen-devel] [PATCH for-4.10] scripts: add a script for build testing"):
> On Mon, Oct 23, 2017 at 03:50:31PM +0100, Anthony PERARD wrote:
> > FIY, I do like to put script and other files in my checkouts, the git
> > clean will remove them.
>
> I changed that to make distclean this morning.
Urgh. This script depends on git, so please continue to use git to
check if the tree is clean.
The right answer would be to *check that the tree is clean* before
starting.
Ian.
On Mon, Oct 23, 2017 at 04:09:58PM +0100, Ian Jackson wrote: > Wei Liu writes ("Re: [Xen-devel] [PATCH for-4.10] scripts: add a script for build testing"): > > On Mon, Oct 23, 2017 at 03:50:31PM +0100, Anthony PERARD wrote: > > > FIY, I do like to put script and other files in my checkouts, the git > > > clean will remove them. > > > > I changed that to make distclean this morning. > > Urgh. This script depends on git, so please continue to use git to > check if the tree is clean. > > The right answer would be to *check that the tree is clean* before > starting. Sure, that's also something I just did.
diff --git a/scripts/build-test.sh b/scripts/build-test.sh new file mode 100755 index 0000000000..a08468e83b --- /dev/null +++ b/scripts/build-test.sh @@ -0,0 +1,40 @@ +#!/bin/sh + +# WARNING: Always backup the branch by creating another reference to it if +# you're not familiar with git-rebase(1). +# +# Use `git rebase` to run command or script on every commit within the range +# specified. If no command or script is provided, use the default one to clean +# and build the whole tree. +# +# If something goes wrong, the script will stop at the commit that fails. Fix +# the failure and run `git rebase --continue`. +# +# If for any reason the tree is screwed, use `git rebase --abort` to restore to +# original state. + +if ! test -f xen/Kconfig; then + echo "Please run this script from top-level directory" + exit 1 +fi + +if test $# -lt 2 ; then + echo "Usage: $0 <BASE> <TIP> [CMD|SCRIPT]" + exit 1 +fi + +BASE=$1 +TIP=$2 +CMD=${3:-git clean -fdx && ./configure && make -j4} + +echo "Running command \"$CMD\" on every commit from $BASE to $TIP" +echo -n "Starting in " + +for i in `seq 5 -1 1`; do + echo -n "$i ... " + sleep 1 +done + +echo + +git rebase $BASE $TIP -x "$CMD"
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> The risk for this is zero, hence the for-4.10 tag. --- scripts/build-test.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100755 scripts/build-test.sh