Re: [PATCH] Overhauled and modularized tests in `instspc.test'.

[Ralf Wildenhues]

Hi Stefano,

* Stefano Lattarini wrote on Tue, Sep 07, 2010 at 07:20:15PM CEST:
> The new patch has been tested on:
> 
>  t1. Debian GNU/Linux with system (GNU) utilities, autoconf 2.67,
>      bash 4.1, and GNU make 3.81
>  t2. Debian GNU/Linux with system (GNU) utilities, autoconf 2.67,
>      pdksh "@(#)PD KSH v5.2.14 99/07/13.2", and FreeBSD make 8.1,
>  t3. Debian GNU/Linux with autoconf 2.67, Heirlooom sh, Heirloom
>      tools and Heirlooom make
>  t4. Solaris 10 with autoconf 2.67, bash 3.0, GNU make 3.82 and
>      mixed system and GNU tools
>  t5. Solaris 10 with autoconf 2.62, /usr/xpg4/bin/make, /bin/ksh,
>      and basic system utilities (those in /usr/bin alone).
>  t6. Solaris 10 with autoconf 2.62, /usr/ccs/bin/make, /bin/sh,
>      basic system utilities (those in /usr/bin alone).
> 
> No test was skipped, all the non-xfailing tests succeeded.
> 
> OK for master?

This patch ought to be tested with AIX sh, Tru64 ksh, and IRIX.
When you're through addressing the nits below, and still don't have
access, ping me and I'll test it.  I'm sorry to have you do another
round on this one.

The patch introduces significant extra testsuite overhead.  Would it be
possible to unify build and install tests pairwise?

A big thanks for this one,
Ralf

> From bc70edd3627fc20785211ad06c9fe386ebecccff Mon Sep 17 00:00:00 2001
> From: Stefano Lattarini <address@hidden>
> Date: Sun, 6 Jun 2010 18:38:27 +0200
> Subject: [PATCH] Overhauled and modularized tests in `instspc.test'.
> 
> The test `instspc.test' was way too big and fragile.  Its running
> time was very long.  It also produced a log that was nearly
> unreadable due to its lenght, making it very difficult to find

length

> out the reason of failures.

s/of/for/  (sorry for being so nitpicky)

> Also, it was too much monolithic, with a single (maybe spurious)
> failure in a corner case causing the whole test to fail (even if
> everything worked as expected in the other 99% of cases).
> The present change should solve these problems, by separating
> `instspc.test' into many smaller, self-contained, auto-generated
> tests.
> 
> * tests/gen-instspc-tests: New file, generates a distributed
> Makefile.am snippet `tests/instspc-tests.am'.  This contains
> rules to generate a host of new tests `instspc*.test', which
> will take over the older `instspc.test'.
> * tests/instspc.test: Moved to ...
> * tests/instspc.sh: ... this, and modified (quite heavily) to
> adapt to the new circumstances (it will be included by all the
> newly generated `instspc*.test').
> * tests/Makefile.am ($(srcdir)/instspc-tests.am): Include this
> snippet, which (among the other things) defines ...
> (instspc_tests): ... this new macro, containing the list of the
> newly generated `instspc*.test' tests, and ...
> (instspc_xfail_tests): ... this new macro, containing the list
> of the `instspc*.test' tests expected to fail.
> ($(instspc_tests:.test=.log)): New rule, registers the dependency
> of all `instspc*.test' tests from the `instspc.sh' script.

s/from/on/

