[PATCH modular] Publish.sh: batch release and autotagging of modules

[Jeremy Huddleston]

Here's a first pass review.  I'm sure I missed a few things, but this is certainly on the right track.

There is a lot of whitespace inconsistency throughout.  Sometimes you indent by 4, sometimes by 3, sometimes you use tabs for 2-indents, ... it would be nice if that were all consistent.  I point out the obvious cases where it stands out in the review below.


On Oct 6, 2011, at 1:09 PM, Gaetan Nadon wrote:
> The srcipt runs 'make dist' to create the tarball. We don't have to prompt
> the user for the tar name, the version number or the section name.

Should we 'make distcheck' instead?

> Interface
> ---------
> Usage: publish.sh [options] section[/module]...
> 
> Section:
> app|data|doc|driver|font|lib|mesa|proto|util|xcb|
> xkeyboard-config|xserver

I would prefer passing in paths to my local git checkouts over section/module.  You should still be able to do the same -<version> foo by checking if the argument is a dir, if not, chop off the -* and check again.

> ...
> +check_local_changes() {
> +    git diff --quiet HEAD > /dev/null 2>&1
> +    if [ $? -ne 0 ]; then
> +        echo ""
> +        echo "Uncommitted changes found. Did you forget to commit? Aborting."
> +	echo ""
> +	echo "You can perform a 'git stash' to save your local changes and"
> +	echo "a 'git stash apply' to recover them after the tarball release."
> +	echo "Make sure to rebuild and run 'make distcheck' again."
> +	echo ""
> +	echo "Alternatively, you can clone the module in another directory"
> +	echo "and run ./configure. No need to build if testing was finished."
> +        echo ""
> +	return 1
> +    fi
> +    return 0
> +}

There are whitespace inconsistencies above ^^^

> ...
> +#------------------------------------------------------------------------------
> +#			Function: generate_announce
> +#------------------------------------------------------------------------------
> +#
> +generate_announce()
> +{
> +    cat <<RELEASE
> +Subject: [ANNOUNCE] $pkg_name $pkg_version
> +To: $list_to
> +CC: $list_cc
> +
> +`git log --no-merges "$tag_range" | git shortlog`
> +
> +git tag: $tar_name
> +
> +http://$host_current/$section_path/$tarbz2
> +MD5:  `$MD5SUM $tarbz2`
> +SHA1: `$SHA1SUM $tarbz2`
> +SHA256: `$SHA256SUM $tarbz2`
> +
> +http://$host_current/$section_path/$targz
> +MD5:  `$MD5SUM $targz`
> +SHA1: `$SHA1SUM $targz`
> +SHA256: `$SHA256SUM $targz`
> +
> +RELEASE
> +}

Why are we duplicating code between release.sh and publish.sh?  Can we instead have publish.sh be a smart wrapper around release.sh.  Alan also sent out a release.sh patch (which I reviewed and provided a followup for) which does some of the same "intelligent" aspects but for the current module.  I would like to either see both of these exist together or update publish.sh to support "." as an argument (which would possibly fall out naturally from my suggestion above about taking paths)

> ...
> +	    # skip comment lines
> +	    echo "$line" | grep "^#" > /dev/null
> +	    if [ $? -eq 0 ]; then
> +		continue
> +	    fi

A tad cleaner:
if echo "$line" | grep -q "^#" ; then
    continue;
fi

> +#------------------------------------------------------------------------------
> +#			Function: print_epilog
> +#------------------------------------------------------------------------------
> +#
> +print_epilog() {
> +
> +epilog="========  Successful Completion"
> +if [ x"$NO_QUIT" != x ]; then
> +    if [ x"$failed_modules" != x ]; then
> +	epilog="========  Partial Completion"
> +    fi
> +elif [ x"$failed_modules" != x ]; then
> +	epilog="========  Stopped on Error"
> +fi
> +
> +echo ""
> +echo "$epilog `date`"
> +
> +# Report about modules that failed for one reason or another
> +if [ x"$failed_modules" != x ]; then
> +    echo "	List of failed modules:"
> +    for mod in $failed_modules; do
> +	echo "	$mod"
> +    done
> +    echo "========"
> +    echo ""
> +fi
> +
> +}

I'm confused why the contents of the above function are not indented...

> +#------------------------------------------------------------------------------
> +#			Function: process_modules
> +#------------------------------------------------------------------------------
> +#
> +# Loop through each module to publish
> +# Exit on error if --no-quit no specified

^^^ "if --no-quit was not specified"

> +#
> +process_modules() {
> +    for MODULE_RPATH in ${INPUT_MODULES}; do
> +	process_module
> +	if [ $? -ne 0 ]; then

again, I would do:

if ! process_module ; then
...
fi

> +#------------------------------------------------------------------------------
> +#			Function: process_module
> +#------------------------------------------------------------------------------
> +# Code 'return 0' on success to process the next module
> +# Code 'return 1' on error to process next module if invoked with --no-quit 
> +#
> +process_module() {
> +
> +    # Skip already processed modules when resuming from a previous failure
> +    if [ x"$RESUME" != x ]; then
> +	if [ x"$RESUME" = x"$MODULE_RPATH" ]; then
> +	    # The current module is the one that failed last time
> +	    echo "Info: resuming from $RESUME module"
> +	    unset RESUME
> +	else
> +	    # The current module has already been processed successfully last time
> +	    echo "Info: skipping $MODULE_RPATH in autoresume mode."
> +	    return 0
> +	fi
> +    fi
> +
> +    echo ""
> +    echo "========  Processing \"$MODULE_RPATH\""
> +
> +    top_src=`pwd`
> +    if [ ! -d $MODULE_RPATH ] ; then
> +        echo "Error: $MODULE_RPATH cannot be found under $top_src."
> +	failed_modules="$failed_modules $MODULE_RPATH"
> +        return 1

^^^ whitespace

> ...
> +    # Mailing lists to be CC according to the project (xorg|dri|xkb)
> +    list_xorg_user="xorg at lists.freedesktop.org"
> +    list_dri_devel="dri-devel at lists.sourceforge.net"
> +    list_xkb="xkb at listserv.bat.ru"

Add in the xcb list?

...

> +    # The jhbuild moduleset to update with relase info
> +    --moduleset)
> +	check_option_args $1 $2
> +        shift
> +        JH_MODULESET=$1
> +        ;;

^^^ Whitespace inconsistency


> +    --*)
> +	echo ""
> +        echo "Error: unknown option: $1"
> +	echo ""
> +	usage
> +        exit 1

^^^ Whitespace inconsistency

> +        ;;
> +    -*)
> +	echo ""
> +        echo "Error: unknown option: $1"
> +	echo ""
> +        usage
> +        exit 1
> +        ;;

^^^ Whitespace inconsistency

> +    *)
> +        if [ x"${MODFILE}" != x ]; then
> +	    echo ""
> +	    echo "Error: specifying both modules and --modfile is not permitted"
> +	    echo ""
> +	    usage
> +            exit 1

^^^ Whitespace inconsistency


---
Jeremy Huddleston

Rebuild Sudan
 - Board of Directors
 - http://www.rebuildsudan.org

Berkeley Foundation for Opportunities in Information Technology
 - Advisory Board
 - http://www.bfoit.org