Re: [PATCH] {maint} tests: use more POSIX shell features our test scripts

[Eric Blake]

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   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature