Re: [PATCH 1/7] Tests defs: various reorderings, some improvements.

[Stefano Lattarini]

On Wednesday 10 November 2010, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:00:47PM CET:
> > This is basically just a reordering patch, which should organize the
> > code in `tests/defs.in' in a clearer and more rational way.
> > 
> > * tests/defs.in: Reordered various snippets of code in a clearer
> > way.  Improved a couple of error messages, by reporting the test
> > name in them.  Some comments added.
> 
> I cannot review this patch.  Either just reordering, or just other
> changes, but not both in one patch please.
Well, the patch is "almost" just code reordering, with the very minor
exceptions that:

  1. in the older version, some error messages could be printed before
     `$me' was defined, so they couldn't use it; now `$me' is defined
     early, so those error messages has been amended to include it;

  2. in the older version, a test might be skipped from within code
     in `tests/defs' *after* the exit trap was installed, so a call
     to "Exit 77" was in order; now the skips only happens before the
     installation of the exit trap, so "exit 77" is enough;

  3. some comments has been slighty adjusted and extended, because the
     act of reordering the code suggested them to me.

Anyway, none of these additional "minor" changes is really required to
make the new `tests/defs.in' work correctly, so I agree that they should
have been done in follow-up patches.  I've respin and split my patch to
do so (see the attached four patches).

Thanks and sorry for the noise,
   Stefano

From d402d06c01d3f065ce7d2c0d05885d1873dac410 Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Wed, 2 Jun 2010 22:25:25 +0200
Subject: [PATCH 1/4] Tests defs: various reorderings.

* tests/defs.in: Reordered various snippets of code in a
clearer way.
---
 ChangeLog     |    6 +
 tests/defs.in |  347 ++++++++++++++++++++++++++++++++-------------------------
 2 files changed, 199 insertions(+), 154 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 657ea37..8fd8bad 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2010-11-10  Stefano Lattarini  <address@hidden>
+
+       Tests defs: various reorderings.
+       * tests/defs.in: Reordered various snippets of code in a
+       clearer way.
+
 2010-11-06  Stefano Lattarini  <address@hidden>
 
        New tests on obsoleted usages of automake/autoconf macros (such
diff --git a/tests/defs.in b/tests/defs.in
index fe67b0f..57a9cde 100644
--- a/tests/defs.in
+++ b/tests/defs.in
@@ -20,6 +20,11 @@
 # Defines for Automake testing environment.
 # Tom Tromey <address@hidden>
 
+
+## --------------------------------------------------------------------- ##
+##  Shell and environment sanitizing (plus some early initializations).  ##
+## --------------------------------------------------------------------- ##
+
 # Absolutely necessary variable(s).
 testsrcdir='@abs_srcdir@'
 top_testsrcdir='@abs_top_srcdir@'
@@ -44,26 +49,20 @@ else
   case `(set -o) 2>/dev/null` in *posix*) set -o posix;; esac
 fi
 
-# Ensure we are running from the right directory.
-test -f ./defs || {
-   echo "defs: not found in current directory" 1>&2
-   exit 1
-}
-
-# Ensure $testsrcdir is set correctly.
-test -f "$testsrcdir/defs.in" || {
-   echo "$testsrcdir/defs.in not found, check \$testsrcdir" 1>&2
-   exit 1
-}
+echo "=== Running test $0"
 
 me=`echo "$0" | sed -e 's,.*[\\/],,;s/\.test$//'`
 
+
+## ----------------------------------------------------------- ##
+##  Initialization: AC_SUBST'ed and/or environment variables.  ##
+## ----------------------------------------------------------- ##
+
 APIVERSION='@APIVERSION@'
 PATH_SEPARATOR='@PATH_SEPARATOR@'
 
 # Make sure we override the user shell.
-SHELL='@SHELL@'
-export SHELL
+SHELL='@SHELL@'; export SHELL
 # User can override various tools used.
 test -z "$PERL" && PERL='@PERL@'
 test -z "$MAKE" && MAKE=make
@@ -75,8 +74,7 @@ test -z "$MISSING" && MISSING=$top_testsrcdir/lib/missing
 # (Tests for which this is inappropriate should use -Wno-error.)
 test -z "$ACLOCAL" && ACLOCAL="aclocal-$APIVERSION -Werror"
 # Extra flags to pass to aclocal before all other flags added by this script.
-ACLOCAL_TESTSUITE_FLAGS=
-export ACLOCAL_TESTSUITE_FLAGS
+ACLOCAL_TESTSUITE_FLAGS=''; export ACLOCAL_TESTSUITE_FLAGS
 
 # See how Automake should be run.  We put --foreign as the default
 # strictness to avoid having to create lots and lots of files.  A test
@@ -86,11 +84,130 @@ export ACLOCAL_TESTSUITE_FLAGS
 # should use -Wnone or/and -Wno-error
 test -z "$AUTOMAKE" && AUTOMAKE="automake-$APIVERSION --foreign -Werror -Wall"
 
-PATH="`pwd`$PATH_SEPARATOR$PATH"
+# POSIX no longer requires 'egrep' and 'fgrep',
+# but some hosts lack 'grep -E' and 'grep -F'.
+EGREP='@EGREP@'
+FGREP='@FGREP@'
+
+# The amount we should wait after modifying files depends on the platform.
+# For instance, Windows '95, '98 and ME have 2-second granularity
+# and can be up to 3 seconds in the future w.r.t. the system clock.
+sleep='sleep @MODIFICATION_DELAY@'
+
+# An old timestamp that can be given to a file, in "touch -t" format.
+# The time stamp should be portable to all file systems of interest.
+# Just for fun, choose the exact time of the announcement of the GNU project
+# in UTC; see <http://www.gnu.org/gnu/initial-announcement.html>.
+old_timestamp=198309271735.59
+
+# Absolute path of the testsuite buildir.
+curdir=`pwd`
+
+# Make our wrapper scripts accessible by default.
+PATH="`pwd`$PATH_SEPARATOR$PATH"; export PATH
+
+
+## -------------------------------------------------------- ##
+##  Initialization: sanity checks and environment cleanup.  ##
+## -------------------------------------------------------- ##
+
+# Ensure we are running from the right directory.
+test -f ./defs || {
+   echo "./defs: not found in current directory" >&2
+   exit 1
+}
+
+# Ensure $testsrcdir is set correctly.
+test -f "$testsrcdir/defs.in" || {
+   echo "$testsrcdir/defs.in not found, check \$testsrcdir" >&2
+   exit 1
+}
+
+# Unset some MAKE... variables that may cause $MAKE to act like a
+# recursively invoked sub-make.  Any $MAKE invocation in a test is
+# conceptually an independent invocation, not part of the main
+# 'automake' build.
+unset MFLAGS MAKEFLAGS AM_MAKEFLAGS MAKELEVEL __MKLVL__ MAKE_JOBS_FIFO
+# Unset verbosity flag.
+unset V
+# Also unset variables that will let `make -e install' divert
+# files into unwanted directories.
+unset DESTDIR
+unset prefix exec_prefix bindir datarootdir datadir docdir dvidir
+unset htmldir includedir infodir libdir libexecdir localedir mandir
+unset oldincludedir pdfdir psdir sbindir sharedstatedir sysconfdir
+# The tests call `make -e' but we do not want $srcdir from the environment
+# to override the definition from the Makefile.
+unset srcdir
+# Also unset variables that control our test driver.  While not
+# conceptually independent, they cause some changed semantics we
+# need to control (and test for) in some of the tests to ensure
+# backward-compatible behavior.
+unset DISABLE_HARD_ERRORS
+unset TESTS
+unset TEST_LOG_COMPILER
+unset TEST_LOGS
+unset RECHECK_LOGS
+unset VERBOSE
+
+
+## ---------------------------- ##
+##  Auxiliary shell functions.  ##
+## ---------------------------- ##
+
+# We use a trap below for cleanup.  This requires us to go through
+# hoops to get the right exit status transported through the signal.
+# So use `Exit STATUS' instead of `exit STATUS' inside of the tests.
+# Turn off errexit here so that we don't trip the bug with OSF1/Tru64
+# sh inside this function.
+Exit ()
+{
+  set +e
+  (exit $1)
+  exit $1
+}
+
+# is_newest FILE FILES
+# --------------------
+# Return false if any file in FILES is newer than FILE.
+# Resolve ties in favor of FILE.
+is_newest ()
+{
+  is_newest_files=`find "$@" -newer "$1"`
+  test -z "$is_newest_files"
+}
+
+# AUTOMAKE_run status [options...]
+# --------------------------------
+# Run Automake with OPTIONS, and fail if automake
+# does not exit with STATUS.
+AUTOMAKE_run ()
+{
+  expected_exitcode=$1
+  shift
+  exitcode=0
+  $AUTOMAKE ${1+"$@"} >stdout 2>stderr || exitcode=$?
+  cat stderr >&2
+  cat stdout
+  test $exitcode = $expected_exitcode || Exit 1
+}
+
+# AUTOMAKE_fails [options...]
+# ---------------------------
+# Run Automake with OPTIONS, and fail if automake
+# does not exit with STATUS.
+AUTOMAKE_fails ()
+{
+  AUTOMAKE_run 1 ${1+"$@"}
+}
+
+
+## ----------------------------------------------------------- ##
+##  Checks for required tools, and additional setups (if any)  ##
+##  required by them.                                          ##
+## ----------------------------------------------------------- ##
+
 echo "$PATH"
