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

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

[Tomi Ollila]

Currently Makefile.local contains some machine executable release
checking functionality. This is unnecessarily complex way to do it:

Multiline script functionality is hard to embed -- from Makefile point
of view there is just one line split using backslashes and every line
ends with ';'. It is hard to maintain such "script" when it gets longer.

The embedded script does not fail as robust as separate script; set -eu
could be added to get same level of robustness -- but the provided
Bourne Again Shell (bash) script exceeds this with 'set -o pipefail',
making the script to fail when any of the commands in pipeline fails
(and not just the last one).

Checking for release is done very seldom compared to all other use;
The whole Makefile.local gets simpler and easier to grasp when most
release checking targets are removed.

When release checking is done, the steps are executed sequentially;
nothing is allowed to be skipped due to some satisfied dependency.
---

This patch obsoletes (now moreinfo) patch in

id:"1344456107-19308-1-git-send-email-tomi.ollila@iki.fi"

 devel/release-checks.sh | 211 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 211 insertions(+)
 create mode 100755 devel/release-checks.sh

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
+
+set -eu
+#set -x # or enter bash -x ... on command line
+
+if [ x"${BASH_VERSION-}" = x ]
+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"
+
+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"
+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
+
+
+# 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' ]
+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 $*
+	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'
+		*/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"`
+	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
+
+# Local variables:
+# mode: shell-script
+# sh-basic-offset: 8
+# tab-width: 8
+# End:
+# vi: set sw=8 ts=8
--
1.7.11.4

[PATCH V2 2/2] {., man}/Makefile.local: edit/remove release-checks.sh related targets

[Tomi Ollila]

Use new target release-checks in place of verify-version-debian,
verify-version-python verify-version-manpage. This target executes
devel/release-checks.sh which does all the verifications the three
dropped targets did, and some more.
---
 Makefile.local     | 28 ++++------------------------
 man/Makefile.local |  9 +--------
 2 files changed, 5 insertions(+), 32 deletions(-)

diff --git a/Makefile.local b/Makefile.local
index de984ab..99f1e19 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -187,7 +187,7 @@ release-message:
 verify-source-tree-and-version: verify-no-dirty-code
 
 .PHONY: verify-no-dirty-code
-verify-no-dirty-code: verify-version-debian verify-version-python verify-version-manpage
+verify-no-dirty-code: release-checks
 ifeq ($(IS_GIT),yes)
 	@printf "Checking that source tree is clean..."
 ifneq ($(shell git ls-files -m),)
@@ -204,29 +204,9 @@ else
 endif
 endif
 
-.PHONY: verify-version-debian
-verify-version-debian: verify-version-components
-	@echo -n "Checking that Debian package version is $(VERSION)-1..."
-	@[ "$(VERSION)-1" = $$(sed '1{ s/).*//; s/.*(//; q; }' debian/changelog) ] || \
-		(echo "No." && \
-		 echo "Please edit version and debian/changelog to have consistent versions." && false)
-	@echo "Good."
-
-.PHONY: verify-version-python
-verify-version-python: verify-version-components
-	@echo -n "Checking that python bindings version is $(VERSION)..."
-	@[ "$(VERSION)" = $$(python -c "execfile('$(PV_FILE)'); print __VERSION__") ] || \
-		(echo "No." && \
-		 echo "Please edit version and $(PV_FILE) to have consistent versions." && false)
-	@echo "Good."
-
-.PHONY: verify-version-components
-verify-version-components:
-	@echo -n "Checking that $(VERSION) consists only of digits and periods..."
-	@echo $(VERSION) | grep -q -x '^[0-9.]*$$' || \
-		(echo "No." && \
-	         echo "Please follow the instructions in RELEASING to choose a version" && false)
-	@echo "Good."
+.PHONY: release-checks
+release-checks:
+	exec devel/release-checks.sh
 
 .PHONY: verify-newer
 verify-newer:
diff --git a/man/Makefile.local b/man/Makefile.local
index d43a949..72e2a18 100644
--- a/man/Makefile.local
+++ b/man/Makefile.local
@@ -32,7 +32,7 @@ COMPRESSED_MAN := $(MAN1_GZ) $(MAN5_GZ) $(MAN7_GZ)
 %.gz: %
 	gzip --stdout $^ > $@
 
-.PHONY: install-man update-man-versions verify-version-manpage
+.PHONY: install-man update-man-versions
 
 install-man: $(COMPRESSED_MAN)
 	mkdir -p "$(DESTDIR)$(mandir)/man1"
@@ -43,13 +43,6 @@ install-man: $(COMPRESSED_MAN)
 	install -m0644 $(MAN7_GZ) $(DESTDIR)/$(mandir)/man7
 	cd $(DESTDIR)/$(mandir)/man1 && ln -sf notmuch.1.gz notmuch-setup.1.gz
 
-verify-version-manpage: verify-version-components
-	@echo -n "Checking that manual page version is $(VERSION)..."
-	@[ "$(VERSION)" = $$(sed -n '/^[.]TH NOTMUCH 1/{s/.*"Notmuch //;s/".*//p;}' $(MAIN_PAGE)) ] || \
-		(echo "No." && \
-		 echo "Please edit version and notmuch.1 to have consistent versions." && false)
-	@echo "Good."
-
 update-man-versions: $(MAN_SOURCE)
 	for file in $(MAN_SOURCE); do \
 	    cp $$file $$file.bak ; \
-- 
1.7.11.4

[PATCH V2 2b/2] {., man}/Makefile.local: edit/remove release-checks.sh related targets

[Tomi Ollila]

Use new target release-checks in place of verify-version-debian,
verify-version-python verify-version-manpage. This target executes
devel/release-checks.sh which does all the verifications the three
dropped targets did, and some more.
---

