Re: [Qemu-devel] [PATCH] script: git script to compile every commit in a range of commits

[Jeff Cody]

On Thu, Jun 06, 2013 at 10:58:25AM +0200, Laszlo Ersek wrote:
> comments below
> 
> On 05/31/13 18:39, Jeff Cody wrote:
> 
> > +usage() {
> > +    echo ""
> > +    echo "$0 [OPTIONS]"
> > +    echo "$desc"
> > +    echo ""
> > +    echo "OPTIONS:"
> > +    echo "      -r git range
> > +            optional; default is '$range'
> > +                    "
> > +    echo "      -c configure options
> > +            optional; default is '$config_opt'
> > +                    "
> > +    echo "      -m make options
> > +            optional; default is '$make_opt'
> > +                    "
> > +    echo "      -d log dir
> > +            optional; default is '$logdir'
> > +                    "
> > +    echo "      -l log filename
> > +            optional; default is output-PROCID, where PROCID is the bash 
> > process id
> > +            note: you may specify a full path for the log filename here, 
> > and exclude the
> > +            -d option.
> > +                    "
> > +    echo "      -f force a git reset and clean
> > +            this will cause a 'git reset --hard; git clean -fdx' to be run 
> > between checkouts.
> > +            !! WARNING: This may cause data loss in your git tree.
> > +                        READ THE git-clean and git-reset man pages and make
> > +                        sure you understand the implications of
> > +                        'git clean -fdx' and 'git reset --hard' before 
> > using !!
> > +                    "
> > +    echo "      -h help"
> > +}
> 
> Sorry for not noticing this before: is there any reason for the trailing
> spaces on most lines in the usage text?
>

No, not particularly, mainly just formatting with an extra newline,
and trying to keep the code somewhat lined up.

> Plus I suggest lower-casing the "THE" in "READ THE".
> 

I agree.

> > +
> > +while getopts ":r:c:m:l:d:hf" opt
> > +do
> > +    case $opt in
> > +        r) range=$OPTARG
> > +            ;;
> > +        c) config_opt=$OPTARG
> > +            ;;
> > +        m) make_opt=$OPTARG
> > +            ;;
> > +        d) logdir=$OPTARG
> > +            ;;
> > +        l) logfile=$OPTARG
> > +            ;;
> > +        f) force_clean='y'
> > +            ;;
> > +        h) usage
> > +           exit
> > +            ;;
> > +        \?) echo "Unknown option: -$OPTARG" >&2
> > +            usage
> > +            exit 1
> > +            ;;
> > +    esac
> > +done
> > +
> > +# append a '/' to logdir if $logdir was specified without one
> > +[[ -n "$logdir" ]] && [[ ${logdir:${#logdir}-1} != "/" ]] && 
> > logdir="${logdir}/"
> > +
> > +logfile="${logdir}${logfile}"
> > +
> > +head=`git rev-parse HEAD`
> > +total=`git rev-list "$range" |wc -l`
> > +
> > +echo "log output: $logfile"
> > +
> > +rm -f "$logfile"
> 
> rm -f -- "$logfile" is safer, but I doubt anyone would pass a pathname
> starting with "-"...

You are right, and no reason not to use '--', so I should add that in.

> 
> > +date > "$logfile"
> > +echo "git compile check for $range." >> "$logfile"
> > +echo "* configure options='$config_opt'" >> "$logfile"
> > +echo "* make options='$make_opt'" >> "$logfile"
> > +echo "Performing a test compile on $total patches" | tee -a "$logfile"
> > +echo "-------------------------------------------------------------" >> 
> > "$logfile"
> > +echo "" | tee -a "$logfile"
> > +
> > +clean_repo() {
> > +    if [[ $force_clean == 'y' ]]
> > +    then
> > +        git reset --hard >> "$logfile" 2>&1 || true
> > +        git clean -fdx -e "$logfile" >> "$logfile" 2>&1 || true
> > +    fi
> > +}
> 
> Does "-e" mean "except"? It's not supported on RHEL-6.
> 

Yes - this way it will preserve the log output (the default log path
is in the current working directory).  Looks like the -e option was
not introduced until 1.7.3, and RHEL-6 is on 1.7.1.

I'll add a check for the git version, and drop the -e if git is too
old to support it (and in that case, the user will need to make sure
if they supply the -f option to this script that they specify a
different log location than the cwd).

> > +
> > +# we want to cleanup and return the git tree back to the previous head
> > +trap cleanup EXIT
> > +
> > +cleanup() {
> > +    echo ""
> > +    echo -n "Cleaning up..."
> > +    clean_repo
> > +    git checkout $head > /dev/null 2>&1
> > +    echo "done."
> > +}
> > +
> > +cnt=1
> > +# don't pipe the git job into read, to avoid subshells
> > +while read hash
> > +do
> > +    txt=`git log --pretty=tformat:"%h: %s" $hash^!`
> > +    echo "${cnt}/${total}: compiling: $txt" | tee -a "$logfile"
> > +    let cnt=$cnt+1;
> > +    echo "####################" >> "$logfile"
> > +    clean_repo
> > +    make clean > /dev/null 2>&1 || true
> > +    git checkout $hash          >> "$logfile" 2>&1 && \
> > +        ./configure $config_opt >> "$logfile" 2>&1 && \
> > +        make $make_opt          >> "$logfile" 2>&1 ||
> > +    (
> > +        echo "" | tee -a "$logfile"
> > +        echo "ERROR: commit $hash failed to build!" | tee -a "$logfile"
> > +        git show --stat $hash | tee -a "$logfile"
> > +        exit 1
> > +    )
> > +done < <(git log $range --pretty=tformat:"%H" --reverse)
> > +
> > +echo "
> > +All patches in $range compiled successfully!" | tee -a "$logfile"
> > +exit 0
> > 
> 
> I think my remarks are not important, the script should work in any
> practical environment. We should get this merged and small fixes can be
> posted incrementally if needed.
> 
> Reviewed-by: Laszlo Ersek <address@hidden>
>

Thanks.  I can either do the above changes for a v2, or as follow on
patches.