[PATCH V2 1/2] devel: add release-checks.sh

Mon Sep 3 07:58:38 PDT 2012

On Mon, Sep 03 2012, Michal Nazarewicz <mina86 at mina86.com> wrote:

> Tomi Ollila <tomi.ollila at iki.fi> writes:
>> diff --git a/devel/release-checks.sh b/devel/release-checks.sh
>> new file mode 100755
>> index 0000000..7dadefa
>> --- /dev/null
>> +++ b/devel/release-checks.sh
>> @@ -0,0 +1,211 @@
>> +#!/usr/bin/env bash
>
> On a side note, the whole script could be relatively easily rewritten
> not to use bash at all and work with plain POSIX shell.

The 'set -o pipefail' is hard to do using POSIX shell. I've been hit
by this when 'find' part in find | sort' failed (due to a typo).

When building release we can choose shell in more relaxed manner and
expect release builder have a recent bash shell -- actually tests
require bash 4...

>> +
>> +set -eu
>> +#set -x # or enter bash -x ... on command line
>> +
>> +if [ x"${BASH_VERSION-}" = x ]
>
> -n perhaps?

Before this test we cannot know what shell is run; the x"$var" = x 
is the most robust way to do this test. Well, if $BASH_VERSION had 
some value then we'd miss this error message and failed in
set -o pipefail

If we'd require bash 4 then the test [ x"${BASHPID-}" != x$$ ]
would be very hard to get wrong...

>> +then	echo
>> +	echo "Please execute this script using 'bash' interpreter"
>> +	echo
>> +	exit 1
>> +fi
>> +
>> +set -o pipefail # bash feature
>> +
>> +# Avoid locale-specific differences in output of executed commands
>> +LANG=C LC_ALL=C; export LANG LC_ALL
>> +
>> +readonly DEFAULT_IFS="$IFS"
>
> Never used.

True, it used to be. Can be removed in followup patch.

>> +
>> +readonly PV_FILE='bindings/python/notmuch/version.py'
>> +
>> +# Using array here turned out to be unnecessarily complicated
>> +emsgs=''
>> +append_emsg ()
>> +{
>> +	emsgs="${emsgs:+$emsgs\n}  $1"
>> +}
>> +
>> +for f in ./version debian/changelog NEWS "$PV_FILE"
>> +do
>> +	test -f $f || { append_emsg "File '$f' is missing"; continue; }
>> +	test -r $f || { append_emsg "File '$f' is unreadable"; continue; }
>> +	test -s $f ||   append_emsg "File '$f' is empty"
>
> if ! [ -f "$f" ]; then
> 	append_emsg "File '$f' is missing"
> elif ! [ -r "$f" ]; then
> 	append_emsg "File '$f' is unreadable"
> elif ! [ -s "$f" ]; then
> 	append_emsg "File '$f' is empty"
> fi

IMHO this short-circuited or (||) version works well due to this
negation handling. The $f's should have been quoted ("$f"), though
(in case some of the items contain whitespace the construct 
would't fail). I am open to other opinions, though.

>> +done
>> +
>> +if [ -n "$emsgs" ]
>> +then
>> +	echo 'Release files problems; fix these and try again:'
>> +	echo -e "$emsgs"
>> +	exit 1
>> +fi
>> +
>> +if read VERSION
>> +then
>> +	if read rest
>> +	then	echo "'version' file contains more than one line"
>> +		exit 1
>> +	fi
>> +else
>> +	echo "Reading './version' file failed (suprisingly!)"
>> +	exit 1
>> +fi < ./version
>> +
>> +readonly VERSION
>> +
>> +verfail ()
>> +{
>> +	echo No.
>> +	echo "$@"
>> +	echo "Please follow the instructions in RELEASING to choose a version"
>> +	exit 1
>> +}
>> +
>> +echo -n "Checking that '$VERSION' is good with digits and periods... "
>> +if [ -z "${VERSION//[0123456789.]/}" ] # bash feature
>> +then
>> +	case $VERSION in
>> +		.*)	verfail "'$VERSION' begins with a period" ;;
>> +		*.)	verfail "'$VERSION' ends with a period" ;;
>> +		*..*)	verfail "'$VERSION' contains two consecutive periods" ;;
>> +		*.*)	echo Yes. ;;
>> +		*)	verfail "'$VERSION' is a single number" ;;
>> +	esac
>> +else
>> +	verfail "'$VERSION' contains other characters than digits and periods"
>> +fi
>
> The outer condition can be put inside of case as so:
>
> 	*[^0-9.]*)	verfail "'$VERSION' contains other characters than digits and periods"
>
> This makes it more readable by putting all checks in case rather than
> splitting one to an outer if.

That's true! Also, one less bashish cannot be bad thing to have.

>> +
>> +
>> +# In the rest of this file, tests collect list of errors to be fixed
>> +
>> +echo -n "Checking that this is Debian package for notmuch... "
>> +read deb_notmuch deb_version rest < debian/changelog
>> +if [ "$deb_notmuch" = 'notmuch' ]
>
> if [ x"$deb_notmuch" = xnotmuch ]
>
> And so in the rest of the conditions below.

builtin bash '[' seems to be robust in these cases(*) ... but I'm now breaking
my own rule about not using most portable expressions; maybe it is just
ugliness of the format in so many places. I'm not changing unless there is
more desire for it :)

(*) at least both of these work:
    bash -c 'set -eu; foo="-a"; [ "$foo" = -a ]'
    bash -c 'set -eu; foo="-z"; [ "$foo" = -z ]'