> (TESTS): Add `$(instspc_tests)', remove `instspc.test'.
> (XFAIL_TESTS): Add `$(xfail_instspc_tests)'.
> (EXTRA_DIST): Distribute gen-instspc-tests and instspc.sh.
> (MAINTAINERCLEANFILES): Added $(instspc_tests).
> Other minor cosmetic changes.
> * bootstrap: Generate instspc-tests.am.
> * tests/.gitignore: Updated.

> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -16,6 +16,9 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> +MAINTAINERCLEANFILES =               ## updated later
> +EXTRA_DIST = ChangeLog-old   ## likewise

Please avoid comments in variable settings.  I know that '##' are
exceptional in that automake removes them, but here, not only would
a comment on a line before that be more readable and less confusing,
just dropping them would be most readable, and not less clear.

>  XFAIL_TESTS =                                        \
>  all.test                                     \
>  auxdir2.test                                 \
> @@ -23,6 +26,7 @@ cond17.test                                 \
>  gcj6.test                                    \
>  txinfo5.test
>  
> +
>  include $(srcdir)/parallel-tests.am
>  
>  $(srcdir)/parallel-tests.am: gen-parallel-tests Makefile.am
> @@ -51,7 +55,22 @@ $(parallel_tests): Makefile.am
>       } > address@hidden
>       $(AM_V_at)chmod a+rx address@hidden && mv -f address@hidden $@
>  
> -MAINTAINERCLEANFILES = $(parallel_tests)
> +MAINTAINERCLEANFILES += $(parallel_tests) $(instspc_tests)
> +EXTRA_DIST += gen-parallel-tests
> +
> +
> +include $(srcdir)/instspc-tests.am
> +
> +$(srcdir)/instspc-tests.am: gen-instspc-tests Makefile.am
> +     $(AM_V_GEN)(cd $(srcdir) && $(SHELL) ./gen-instspc-tests) >$@

This cd needs to be $(am__cd); the one in the parallel_tests.am rule
would need it too (separate patch for the latter is preapproved).

> +# All instspc*.test tests work by sourcing the instspc.sh script.
> +$(instspc_tests:.test=.log): $(srcdir)/instspc.sh
> +
> +MAINTAINERCLEANFILES += $(parallel_tests) $(instspc_tests)
> +EXTRA_DIST += gen-instspc-tests instspc.sh
> +XFAIL_TESTS += $(instspc_xfail_tests)
> +
>  
>  TESTS = \
>  aclibobj.test \
> @@ -411,7 +430,6 @@ instman2.test \
>  instmany.test \
>  instmany-mans.test \
>  instmany-python.test \
> -instspc.test \
>  interp.test \
>  interp2.test \
>  java.test \
> @@ -813,9 +831,11 @@ yaccpp.test \
>  yaccvpath.test \
>  yflags.test \
>  yflags2.test \
> -$(parallel_tests)
> +$(parallel_tests) \
> +$(instspc_tests)

There is no need to insert them at the end, you can just put them where
instspc.test was before.  The parallel tests were only added there
because there is no canonical alphabetic place for them.

> +
> +EXTRA_DIST += $(TESTS)
>  
> -EXTRA_DIST = ChangeLog-old gen-parallel-tests $(TESTS)
>  
>  # Each test case depends on defs, aclocal, and automake.
>  $(TEST_LOGS): defs aclocal-$(APIVERSION) automake-$(APIVERSION)


> --- /dev/null
> +++ b/tests/gen-instspc-tests

> +# Generate a Makefile fragment which defines:
> +#  1. Rules to generate tests that we can building from, and/or install
> +#     to, directories having in their name shell/make metacharacters,
> +#     control characters, white spaces, autoconf quadrigraphs, or other
> +#     suspicious/problematic tokens.
> +#  2. The macro $(instspc_tests), containing the list of the tests
> +#     generated by the makefile fragment.
> +#  3. The macro $(xfail_instspc_test), containing the list of those

Typo?  s/test/&s/  ?

> +#     generated tests expected to fail.
> +
> +# Original report from James Amundson about file names with spaces.
> +# Other characters added by Paul Eggert.
> +
> +set -e # abort on unhandled errors

Does this really need a comment?

> +# Defined to help avoiding headaches with multiple escaping into
> +# backquotes, below.
> +escape_dollar() { sed 's/\$/$$/g'; }

GNU style is space before (), newlines before { and }
more instances below

> +
> +cat <<'END'
> +## Generated by gen-instspc-tests.  DO NOT EDIT!
> +instspc_tests =
> +instspc_xfail_tests =
> +END
> +
> +count=0
> +# Some of these contain symbolic reperesentations of problematic

representations

> +# characters, which could easily confuse make (e.g. `#', `$' or
> +# newline).  They will be properly expanded by instspc.sh.
> +for weird_chars in \

"weird" is colloquial, and also not very fitting IMVHO.  Please use
just $file or so, like the original code did.  I guess I simply don't
understand why this code deviates so much from the original.

> +  '!' '"' '${sh}' '${dl}' '%' '&' '${sq}' '(' ')' '*' '+' ',' '-' \
> +  ':' ';' '<' '=' '>' '?' '@' '[' '\' ']' '^' '`' '{' '|' '}' '~' \
> +  '${bs}' '${cr}' '${ff}' '${ht}' '${lf}' '${sp}' \
> +  '@<:@' '@:>@' '@S|@' '@%:@' '@&t@' \
> +  'a${sp}b' 'a${sp}${sp}b' 'a${lf}b' '...' 'a:'
> +do
> +  count=`expr $count + 1`
> +  # Simply naming the tests using incremental numbers seems to
> +  # be the best policy.

Would've been nice to have names reflecting the characters (maybe an
ascii translation), but I understand that this might be too much to ask
for.

> +  tst=instspc$count
> +  # We have to escape `$' in makefiles.
> +  case "$weird_chars" in
> +    *\$*) weird_escaped=`printf '%s' "$weird_chars" | escape_dollar`;;
> +    *) weird_escaped=$weird_chars;;
> +  esac
> +  echo # separate sections for different tests

