Re: [PATCH 08/10] Add more tests about AUTOMAKE_OPTIONS.

[Stefano Lattarini]

On Sunday 02 January 2011, Ralf Wildenhues wrote:
> * address@hidden wrote on Thu, Dec 23, 2010 at 12:27:44PM CET:
> > In view of soon-to-follow refactorings (still in the pursuit of a
> > fix for Automake bug#7669 a.k.a. PR/547), we add some more tests
> 
> How about s/we //
> 
> > on AUTOMAKE_OPTIONS support, to prevent obvious regressions.
> > 
> > * tests/amopts-indirect.test: New test.
> > * tests/amopts-location.test: Likewise.
> > * tests/Makefile.am (TESTS): Update.
> 
> > --- /dev/null
> > +++ b/tests/amopts-indirect.test
> 
> > +# Check that AUTOMAKE_OPTIONS support indirections.
> 
> s/indirections/variable expansion/  and adjust test name?
>
Fine with me.  Done.

> > +cat > Makefile.am <<'END'
> > +AUTOMAKE_OPTIONS = $(foo) foreign
> > +AUTOMAKE_OPTIONS += ${bar}
> > +foo = $(foo1)
> > +foo1 = ${foo2}
> > +foo2 = -Wnone
> > +foo2 += $(foo3)
> > +foo3 = -Wno-error
> > +bar = -Wportability
> 
> This seems a wee bit convoluted (i.e., hard to parse by human upon first
> reading), but oh well, that's more of a testsuite QoI nit.
>
Do you have any clarifying comment to suggest?  I'd be happy to add it.

> > --- /dev/null
> > +++ b/tests/amopts-location.test
> > @@ -0,0 +1,80 @@
>
> [CUT]
>
> > +
> > +$ACLOCAL
> > +AUTOMAKE_fails
> 
> What are the expected failures here?
>
Aren't the next greps explicit enough?  If not, would this comment help
in clarifying the situation?

 # Automake options 'tar-v7', 'tar-ustar' and 'tar-pax' can only be used
 # as argument to AM_INIT_AUTOMAKE, and not in AUTOMAKE_OPTIONS.

In the meantime, I've added it just before the call to AUTOMAKE_fails;
let me know if it's not clear enough.

> (Not sure if they should be mentioned in the test source, but
> I sure am curious about what automake prints.)
>
Here it is:

  Makefile3.am:2: error: option `tar-v7' can only be used as argument to 
AM_INIT_AUTOMAKE
  Makefile3.am:2: but not in AUTOMAKE_OPTIONS makefile statements
  Makefile2.am:6: error: option `tar-ustar' can only be used as argument to 
AM_INIT_AUTOMAKE
  Makefile2.am:6: but not in AUTOMAKE_OPTIONS makefile statements
  Makefile1.am:1: error: option `tar-pax' can only be used as argument to 
AM_INIT_AUTOMAKE
  Makefile1.am:1: but not in AUTOMAKE_OPTIONS makefile statements
  Makefile.am:3:   `Makefile0.am' included from here
  Makefile0.am:4:   `Makefile1.am' included from here

> Just to be sure, this patch does not rely upon any of the previous ones,
> right?
>
Hmm... I didn't pay attention to this fact before, but yes, I believe
the patch should not depend on earlier ones.

Anyway, I bacame aware the necessity of the new tests only when I got to
modify the automake code dealing with AUTOMAKE_OPTIONS; and that's why
I didn't add them in an earlier patch.

> > +grep '^Makefile1\.am:1:.*tar-pax' stderr
> > +grep '^Makefile2\.am:6:.*tar-ustar' stderr
> > +grep '^Makefile3\.am:2:.*tar-v7' stderr
> > +grep '^Makefile\.am:3:.*Makefile0\.am.*included from here' stderr
> > +grep '^Makefile0\.am:4:.*Makefile1\.am.*included from here' stderr
> 
> > +cat stderr \
> > +  | grep -v '^Makefile\.am:3:'  \
> > +  | grep -v '^Makefile0\.am:4:' \
> > +  | grep -v '^Makefile1\.am:1:' \
> > +  | grep -v '^Makefile2\.am:6:' \
> > +  | grep -v '^Makefile3\.am:2:' \
> > +  | grep . && Exit 1
> 
> What is this for, to ensure there are not more warnings?
>
In truth, it's to ensure that there are no warnings referring to botched
line numbers.  Technically, the grep above might be a little too strict
in this respect, but since our Makefile.ams are so simple, that shouldn't
be a problem.

JTBS, I've added a couple of new comments about this too.

> How about just testing for 5 lines of output?
>
They are in fact eight, which shows ...

> Even that is a bit fragile though, as we might have multi-line warnings.
>
... that you're right here.  I'd personally leave the above grepping
alone.  Objections?

Thanks,
   Stefano

From 229aa74326b835dda56889ebbbe916485d2a0e1b Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Sun, 2 Jan 2011 18:07:20 +0100
Subject: [PATCH] Squashed in

---
 ChangeLog                                          |    5 +++--
 tests/Makefile.am                                  |    2 +-
 tests/Makefile.in                                  |    2 +-
 tests/amopts-location.test                         |    7 ++++++-
 ...ndirect.test => amopts-variable-expansion.test} |    4 ++--
 5 files changed, 13 insertions(+), 7 deletions(-)
 rename tests/{amopts-indirect.test => amopts-variable-expansion.test} (92%)

diff --git a/ChangeLog b/ChangeLog
index f3b411a..af01192 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,10 +1,11 @@
-2010-12-20  Stefano Lattarini  <address@hidden>
+2011-01-02  Stefano Lattarini  <address@hidden>
 
+       For PR automake/547:
        Add more tests about AUTOMAKE_OPTIONS.
        In view of soon-to-follow refactorings (still in the pursuit of a
        fix for Automake bug#7669 a.k.a. PR/547), we add some more tests
        on AUTOMAKE_OPTIONS support, to prevent obvious regressions.
-       * tests/amopts-indirect.test: New test.
+       * tests/amopts-variable-expansion.test: New test.
        * tests/amopts-location.test: Likewise.
        * tests/Makefile.am (TESTS): Update.
 
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9ecf70d..533003a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -134,8 +134,8 @@ alpha2.test \
 amassign.test \
 ammissing.test \
 amopt.test \
-amopts-indirect.test \
 amopts-location.test \
+amopts-variable-expansion.test \
 amsubst.test \
 ansi.test \
 ansi2.test \
diff --git a/tests/Makefile.in b/tests/Makefile.in
index 478ff6a..1f28202 100644
--- a/tests/Makefile.in
+++ b/tests/Makefile.in
@@ -397,8 +397,8 @@ alpha2.test \
 amassign.test \
 ammissing.test \
 amopt.test \
-amopts-indirect.test \
 amopts-location.test \
+amopts-variable-expansion.test \
 amsubst.test \
 ansi.test \
 ansi2.test \
diff --git a/tests/amopts-location.test b/tests/amopts-location.test
index 7c3fbc6..c6d06e9 100755
--- a/tests/amopts-location.test
+++ b/tests/amopts-location.test
@@ -1,5 +1,5 @@
 #! /bin/sh
-# Copyright (C) 2010 Free Software Foundation, Inc.
+# Copyright (C) 2011 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
@@ -61,14 +61,19 @@ AC_CONFIG_FILES([Makefile2 Makefile3])
 END
 
 $ACLOCAL
+# Automake options 'tar-v7', 'tar-ustar' and 'tar-pax' can only be used
+# as argument to AM_INIT_AUTOMAKE, and not in AUTOMAKE_OPTIONS.
 AUTOMAKE_fails
 
+# Check that all the expected line numbers are correctly reported
+# in automake warning/error messages.
 grep '^Makefile1\.am:1:.*tar-pax' stderr
 grep '^Makefile2\.am:6:.*tar-ustar' stderr
 grep '^Makefile3\.am:2:.*tar-v7' stderr
 grep '^Makefile\.am:3:.*Makefile0\.am.*included from here' stderr
 grep '^Makefile0\.am:4:.*Makefile1\.am.*included from here' stderr
 
+# And also check that no botched line number is reported.
 cat stderr \
   | grep -v '^Makefile\.am:3:'  \
   | grep -v '^Makefile0\.am:4:' \
diff --git a/tests/amopts-indirect.test b/tests/amopts-variable-expansion.test
similarity index 92%
rename from tests/amopts-indirect.test
rename to tests/amopts-variable-expansion.test
index dccc888..508ff89 100755
--- a/tests/amopts-indirect.test
+++ b/tests/amopts-variable-expansion.test
@@ -1,5 +1,5 @@
 #! /bin/sh
-# Copyright (C) 2010 Free Software Foundation, Inc.
+# Copyright (C) 2011 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
@@ -14,7 +14,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-# Check that AUTOMAKE_OPTIONS support indirections.
+# Check that AUTOMAKE_OPTIONS support variable expansion.
 
 . ./defs || Exit 1
 
-- 
1.7.2.3