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

[Ralf Wildenhues]

* 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".)

The same applies to patches 04, 07, 08, and 09 of this series.

> +     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.

> +  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)?

> +  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.

>    return unless @ARGV;
>  
>    if ($ARGV[0] =~ /^-./)

> --- 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.

>  END
>  
>  # Needed by --gnu.
> @@ -40,12 +42,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
> +
> +:

> --- /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.

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

-eq

> +}
> +
> +# 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