-# Some shells forget to export modified environment variables.
-# (See note about `export' in the Autoconf manual.)
-export PATH
 
 for tool in : $required
 do
@@ -238,99 +355,6 @@ do
   esac
 done
 
-# We use a trap below for cleanup.  This requires us to go through
-# hoops to get the right exit status transported through the signal.
-# So use `Exit STATUS' instead of `exit STATUS' inside of the tests.
-# Turn off errexit here so that we don't trip the bug with OSF1/Tru64
-# sh inside this function.
-Exit ()
-{
-  set +e
-  (exit $1)
-  exit $1
-}
-
-curdir=`pwd`
-testSubDir=$me.dir
-test ! -d $testSubDir || {
-  find $testSubDir -type d ! -perm -200 -exec chmod u+w {} ";"
-  rm -rf $testSubDir
-}
-mkdir $testSubDir
-
address@hidden@
-if test "$sh_errexit_works" = yes; then
-  trap 'exit_status=$?
-    set +e
-    cd "$curdir"
-    case $exit_status,$keep_testdirs in
-    0,)
-      find $testSubDir -type d ! -perm -200 -exec chmod u+w {} ";"
-      rm -rf $testSubDir
-      ;;
-    esac
-    test "$signal" != 0 &&
-      echo "$me: caught signal $signal"
-    echo "$me: exit $exit_status"
-    exit $exit_status
-  ' 0
-  for signal in 1 2 13 15; do
-    trap 'signal='$signal'; { Exit 1; }' $signal
-  done
-fi
-signal=0
-
-# Copy in some files we need.
-for file in install-sh missing depcomp; do
-   cp "$top_testsrcdir/lib/$file" "$testSubDir/$file" || Exit 1
-done
-
-cd ./$testSubDir
-
-# Build appropriate environment in test directory.  Eg create
-# configure.in, touch all necessary files, etc.
-# Don't use AC_OUTPUT, but AC_CONFIG_FILES so that appending
-# still produces a valid configure.ac.  But then, tests running
-# config.status really need to append AC_OUTPUT.
-{
-  echo "AC_INIT([$me], [1.0])"
-  if test x"$parallel_tests" = x"yes"; then
-    echo "AM_INIT_AUTOMAKE([parallel-tests])"
-  else
-    echo "AM_INIT_AUTOMAKE"
-  fi
-  echo "AC_CONFIG_FILES([Makefile])"
-} >configure.in
-
-# Unset some MAKE... variables that may cause $MAKE to act like a
-# recursively invoked sub-make.  Any $MAKE invocation in a test is
-# conceptually an independent invocation, not part of the main
-# 'automake' build.
-unset MFLAGS MAKEFLAGS AM_MAKEFLAGS MAKELEVEL __MKLVL__ MAKE_JOBS_FIFO
-# Unset verbosity flag.
-unset V
-# Also unset variables that will let `make -e install' divert
-# files into unwanted directories.
-unset DESTDIR
-unset prefix exec_prefix bindir datarootdir datadir docdir dvidir
-unset htmldir includedir infodir libdir libexecdir localedir mandir
-unset oldincludedir pdfdir psdir sbindir sharedstatedir sysconfdir
-# The tests call `make -e' but we do not want $srcdir from the environment
-# to override the definition from the Makefile.
-unset srcdir
-# Also unset variables that control our test driver.  While not
-# conceptually independent, they cause some changed semantics we
-# need to control (and test for) in some of the tests to ensure
-# backward-compatible behavior.
-unset DISABLE_HARD_ERRORS
-unset TESTS
-unset TEST_LOG_COMPILER
-unset TEST_LOGS
-unset RECHECK_LOGS
-unset VERBOSE
-
-echo "=== Running test $0"
-
 # We might need extra macros, e.g., from Libtool or Gettext.
 # Find them on the system.
 # Use `-I $top_testsrcdir/m4' in addition to `--acdir=$top_testsrcdir/m4',
@@ -374,56 +398,71 @@ case " $required " in
     ;;
 esac
 
-# POSIX no longer requires 'egrep' and 'fgrep',
-# but some hosts lack 'grep -E' and 'grep -F'.
-EGREP='@EGREP@'
-FGREP='@FGREP@'
 
-# The amount we should wait after modifying files depends on the platform.
-# For instance, Windows '95, '98 and ME have 2-second granularity
-# and can be up to 3 seconds in the future w.r.t. the system clock.
-sleep='sleep @MODIFICATION_DELAY@'
+## ---------------------------------------------------------------- ##
+##  Create and set up of the temporary directory used by the test.  ##
+##  Set up of the exit trap for cleanup of said directory.          ##
+## ---------------------------------------------------------------- ##
 
-# An old timestamp that can be given to a file, in "touch -t" format.
-# The time stamp should be portable to all file systems of interest.
-# Just for fun, choose the exact time of the announcement of the GNU project
-# in UTC; see <http://www.gnu.org/gnu/initial-announcement.html>.
-old_timestamp=198309271735.59
+# The subdirectory where the current test script will run and write its
+# temporary/data files.  This will be created shortly, and will be removed
+# by the cleanup trap below if the test passes.  If the test doesn't pass,
+# this directory will be kept, to facilitate debugging.
+testSubDir=$me.dir
 
