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-}" = 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