Do we really have to comment this?

> +  echo "$tst-build.test $tst-install.test: instspc-tests.am"
> +  cat <<'END'
> +     $(AM_V_at)rm -f $@ address@hidden
> +## The apparently useless ":;" works around a bug of Bash 3.2 and earlier.

s/apparently useless //
s/of/in/

> +## See section "Limitations of Shell Builtins" in the Autoconf manual.
> +     $(AM_V_GEN) :; { \
> +       action=`echo $@ | sed -e 's/\.test$$//' -e s/^.*-//;`; \
> +       echo '#!/bin/sh'; \
> +       echo '# DO NOT EDIT! GENERATED AUTOMATICALLY!'; \
> +       echo 'required=gcc # FIXME: any C compiler should be ok!'; \

I don't understand the FIXME.  Either any compiler is ok, in which case
we could remove the line, or it isn't, in which case there is nothing
to fix.

In this case, when you remove the required line, chances are nonzero
that we'll get spurious extra failures due to some compilers having
crappily-programmed shell wrapper drivers or so.  The question is
whether we want to see those issues as test failures or not.  I'm not
sure.

> +       echo '. ./defs || Exit 99 || exit 99'; \

;-)

> +       echo action=$$action; \
> +END
> +  # leading tab here
> +  echo "       echo weird_chars=\\''$weird_escaped'\\'; \\"
> +  cat <<'END'
> +       echo '. "$$testsrcdir/instspc.sh"'; \
> +     } > address@hidden
> +     $(AM_V_at)chmod a+rx address@hidden && mv -f address@hidden $@
> +END
> +  echo "instspc_tests += $tst-build.test $tst-install.test"
> +  # Some of the above "weird" file names cannot be used as a build

See above.

> +  # directory on a POSIX host.  This list should be empty, but is
> +  # not due to limitations in Autoconf, Automake, Make, M4, or the
> +  # shell.
> +  case $weird_chars in
> +    \"|'${sh}'|'${dl}'|'${sq}'|\&|\\|\`|'${lf}'|'@&t@'|'a${lf}b')
> +      echo "instspc_xfail_tests += $tst-build.test";;
> +  esac
> +  # Similarly, some of the "weird" file names cannot be used as an
> +  # install directory on a POSIX host.  This list should ideally
> +  # also be empty.
> +  case $weird_chars in
> +    \"|'${sh}'|'${dl}'|'${sq}'|\`|'${lf}'|'a${lf}b')
> +      echo "instspc_xfail_tests += $tst-install.test";;
> +  esac
> +done

> --- /dev/null
> +++ b/tests/instspc.sh

> +# Check that building from, or installing to, directories with shell
> +# metacharacters succeed.
> +
> +# This script is expected to be sourced by specific, individual tests,
> +# which, before its inclusion, should properly define the variable
> +# `$weird_chars' and `$action' (this last one to either "build"
> +# or "install").
> +
> +# Original report from James Amundson about file names with spaces.
> +# Other characters added by Paul Eggert.
> +
> +# Transition from `instspc.test' to `instspc.sh' done
> +# by Stefano Lattarini.

I was kinda hoping to avoid territoriality in Automake.  Fine if you
insist, but I suggest reading the following for a discussion of the
topic: <http://producingoss.com/en/producingoss.html#territoriality>
(the whole book is very interesting BTW).

> +# This is mostly the same input as nobase.test, but we do not use
> +# libtool libraries, because Libtool does not preserve space in
> +# file names (Issue observed with ltmain.sh (GNU libtool) 1.5a (1.1323
> +# 2003/11/10 21:06:47))

Yeah, that reference to old code is not really relevant.  libtool does
not preserve spaces, not even today.

> +set -e
> +
> +# This file will be sourced by many tests, so avoid cluttering up
> +# the verbose logs too much.
> +set +x

I understand, and agree, with the sentiment.  Problem however is that
this will make it harder to debug your shell script, because
  sh -x instspc-$foo.test

will be useless.  Not sure how to avoid that.

