On 06/14/2012 10:17 AM, Stefano Lattarini wrote: > Since commit 'v1.12-36-g2d68fd9' of 2012-05-07, "configure: search a > sturdy POSIX shell to be used in the testsuite", the shell running > our test script is assured to be a POSIX-conforming shell, so we can > use the more modern and flexible idioms and features that we couldn't > use when we also aimed at compatibility with non-POSIX Bourne shells, > like Solaris /bin/sh. > > * t/README: Suggest to use POSIX shell features liberally in test cases, > with possible exception of Makefile recipes and configure shell code. > * Several tests: Adjust to use more POSIX shell features; e.g., $(...) > rather than `...`, $((...)) rather than `expr ...`, "if ! CMD; then ..." > instead of "if CMD; then :; else ...", and so on. > In several places, when using the 'test' built-in, prefer '-eq' over > '=' for numeric comparisons,
Good.
> and prefer "grep -c PATTERN FILE" over
> "grep PATTERN FILE | wc -l".
Unrelated to POSIX shell, but worth doing. At this point, though, it's
not worth splitting into a separate patch.
A few questions, and points for further improvements:
> - # Else, use GNU seq if available.
> - seq "$@" && return 0
> # Otherwise revert to a slower loop using expr(1).
> i=$seq_first
> while test $i -le $seq_last; do
> echo $i
> - i=`expr $i + $seq_incr`
> + i=$(($i + $seq_incr))
The comment about expr is now outdated.
> -ACLOCAL_PATH="`pwd`/none:`pwd`/none2" $ACLOCAL --install && Exit 1
> +ACLOCAL_PATH="$(pwd)/none:$(pwd)/none2" $ACLOCAL --install && Exit 1
General observation - now that we guarantee a POSIX shell, we are
guaranteed that $PWD is sane, and can shave a fork instead of using
$(pwd). Probably worth a followup patch.
> @@ -79,7 +79,7 @@ check_count=0
> check_ ()
> {
> set +x # Temporary disable shell traces to remove noise from log files.
> - incr_ check_count
> + check_count=$(($check_count + 1))
incr_ still looks nicer, and worth keeping as a utility function (even
if it is changed to be implemented with $(()) instead of expr under the
hood).
> +++ b/t/autodist.sh
> @@ -39,7 +39,7 @@ list=`$AUTOMAKE --help \
> | sed -n '/^Files.*automatically distributed.*if found.*always/,/^
> *$/p' \
> | sed 1d`
> # Normalize whitespace, just in case.
> -list=`echo $list`
> +list=$(echo $list)
This idiom seems common, but wastes a command substitution fork() in
place of:
set - $list
list="$*"
> +++ b/t/ax/tap-functions.sh
> @@ -31,24 +31,10 @@ tap_fail_count_=0
> tap_xfail_count_=0
> tap_xpass_count_=0
>
> -# The first "test -n" tries to avoid extra forks when possible.
> -if test -n "${ZSH_VERSION}${BASH_VERSION}" \
> - || (eval 'test $((1 + 1)) = 2') >/dev/null 2>&1
> -then
> - # Outer use of 'eval' needed to protect dumber shells from parsing
> - # errors.
> - eval 'incr_ () { eval "$1=\$((\${$1} + 1))"; }'
I think we might as well have:
incr_ () { eval "$1=\$(($1 + 1))"; }
as it is still a useful idiom.
> case $tap_result_,$tap_directive_ in
> - ok,) incr_ tap_pass_count_;; # Passed.
> - not\ ok,TODO) incr_ tap_xfail_count_;; # Expected failure.
> - not\ ok,*) incr_ tap_fail_count_ ;; # Failed.
> - ok,TODO) incr_ tap_xpass_count_ ;; # Unexpected pass.
> - ok,SKIP) incr_ tap_skip_count_ ;; # Skipped.
> - *) bailout_ "internal error in 'result_'";; # Can't happen.
> + ok,) # Passed.
> + tap_pass_count_=$(($tap_pass_count_ + 1)) ;;
Case in point - look at the size explosion when you don't keep incr_ around.
> @@ -169,7 +161,7 @@ skip_ () { result_ 'ok' -D SKIP ${1+"$@"}; }
> skip_row_ ()
> {
> skip_count_=$1; shift
> - for i_ in `seq_ $skip_count_`; do skip_ ${1+"$@"}; done
> + for i_ in $(seq_ $skip_count_); do skip_ ${1+"$@"}; done
Now that we assume POSIX shells, is it safe to assume that "$@" is no
longer buggy? (Honest question; I haven't researched it well enough
yet). Or, should we enhance our filter that guarantees us a decent
shell to also weed out shells where "$@" is broken?
> +++ b/t/compile4.sh
> @@ -25,25 +25,24 @@ get_shell_script compile
> mkdir sub
>
> cat >sub/foo.c <<'EOF'
> -int
> -foo ()
> +int foo (void)
Why the cosmetic change? This violates the GNU Coding Standards
preferred formatting.
> +++ b/t/compile5.sh
> @@ -46,7 +46,7 @@ case '@host_os@' in
> skip_ "target OS is not MinGW"
> ;;
> esac
> -case @build_os@ in
> +case '@build_os@' in
Why the added quotes? Will @biuld_os@ ever expand to more than one
shell word?
> @@ -66,45 +66,56 @@ setup_vars_for_compression_format ()
> esac
> }
>
> +have_compressor ()
> +{
> + test $# -eq 1 || fatal_ "have_compressor(): bad usage"
> + case $1 in
> + # Assume gzip(1) is available on every reasonable portability target.
> + gzip)
> + return 0;;
> + # On Cygwin, as of 9/2/2012, 'compress' is provided by sharutils
> + # and is just a dummy script that is not able to actually compress
> + # (it can only decompress). So, check that the 'compress' program
> + # is actually able to compress input.
> + # Note that, at least on GNU/Linux, 'compress' does (and is
> + # documented to) exit with status 2 if the output is larger than
> + # the input after (attempted) compression; so we need to pass it
> + # an input that it can actually reduce in size when compressing.
> + compress)
> + for x in 1 2 3 4 5 6 7 8; do
> + echo aaaaaaaaaaaaaaaaaaaaa
> + done | compress -c >/dev/null && return 0
> + return 1
> + ;;
> + *)
> + # Redirect to stderr to avoid pollutinh the output, in case this
s/pollutinh/polluting/ while doing this code motion.
> @@ -171,7 +182,7 @@ can_compress ()
> && $MAKE dist-$format \
> && test -f $tarname-1.0.$suffix \
> && ls -l *$tarname* \
> - && test "`ls *$tarname*`" = $tarname-1.0.$suffix'
> + && test "$(ls *$tarname*)" = $tarname-1.0.$suffix'
Why are we wasting an ls process? Wouldn't echo work? For that matter,
even:
&& test *$tarname* = $tarname-1.0.$suffix
would work in the success case (and hopefully cause a test syntax error
in the failure case).
> @@ -430,7 +441,7 @@ ls -l # For debugging.
> cd ..
>
> oPATH=$PATH
> -PATH=`pwd`/bin$PATH_SEPARATOR$PATH; export PATH
> +PATH=$(pwd)/bin$PATH_SEPARATOR$PATH; export PATH
We're assuming a POSIX shell; shouldn't that mean that we can now write:
export PATH=$PWD/bin$PATH_SEPARATOR$PATH
or do we still have to worry about shells that don't support 'export
name=$expansion'? And even if we assume export with assignment works,
do we still have to worry about shells that word-split expansion, where
you'd have to write:
export PATH="$PWD/bin$PATH_SEPARATOR$PATH"
> +++ b/t/gcj3.sh
> @@ -31,7 +31,6 @@ END
> $ACLOCAL
> $AUTOMAKE
>
> -num=`grep depcomp Makefile.in | wc -l`
> -test $num -gt 1
> +test $($FGREP -c depcomp Makefile.in) -gt 1
Are you sure this idiom is safe on shells that don't handle Ctrl-C very
well? As documented in autoconf, even some /bin/sh that otherwise look
like POSIX (I think it was a BSD machine) will elide a terminateed
command substitution and still evaluate the resulting expression; in
cases where the result is like 'test -gt 1', the syntax error has the
desired effect, but I'm worried there are other places where you could
end up with problems when mixing a command substitution in the middle of
a complex command instead of using a temporary.
> +++ b/t/hdr-vars-defined-once.sh
This is as far as I got in my line-by-line review in the time I have
today; if you want to wait for another few days, I can resume my review
in time for your initial push. Or you can go ahead and push, and I can
report any further issues for followup patches, since the overall idea
looks good and most of the changes appear to have been mechanical.
--
Eric Blake ebl...@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
