Re: [PATCH 2/2] Move portable shell tests from the old to the new testsuite.

[Ralf Wildenhues]

Hi Peter,

* Peter Rosin wrote on Fri, Sep 17, 2010 at 12:37:30PM CEST:
> Subject: [PATCH 2/2] Move portable shell tests from the old to the new 
> testsuite.
> 
> * tests/sh.test: Move this...
> * tests/sh.at: ...to here, and adjust to the new testsuite.
> * Makefile.am: Update.

> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -440,6 +440,7 @@ dist-hook:
>  # The testsuite files are evaluated in the order given here.
>  TESTSUITE    = tests/testsuite
>  TESTSUITE_AT = tests/testsuite.at \
> +               tests/sh.at \
>                 tests/getopt-m4sh.at \
>                 tests/libtoolize.at \
>                 tests/help.at \

At some point we should probably clean up ordering of tests and banners
a bit ...

> diff --git a/tests/sh.at b/tests/sh.at
> new file mode 100644
> index 0000000..7c5633c
> --- /dev/null
> +++ b/tests/sh.at
> @@ -0,0 +1,143 @@
> +# sh.at - check for some nonportable or dubious or undesired shell
> +#         constructs in shell scripts.

Not sure if emacs wants '-*- Autotest -*-' in that first line somewhere
for syntax highlighting?

> +#
> +#   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2010 Free
> +#   Software Foundation, Inc.
> +#   Written by Gary V. Vaughan, 2003

I really don't know how to handle author annotations well when moving
things.  "Code taken from sh.test, written by ..."?

(As an aside issue, I've pondered removing author tags from tests which
*I* alone wrote; but I understand the topic is sensitive and subjective
and I respect differing opinions on this, so I'm not suggesting it for
anybody else ... see recent discussions on automake-patches and
bug-gnulib.)

> +AT_BANNER([Maintainer checks.])
> +
> +AT_SETUP([portable m4sh scripts])
> +
> +m4dir=$top_srcdir/libltdl/m4
> +auxdir=$top_srcdir/libltdl/config
> +scripts="$auxdir/ltmain.m4sh $top_srcdir/libtoolize.m4sh"
> +
> +# Check all the "portable" shell scripts.
> +status=:

status is a special variable for zsh, so should not be used.  But see
below.

> +[
> +# Check for bad binary operators.
> +if $EGREP -n -e 'if[         ]+["'\'']?\$[^  ]+[     
> ]+(=|-[lg][te]|-eq|-ne)' $scripts; then
> +  echo "use \`if test \$something =' instead of \`if \$something ='"
> +  status=false
> +fi

Please don't do this.  The Autotest way is to use AT_CHECK for anything
that can meaningfully go wrong, and whose output may be interesting to
have logged (or checked, or both).  Otherwise TESTSUITEFLAGS=-v will not
produce helpful output.

Hmm, according to autoconf.texi, egrep shares grep's limitations, and
grep -e is not portable; I wonder if that is not actually a problem,
as Solaris 2.6 /usr/bin/egrep groks -e, unlike its /usr/bin/grep.

While rewriting these tests, it would be nice to have TABs before
spaces, less likely to get garbage-collected by editors.

So, I'd write the above as something like this (mind the m4 double
quoting so the regexes come out right):

  # Check for bad binary operators.
  AT_CHECK([[$EGREP -n 'if[     ]+["'\'']?\$[^  ]+[     
]+(=|-[lg][te]|-eq|-ne)' ]]dnl
           [$scripts], [1], [], [],
           [echo "use \`if test \$something =' instead of \`if \$something ='"])

Likewise for all the other tests.

The patch is OK with those issues fixed, but since the chance of typos
is fairly high it'd be nice if you could retest and give us 24 hours to
check your updated patch for issues on some systems.  And since IIRC
Gary wanted to do the release this weekend, I wonder whether this isn't
more safely pushed to after the relase.  WDYT?

Thanks,
Ralf

> +# Check for bad unary operators.
> +if $EGREP -n -e 'if[         ]+-' $scripts; then
> +  echo "use \`if test -X' instead of \`if -X'"
> +  status=false
> +fi
> +
> +# Check for using `@<:@' instead of `test'. @<:@ is a square bracket.
> +if $EGREP -n -e 'if[         ]+\@<:@' $scripts; then
> +  echo "use \`if test' instead of \`if @<:@'"
> +  status=false
> +fi
> +
> +# Check for using test X... instead of test "X...
> +if $EGREP -n -e 'test[       ]+(![   ])?(-.[         ]+)?X' $scripts; then
> +  echo "use \`test \"X...\"' instead of \`test X'"
> +  status=false
> +fi
> +
> +# Check for using test $... instead of test "$...
> +if $EGREP -n -e 'test[       ]+(![   ])?(-.[         ]+)?X?\$' $scripts; then
> +  echo "use \`test \"\$...\"' instead of \`test \$'"
> +  status=false
> +fi
> +
> +# Never use test -e.
> +if $EGREP -n -e 'test[       ]+(![   ])?-e' $scripts; then
> +  echo "use \`test -f' instead of \`test -e'"
> +  status=false
> +fi
> +
> +# Check for uses of Xsed without corresponding echo "X
> +if $EGREP -n -e '\$Xsed' $scripts | $EGREP -v -n -e '\$ECHO \\*"X'; then
> +  echo "occurrences of \`\$Xsed\' without \`echo \"X\' on the same line"
> +  status=false
> +fi
> +
> +# Check for quotes within backquotes within quotes "`"bar"`"
> +if $EGREP -n -e '"[^`"]*`[^"`]*"[^"`]*".*`[^`"]*"' $scripts | \
> +   $EGREP -v "### testsuite: skip nested quoting test$"; then
> +  echo "nested quotes are dangerous"
> +  status=false
> +fi
> +
> +# Check for using set -- instead of set dummy
> +if $EGREP -n -e 'set[        ]+--[   ]+' $scripts; then
> +  echo "use \`set dummy ...' instead of \`set -- ...'"
> +  status=false
> +fi
> +
> +# Check for using shift after set dummy (same or following line).
> +for s in $scripts
> +do
> +  if $SED -n '
> +      /set[  ][      ]*dummy/{
> +       /set.*dummy.*;.*shift/d
> +       N
> +       /\n.*shift/D
> +       p
> +      }' "$s" | $EGREP .; then
> +    echo "use \`shift' after \`set dummy' in $s"
> +    status=false
> +  fi
> +done
> +
> +# Check for opening brace on next line in shell function definition.
> +# redirect stderr so we also barf when sed issues diagnostics.
> +for s in $scripts
> +do
> +  if $SED -n '
> +      /^func_.*(/{
> +       N
> +       /^func_[^     ]* ()\n{$/d
> +       p
> +      }' "$s" 2>&1 | $EGREP .; then
> +    echo "Function definitions should look like this in $s:
> +func_foo ()
> +{
> +  # ...
> +}"
> +    status=false
> +  fi
> +done
> +
> +# Check for correct usage of $cc_basename.
> +# redirect stderr so we also barf when sed issues diagnostics.
> +for s in "$m4dir/libtool.m4"
> +do
> +  if $SED -n '/case \$cc_basename in/,/esac/ {
> +           /^[       ]*[a-zA-Z][a-zA-Z0-9+]*[^*][    ]*)/p
> +           }'  $s 2>&1 | $EGREP .; then
> +    echo "\$cc_basename matches should include a trailing \`*' in $s."
> +    status=false
> +  fi
> +done
> +]
> +
> +AT_CHECK([$status], [], [ignore])
> +
> +AT_CLEANUP