The only change to 

     id:"1346491928-2356-2-git-send-email-tomi.ollila@iki.fi"

is that 'exec' is removed from devel/release-checks.sh invocation.
I thought it would have saved one fork but experimentation showed
that is not the case.

 Makefile.local     |   28 ++++------------------------
 man/Makefile.local |    9 +--------
 2 files changed, 5 insertions(+), 32 deletions(-)

diff --git a/Makefile.local b/Makefile.local
index de984ab..99f1e19 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -187,7 +187,7 @@ release-message:
 verify-source-tree-and-version: verify-no-dirty-code
 
 .PHONY: verify-no-dirty-code
-verify-no-dirty-code: verify-version-debian verify-version-python verify-version-manpage
+verify-no-dirty-code: release-checks
 ifeq ($(IS_GIT),yes)
 	@printf "Checking that source tree is clean..."
 ifneq ($(shell git ls-files -m),)
@@ -204,29 +204,9 @@ else
 endif
 endif
 
-.PHONY: verify-version-debian
-verify-version-debian: verify-version-components
-	@echo -n "Checking that Debian package version is $(VERSION)-1..."
-	@[ "$(VERSION)-1" = $$(sed '1{ s/).*//; s/.*(//; q; }' debian/changelog) ] || \
-		(echo "No." && \
-		 echo "Please edit version and debian/changelog to have consistent versions." && false)
-	@echo "Good."
-
-.PHONY: verify-version-python
-verify-version-python: verify-version-components
-	@echo -n "Checking that python bindings version is $(VERSION)..."
-	@[ "$(VERSION)" = $$(python -c "execfile('$(PV_FILE)'); print __VERSION__") ] || \
-		(echo "No." && \
-		 echo "Please edit version and $(PV_FILE) to have consistent versions." && false)
-	@echo "Good."
-
-.PHONY: verify-version-components
-verify-version-components:
-	@echo -n "Checking that $(VERSION) consists only of digits and periods..."
-	@echo $(VERSION) | grep -q -x '^[0-9.]*$$' || \
-		(echo "No." && \
-	         echo "Please follow the instructions in RELEASING to choose a version" && false)
-	@echo "Good."
+.PHONY: release-checks
+release-checks:
+	devel/release-checks.sh
 
 .PHONY: verify-newer
 verify-newer:
diff --git a/man/Makefile.local b/man/Makefile.local
index d43a949..72e2a18 100644
--- a/man/Makefile.local
+++ b/man/Makefile.local
@@ -32,7 +32,7 @@ COMPRESSED_MAN := $(MAN1_GZ) $(MAN5_GZ) $(MAN7_GZ)
 %.gz: %
 	gzip --stdout $^ > $@
 
-.PHONY: install-man update-man-versions verify-version-manpage
+.PHONY: install-man update-man-versions
 
 install-man: $(COMPRESSED_MAN)
 	mkdir -p "$(DESTDIR)$(mandir)/man1"
@@ -43,13 +43,6 @@ install-man: $(COMPRESSED_MAN)
 	install -m0644 $(MAN7_GZ) $(DESTDIR)/$(mandir)/man7
 	cd $(DESTDIR)/$(mandir)/man1 && ln -sf notmuch.1.gz notmuch-setup.1.gz
 
-verify-version-manpage: verify-version-components
-	@echo -n "Checking that manual page version is $(VERSION)..."
-	@[ "$(VERSION)" = $$(sed -n '/^[.]TH NOTMUCH 1/{s/.*"Notmuch //;s/".*//p;}' $(MAIN_PAGE)) ] || \
-		(echo "No." && \
-		 echo "Please edit version and notmuch.1 to have consistent versions." && false)
-	@echo "Good."
-
 update-man-versions: $(MAN_SOURCE)
 	for file in $(MAN_SOURCE); do \
 	    cp $$file $$file.bak ; \
-- 
1.7.1

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

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 7131 bytes --]

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.

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

-n perhaps?

> +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.

> +
> +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

> +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.

> +
> +
> +# 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.

> +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 -- $*”?

> +	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”?

> +		*/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 “[.]”.

> +	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.

> +
> +# 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)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

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

[David Bremner]

Michal Nazarewicz <mina86@mina86.com> writes:

>
> On a side note, the whole script could be relatively easily rewritten
> not to use bash at all and work with plain POSIX shell.
>

How does one simulate pipefail?  

POSIXness is fine, but I wouldn't want to complicate things too much to
achieve it, since this script is not used by regular build process.

I leave you and Tomi to debate the remaining points of sh script
style ;).

cheers,

d

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

[Tomi Ollila]

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

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

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

>> Tomi Ollila <tomi.ollila@iki.fi> writes:
>>> +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"

> On Mon, Sep 03 2012, Michal Nazarewicz <mina86@mina86.com> wrote:
>> 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

Tomi Ollila <tomi.ollila@iki.fi> writes:
> 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.

I personally don't like “||” or “&&” as a replacement for if as I find
it less readable.  Especially here as you use braces and “continue”.

>> 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 :)

It's not even bash.  Newest POSIX requires that behaviour, but I always
use x anyway myself, hence the comment.

>>> +	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.

Interesting.  I must start doing that. :)

>>> +	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.

I guess it's a valid point.

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

Sure thing.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

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

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

David Bremner <david@tethera.net> writes:
> Michal Nazarewicz <mina86@mina86.com> writes:
>> On a side note, the whole script could be relatively easily rewritten
>> not to use bash at all and work with plain POSIX shell.

> How does one simulate pipefail?  

pipefail is used only for “find ... | sort”, but I find sorting
unnecessary, so it could be simply removed, and thus pipefail would not
be needed. :)

But I don't have strong feelings (well, I do, but I don't want to impose
them :P ).

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]