[PATCH 2/4] Consistently process -W(no-)error after all other warning options.

[Zack Weinberg]

automake and aclocal were processing ‘-W(no-)error’ whenever it
appeared on the command line, which means that
‘-Werror,something-strange’ would issue a hard error, but
‘-Wsomething-strange,error’ would only issue a warning.

It is not desirable for warnings about unknown warning categories ever to be
treated as a hard error; that leads to problems for driver scripts like
autoreconf, which would like to pass whatever -W options it got on its own
command line down to all the tools and not worry about which tools understand
which warning categories.  Also, this sort of order dependence is confusing
for humans.

Change parse_warnings to take just one option, the _complete_ list of warning
categories seen on the command line, and to process -Werror / -Wno-error after
processing all other warnings options.  Thus, unknown warnings categories will
always just be a plain warning.  This does mean aclocal has to stop using
parse_warnings as a Getopt::Long callback, but that’s not a big deal.

Similarly, change parse_WARNINGS to record whether ‘error’ appeared in the
environment variable, but not activate warnings-are-errors mode itself.
parse_warnings picks up the record and honors it, unless it’s overridden by
the command line.

* lib/Automake/ChannelDefs.pm ($werror): New package global (not exported).
  (parse_WARNINGS): Do not call switch_warning for ‘error’ / ‘no-error’;
  just toggle the value of $werror.
  (parse_warnings): Do not call switch_warning immediately for
  ‘error’ / ‘no-error’; toggle $werror instead.  Call switch_warning ‘error’
  at the very end if $werror is true.  Remove unused $OPTION argument.
* bin/automake.in: parse_warnings now takes only one argument.
* bin/aclocal.in: Call parse_warnings after parse_options instead of
  using it as a parse_options callback.
---
 bin/aclocal.in              |  4 ++-
 bin/automake.in             |  2 +-
 lib/Automake/ChannelDefs.pm | 50 ++++++++++++++++++++++++++++---------
 3 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/bin/aclocal.in b/bin/aclocal.in
index 1e734ea48..42e8d839a 100644
--- a/bin/aclocal.in
+++ b/bin/aclocal.in
@@ -1081,6 +1081,7 @@ sub parse_arguments ()
 {
   my $print_and_exit = 0;
   my $diff_command;
+  my @warnings = ();
 
   my %cli_options =
     (
@@ -1096,11 +1097,12 @@ sub parse_arguments ()
      'output=s'                => \$output_file,
      'print-ac-dir'     => \$print_and_exit,
      'verbose'         => sub { setup_channel 'verb', silent => 0; },
-     'W|warnings=s'     => \&parse_warnings,
+     'W|warnings=s'     => \@warnings,
      );
 
   use Automake::Getopt ();
   Automake::Getopt::parse_options %cli_options;
+  parse_warnings @warnings;
 
   if (@ARGV > 0)
     {
diff --git a/bin/automake.in b/bin/automake.in
index ad91d875a..67b729045 100644
--- a/bin/automake.in
+++ b/bin/automake.in
@@ -8184,7 +8184,7 @@ sub parse_arguments ()
   set_strictness ($strict);
   my $cli_where = new Automake::Location;
   set_global_option ('no-dependencies', $cli_where) if $ignore_deps;
-  parse_warnings ('-W', @warnings);
+  parse_warnings @warnings;
 
   return unless @ARGV;
 
diff --git a/lib/Automake/ChannelDefs.pm b/lib/Automake/ChannelDefs.pm
index 59728bdbe..1693c05d5 100644
--- a/lib/Automake/ChannelDefs.pm
+++ b/lib/Automake/ChannelDefs.pm
@@ -352,36 +352,62 @@ Parse the WARNINGS environment variable.
 
 =cut
 
+# Used to communicate from parse_WARNINGS to parse_warnings.
+our $_werror = 0;
+
 sub parse_WARNINGS ()
 {
   if (exists $ENV{'WARNINGS'})
     {
       # Ignore unknown categories.  This is required because WARNINGS
       # should be honored by many tools.
-      switch_warning $_ foreach (split (',', $ENV{'WARNINGS'}));
+      # For the same reason, do not turn on -Werror at this point, just
+      # record that we saw it; parse_warnings will turn on -Werror after
+      # the command line has been processed.
+      foreach (split (',', $ENV{'WARNINGS'}))
+        {
+          if (/^(no-)?error$/)
+            {
+              $_werror = !defined $1;
+            }
+          else
+            {
+              switch_warning $_;
+            }
+        }
     }
 }
 
-=item C<parse_warnings ($OPTION, @ARGUMENT)>
+=item C<parse_warnings (@CATEGORIES)>
 
 Parse the argument of C<--warning=CATEGORY> or C<-WCATEGORY>.
+C<@CATEGORIES> is the accumulated set of warnings categories.
+Use like this:
 
-C<$OPTIONS> is C<"--warning"> or C<"-W">, C<@ARGUMENT> is a list of
-C<CATEGORY>.
-
-This can be used as an argument to C<Getopt>.
+    Automake::GetOpt::parse_options (
+        # ...
+        'W|warnings=s' => \@warnings,
+    )
+    # possibly call set_strictness here
+    parse_warnings @warnings;
 
 =cut
 
-sub parse_warnings ($@)
+sub parse_warnings (@)
 {
-  my ($opt, @categories) = @_;
-
-  foreach my $cat (map { split ',' } @categories)
+  foreach my $cat (map { split ',' } @_)
     {
-      msg 'unsupported', "unknown warning category '$cat'"
-       if switch_warning $cat;
+      if ($cat =~ /^(no-)?error$/)
+        {
+          $_werror = !defined $1;
+        }
+      elsif (switch_warning $cat)
+        {
+          msg 'unsupported', "unknown warning category '$cat'";
+        }
     }
+
+  switch_warning ($_werror ? 'error' : 'no-error');
 }
 
 =item C<set_strictness ($STRICTNESS_NAME)>
-- 
2.28.0