FYI: -Wgnu warns about user variable being overriden

[Alexandre Duret-Lutz]

>>> "adl" == Alexandre Duret-Lutz <address@hidden> writes:

[...]

 adl> - warn if the Makefile.am authors is overriding a user 
 adl> variable like CFLAGS.

[...]

In order to do this, I had to know whether a variable was last
defined in a Makefile.am (i.e. obviously we shouldn't warn if such
a variable is defined by configure.ac).

I've changed all uses of $var_is_am{$var} (a boolean that says
whether the variable was defined by Automake or by the User;
where user == configure.ac + Makefile.am) into $var_owner{$var}
which can take three values: VAR_AUTOMAKE, VAR_CONFIGURE, or
VAR_MAKEFILE.

Now, catching this error simply consists in listing langages
flags which are VAR_MAKEFILE.

Running `make check' after this change, I've found a bug related
to require_variables.  Sometimes we require `ANSI2KNR', but this
variable is a bit special: we detect that it is AC_SUBSTed, so
we add it to %configure_var, however as an exception we don't
call macro_define for it.  Therefore checking for
$var_location{'ANSI2KNR'} is not enough to detect whether
AM_C_PROTOTYPES was used, require_variables needs to check
%configure_var too.  Actually I have no idea why this didn't
caused problems earlier.

Here is what I'm committing.

2002-07-12  Alexandre Duret-Lutz  <address@hidden>

        * automake.in: Register warning channel `gnu'.
        (set_strictness): Turn on `gnu' in --gnu and --gnits.
        (usage): Mention the `gnu' category.
        (%var_is_am): Replace by ...
        (%var_owner): ... this, which uses ...
        (VAR_AUTOMAKE, VAR_CONFIGURE, VAR_MAKEFILE): ... these new constants.
        Adjust all uses of %var_is_am.
        (handle_languages): Warn about user variables being overriden.
        (require_variables): Also check %configure_vars for the existence
        of a required variable.
        * automake.texi (Invoking Automake): Document -Wgnu.
        * tests/yacc2.test, tests/yacc3.test: Use -Wno-gnu when
        we test YFLAGS.
        * tests/gnuwarn.test: New file.
        * tests/Makefile.am (TESTS): Add gnuwarn.test.

Index: automake.in
===================================================================
RCS file: /cvs/automake/automake/automake.in,v
retrieving revision 1.1327
diff -u -r1.1327 automake.in
--- automake.in 11 Jul 2002 20:10:38 -0000      1.1327
+++ automake.in 12 Jul 2002 07:57:33 -0000
@@ -492,12 +492,19 @@
 # - $var_location{$VAR} is where it was defined,
 # - $var_comment{$VAR} are the comments associated to it.
 # - $var_type{$VAR} is how it has been defined (`', `+', or `:'),
-# - $var_is_am{$VAR} is true if the variable is owned by Automake.
+# - $var_owner{$VAR} tells who owns the variable (VAR_AUTOMAKE,
+#     VAR_CONFIGURE, or VAR_MAKEFILE).
 my %var_value;
 my %var_location;
 my %var_comment;
 my %var_type;
-my %var_is_am;
+my %var_owner;
+# Possible values for var_owner.  Defined so that the owner of
+# a variable can only be increased (e.g Automake should not
+# override a configure or Makefile variable).
+use constant VAR_AUTOMAKE => 0; # Variable defined by Automake.
+use constant VAR_CONFIGURE => 1;# Variable defined in configure.ac.
+use constant VAR_MAKEFILE => 2; # Variable defined in Makefile.am.
 
 # This holds a 1 if a particular variable was examined.
 my %content_seen;
@@ -706,7 +713,7 @@
     %var_location = ();
     %var_comment = ();
     %var_type = ();
-    %var_is_am = ();
+    %var_owner = ();
 
     %content_seen = ();
 
@@ -861,8 +868,10 @@
 register_channel 'unused', type => 'warning';
 # Warnings about obsolete features (silent by default).
 register_channel 'obsolete', type => 'warning', silent => 1;
-# Warnings about non-portable construct.
+# Warnings about non-portable constructs.
 register_channel 'portability', type => 'warning', silent => 1;
+# Warnings related to GNU Coding Standards.
+register_channel 'gnu', type => 'warning';
 
 # For &verb.
 register_channel 'verb', type => 'debug', silent => 1;
@@ -1545,7 +1554,7 @@
     foreach my $var ('PRE_INSTALL', 'POST_INSTALL', 'NORMAL_INSTALL')
       {
        reject_var $var, "`$var' should not be defined"
