Re: [PATCH 2/6] Testsuite: fix missing*.test for non-default autotools.

[Stefano Lattarini]

On Saturday 21 August 2010, Ralf Wildenhues wrote:
> Hi Stefano,
> 
> * Stefano Lattarini wrote on Thu, Aug 19, 2010 at 02:57:04PM CEST:
> > * tests/missing.test: Build and use our own `autoconf' script, to
> > avoid spurious failures due to configure-time value of $AUTOCONF
> > being an absolute path.  Prefer passing overrides to ./configure
> > on the command line rather than in the environment.  Some other
> > related and unrelated improvements.
> > * tests/missing2.test: Likewise, and keep more in sync with
> > `missing.test' by adding checks with version number suffixes.
> > 
> > --- a/tests/missing.test
> > +++ b/tests/missing.test
> > 
> > @@ -16,6 +16,7 @@
> > 
> >  # along with this program.  If not, see
> >  <http://www.gnu.org/licenses/>.
> >  
> >  # Test missing with version mismatches.
> > 
> > +# Keep this in sync with sister test `missing.test'.
> 
> Referring to self.
Oops, should be `missing2.test'.  Fixed.

> > @@ -31,36 +32,48 @@ $ACLOCAL
> > 
> >  $AUTOCONF
> >  $AUTOMAKE --add-missing
> > 
> > +# Avoid problems with configure-time $AUTOCONF that is absolute
> > values.
> 
> "Avoid problems when $AUTOCONF has an absolute value at configure
> time."
Better (my formulation also had the typo `values' for `value')
 
> > +# Such problems already occurred in practice.
> 
> I'm wary of comments that go out of date.  Is this such a comment?
> Do we have a bug report to this end?  Are you the reporter?
Well, I experinced the problem, but didn't report it (I fixed it
instead, with this patch).

> What is the actual problem by the way?
Basically, `missing --run /abs/path/to/autoconf' does not recognize
the program being run as an "autoconf":

  $ sed -n '147,166p' lib/missing 
  # If it does not exist, or fails to run (possibly an outdated version),
  # try to emulate it.
  case $program in
    aclocal*)
      echo 1>&2 "\
  WARNING: \`$1' is $msg.  You should only need it if
           you modified \`acinclude.m4' or \`${configure_ac}'.  You might want
           to install the \`Automake' and \`Perl' packages.  Grab them from
           any GNU archive site."
      touch aclocal.m4
      ;;
    autoconf*)
      echo 1>&2 "\
  WARNING: \`$1' is $msg.  You should only need it if
           you modified \`${configure_ac}'.  You might want to install the
           \`Autoconf' and \`GNU m4' packages.  Grab them from any GNU
           archive site."
      touch configure
      ;;

> This should be answered in the comment.
Right.  I've went for this formulation:
  
  # Avoid problems when $AUTOCONF has an absolute value at configure time.
  # Such problems already occurred in practice.
  # They are due the way the `missing' script matches the program it has
  # to wrap:
  #   case $program in 
  #     aclocal*) ...;;
  #     autconf*) ...;;
  #     ...
  #   esac
  # This does not account for absolute paths.  Which makes perfect sense,
  # given the way missing is used in practie, but would break a hack like:
  #  ./configure AUTOCONF="./missing --run /bas/path/to/autoconf"

> > +mkdir xbin
> 
> Why not  s/xbin/bin/g  throughout?  That seems to be more
> conventional.
OK (`xbin' was for "eXtra BINaries" or `eXtended BINaries",
but a simple `bin' is just fine).
 
> > +cat > xbin/autoconf <<END
> > +#!/bin/sh
> > +exec $AUTOCONF \${1+"\$@"}
> > +END
> > +chmod a+x xbin/autoconf
> > +cat xbin/autoconf # for debugging
> > +cp xbin/autoconf xbin/autoconf6789
> > +
> > +PATH=`pwd`/xbin:$PATH; export PATH
> 
> Oh, we keep forgetting PATH_SEPARATOR in our tests.  Could (and
> should) be addressed in an independent followup patch and probably
> checked for by maintainer-check.
Yes, I'm planning to to that, sooner or later.  I forgot to add a
proper "FIXME" comment here, though.  Now added.

> Do you think it is such a good idea to have a script named
> 'autoconf' early in $PATH that itself calls $AUTOCONF, when
> $AUTOCONF could expand to 'autoconf'?
No, now that I think about it, it's horrible.  I should really
save the older $PATH, and restore it in the fake autoconf before
calling $AUTOCONF.  Done in the amended patch.
> Looks like a potential fork bomb to me;
Luckily, since I use `exec' in the fake autoconf, it would be just
and endless loop, hanging the testsuite.  But is bad nonetheless.
> I mean, in the case there is a bug and something calls
> 'autoconf' when it should have called $MYAUTOCONF below ...

> At this point, I'm stopping review of this patch series for the
> moment. Not because I think it is bad, just to await feedback from
> you for the remark I gave for 4/6 (dunno if it will let you make
> bigger changes), and guessing that you might have more than just
> above place to fix for potential fork bombs.  I'll continue (or
> review an updated series) after feedback.
Good idea.  I will post amended patches where it seems necessary.