> +echo "INFO: $me: shell traces disabled"

This weird "INFO: " prefix doesn't look like normal GCS-like messages.

Status messages on stderr?  At least for error messages.

> +echo "INFO: $me: action=$action"
> +echo "INFO: $me: weird_chars='$weird_chars'"
> +
> +# Sanity check 1.
> +# NOTE: Don't use `test -z' here since Solaris 10 /bin/ksh seems
> +# to have problems grokking `test -z ")"'.
> +if test x"$weird_chars" = x; then
> +  echo "$me: FATAL: \$weird_chars unset or empty"
> +  Exit 99
> +fi
> +
> +# Sanity check 2.
> +case $action in
> +  "") echo "$me: FATAL: \$action unset or empty"; Exit 99;;

inconsistent "$me: " prefixing order, see INFO: above.  FWIW, I'd just
drop INFO: and FATAL:, it is not used anywhere else in the testsuite,
and things should be clear from the exit status.  If you really want
such a prefix, GCC uses 'warning: ' and 'error: ' and I think 'notice: '.

More information on this topic in "info standards Errors".

> +  build|install) ;;
> +  *) echo "$me: FATAL: invalid \$action: '$action'"; Exit 99;;
> +esac
> +
> +# Some control characters that are white space.
> +bs='' # back space
> +cr='
' # carriage return
> +ff='' # form feed
> +ht=' ' # horizontal tab
> +lf='
> +'  # line feed (aka newline)

comment alignment?

> +sp=' ' # space
> +
> +# Some other character that might be problematic in makefiles.
> +dl=\$ # dollar
> +sq=\' # single quote
> +sh=\# # sharp
> +
> +# Now we might have to perform some substitutions in the given "weird
> +# file name".  This is necessary, because some "unusual" characters
> +# can easily wreak havoc in makefiles (see the comments in the script
> +# `gen-instspc-tests' for more information).  Since we are at it, we
> +# perform some more sanity checks.
> +case $weird_chars in
> +  *\#*|*"$lf"*|*'${lf}${lf}')

The second pattern can just be *$lf* here.

> +    echo "$me: invalid \$weird_chars: '$weird_chars'" >&2

Are you guarding against broken shells here, that mismatch patterns?
I don't understand what the failure scenario is for this.

> +    Exit 99
> +    ;;
> +  '${bs}'|'${cr}'|'${ff}'|'${ht}'|'${lf}'|'${sp}'|'${dl}'|'${sq}'|'${sh}')
> +    eval "weird_chars=$weird_chars" || Exit 99

How can this eval go wrong?  Oh well, guess being extra safe is not a
problem.

> +    if test -z "$weird_chars"; then
> +      echo "$me: \"weird\" char '$weird_chars' evaluated to empty" >&2
> +      Exit 99
> +    fi
> +    ;;
> +  *'${sp}'*|*'${lf}'*)
> +    #
> +    # Whow, this is tricky!
> +    #
> +    # NOTE 1: The `subst_spaces' function helps us avoiding headaches
> +    #         with backslashes into backquotes.
> +    #
> +    # NOTE 2: The use of `#' and newline as a characters "impossible in
> +    #         input" is OK, because we $weird_chars should never contain
> +    #         newline or a `#' character (the first match in the current
> +    #         "case" statement takes care of this check).
> +    #
> +    # NOTE 3: We must add a trailing newline to the expression printed by
> +    #         printf, to remain portable to all sed implementations.
> +    #
> +    # NOTE 4: We must be careful not to let the command suctitution strip
> +    #         significant trailing newlines.  This is not difficult, as
> +    #         we can be positivie that there is at most one such trailing
> +    #         newline (the first match in the current "case" statement
> +    #         takes care of this check).
> +    #
> +    subst_spaces() {
> +      printf '%s\n' "$*" | sed -e 's/\${sp}/ /g' -e 's/\${lf}/#/g' \
> +        | tr -d "$lf" | tr "#" "$lf"

I'm not so sure $* inside double-quotes is portable to old shells.
But see below.

> +    }
> +    new_weird_chars=`subst_spaces "$weird_chars"` || Exit 99
> +    case $weird_chars in
> +      *'${lf}')
> +        # Add back trailing newline stripped by command substitution.
> +        weird_chars=${new_weird_chars}${lf};;
> +      *)
> +        weird_chars=$new_weird_chars;;
> +    esac

Is this tested on MSYS?

> +    unset new_weird_chars

I don't get this whole section.  Why not just
      \${sp}) weird_chards=" " ;;
      \${lf}) weird_chards="$lf" ;;