-# is_newest FILE FILES
-# --------------------
-# Return false if any file in FILES is newer than FILE.
-# Resolve ties in favor of FILE.
-is_newest ()
-{
-  is_newest_files=`find "$@" -newer "$1"`
-  test -z "$is_newest_files"
+test ! -d $testSubDir || {
+  find $testSubDir -type d ! -perm -200 -exec chmod u+w {} ";"
+  rm -rf $testSubDir
 }
+mkdir $testSubDir
 
+cd ./$testSubDir
 
-# AUTOMAKE_run status [options...]
-# --------------------------------
-# Run Automake with OPTIONS, and fail if automake
-# does not exit with STATUS.
-AUTOMAKE_run ()
-{
-  expected_exitcode=$1
-  shift
-  exitcode=0
-  $AUTOMAKE ${1+"$@"} >stdout 2>stderr || exitcode=$?
-  cat stderr >&2
-  cat stdout
-  test $exitcode = $expected_exitcode || Exit 1
-}
address@hidden@
+if test "$sh_errexit_works" = yes; then
+  trap 'exit_status=$?
+    set +e
+    cd "$curdir"
+    case $exit_status,$keep_testdirs in
+    0,)
+      find $testSubDir -type d ! -perm -200 -exec chmod u+w {} ";"
+      rm -rf $testSubDir
+      ;;
+    esac
+    test "$signal" != 0 && echo "$me: caught signal $signal"
+    echo "$me: exit $exit_status"
+    exit $exit_status
+  ' 0
+  for signal in 1 2 13 15; do
+    trap 'signal='$signal'; { Exit 1; }' $signal
+  done
+  signal=0
+fi
 
-# AUTOMAKE_fails [options...]
-# ---------------------------
-# Run Automake with OPTIONS, and fail if automake
-# does not exit with STATUS.
-AUTOMAKE_fails ()
+# Copy in some files we need.
+for file in install-sh missing depcomp; do
+   cp "$top_testsrcdir/lib/$file" . || Exit 1
+done
+
+# Build appropriate environment in test directory.  Eg create
+# configure.in, touch all necessary files, etc.
+# Don't use AC_OUTPUT, but AC_CONFIG_FILES so that appending
+# still produces a valid configure.ac.  But then, tests running
+# config.status really need to append AC_OUTPUT.
 {
-  AUTOMAKE_run 1 ${1+"$@"}
-}
+  echo "AC_INIT([$me], [1.0])"
+  if test x"$parallel_tests" = x"yes"; then
+    echo "AM_INIT_AUTOMAKE([parallel-tests])"
+  else
+    echo "AM_INIT_AUTOMAKE"
+  fi
+  echo "AC_CONFIG_FILES([Makefile])"
+} >configure.in
+
+
+## ---------------- ##
+##  Ready to go...  ##
+## ---------------- ##
 
 # Turn on shell traces.
 set -x
-- 
1.7.1

From 498bb93fe6fcd255959990c411a438a2d0cf1fa3 Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Wed, 10 Nov 2010 23:46:57 +0100
Subject: [PATCH 2/4] Tests defs: improve and extends comments.

* tests/defs.in: Improve and extends some comments, especially in
relation with the changes introduced by the previous reordering.
---
 ChangeLog     |    4 ++++
 tests/defs.in |    9 ++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 8fd8bad..5761e9f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2010-11-10  Stefano Lattarini  <address@hidden>
 
+       Tests defs: improve and extends comments.
+       * tests/defs.in: Improve and extends some comments, especially in
+       relation with the changes introduced by the previous reordering.
+
        Tests defs: various reorderings.
        * tests/defs.in: Reordered various snippets of code in a
        clearer way.
diff --git a/tests/defs.in b/tests/defs.in
index 57a9cde..022b2a5 100644
--- a/tests/defs.in
+++ b/tests/defs.in
@@ -51,6 +51,7 @@ fi
 
 echo "=== Running test $0"
 
+# The name of the current test (without the `.test' suffix.)
 me=`echo "$0" | sed -e 's,.*[\\/],,;s/\.test$//'`
 
 
@@ -207,8 +208,12 @@ AUTOMAKE_fails ()
 ##  required by them.                                          ##
 ## ----------------------------------------------------------- ##
 
+# Print it here, so that the user will see it also if the test
+# will be skipped due to some tool missing in $PATH itslef.
 echo "$PATH"
 
+# Look for (and maybe set up) required tools and/or system features; skip
+# the current test if they are not found.
 for tool in : $required
 do
   # Check that each required tool is present.
@@ -390,7 +395,9 @@ case " $required " in
     esac
     # Libtool cannot cope with spaces in the build tree.  Our testsuite setup
     # cannot cope with spaces in the source tree name for Libtool and gettext
-    # tests.
+    # tests.  Using just "`pwd`" for the check here is ok, since the further
+    # temporary subdirectory where the test will be run is ensured not to
+    # contain any space.
     case $testsrcdir,`pwd` in
       *\ * | *\        *) Exit 77;;
     esac
-- 
1.7.1