> Cheers, and thanks for working on this!
> Ralf
Thanks for the review,
  Stefano

From 72667faed237987607030d688cfead7b8b7d7a6d Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Wed, 18 Aug 2010 19:38:53 +0200
Subject: [PATCH] Testsuite: fix missing*.test for non-default autotools.

* tests/missing.test: Build and use our own `autoconf' script, to
avoid spurious failures due to configure-time value of $AUTOCONF
being an absolute path.  Prefer passing overrides to ./configure
on the command line rather than in the environment.  Some other
related and unrelated improvements.
* tests/missing2.test: Likewise, and keep more in sync with
`missing.test' by adding checks with version number suffixes.
---
 ChangeLog           |    9 ++++++++
 tests/missing.test  |   58 ++++++++++++++++++++++++++++++++++++--------------
 tests/missing2.test |   55 ++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 97 insertions(+), 25 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 92f2063..7f817e0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
 2010-08-19  Stefano Lattarini  <address@hidden>
 
+       Testsuite: fix missing*.test for non-default autotools.
+       * tests/missing.test: Build and use our own `autoconf' script, to
+       avoid spurious failures due to configure-time value of $AUTOCONF
+       being an absolute path.  Prefer passing overrides to ./configure
+       on the command line rather than in the environment.  Some other
+       related and unrelated improvements.
+       * tests/missing2.test: Likewise, and keep more in sync with
+       `missing.test' by adding checks with version number suffixes.
+
        Test that autoconf(s) used by the testsuite and by aclocal match.
        * tests/remake0.test: New test.
        * tests/Makefile.am (TESTS): Updated.
diff --git a/tests/missing.test b/tests/missing.test
index eaf6f54..861d7bf 100755
--- a/tests/missing.test
+++ b/tests/missing.test
@@ -1,6 +1,6 @@
 #! /bin/sh
-# Copyright (C) 2003, 2004, 2006, 2007, 2008  Free Software Foundation,
-# Inc.
+# Copyright (C) 2003, 2004, 2006, 2007, 2008, 2010 Free Software
+# Foundation, Inc.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -16,6 +16,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 # Test missing with version mismatches.
+# Keep this in sync with sister test `missing2.test'.
 
 . ./defs || Exit 1
 
@@ -31,36 +32,61 @@ $ACLOCAL
 $AUTOCONF
 $AUTOMAKE --add-missing
 
+# Avoid problems when $AUTOCONF has an absolute value at configure time.
+# Such problems already occurred in practice.
+# They are due the way the `missing' script matches the program it has
+# to wrap:
+#   case $program in 
+#     aclocal*) ...;;
+#     autconf*) ...;;
+#     ...
+#   esac
+# This does not account for absolute paths.  Which makes perfect sense,
+# given the way missing is used in practice, but would break a hack like:
+#  ./configure AUTOCONF="./missing --run /bas/path/to/autoconf"
+mkdir bin
+cat > bin/autoconf <<END
+#!/bin/sh
+# Restoring PATH avoids "fork bombs" in case \$AUTOCONF is "autoconf".
+PATH='$PATH'; export PATH;
+exec $AUTOCONF \${1+"\$@"}
+END
+chmod a+x bin/autoconf
+cat bin/autoconf # for debugging
+cp bin/autoconf bin/autoconf6789
+
+# FIXME: this is not portable! Use @address@hidden
+PATH=`pwd`/bin:$PATH; export PATH
+
 # Make sure we do use missing, even if the user exported AUTOCONF.
 # (We cannot export this new value, because it would be used by Automake
 # when tracing, and missing is no good for this.)
-MYAUTOCONF="./missing --run $AUTOCONF"
 unset AUTOCONF
+MYAUTOCONF="./missing --run autoconf"
 
 ./configure AUTOCONF="$MYAUTOCONF"
+
 $MAKE
 $sleep
 # Hopefully the install version of Autoconf cannot compete with this one...
-echo 'AC_PREREQ(9999)' >> aclocal.m4
+echo 'AC_PREREQ([9999])' >> aclocal.m4
 $MAKE distdir
 