and so on?  That would allow you to get rid of the big NOTEs above,
subst_spaces, and all code regarding new_weird_chars, right?

> +    ;;
> +  *\$*)
> +    echo "$me: invalid \$weird_chars: '$weird_chars'" >&2
> +    Exit 99
> +    ;;
> +esac
> +
> +echo "INFO: $me: munged x\"\$weird_chars\"x: x${weird_chars}x"
> +echo "INFO: $me: enabling shell traces"
> +set -x
> +
> +# Skip if this system doesn't support this characters in file names.

Either "this character" or "these characters".

> +# The created directory will also be used below, so don't remove.

Useless comment.

> +mkdir "./$weird_chars" || Exit 77
> +
> +mkdir sub1  # Will be used below.

Ditto.

> +cat >> configure.in << 'EOF'
> +AC_PROG_CC
> +AC_PROG_RANLIB
> +AC_OUTPUT
> +EOF
> +
> +mkdir sub
> +
> +: > sub/base.h
> +: > sub/nobase.h
> +: > sub/base.dat
> +: > sub/nobase.dat
> +: > sub/base.sh
> +: > sub/nobase.sh
> +
> +cat > source.c << 'EOF'
> +int
> +main (int argc, char **argv)
> +{
> +  return 0;
> +}
> +EOF
> +cp source.c source2.c
> +
> +cat > Makefile.am << 'EOF'
> +foodir = $(prefix)/foo
> +fooexecdir = $(prefix)/foo
> +
> +foo_HEADERS = sub/base.h
> +nobase_foo_HEADERS = sub/nobase.h
> +
> +dist_foo_DATA = sub/base.dat
> +nobase_dist_foo_DATA = sub/nobase.dat
> +
> +dist_fooexec_SCRIPTS = sub/base.sh
> +nobase_dist_fooexec_SCRIPTS = sub/nobase.sh
> +
> +fooexec_PROGRAMS = sub/base
> +nobase_fooexec_PROGRAMS = sub/nobase
> +sub_base_SOURCES = source.c
> +sub_nobase_SOURCES = source.c
> +
> +fooexec_LIBRARIES = sub/libbase.a
> +nobase_fooexec_LIBRARIES = sub/libnobase.a
> +sub_libbase_a_SOURCES = source.c
> +sub_libnobase_a_SOURCES = source.c
> +
> +.PHONY: test-install-sep
> +test-install-sep: install
> +     test   -f '$(DESTDIR)/$(file)-prefix/foo/sub/nobase.h'
> +     test ! -f '$(DESTDIR)/$(file)-prefix/foo/nobase.h'
> +     test   -f '$(DESTDIR)/$(file)-prefix/foo/base.h'
> +     test   -f '$(DESTDIR)/$(file)-prefix/foo/sub/nobase.dat'
> +     test ! -f '$(DESTDIR)/$(file)-prefix/foo/nobase.dat'
> +     test   -f '$(DESTDIR)/$(file)-prefix/foo/base.dat'
> +     test   -f '$(DESTDIR)/$(file)-prefix/foo/sub/nobase.sh'
> +     test ! -f '$(DESTDIR)/$(file)-prefix/foo/nobase.sh'
> +     test   -f '$(DESTDIR)/$(file)-prefix/foo/base.sh'
> +     test   -f '$(DESTDIR)/$(file)-prefix/foo/sub/nobase$(EXEEXT)'
> +     test ! -f '$(DESTDIR)/$(file)-prefix/foo/nobase$(EXEEXT)'
> +     test   -f '$(DESTDIR)/$(file)-prefix/foo/base$(EXEEXT)'
> +     test   -f '$(DESTDIR)/$(file)-prefix/foo/sub/libnobase.a'
> +     test ! -f '$(DESTDIR)/$(file)-prefix/foo/libnobase.a'
> +     test   -f '$(DESTDIR)/$(file)-prefix/foo/libbase.a'
> +EOF
> +
> +$ACLOCAL
> +$AUTOCONF
> +$AUTOMAKE -a
> +
> +case $action in
> +  build)
> +    build=$weird_chars
> +    dest=`pwd`/sub1;;
> +  install)
> +    build=sub1
> +    dest=`pwd`/$weird_chars;;
> +esac
> +
> +cd "./$build"
> +
> +../configure --prefix "/$weird_chars-prefix"
> +$MAKE
> +DESTDIR="$dest" file="$weird_chars" $MAKE -e test-install-sep
> +
> +: