Re: [PATCH 03/10] Warnings win over strictness on command line.

[Stefano Lattarini]

On Sunday 02 January 2011, Ralf Wildenhues wrote:
> * address@hidden wrote on Thu, Dec 23, 2010 at 12:27:39PM CET:
> >  2010-12-20  Stefano Lattarini  <address@hidden>
> >  
> > +   Warnings win over strictness on command line.
> 
> Please add a line like
>       For PR automake/547:
> 
> on a line by itself here, as done in some (older) ChangeLog entries, and
> put address@hidden in Cc:, that way your mail gets
> added to the Gnats trail.  Thanks.  You can omit the other mention of
> 547 below then.
> 
> (I'm not actually sure about the exact regex used to track Gnats
> additions, but it did include something like "PR $PACKAGE/$number".)
>
Done.

> The same applies to patches 04, 07, 08, and 09 of this series.
>
I'll fix them too.

> > +   This change ensures that, on the command line at least, explicitly
> 
> "Ensure that on the command line, explicitly ..."
> (it is clear that you are speaking about this change).
> 
> > +   defined warnings always take precedence over implicit strictness
> > +   implied warnings.  Related to Automake bug#7669 a.k.a. PR/547.
> > +   * automake.in (parse_arguments): Parse warnings only after the
> > +   strictness level has been processed.
> > +   * tests/gnuwarn.test: Update, plus miscellaneous improvements.
> > +   * tests/warnings-win-over-strictness.test: New test.
> > +   * tests/Makefile.am (TESTS): Update.
> 
> > --- a/automake.in
> > +++ b/automake.in
> > @@ -8444,26 +8444,26 @@ EOF
> >  # Parse command line.
> >  sub parse_arguments ()
> >  {
> > -  # Start off as gnu.
> > -  set_strictness ('gnu');
> > +  my $strictness = 'gnu';
> 
> This local variable shadows the global $strictness.  Please use $strict
> or so instead, to avoid ambiguities for the reader.
>
OK.

> > +  my $cygnus = 0;
> > +  my $ignore_deps = 0;
> > +  my @warnings = ();
> >  
> > -  my $cli_where = new Automake::Location;
> >    my %cli_options =
> >      (
> >       'libdir=s'    => \$libdir,
> > -     'gnu'         => sub { set_strictness ('gnu'); },
> > -     'gnits'               => sub { set_strictness ('gnits'); },
> > -     'cygnus'              => sub { set_global_option ('cygnus', 
> > $cli_where); },
> > -     'foreign'          => sub { set_strictness ('foreign'); },
> > -     'include-deps'        => sub { unset_global_option 
> > ('no-dependencies'); },
> > -     'i|ignore-deps'       => sub { set_global_option ('no-dependencies',
> > -                                               $cli_where); },
> > +     'gnu'         => sub { $strictness = 'gnu'; },
> > +     'gnits'               => sub { $strictness = 'gnits'; },
> > +     'foreign'             => sub { $strictness = 'foreign'; },
> > +     'cygnus'              => \$cygnus,
> > +     'include-deps'        => sub { $ignore_deps = 0; },
> > +     'i|ignore-deps'       => sub { $ignore_deps = 1; },
> >       'no-force'    => sub { $force_generation = 0; },
> >       'f|force-missing'  => \$force_missing,
> >       'a|add-missing'       => \$add_missing,
> >       'c|copy'              => \$copy_missing,
> >       'v|verbose'   => sub { setup_channel 'verb', silent => 0; },
> > -     'W|warnings=s'     => \&parse_warnings,
> > +     'W|warnings=s'     => address@hidden,
> >       );
> >    use Getopt::Long;
> >    Getopt::Long::config ("bundling", "pass_through");
> > @@ -8492,6 +8492,15 @@ sub parse_arguments ()
> >    Getopt::Long::GetOptions %cli_options, 'version' => sub {}, 'help' => 
> > sub {}
> >      or exit 1;
> >  
> > +  set_strictness ($strictness);
> > +  my $cli_where = new Automake::Location;
> > +  set_global_option ('cygnus', $cli_where) if $cygnus;
> > +  set_global_option ('no-dependencies', $cli_where) if $ignore_deps;
> 
> I wonder whether there was a rule to put postponed Perl-style 'if $cond'
> on a line of its own, indented by two more spaces, except for very short
> statements preceding the 'if'.  The current code doesn't do this
> throughout, but it might improve readability.
> 
> Does the new code still preserve the "cygnus implies foreign strictness"
> semantics (I'm assuming yes, since you added tests around that area)?
>
Yes, it does (the test 'cygnus-imply-foreign.test' still passes).

Ideally, each patch in this series should cause no regression whatsoever
in the pre-existing test cases (altough I haven't checked this for *all*
the respins this series was subjected to).

> > +  for my $warning (@warnings)
> > +  {
> > +    &parse_warnings('-W', $warning);
> > +  }
> 
> Please use GNU-style brace indentation and spacing:
> 
>     for my $warning (@warnings)
>       {
>         &parse_warnings ('-W', $warning);
>       }
> 
> I know it's a bit awkward for Perl code, but the existing code uses it
> throughout and I prefer consistency.
>
Sorry, that is a silly overlooking; I really have no intention to worsen
the already non-so-great[1] consistency of the codebase.

[1] I know that's due to hisorical reasons (mostly perl 4 support); JTBC,
I mean no criticism here.

> > --- a/tests/gnuwarn.test
> > +++ b/tests/gnuwarn.test
> 
> > @@ -20,9 +20,11 @@
> >  
> >  set -e
> >  
> > +# We need (almost) complete control over automake options.
> > +AUTOMAKE="$original_AUTOMAKE -Werror"
> > +
> >  cat >> configure.in << 'END'
> >  AC_PROG_CC
> > -AC_OUTPUT
> 
> Let's keep this line, even if not absolutely needed.
> 
OK.

> 
> > --- /dev/null
> > +++ b/tests/warnings-win-over-strictness.test
> > @@ -0,0 +1,54 @@
> 
> > +# Check that, on the command line, explicitly-defined warnings take
> > +# precedence over implicit strictness-implied warnings.
> > +
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +# We want complete control over automake options.
> > +AUTOMAKE=$original_AUTOMAKE
> > +
> > +ok()
> 
> space before paren; two instances.
>
Again :-(

I hope I'll manage to get my head around the "formatting rules" of the
GNU Coding Standars at last.  For the moment, sorry for the noise.

> > +{
> > +  AUTOMAKE_run 0 $*
> > +  test ! -s stderr
> > +}
> > +
> > +ko()
> > +{
> > +  AUTOMAKE_run 0 $*
> > +  grep '^Makefile\.am:.*:=.*not portable' stderr
> > +  test `wc -l <stderr` = 1
> 
> -eq
>
Yes, safer.  Fixed.

> > +}
> > +
> > +# Files required in gnu strictness.
> > +touch README INSTALL NEWS AUTHORS ChangeLog COPYING
> > +
> > +cat > Makefile.am <<END
> > +FOO := bar
> > +END
> > +
> > +$ACLOCAL
> > +
> > +ko --foreign -Wportability
> > +ko -Wportability --foreign
> > +ok --gnu -Wno-portability
> > +ok -Wno-portability --gnu
> 
> Patch is OK with above nits addressed.  Just to avoid misunderstanding,
> 01 and 02 are OK likewise.
> 
> Thank you for attacking this!
> Ralf
> 
Below is what I squashed in.  Attached is the amended
patch, for reference.

Thanks,
   Stefano

-*-*-*-

diff --git a/ChangeLog b/ChangeLog
index 8561491..6646150 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,9 +1,10 @@
-2010-12-20  Stefano Lattarini  <address@hidden>
+2011-01-02  Stefano Lattarini  <address@hidden>
 
+       For PR automake/547:
        Warnings win over strictness on command line.
-       This change ensures that, on the command line at least, explicitly
-       defined warnings always take precedence over implicit strictness
-       implied warnings.  Related to Automake bug#7669 a.k.a. PR/547.
+       Ensure that, on the command line at least, explicitly defined
+       warnings always take precedence over implicit strictness-implied
+       warnings.  Related to Automake bug#7669.
        * automake.in (parse_arguments): Parse warnings only after the
        strictness level has been processed.
        * tests/gnuwarn.test: Update, plus miscellaneous improvements.
diff --git a/automake.in b/automake.in
index 52c4ceb..d38682f 100644
--- a/automake.in
+++ b/automake.in
@@ -8454,7 +8454,7 @@ EOF
 # Parse command line.
 sub parse_arguments ()
 {
-  my $strictness = 'gnu';
+  my $strict = 'gnu';
   my $cygnus = 0;
   my $ignore_deps = 0;
   my @warnings = ();
@@ -8462,9 +8462,9 @@ sub parse_arguments ()
   my %cli_options =
     (
      'libdir=s'        => \$libdir,
-     'gnu'             => sub { $strictness = 'gnu'; },
-     'gnits'           => sub { $strictness = 'gnits'; },
-     'foreign'         => sub { $strictness = 'foreign'; },
+     'gnu'             => sub { $strict = 'gnu'; },
+     'gnits'           => sub { $strict = 'gnits'; },
+     'foreign'         => sub { $strict = 'foreign'; },
      'cygnus'          => \$cygnus,
      'include-deps'    => sub { $ignore_deps = 0; },
      'i|ignore-deps'   => sub { $ignore_deps = 1; },
@@ -8502,14 +8502,14 @@ sub parse_arguments ()
   Getopt::Long::GetOptions %cli_options, 'version' => sub {}, 'help' => sub {}
     or exit 1;
 
-  set_strictness ($strictness);
+  set_strictness ($strict);
   my $cli_where = new Automake::Location;
   set_global_option ('cygnus', $cli_where) if $cygnus;
   set_global_option ('no-dependencies', $cli_where) if $ignore_deps;
   for my $warning (@warnings)
-  {
-    &parse_warnings('-W', $warning);
-  }
+    {
+      &parse_warnings ('-W', $warning);
+    }
 
   return unless @ARGV;
 
diff --git a/tests/gnuwarn.test b/tests/gnuwarn.test
index d637a8c..dffed49 100755
--- a/tests/gnuwarn.test
+++ b/tests/gnuwarn.test
@@ -1,5 +1,5 @@
 #! /bin/sh
-# Copyright (C) 2002, 2003, 2010 Free Software Foundation, Inc.
+# Copyright (C) 2002, 2003, 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
@@ -25,6 +25,7 @@ AUTOMAKE="$original_AUTOMAKE -Werror"
 
 cat >> configure.in << 'END'
 AC_PROG_CC
+AC_OUTPUT
 END
 
 # Needed by --gnu.
diff --git a/tests/warnings-win-over-strictness.test 
b/tests/warnings-win-over-strictness.test
index 977ed64..ef42c4f 100755
--- a/tests/warnings-win-over-strictness.test
+++ b/tests/warnings-win-over-strictness.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
@@ -24,17 +24,17 @@ set -e
 # We want complete control over automake options.
 AUTOMAKE=$original_AUTOMAKE
 
-ok()
+ok ()
 {
   AUTOMAKE_run 0 $*
   test ! -s stderr
 }
 
-ko()
+ko ()
 {
   AUTOMAKE_run 0 $*
   grep '^Makefile\.am:.*:=.*not portable' stderr
-  test `wc -l <stderr` = 1
+  test `wc -l <stderr` -eq 1
 }
 
 # Files required in gnu strictness.

From b7a0eb0957f920cba8f2263d743c64c28024a75b Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Mon, 20 Dec 2010 12:10:56 +0100
Subject: [PATCH] Warnings win over strictness on command line.

This change ensures that, on the command line at least, explicitly
defined warnings always take precedence over implicit strictness
implied warnings.  Related to Automake bug#7669 a.k.a. PR/547.

* automake.in (parse_arguments): Parse warnings only after the
strictness level has been processed.
* tests/gnuwarn.test: Update, plus miscellaneous improvements.
* tests/warnings-win-over-strictness.test: New test.
* tests/Makefile.am (TESTS): Update.
---
 ChangeLog                               |   13 +++++++
 automake.in                             |   31 +++++++++++------
 tests/Makefile.am                       |    1 +
 tests/Makefile.in                       |    1 +
 tests/gnuwarn.test                      |   17 ++++++---
 tests/warnings-win-over-strictness.test |   54 +++++++++++++++++++++++++++++++
 6 files changed, 100 insertions(+), 17 deletions(-)
 create mode 100755 tests/warnings-win-over-strictness.test

diff --git a/ChangeLog b/ChangeLog
index e57d6e8..b1e3fc4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,18 @@
 2011-01-02  Stefano Lattarini  <address@hidden>
 
+       For PR automake/547:
+       Warnings win over strictness on command line.
+       Ensure that, on the command line at least, explicitly defined
+       warnings always take precedence over implicit strictness-implied
+       warnings.  Related to Automake bug#7669.
+       * automake.in (parse_arguments): Parse warnings only after the
+       strictness level has been processed.
+       * tests/gnuwarn.test: Update, plus miscellaneous improvements.
+       * tests/warnings-win-over-strictness.test: New test.
+       * tests/Makefile.am (TESTS): Update.
+
+2011-01-02  Stefano Lattarini  <address@hidden>
+
        New test on silent-rules mode and portability warnings.
        * tests/silent-nowarn.test: New test.
        * tests/Makefile.am (TESTS): Update.
diff --git a/automake.in b/automake.in
index 57edd04..f464983 100644
--- a/automake.in
+++ b/automake.in
@@ -8454,26 +8454,26 @@ EOF
 # Parse command line.
 sub parse_arguments ()
 {
-  # Start off as gnu.
-  set_strictness ('gnu');
+  my $strict = 'gnu';
+  my $cygnus = 0;
+  my $ignore_deps = 0;
+  my @warnings = ();
 
-  my $cli_where = new Automake::Location;
   my %cli_options =
     (
      'libdir=s'        => \$libdir,
-     'gnu'             => sub { set_strictness ('gnu'); },
-     'gnits'           => sub { set_strictness ('gnits'); },
-     'cygnus'          => sub { set_global_option ('cygnus', $cli_where); },
-     'foreign'          => sub { set_strictness ('foreign'); },
-     'include-deps'    => sub { unset_global_option ('no-dependencies'); },
-     'i|ignore-deps'   => sub { set_global_option ('no-dependencies',
-                                                   $cli_where); },
+     'gnu'             => sub { $strict = 'gnu'; },
+     'gnits'           => sub { $strict = 'gnits'; },
+     'foreign'         => sub { $strict = 'foreign'; },
+     'cygnus'          => \$cygnus,
+     'include-deps'    => sub { $ignore_deps = 0; },
+     'i|ignore-deps'   => sub { $ignore_deps = 1; },
      'no-force'        => sub { $force_generation = 0; },
      'f|force-missing'  => \$force_missing,
      'a|add-missing'   => \$add_missing,
      'c|copy'          => \$copy_missing,
      'v|verbose'       => sub { setup_channel 'verb', silent => 0; },
-     'W|warnings=s'     => \&parse_warnings,
+     'W|warnings=s'     => address@hidden,
      );
   use Getopt::Long;
   Getopt::Long::config ("bundling", "pass_through");
@@ -8502,6 +8502,15 @@ sub parse_arguments ()
   Getopt::Long::GetOptions %cli_options, 'version' => sub {}, 'help' => sub {}
     or exit 1;
 
+  set_strictness ($strict);
+  my $cli_where = new Automake::Location;
+  set_global_option ('cygnus', $cli_where) if $cygnus;
+  set_global_option ('no-dependencies', $cli_where) if $ignore_deps;
+  for my $warning (@warnings)
+    {
+      &parse_warnings ('-W', $warning);
+    }
+
   return unless @ARGV;
 
   if ($ARGV[0] =~ /^-./)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index cee1fee..0751589 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -935,6 +935,7 @@ vtexi2.test \
 warnings-override.test \
 warnings-precedence.test \
 warnopts.test \
+warnings-win-over-strictness.test \
 werror.test \
 werror2.test \
 werror3.test \
diff --git a/tests/Makefile.in b/tests/Makefile.in
index 41a713d..481a101 100644
--- a/tests/Makefile.in
+++ b/tests/Makefile.in
@@ -1198,6 +1198,7 @@ vtexi2.test \
 warnings-override.test \
 warnings-precedence.test \
 warnopts.test \
+warnings-win-over-strictness.test \
 werror.test \
 werror2.test \
 werror3.test \
diff --git a/tests/gnuwarn.test b/tests/gnuwarn.test
index 9c8aeb4..dffed49 100755
--- a/tests/gnuwarn.test
+++ b/tests/gnuwarn.test
@@ -1,5 +1,5 @@
 #! /bin/sh
-# Copyright (C) 2002, 2003  Free Software Foundation, Inc.
+# Copyright (C) 2002, 2003, 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
@@ -20,6 +20,9 @@
 
 set -e
 
+# We need (almost) complete control over automake options.
+AUTOMAKE="$original_AUTOMAKE -Werror"
+
 cat >> configure.in << 'END'
 AC_PROG_CC
 AC_OUTPUT
@@ -40,12 +43,14 @@ END
 
 $ACLOCAL
 # Don't warn in foreign mode
-$AUTOMAKE -Wnone --add-missing --foreign
+$AUTOMAKE --add-missing --foreign
 # Warn in gnu mode
-AUTOMAKE_fails -Wnone --add-missing --gnu
-grep 'Makefile.am:1:.*CFLAGS' stderr
-grep 'Makefile.am:2:.*LDFLAGS' stderr
+AUTOMAKE_fails --add-missing --gnu
+grep '^Makefile\.am:1:.*CFLAGS' stderr
+grep '^Makefile\.am:2:.*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
+$AUTOMAKE --gnu -Wno-gnu
+
+:
diff --git a/tests/warnings-win-over-strictness.test 
b/tests/warnings-win-over-strictness.test
new file mode 100755
index 0000000..ef42c4f
--- /dev/null
+++ b/tests/warnings-win-over-strictness.test
@@ -0,0 +1,54 @@
+#! /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, on the command line, explicitly-defined warnings take
+# precedence over implicit strictness-implied warnings.
+
+. ./defs || Exit 1
+
+set -e
+
+# We want complete control over automake options.
+AUTOMAKE=$original_AUTOMAKE
+
+ok ()
+{
+  AUTOMAKE_run 0 $*
+  test ! -s stderr
+}
+
+ko ()
+{
+  AUTOMAKE_run 0 $*
+  grep '^Makefile\.am:.*:=.*not portable' stderr
+  test `wc -l <stderr` -eq 1
+}
+
+# Files required in gnu strictness.
+touch README INSTALL NEWS AUTHORS ChangeLog COPYING
+
+cat > Makefile.am <<END
+FOO := bar
+END
+
+$ACLOCAL
+
+ko --foreign -Wportability
+ko -Wportability --foreign
+ok --gnu -Wno-portability
+ok -Wno-portability --gnu
+
+:
-- 
1.7.2.3