-# Try version number suffixes if we can add them safely.
-case $MYAUTOCONF in *autoconf)
-  ./configure AUTOCONF="${MYAUTOCONF}6789"
-  $MAKE
-  $sleep
-  # Hopefully the install version of Autoconf cannot compete with this one...
-  echo 'AC_PREREQ(9999)' >> aclocal.m4
-  $MAKE distdir
-esac
+# Try version number suffixes.
+./configure AUTOCONF="${MYAUTOCONF}6789"
+$MAKE
+$sleep
+touch aclocal.m4
+$MAKE distdir
 
-# Run again, but without missing, to ensure that timestamps were updated.
-export AUTOMAKE ACLOCAL
-./configure AUTOCONF="$MYAUTOCONF"
+# Run again, but without missing to wrap aclocal/automake, to ensure that
+# timestamps were updated.
+./configure AUTOCONF="$MYAUTOCONF" AUTOMAKE="$AUTOMAKE" ACLOCAL="$ACLOCAL"
 $MAKE
 
 # Make sure $MAKE fails when timestamps aren't updated and missing is not used.
 $sleep
 touch aclocal.m4
 $MAKE && Exit 1
+
 :
diff --git a/tests/missing2.test b/tests/missing2.test
index 2629198..9ffd1f2 100755
--- a/tests/missing2.test
+++ b/tests/missing2.test
@@ -1,5 +1,6 @@
 #! /bin/sh
-# Copyright (C) 2003, 2004, 2006, 2007  Free Software Foundation, Inc.
+# Copyright (C) 2003, 2004, 2006, 2007, 2010 Free Software Foundation,
+# Inc.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -15,6 +16,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 # Test missing with version mismatches.
+# Keep this in sync with sister test `missing.test'.
 
 . ./defs || Exit 1
 
@@ -26,32 +28,67 @@ AC_OUTPUT
 EOF
 
 : > v.m4
-
 : > Makefile.am
 
 $ACLOCAL
 $AUTOCONF
 $AUTOMAKE --add-missing
 
-# See missing.test for explanations about this.
-MYAUTOCONF="./missing --run $AUTOCONF"
+# Avoid problems when $AUTOCONF has an absolute value at configure time.
+# Such problems already occurred in practice.
+# They are due the way the `missing' script matches the program it has
+# to wrap:
+#   case $program in 
+#     aclocal*) ...;;
+#     autconf*) ...;;
+#     ...
+#   esac
+# This does not account for absolute paths.  Which makes perfect sense,
+# given the way missing is used in practice, but would break a hack like:
+#  ./configure AUTOCONF="./missing --run /bas/path/to/autoconf"
+mkdir bin
+cat > bin/autoconf <<END
+#!/bin/sh
+# Restoring PATH avoids "fork bombs" in case \$AUTOCONF is "autoconf".
+PATH='$PATH'; export PATH;
+exec $AUTOCONF \${1+"\$@"}
+END
+chmod a+x bin/autoconf
+cat bin/autoconf # for debugging
+cp bin/autoconf bin/autoconf6789
+
+# FIXME: this is not portable! Use @address@hidden
+PATH=`pwd`/bin:$PATH; export PATH
+
+# Make sure we do use missing, even if the user exported AUTOCONF.
+# (We cannot export this new value, because it would be used by Automake
+# when tracing, and missing is no good for this.)
 unset AUTOCONF
+MYAUTOCONF="./missing --run autoconf"
 
 ./configure AUTOCONF="$MYAUTOCONF"
 
 $MAKE
 $sleep
 # Hopefully the install version of Autoconf cannot compete with this one...
-echo 'AC_PREREQ(9999)' > v.m4
+echo 'AC_PREREQ([9999])' > v.m4
 $MAKE distdir
 
-# Run again, but without missing, to ensure that timestamps were updated.
-export AUTOMAKE ACLOCAL
-./configure AUTOCONF="$MYAUTOCONF"
+# Try version number suffixes.
+./configure AUTOCONF="${MYAUTOCONF}6789"
+$MAKE
+$sleep
+touch v.m4
+$MAKE distdir
+
+# Run again, but without missing to wrap aclocal/automake, to ensure that
+# timestamps were updated.
+./configure AUTOCONF="$MYAUTOCONF" AUTOMAKE="$AUTOMAKE" ACLOCAL="$ACLOCAL"
 $MAKE
 
-# Make sure $MAKE fail when timestamps aren't updated and missing is not used.
+# Make sure $MAKE fails when timestamps aren't updated and missing is not used.
 $sleep
 touch v.m4
 $MAKE && Exit 1
+
 :
-- 
1.7.1