>
>> +then
>> +	echo Yes.
>> +else
>> +	echo No.
>> +	append_emsg "Package name '$deb_notmuch' is not 'notmuch' in debian/changelog"
>> +fi
>> +
>> +echo -n "Checking that Debian package version is $VERSION-1... "
>> +
>> +if [ "$deb_version" = "($VERSION-1)" ]
>> +then
>> +	echo Yes.
>> +else
>> +	echo No.
>> +	append_emsg "Version '$deb_version' is not '($VERSION-1)' in debian/changelog"
>> +fi
>> +
>> +echo -n "Checking that python bindings version is $VERSION... "
>> +py_version=`python -c "execfile('$PV_FILE'); print __VERSION__"`
>> +if [ "$py_version" = "$VERSION" ]
>> +then
>> +	echo Yes.
>> +else
>> +	echo No.
>> +	append_emsg "Version '$py_version' is not '$VERSION' in $PV_FILE"
>> +fi
>> +
>> +echo -n "Checking that this is Notmuch NEWS... "
>> +read news_notmuch news_version news_date < NEWS
>> +if [ "$news_notmuch" = "Notmuch" ]
>> +then
>> +	echo Yes.
>> +else
>> +	echo No.
>> +	append_emsg "First word '$news_notmuch' is not 'Notmuch' in NEWS file"
>> +fi
>> +
>> +echo -n "Checking that NEWS version is $VERSION... "
>> +if [ "$news_version" = "$VERSION" ]
>> +then
>> +	echo Yes.
>> +else
>> +	echo No.
>> +	append_emsg "Version '$news_version' in NEWS file is not '$VERSION'"
>> +fi
>> +
>> +#eval `date '+year=%Y mon=%m day=%d'`
>> +today0utc=`date --date=0Z +%s` # gnu date feature
>> +
>> +echo -n "Checking that NEWS date is right... "
>> +case $news_date in
>> + '('[2-9][0-9][0-9][0-9]-[01][0-9]-[0123][0-9]')')
>> +	newsdate0utc=`nd=${news_date#\\(}; date --date="${nd%)} 0Z" +%s`
>> +	ddiff=$((newsdate0utc - today0utc))
>> +	if [ $ddiff -lt -86400 ] # since beginning of yesterday...
>> +	then
>> +		echo No.
>> +		append_emsg "Date $news_date in NEWS file is too much in the past"
>> +	elif [ $ddiff -gt 172800 ] # up to end of tomorrow...
>> +	then
>> +		echo No.
>> +		append_emsg "Date $news_date in NEWS file is too much in the future"
>> +	else
>> +		echo Yes.
>> +	fi ;;
>> + *)
>> +	echo No.
>> +	append_emsg "Date '$news_date' in NEWS file is not in format (yyyy-mm-dd)"
>> +esac
>> +
>> +readonly DATE=${news_date//[()]/} # bash feature
>> +manthdata ()
>> +{
>> +	set x $*
>
> Uh?  Did you mean “set -- $*”?

Nope, set in some shells don't know '--' (now I'm following the most
portable code snippet principle) so with 'x' and just referencing
positional parameters with number one larger than with '--' does the trick.

>
>> +	if [ $# != 7 ]
>> +	then
>> +		append_emsg "'$mp' has too many '.TH' lines"
>> +		man_mismatch=1
>> +	fi
>> +	man_date=${5-} man_version=${7-}
>> +}
>> +
>> +echo -n "Checking that manual page dates and versions are $DATE and $VERSION... "
>> +manfiles=`find man -type f | sort`
>> +man_pages_ok=Yes
>> +for mp in $manfiles
>> +do
>> +	case $mp in *.[0-9]) ;; # fall below this 'case ... esac'
>
> Perhaps new line after “in”?

Can be added...

>
>> +		*/Makefile.local | */Makefile ) continue ;;
>> +		*/.gitignore)	continue ;;
>> +		*.bak)		continue ;;
>> +
>> +		*)	append_emsg "'$mp': extra file"
>> +			man_pages_ok=No
>> +			continue
>> +	esac
>> +	manthdata `sed -n '/^[.]TH NOTMUCH/ { y/"/ /; p; }' "$mp"`
>
> Alternatively “\.” instead of “[.]”.

I prefer [.] as a more robust alternative -- backslashes may get 
"expanded away" in some cases; using [.] just frees me from checking
whether that may happen.

>> +	if [ "$man_version" != "$VERSION" ]
>> +	then	append_emsg "Version '$man_version' is not '$VERSION' in $mp"
>> +		mman_pages_ok=No
>> +	fi
>> +	if [ "$man_date" != "$DATE" ]
>> +	then	append_emsg "DATE '$man_date' is not '$DATE' in $mp"
>> +		man_pages_ok=No
>> +	fi
>> +done
>> +echo $man_pages_ok.
>> +
>> +if [ -n "$emsgs" ]
>> +then
>> +	echo
>> +	echo 'Release check failed; check these issues:'
>> +	echo -e "$emsgs"
>> +	exit 1
>> +fi
>> +
>> +echo 'All checks this script executed completed successfully.'
>> +echo 'Make sure that everything else mentioned in RELEASING'
>> +echo 'file is in order, too.'
>> +
>> +exit 0
>
> Unnecessary.

True. 

Thanks for the review, I'll probably send new version tomorrow...

>> +
>> +# Local variables:
>> +# mode: shell-script
>> +# sh-basic-offset: 8
>> +# tab-width: 8
>> +# End:
>> +# vi: set sw=8 ts=8
>
> -- 
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
> ..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)

Tomi