From: Tomi Ollila <tomi.ollila@iki.fi>
To: Michal Nazarewicz <mina86@mina86.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH V2 1/2] devel: add release-checks.sh
In-Reply-To: <xa1t7gsbwbxe.fsf@mina86.com>
References: <1346491928-2356-1-git-send-email-tomi.ollila@iki.fi>
	<xa1t7gsbwbxe.fsf@mina86.com>
User-Agent: Notmuch/0.14+11~gd9bf007 (http://notmuchmail.org) Emacs/24.2.1
	(x86_64-unknown-linux-gnu)
Date: Mon, 03 Sep 2012 17:58:38 +0300
Message-ID: <m2sjazdug1.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list

On Mon, Sep 03 2012, Michal Nazarewicz <mina86@mina86.com> wrote:

> Tomi Ollila <tomi.ollila@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-}" =3D x ]
>
> -n perhaps?

Before this test we cannot know what shell is run; the x"$var" =3D x=20
is the most robust way to do this test. Well, if $BASH_VERSION had=20
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-}" !=3D 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=3DC LC_ALL=3DC; export LANG LC_ALL
>> +
>> +readonly DEFAULT_IFS=3D"$IFS"
>
> Never used.

True, it used to be. Can be removed in followup patch.

>> +
>> +readonly PV_FILE=3D'bindings/python/notmuch/version.py'
>> +
>> +# Using array here turned out to be unnecessarily complicated
>> +emsgs=3D''
>> +append_emsg ()
>> +{
>> +	emsgs=3D"${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=20
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" =3D 'notmuch' ]
>
> if [ x"$deb_notmuch" =3D xnotmuch ]
>
> And so in the rest of the conditions below.

builtin bash '[' seems to be robust in these cases(*) ... but I'm now break=
ing
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=3D"-a"; [ "$foo" =3D -a ]'
    bash -c 'set -eu; foo=3D"-z"; [ "$foo" =3D -z ]'

>
>> +then
>> +	echo Yes.
>> +else
>> +	echo No.
>> +	append_emsg "Package name '$deb_notmuch' is not 'notmuch' in debian/ch=
angelog"
>> +fi
>> +
>> +echo -n "Checking that Debian package version is $VERSION-1... "
>> +
>> +if [ "$deb_version" =3D "($VERSION-1)" ]
>> +then
>> +	echo Yes.
>> +else
>> +	echo No.
>> +	append_emsg "Version '$deb_version' is not '($VERSION-1)' in debian/ch=
angelog"
>> +fi
>> +
>> +echo -n "Checking that python bindings version is $VERSION... "
>> +py_version=3D`python -c "execfile('$PV_FILE'); print __VERSION__"`
>> +if [ "$py_version" =3D "$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" =3D "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" =3D "$VERSION" ]
>> +then
>> +	echo Yes.
>> +else
>> +	echo No.
>> +	append_emsg "Version '$news_version' in NEWS file is not '$VERSION'"
>> +fi
>> +
>> +#eval `date '+year=3D%Y mon=3D%m day=3D%d'`
>> +today0utc=3D`date --date=3D0Z +%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=3D`nd=3D${news_date#\\(}; date --date=3D"${nd%)} 0Z" +%s`
>> +	ddiff=3D$((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=3D${news_date//[()]/} # bash feature
>> +manthdata ()
>> +{
>> +	set x $*
>
> Uh?  Did you mean =E2=80=9Cset -- $*=E2=80=9D?

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 [ $# !=3D 7 ]
>> +	then
>> +		append_emsg "'$mp' has too many '.TH' lines"
>> +		man_mismatch=3D1
>> +	fi
>> +	man_date=3D${5-} man_version=3D${7-}
>> +}
>> +
>> +echo -n "Checking that manual page dates and versions are $DATE and $VE=
RSION... "
>> +manfiles=3D`find man -type f | sort`
>> +man_pages_ok=3DYes
>> +for mp in $manfiles
>> +do
>> +	case $mp in *.[0-9]) ;; # fall below this 'case ... esac'
>
> Perhaps new line after =E2=80=9Cin=E2=80=9D?

Can be added...

>
>> +		*/Makefile.local | */Makefile ) continue ;;
>> +		*/.gitignore)	continue ;;
>> +		*.bak)		continue ;;
>> +
>> +		*)	append_emsg "'$mp': extra file"
>> +			man_pages_ok=3DNo
>> +			continue
>> +	esac
>> +	manthdata `sed -n '/^[.]TH NOTMUCH/ { y/"/ /; p; }' "$mp"`
>
> Alternatively =E2=80=9C\.=E2=80=9D instead of =E2=80=9C[.]=E2=80=9D.

I prefer [.] as a more robust alternative -- backslashes may get=20
"expanded away" in some cases; using [.] just frees me from checking
whether that may happen.

>> +	if [ "$man_version" !=3D "$VERSION" ]
>> +	then	append_emsg "Version '$man_version' is not '$VERSION' in $mp"
>> +		mman_pages_ok=3DNo
>> +	fi
>> +	if [ "$man_date" !=3D "$DATE" ]
>> +	then	append_emsg "DATE '$man_date' is not '$DATE' in $mp"
>> +		man_pages_ok=3DNo
>> +	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.=20

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=3D8 ts=3D8
>
> --=20
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=3D./ `o
> ..o | Computer Science,  Micha=C5=82 =E2=80=9Cmina86=E2=80=9D Nazarewicz =
   (o o)

Tomi