-         if ! $var_is_am{$var};
+         if $var_owner{$var} != VAR_AUTOMAKE;
       }
 
     # Catch some obsolete variables.
@@ -1605,7 +1614,7 @@
 
     # Re-init SOURCES.  FIXME: other code shouldn't depend on this
     # (but currently does).
-    macro_define ('SOURCES', 1, '', 'TRUE', "@sources", 'internal');
+    macro_define ('SOURCES', VAR_AUTOMAKE, '', 'TRUE', "@sources", 'internal');
     define_pretty_variable ('DIST_SOURCES', '', @dist_sources);
 
     &handle_multilib;
@@ -2104,6 +2113,23 @@
 
        # Call the finisher.
        $lang->finish;
+
+       # Flags listed in `->flags' are user variables (per GNU Standards),
+       # they should not be overriden in the Makefile...
+       my @dont_override = @{$lang->flags};
+       # ... and so is LDFLAGS.
+       push @dont_override, 'LDFLAGS' if $lang->link;
+
+       foreach my $flag (@dont_override)
+         {
+           if (exists $var_owner{$flag} &&
+               $var_owner{$flag} == VAR_MAKEFILE)
+             {
+               msg ('gnu', $var_location{$flag},
+                    "`$flag' is a user variable, you should not "
+                    . "override it;\nuse `AM_$flag' instead.");
+             }
+         }
     }
 
     # If the project is entirely C++ or entirely Fortran 77 (i.e., 1
@@ -6013,12 +6039,12 @@
   return @res;
 }
 
-# &macro_define($VAR, $VAR_IS_AM, $TYPE, $COND, $VALUE, $WHERE)
+# &macro_define($VAR, $OWNER, $TYPE, $COND, $VALUE, $WHERE)
 # -------------------------------------------------------------
 # The $VAR can go from Automake to user, but not the converse.
 sub macro_define ($$$$$$)
 {
-  my ($var, $var_is_am, $type, $cond, $value, $where) = @_;
+  my ($var, $owner, $type, $cond, $value, $where) = @_;
 
   err $where, "bad characters in variable name `$var'"
     if $var !~ /$MACRO_PATTERN/o;
@@ -6033,7 +6059,7 @@
   # An Automake variable must be consistently defined with the same
   # sign by Automake.  A user variable must be set by either `=' or
   # `:=', and later promoted to `+='.
-  if ($var_is_am)
+  if ($owner == VAR_AUTOMAKE)
     {
       err ($where, "$var was set with `$var_type{$var}=' "
           . "and is now set with `$type='")
@@ -6049,7 +6075,7 @@
   # When adding, since we rewrite, don't try to preserve the
   # Automake continuation backslashes.
   $value =~ s/\\$//mg
-    if $type eq '+' && $var_is_am;
+    if $type eq '+' && $owner == VAR_AUTOMAKE;
 
   # Differentiate assignment types.
 
@@ -6103,7 +6129,7 @@
              {
                # Yes, let's simply append to it.
                $var = $appendvar{$key};
-               $var_is_am = 1;
+               $owner = VAR_AUTOMAKE;
              }
            else
              {
@@ -6111,7 +6137,8 @@
                my $num = 1 + keys (%appendvar);
                my $hvar = "am__append_$num";
                $appendvar{$key} = $hvar;
-               &macro_define ($hvar, 1, '+', $cond, $value, $where);
+               &macro_define ($hvar, VAR_AUTOMAKE, '+',
+                              $cond, $value, $where);
                push @var_list, $hvar;
                # Now HVAR is to be added to VAR.
                $value = "\$($hvar)";
@@ -6142,7 +6169,7 @@
            }
          else
            {
-             &macro_define ($var, $var_is_am, '+', $vcond, $value, $where);
+             &macro_define ($var, $owner, '+', $vcond, $value, $where);
            }
        }
     }
@@ -6159,7 +6186,9 @@
 
       # If Automake tries to override a value specified by the user,
       # just don't let it do.
-      if (defined $var_value{$var}{$cond} && !$var_is_am{$var} && $var_is_am)
+      if (defined $var_value{$var}{$cond}
+         && $var_owner{$var} != VAR_AUTOMAKE
+         && $owner == VAR_AUTOMAKE)
        {
          verb ("refusing to override the user definition of:\n"
                . macro_dump ($var)
@@ -6171,18 +6200,20 @@
          # an Automake variable or an AC_SUBST variable for an existing
          # condition.
          check_ambiguous_conditional ($var, $cond)
-           unless (($var_is_am{$var} && !$var_is_am
-                    || exists $configure_vars{$var})
-                   && exists $var_value{$var}{$cond});
+           unless (exists $var_value{$var}{$cond}
+                   && (($var_owner{$var} == VAR_AUTOMAKE
+                        && $owner != VAR_AUTOMAKE)
+                       || $var_owner{$var} == VAR_CONFIGURE));
 
          $var_value{$var}{$cond} = $value;
        }
     }
 
-  # An Automake variable can be given to the user, but not the converse.
-  if (! defined $var_is_am{$var} || !$var_is_am)
+  # The owner of a variable can only increase, because an Automake
+  # variable can be given to the user, but not the converse.
+  if (! defined $var_owner{$var} || $owner > $var_owner{$var})
     {
-      $var_is_am{$var} = $var_is_am;
+      $var_owner{$var} = $owner;
     }
 
   # Call var_VAR_trigger if it's defined.
@@ -6206,7 +6237,7 @@
     {
       delete $var_value{$var};
       delete $var_location{$var};
-      delete $var_is_am{$var};
+      delete $var_owner{$var};
       delete $var_comment{$var};
       delete $var_type{$var};
     }
@@ -6233,12 +6264,32 @@
     }
   else
     {
-      my $var_is_am = $var_is_am{$var} ? "Automake" : "User";
+      prog_error ("`$var' is a key in \$var_value, but not in \$var_owner\n")
+       unless exists $var_owner{$var};
+      my $var_owner;
+      if ($var_owner{$var} == VAR_AUTOMAKE)
+       {
+         $var_owner = 'Automake';
+       }
+      elsif ($var_owner{$var} == VAR_CONFIGURE)
+       {
+         $var_owner = 'Configure';
+       }
+      elsif ($var_owner{$var} == VAR_MAKEFILE)
+       {
+         $var_owner = 'Makefile';
+       }
+      else
+       {
+         prog_error ("unexpected value for `\$var_owner{$var}': "
+                     . $var_owner{$var})
+           unless defined $var_owner;
+       }
       my $where = (defined $var_location{$var}
                   ? $var_location{$var} : "undefined");
       $text .= "$var_comment{$var}"
        if defined $var_comment{$var};
-      $text .= "  $var ($var_is_am, where = $where) $var_type{$var}=\n  {\n";
+      $text .= "  $var ($var_owner, where = $where) $var_type{$var}=\n  {\n";
       foreach my $vcond (sort by_condition keys %{$var_value{$var}})
        {
          $text .= "    $vcond => $var_value{$var}{$vcond}\n";
@@ -6919,7 +6970,7 @@
 
     if (! variable_defined ($var, $cond))
     {
-        macro_define ($var, 1, '', $cond, "@value", undef);
+        macro_define ($var, VAR_AUTOMAKE, '', $cond, "@value", undef);
        variable_pretty_output ($var, $cond || 'TRUE');
        $content_seen{$var} = 1;
     }
@@ -6949,9 +7000,8 @@
       # appreciated by Make.
       && ! grep { $_ eq $var } (qw(ANSI2KNR AMDEPBACKSLASH)))
     {
-      # A macro defined via configure is a `user' macro -- we should not
-      # override it.
-      macro_define ($var, 0, '', 'TRUE', subst $var, $configure_vars{$var});
+      macro_define ($var, VAR_CONFIGURE, '', 'TRUE',
+                   subst $var, $configure_vars{$var});
       variable_pretty_output ($var, 'TRUE');
     }
 }
@@ -7277,7 +7327,7 @@
                {
                  append_comments $last_var_name, $spacing, $comment;
                  $comment = $spacing = '';
-                 macro_define ($last_var_name, 0,
+                 macro_define ($last_var_name, VAR_MAKEFILE,
                                $last_var_type, $cond,
                                $last_var_value, $here)
                    if $cond ne 'FALSE';
@@ -7335,7 +7385,7 @@
                append_comments $last_var_name, $spacing, $comment;
                $comment = $spacing = '';
 
-               macro_define ($last_var_name, 0,
+               macro_define ($last_var_name, VAR_MAKEFILE,
                              $last_var_type, $cond,
                              $last_var_value, $here)
                  if $cond ne 'FALSE';
@@ -7431,21 +7481,19 @@
     &read_am_file ($amfile);
 
     # Output all the Automake variables.  If the user changed one,
-    # then it is now marked as owned by the user.
+    # then it is now marked as VAR_CONFIGURE or VAR_MAKEFILE.
     foreach my $var (uniq @var_list)
     {
-       # Don't process user variables.
-        variable_output ($var)
-         unless !$var_is_am{$var};
+      variable_output ($var)
+       if exists $var_owner{$var} && $var_owner{$var} == VAR_AUTOMAKE;
     }
 
     # Now dump the user variables that were defined.  We do it in the same
     # order in which they were defined (skipping duplicates).
     foreach my $var (uniq @var_list)
     {
-       # Don't process Automake variables.
-        variable_output ($var)
-         unless $var_is_am{$var};
+      variable_output ($var)
+       if exists $var_owner{$var} && $var_owner{$var} != VAR_AUTOMAKE;
     }
 }
 
@@ -7747,7 +7795,8 @@
 
            # Accumulating variables must not be output.
            append_comments $var, $spacing, $comment;
-           macro_define ($var, $is_am, $type, $cond, $val, $file)
+           macro_define ($var, $is_am ? VAR_AUTOMAKE : VAR_MAKEFILE,
+                         $type, $cond, $val, $file)
              if $cond ne 'FALSE';
            push (@var_list, $var);
 
@@ -7755,7 +7804,8 @@
            # of (which is detected by the first reading of
            # `header-vars.am'), we must not output them.
            $result_vars .= "$spacing$comment$_\n"
-             if $type ne '+' && $var_is_am{$var} && $cond ne 'FALSE';
+             if ($type ne '+' && exists $var_owner{$var}
+                 && $var_owner{$var} == VAR_AUTOMAKE && $cond ne 'FALSE');
 
            $comment = $spacing = '';
        }
@@ -7901,15 +7951,12 @@
     {
       # Automake is allowed to define variables that look like primaries
       # but which aren't.  E.g. INSTALL_sh_DATA.
-      next
-       if $var_is_am{$varname};
       # Autoconf can also define variables like INSTALL_DATA, so
-      # ignore all configure variables.
-      # FIXME: Actually we'd better ignore configure variables which
-      # are not overridden in Makefile.am; but it's not clear how to
-      # do this presently.
+      # ignore all configure variables (at least those which are not
+      # redefined in Makefile.am).
       next
-       if exists $configure_vars{$varname};
+       if (exists $var_owner{$varname}
+           && $var_owner{$varname} != VAR_MAKEFILE);
 
       if ($varname =~ /^(nobase_)?(dist_|nodist_)?(.*)_$primary$/)
        {
@@ -8428,7 +8475,7 @@
 {
   prog_error "push_dist_common run after handle_dist"
     if $handle_dist_run;
-  macro_define ('DIST_COMMON', 1, '+', '', "@_", '');
+  macro_define ('DIST_COMMON', VAR_AUTOMAKE, '+', '', "@_", '');
 }
 
 
@@ -8443,6 +8490,7 @@
       setup_channel 'error-gnu/warn', silent => 0, type => 'error';
       setup_channel 'error-gnits', silent => 1;
       setup_channel 'portability', silent => 0;
+      setup_channel 'gnu', silent => 0;
     }
   elsif ($strictness_name eq 'gnits')
     {
@@ -8451,6 +8499,7 @@
       setup_channel 'error-gnu/warn', silent => 0, type => 'error';
       setup_channel 'error-gnits', silent => 0;
       setup_channel 'portability', silent => 0;
+      setup_channel 'gnu', silent => 0;
     }
   elsif ($strictness_name eq 'foreign')
     {
@@ -8459,6 +8508,7 @@
       setup_channel 'error-gnu/warn', silent => 0, type => 'warning';
       setup_channel 'error-gnits', silent => 1;
       setup_channel 'portability', silent => 1;
+      setup_channel 'gnu', silent => 1;
     }
   else
     {
@@ -8494,8 +8544,10 @@
 
   foreach my $var (@vars)
     {
-      # Nothing to do if the variable exists.
-      next if (exists $var_value{$var});
+      # Nothing to do if the variable exists.  The $configure_vars test
+      # needed for strange variables like AMDEPBACKSLASH or ANSI2KNR
+      # that are AC_SUBST'ed but never macro_define'd.
+      next if (exists $var_value{$var} || exists $configure_vars{$var});
 
       ++$res;
 
@@ -8559,10 +8611,11 @@
   -f, --force-missing    force update of standard files
 
 Warning categories include:
+  `gnu'           GNU coding standards (default in gnu and gnits modes)
   `obsolete'      obsolete features or constructions
   `unsupported'   unsupported or incomplete features (default)
   `unused'        unused variables (default)
-  `portability'   portability issues (default in gnu and gnits mode)
+  `portability'   portability issues (default in gnu and gnits modes)
   `all'           all the warnings
   `no-CATEGORY'   turn off warnings in CATEGORY
   `none'          turn off all the warnings
Index: automake.texi
===================================================================
RCS file: /cvs/automake/automake/automake.texi,v
retrieving revision 1.289
diff -u -r1.289 automake.texi
--- automake.texi       11 Jul 2002 20:10:39 -0000      1.289
+++ automake.texi       12 Jul 2002 07:57:50 -0000
@@ -1037,6 +1037,9 @@
 Output warnings falling in @var{category}.  @var{category} can be
 one of:
 @table @samp
address@hidden gnu
+warnings related to the GNU Coding Standards
+(@pxref{Top, , , standards, The GNU Coding Standards}).
 @item obsolete
 obsolete features or constructions
 @item unsupported
@@ -1058,7 +1061,8 @@
 variables.
 
 The categories output by default are @samp{unsupported} and
address@hidden
address@hidden  Additionally, @samp{gnu} and @samp{portability} warnings
+are enabled in @samp{--gnu} and @samp{--gnits} strictness.
 
 @vindex WARNINGS
 The environment variable @samp{WARNINGS} can contain a comma separated
Index: tests/Makefile.am
===================================================================
RCS file: /cvs/automake/automake/tests/Makefile.am,v
retrieving revision 1.416
diff -u -r1.416 Makefile.am
--- tests/Makefile.am   11 Jul 2002 20:10:39 -0000      1.416
+++ tests/Makefile.am   12 Jul 2002 07:57:52 -0000
@@ -175,6 +175,7 @@
 gcj4.test \
 gcj5.test \
 getopt.test \
+gnuwarn.test \
 gnits.test \
 gnits2.test \
 header.test \
Index: tests/gnuwarn.test
===================================================================
RCS file: tests/gnuwarn.test
diff -N tests/gnuwarn.test
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ tests/gnuwarn.test  12 Jul 2002 07:57:53 -0000
@@ -0,0 +1,38 @@
+#! /bin/sh
+
+# Check that Automake warns about user variables being overriden.
+
+. $srcdir/defs || exit 1
+
+set -e
+
+cat >> configure.in << 'END'
+AC_PROG_CC
+AC_OUTPUT
+END
+
+# Needed by --gnu.
+: > NEWS
+: > README
+: > AUTHORS
+: > ChangeLog
+
+cat > Makefile.am << 'END'
+CFLAGS = -I..
+LDFLAGS = -lfoo
+CXXFLAGS = -Wall
+bin_PROGRAMS = bar
+END
+
+$ACLOCAL
+# Don't warn in foreign mode
+$AUTOMAKE -Wnone --add-missing --foreign
+# Warn in gnu mode
+$AUTOMAKE -Wnone  --add-missing --gnu 2>stderr && exit 1
+cat stderr
+grep CFLAGS stderr
+grep LDFLAGS stderr
+# No reason to warn about CXXFLAGS since it's not used.
+grep CXXFLAGS stderr && exit 1
+# Don't warn if -Wno-gnu.
+$AUTOMAKE -Wnone --gnu -Wno-gnu
Index: tests/yacc2.test
===================================================================
RCS file: /cvs/automake/automake/tests/yacc2.test,v
retrieving revision 1.3
diff -u -r1.3 yacc2.test
--- tests/yacc2.test    9 Apr 2001 14:50:53 -0000       1.3
+++ tests/yacc2.test    12 Jul 2002 07:57:53 -0000
@@ -52,7 +52,11 @@
 cp Makefile.src Makefile.am
 echo 'YFLAGS = -d' >> Makefile.am
 
-$AUTOMAKE || exit 1
+# YFLAGS is a use variable.
+$AUTOMAKE 2>stderr && exit 1
+cat stderr
+grep 'YFLAGS' stderr || exit 1
+$AUTOMAKE -Wno-gnu || exit 1
 
 # If zardoz.h is NOT mentioned, fail
 grep 'zardoz.h' Makefile.in > /dev/null || exit 1
@@ -62,7 +66,7 @@
 cp Makefile.src Makefile.am
 echo 'YFLAGS = ' >> Makefile.am
 
-$AUTOMAKE || exit 1
+$AUTOMAKE -Wno-gnu || exit 1
 
 # If zardoz.h IS mentioned, fail
 grep 'zardoz.h' Makefile.in > /dev/null && exit 1
Index: tests/yacc3.test
===================================================================
RCS file: /cvs/automake/automake/tests/yacc3.test,v
retrieving revision 1.4
diff -u -r1.4 yacc3.test
--- tests/yacc3.test    30 May 2002 06:05:05 -0000      1.4
+++ tests/yacc3.test    12 Jul 2002 07:57:53 -0000
@@ -32,6 +32,6 @@
    cp Save Makefile.am
    echo "$flag = -d" >> Makefile.am
 
-   $AUTOMAKE || exit 1
+   $AUTOMAKE -Wno-gnu || exit 1
    grep 'zardoz.h' Makefile.in || exit 1
 done
-- 
Alexandre Duret-Lutz