From 733cd3d8445f65e25ec8bdb507fa6dda2dbcb985 Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Wed, 10 Nov 2010 23:49:32 +0100
Subject: [PATCH 3/4] Tests defs: do not use `Exit' where plain `exit' suffices.

* tests/defs.in: Use "exit 77" rather than "Exit 77" to skip the
test when required libtool/gettext macros are not found, since
such skips would take place before the exit trap is installed.
---
 ChangeLog     |    5 +++++
 tests/defs.in |    6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5761e9f..edd2406 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2010-11-10  Stefano Lattarini  <address@hidden>
 
+       Tests defs: do not use `Exit' where plain `exit' suffices.
+       * tests/defs.in: Use "exit 77" rather than "Exit 77" to skip the
+       test when required libtool/gettext macros are not found, since
+       such skips would take place before the exit trap is installed.
+
        Tests defs: improve and extends comments.
        * tests/defs.in: Improve and extends some comments, especially in
        relation with the changes introduced by the previous reordering.
diff --git a/tests/defs.in b/tests/defs.in
index 022b2a5..dd42e0f 100644
--- a/tests/defs.in
+++ b/tests/defs.in
@@ -390,8 +390,8 @@ case " $required " in
       fi
     done
     case " $required " in
-      *' libtool '* | *' libtoolize '* ) test $libtool_found = yes || Exit 77;;
-      *' gettext '* ) test $gettext_found = yes || Exit 77;;
+      *' libtool '* | *' libtoolize '* ) test $libtool_found = yes || exit 77;;
+      *' gettext '* ) test $gettext_found = yes || exit 77;;
     esac
     # Libtool cannot cope with spaces in the build tree.  Our testsuite setup
     # cannot cope with spaces in the source tree name for Libtool and gettext
@@ -399,7 +399,7 @@ case " $required " in
     # temporary subdirectory where the test will be run is ensured not to
     # contain any space.
     case $testsrcdir,`pwd` in
-      *\ * | *\        *) Exit 77;;
+      *\ * | *\        *) exit 77;;
     esac
     ACLOCAL="$ACLOCAL -Wno-syntax -I $top_testsrcdir/m4 $extra_includes -I 
$aclocaldir"
     ;;
-- 
1.7.1

From b06239692cb5808af5834b71a3acf45a1c763888 Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Wed, 10 Nov 2010 23:53:28 +0100
Subject: [PATCH 4/4] Tests defs: use `$me' in more error messages.

* tests/defs.in: Also use `$me' in error messages referring to
missing `defs' or `defs.in', since that variable is now defined
before those checks.
---
 ChangeLog     |    5 +++++
 tests/defs.in |    4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index edd2406..6c17916 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2010-11-10  Stefano Lattarini  <address@hidden>
 
+       Tests defs: use `$me' in more error messages.
+       * tests/defs.in: Also use `$me' in error messages referring to
+       missing `defs' or `defs.in', since that variable is now defined
+       before those checks.
+
        Tests defs: do not use `Exit' where plain `exit' suffices.
        * tests/defs.in: Use "exit 77" rather than "Exit 77" to skip the
        test when required libtool/gettext macros are not found, since
diff --git a/tests/defs.in b/tests/defs.in
index dd42e0f..9ce76c4 100644
--- a/tests/defs.in
+++ b/tests/defs.in
@@ -114,13 +114,13 @@ PATH="`pwd`$PATH_SEPARATOR$PATH"; export PATH
 
 # Ensure we are running from the right directory.
 test -f ./defs || {
-   echo "./defs: not found in current directory" >&2
+   echo "$me: ./defs: not found in current directory" >&2
    exit 1
 }
 
 # Ensure $testsrcdir is set correctly.
 test -f "$testsrcdir/defs.in" || {
-   echo "$testsrcdir/defs.in not found, check \$testsrcdir" >&2
+   echo "$me: $testsrcdir/defs.in not found, check \$testsrcdir" >&2
    exit 1
 }
 
-- 
1.7.1