Re: [PATCH] yacc: warn about conditional content in *YFLAGS variables

[Stefano Lattarini]

On Monday 10 January 2011, Stefano Lattarini wrote:
> On Monday 10 January 2011, Ralf Wildenhues wrote:
> > [ dropping bug-automake ]
> > 
> > * Stefano Lattarini wrote on Mon, Jan 10, 2011 at 03:59:10PM CET:
> > > The attached patch should fix the bug.  OK for the temporary branch
> > > 'yacc-work', to be merged to master afterwards?
> > 
> > OK, thanks.  Please add a short NEWS blurb (or merge with other NEWS
> > about yacc changes).
> >
> Oops, sorry!  I don't know I could forget to do so right away ... :-(
> 
> > How about ensuring in the test that the user can override
> > automake by setting warning flags suitably (preapproved)?
> >
> Good idea (I'll assume that he won't try to make `-d' conditional, as
> the behaviour should remain undefined in this case IMHO).  I'll amend
> the patch with a new testcase (tonight or tomorrow).
> 
Attached is what I pushed.  This should fix automake bug#7804.

Regards,
  Stefano

From 834dc3a98272e7eb1869e2a1972ac14ea45e0ec0 Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Mon, 10 Jan 2011 15:50:35 +0100
Subject: [PATCH] yacc: warn about conditional content in *YFLAGS variables

This commit fixes automake bug#7804.

* automake.in (lang_yacc_target_hook): Warn if any of the relevant
*YFLAGS variables has conditional contents (not only a conditional
definition).  Related refactoring.
* NEWS: Updated.
* tests/yflags-conditional.test: Updated and extended.
* tests/yflags-conditional-force.test: New test.
* tests/Makefile.am (TESTS): Updated.
---
 ChangeLog                           |   12 ++++
 NEWS                                |    3 +
 automake.in                         |   34 +++++++---
 tests/Makefile.am                   |    1 +
 tests/Makefile.in                   |    1 +
 tests/yflags-conditional.test       |  117 ++++++++++++++++++++++++++++++++---
 tests/yflags-force-conditional.test |   95 ++++++++++++++++++++++++++++
 7 files changed, 243 insertions(+), 20 deletions(-)
 create mode 100755 tests/yflags-force-conditional.test

diff --git a/ChangeLog b/ChangeLog
index 06f9b84..90eb69e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2011-01-10   Stefano Lattarini  <address@hidden>
+
+       yacc: warn about conditional content in *YFLAGS variables
+       This change fixes automake bug#7804.
+       * automake.in (lang_yacc_target_hook): Warn if any of the relevant
+       *YFLAGS variables has conditional contents (not only a conditional
+       definition).  Related refactoring.
+       * NEWS: Updated.
+       * tests/yflags-conditional.test: Updated and extended.
+       * tests/yflags-conditional-force.test: New test.
+       * tests/Makefile.am (TESTS): Updated.
+
 2011-01-08   Stefano Lattarini  <address@hidden>
 
        yacc: support variable expansions in *YFLAGS definition.
diff --git a/NEWS b/NEWS
index 79860ad..a947af9 100644
--- a/NEWS
+++ b/NEWS
@@ -62,6 +62,9 @@ Bugs fixed in 1.11.0a:
     through other variables, such as in:
       foo_opts = -d
       AM_YFLAGS = $(foo_opts)
+
+  - Automake now complains if a `*YFLAGS' variable has any conditional
+    content, not only a conditional definition.
 
 New in 1.11:
 
diff --git a/automake.in b/automake.in
index 2bffe48..fa458d6 100755
--- a/automake.in
+++ b/automake.in
@@ -6061,15 +6061,29 @@ sub lang_yacc_target_hook
 {
     my ($self, $aggregate, $output, $input, %transform) = @_;
 
-    my $flagvar = var ($aggregate . "_YFLAGS");
-    my $YFLAGSvar = var ('YFLAGS');
-    # We cannot work reliably with conditionally-defined YFLAGS.
-    $flagvar->check_defined_unconditionally if $flagvar;
-    $YFLAGSvar->check_defined_unconditionally if $YFLAGSvar;
-    my @flags = $flagvar ? $flagvar->value_as_list_recursive : ();
-    my @YFLAGS = $YFLAGSvar ? $YFLAGSvar->value_as_list_recursive : ();
-    if (grep (/^-d$/, @flags) || grep (/^-d$/, @YFLAGS))
-    {
+    # If some relevant *YFLAGS variable contains the `-d' flag, we'll
+    # have to to generate special code.
+    my $yflags_contains_minus_d = 0;
+
+    foreach my $pfx ("", "${aggregate}_")
+      {
+       my $yflagsvar = var ("${pfx}YFLAGS");
+       next unless $yflagsvar;
+       # We cannot work reliably with conditionally-defined YFLAGS.
+       if ($yflagsvar->has_conditional_contents)
+         {
+           msg_var ('unsupported', $yflagsvar,
+                    "`${pfx}YFLAGS' cannot have conditional contents");
+         }
+       else
+         {
+           $yflags_contains_minus_d = 1
+             if grep (/^-d$/, $yflagsvar->value_as_list_recursive);
+         }
+      }
+
+    if ($yflags_contains_minus_d)
+      {
        (my $output_base = $output) =~ s/$KNOWN_EXTENSIONS_PATTERN$//;
        my $header = $output_base . '.h';
 
@@ -6098,7 +6112,7 @@ sub lang_yacc_target_hook
        # then we want to remove them with "make clean"; otherwise,
        # "make distcheck" will fail.
        $clean_files{$header} = $transform{'DIST_SOURCE'} ? MAINTAINER_CLEAN : 
CLEAN;
-    }
+      }
     # See the comment above for $HEADER.
     $clean_files{$output} = $transform{'DIST_SOURCE'} ? MAINTAINER_CLEAN : 
CLEAN;
 }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index bb1d786..39f1a39 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -817,6 +817,7 @@ yflags-cmdline-override.test \
 yflags-conditional.test \
 yflags-d-false-positives.test \
 yflags-force-override.test \
+yflags-force-conditional.test \
 yflags-var-expand.test \
 $(parallel_tests)
 
diff --git a/tests/Makefile.in b/tests/Makefile.in
index e83cf33..0ea9825 100644
--- a/tests/Makefile.in
+++ b/tests/Makefile.in
@@ -1084,6 +1084,7 @@ yflags-cmdline-override.test \
 yflags-conditional.test \
 yflags-d-false-positives.test \
 yflags-force-override.test \
+yflags-force-conditional.test \
 yflags-var-expand.test \
 $(parallel_tests)
 
diff --git a/tests/yflags-conditional.test b/tests/yflags-conditional.test
index 8c673b1..91e3da4 100755
--- a/tests/yflags-conditional.test
+++ b/tests/yflags-conditional.test
@@ -14,7 +14,8 @@
 # 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 complains about conditionally-defined *_YFLAGS.
+# Check that automake complains about *_YFLAGS variables which have
+# conditional content.
 
 . ./defs || Exit 1
 
@@ -22,25 +23,121 @@ set -e
 
 cat >> configure.in <<'END'
 AC_PROG_CC
-AC_PROG_YACC
+
+# `YFLAGS' is AC_SUBST'd by AC_PROG_YACC by default, but we
+# don't want this, since it might confuse our error messages.
+# Also, AM_SUBST_NOTMAKE seems not to help about this.
+# So we simply define $(YACC) by hand.
+AC_SUBST([YACC], [yacc])
+
 AM_CONDITIONAL([COND], [:])
 END
 
+$ACLOCAL
+
 cat > Makefile.am <<'END'
-bin_PROGRAMS = foo bar
+bin_PROGRAMS = foo zardoz
 foo_SOURCES = foo.y
-bar_SOURCES = bar.y
+zardoz_SOURCES = zardoz.y
 if COND
-AM_YFLAGS = $(YFLAGS)
-bar_YFLAGS = -v
+AM_YFLAGS = -v
+zardoz_YFLAGS = -v
 endif COND
 END
 
+cat > Makefile1.am <<'END'
+bin_PROGRAMS = foo
+foo_SOURCES = foo.y
+## dummy comment to keep line count right
+if COND
+YFLAGS = foo
+endif COND
+END
+
+cat > Makefile2.am <<'END'
+bin_PROGRAMS = foo
+foo_SOURCES = foo.y
+AM_YFLAGS = am_yflags
+if COND
+YFLAGS = yflags
+endif COND
+END
+
+cat > Makefile3.am <<'END'
+bin_PROGRAMS = foo
+foo_SOURCES = foo.y
+foo_YFLAGS = foo_yflags
+if COND
+YFLAGS = yflags
+endif COND
+END
+
+cat > Makefile4.am <<'END'
+bin_PROGRAMS = foo zardoz
+
+foo_SOURCES = foo.y
+zardoz_SOURCES = $(foo_SOURCES)
+
+YFLAGS =
+AM_YFLAGS = $(COND_VAR1)
+zardoz_YFLAGS = $(COND_VAR2:z=r)
+
+COND_VAR2 = foo
+if COND
+YFLAGS += -v
+COND_VAR2 += bar
+else !COND
+COND_VAR1 = -d
+endif !COND
+END
+
+cat > Makefile5.am <<'END'
+bin_PROGRAMS = foo zardoz
+foo_SOURCES = foo.y
+zardoz_SOURCES = zardoz.y
+YFLAGS = -v
+AM_YFLAGS = -v
+if COND
+zardoz_YFLAGS = -v
+endif
+END
+
+cat > Makefile6.am <<'END'
+bin_PROGRAMS = foo
+foo_SOURCES = foo.y
+foo_YFLAGS = -v
+if COND
+quux_YFLAGS = -v
+AM_YFLAGS = -v
+endif
+END
+
 : > ylwrap
 
-$ACLOCAL
-AUTOMAKE_fails
-grep "Makefile\.am:5:.*AM_YFLAGS.* defined conditionally" stderr
-grep "Makefile\.am:6:.*bar_YFLAGS.* defined conditionally" stderr
+LC_ALL=C; export LC_ALL  # For grep regexes below.
+
+AUTOMAKE_fails -Wnone -Wunsupported Makefile
+grep '^Makefile\.am:5:.*AM_YFLAGS.* conditional contents' stderr
+grep '^Makefile\.am:6:.*zardoz_YFLAGS.* conditional contents' stderr
+
+for i in 1 2 3; do
+  AUTOMAKE_fails -Wnone -Wunsupported Makefile$i
+  grep "^Makefile$i\\.am:5:.*[^a-zA-Z0-9_]YFLAGS.* conditional contents" \
+       stderr
+done
+
+AUTOMAKE_fails -Wnone -Wunsupported Makefile4
+grep '^Makefile4\.am:6:.*[^a-zA-Z0-9_]YFLAGS.* conditional contents' stderr
+grep '^Makefile4\.am:7:.*AM_YFLAGS.* conditional contents' stderr
+grep '^Makefile4\.am:8:.*zardoz_YFLAGS.* conditional contents' stderr
+
+# Now let's check we avoid false positives.
+
+# Disable `gnu' warnings because we override the user variable `YFLAGS'.
+AUTOMAKE_fails -Wno-gnu Makefile5
+grep -v '^Makefile5\.am:.*zardoz_YFLAGS' stderr | grep . && Exit 1
+
+# Disable `gnu' warnings because we override the user variable `YFLAGS'.
+$AUTOMAKE -Wno-gnu Makefile6
 
 :
diff --git a/tests/yflags-force-conditional.test 
b/tests/yflags-force-conditional.test
new file mode 100755
index 0000000..65f3197
--- /dev/null
+++ b/tests/yflags-force-conditional.test
@@ -0,0 +1,95 @@
+#! /bin/sh
+# 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
+# the Free Software Foundation; either version 2, or (at your option)
+# any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# 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 the user can force automake to use *_YFLAGS variables
+# which have conditional content.
+
+. ./defs || Exit 1
+
+set -e
+
+cat >> configure.in <<'END'
+AC_PROG_CC
+AC_PROG_YACC
+AM_CONDITIONAL([COND], [test x"$cond" = x"yes"])
+AC_OUTPUT
+END
+
+mkdir bin
+cat > bin/fake-yacc <<'END'
+#!/bin/sh
+echo "/* $* */" > y.tab.c
+echo 'extern int dummy;' >> y.tab.c
+END
+chmod a+x bin/fake-yacc
+PATH=`pwd`/bin$PATH_SEPARATOR$PATH; export PATH
+YACC=fake-yacc; export YACC
+
+cat > Makefile.am <<'END'
+bin_PROGRAMS = foo bar
+foo_SOURCES = foo.y main.c
+bar_SOURCES = $(foo_SOURCES)
+bar_YFLAGS = $(bar_yflags2)
+if COND
+AM_YFLAGS = __am_cond_yes__
+bar_YFLAGS += __bar_cond_yes__
+else !COND
+AM_YFLAGS = __am_cond_no__
+bar_yflags2 = __bar_cond_no__
+endif !COND
+END
+
+cat > main.c <<'END'
+int main (void)
+{
+  return 0;
+}
+END
+
+: > foo.y
+
+$ACLOCAL
+$AUTOCONF
+$AUTOMAKE -a -Wno-unsupported
+
+$EGREP '(YFLAGS|yflags|am__append)' Makefile.in # For debugging.
+
+./configure cond=yes
+$MAKE
+
+ls -l
+cat foo.c
+cat bar-foo.c
+
+$FGREP ' __am_cond_yes__ ' foo.c
+$FGREP ' __bar_cond_yes__ ' bar-foo.c
+$FGREP 'cond_no' foo.c bar-foo.c && Exit 1
+
+$MAKE maintainer-clean
+ls -l
+
+./configure cond=no
+$MAKE
+
+ls -l
+cat foo.c
+cat bar-foo.c
+
+$FGREP ' __am_cond_no__ ' foo.c
+$FGREP ' __bar_cond_no__ ' bar-foo.c
+$FGREP 'cond_yes' foo.c bar-foo.c && Exit 1
+
+:
-- 
1.7.2.3