Re: [PATCH 4/9] Warnings win over strictness in AM_INIT_AUTOMAKE.

[Stefano Lattarini]

On Wednesday 12 January 2011, Ralf Wildenhues wrote:
> Here comes my (belated, as always) re-review; I am not repeating nits
> already dealt with before.
> 
> * Stefano Lattarini wrote on Wed, Jan 05, 2011 at 08:22:13PM CET:
> > On Wednesday 05 January 2011, Ralf Wildenhues wrote:
> > > * Stefano Lattarini wrote on Tue, Jan 04, 2011 at 06:38:04PM CET:
> > > > This is derived from [PATCH 07/10] of the older series.
> > > > It requires a review.
> 
> > > > Warnings win over strictness in AM_INIT_AUTOMAKE.
> > > > 
> > > > This change ensures that, for what concerns the options specified
> > > > in AM_INIT_AUTOMAKE,  explicitly-defined warnings always take
> > > > precedence over implicit strictness-implied warnings.  Related to
> > > > Automake bug#7669 a.k.a. PR/547.
> 
> > > > * lib/Automake/Options.pm (_process_option_list): Parse explicit
> > > > warnings only after the strictness level has been set.
> > > > * tests/warnings-win-over-strictness.test: Extend.
> > > 
> > > > --- a/lib/Automake/Options.pm
> > > > +++ b/lib/Automake/Options.pm
> 
> > > > @@ -313,11 +314,7 @@ sub _process_option_list (\%$@)
> > > >         }
> > > >        elsif (/^(?:--warnings=|-W)(.*)$/)
> > > >         {
> > > > -         foreach my $cat (split (',', $1))
> > > > -           {
> > > > -             msg 'unsupported', $where, "unknown warning category 
> > > > `$cat'"
> > > > -               if switch_warning $cat;
> > > > -           }
> > > > +         push @warnings, split (',', $1);
> > > >         }
> > > >        else
> > > >         {
> > > > @@ -326,6 +323,14 @@ sub _process_option_list (\%$@)
> > > >           return 1;
> > > >         }
> > > >      }
> > > > +  # We process warnings here, so that any explicitly-given warning 
> > > > setting
> > > > +  # will take precedence over warning settings defined implicitly by 
> > > > the
> > > > +  # strictness.
> > > 
> > > Well, this works in the current code base, but only by accident: namely,
> > > only because process_option_list is only ever called once, and with all
> > > options at once.
> > >
> > Hmm... no, it's potentially called many times in `handle_options()'.
> > But the later [PATCH 7/9] takes care of this.
> 
> But that's exactly what I mean: you need patch 7/9 precisely because
> you have changed the requirements that you push onto callers of
> process_*options_list.  Let me show you what I mean: with your patch
> series, you would also need something like the following:
> 
> To this POD text in Options.pm:
> 
> | =item C<process_option_list ($where, @options)>
> | 
> | =item C<process_global_option_list ($where, @options)>
> | 
> | Process Automake's option lists.  C<@options> should be a list of
> | words, as they occur in C<AUTOMAKE_OPTIONS> or C<AM_INIT_AUTOMAKE>.
> 
> you would need to add the following:
> 
>   These functions should be called at most once for each set of options
>   having the same precedence; i.e., do not call it twice for two options
>   from C<AM_INIT_AUTOMAKE>.
> 
> It is important for maintainability that the POD documentation is
> correct.
>
Agreed on this.

> And actually, now that we see the requirement printed, it is clear that
> this is not a good move: the API has suddenly become less easy to use,
>
Well, before it was easier to use, but wrong.  So I think this would
be a good enough move indeed (not great or perfect, but good enough).

> because it is easier for the callers of process_*option_list to use it
> wrongly.  http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
> 
> One better API would be one that either
>
> - checks that options of the same precedence are not split up when
>   passed, or
>
This check would indeed be useful, and I think the patch could be easily
amended to add it.  In fact, this wouldn't create a new API, but just
add some sanity checks to the old one (+1 from me thus).

> - computes the set of active warnings only strictly after all options
>   (of a certain precedence) have been processed.
>
This seems even cleaner in the long run, but it can come later IMVHO,
if and when the necessity arises.

> And upon rereading the code, it is also quite clear where the other
> reported precedence bugs come from: the global_options list is abused
> for two sets of options: those from AM_INIT_AUTOMAKE, which should have
> lower precedence than AUTOMAKE_OPTIONS, and those from the command line,
> which should have higher precedence than AUTOMAKE_OPTIONS.  The early
> conflation made correct precedence semantics impossible.  The right way
> here would be to have three stores of options.
>
I agree, but I contend that implementing this should be a prerequisite
to the fix of (after all pretty simple) "-Wportability --foreign" bug.

> And now that also makes it clear why the automake code printed some of
> the options as arguments in the rebuild rule for Makefile.in: because
> precedence was botched, that was needed in order to avoid these options
> to get lost.  Or so at least I believe.
>
How so?  The precedence of the command-line options is even lower than
that of the AM_INIT_AUTOMAKE options ...

> Hmm, set_strictness has some of the same problems, except one
> complication is that some warnings may apply already at global
> level, before any Makefile.am file is even opened.  I'm guessing
> three sets are needed here too.
>
In the long run, yes.  I was thinking about that myself, but before
embarking in such a non-obvious refactoring I'd like to see the queue
of pending patches flushed a bit.  Not to mention that there are some
user-visible bug that would be nice to fix first (Lex/Yacc support).
 
> So, now with that said, I'm not sure whether I should approve this
> patch.  What do you think?
>
I think that you should, provided that I add the sanity check you
suggested above.

> > >  If some code later calls it like
> > >   process_option_list (first-set-of-options);
> > >   process_option_list (second-set-of-options);
> > > 
> > > then things will go wrong again.  I suspect that it will mean that
> > >   AM_INIT_AUTOMAKE([foreign -Wno-portability])
> > >   AUTOMAKE_OPTIONS = gnu
> > > 
> > > won't do what we want.  Hmm.  What exactly is it that we want to happen
> > > in this case?  Should gnu override -Wno-portability if specified in a
> > > (to-be) higher order place?
> > >
> > I assumed without saying that yes, this was to be the intended behaviour.
> > And I still think it should be.  Sorry for not having been explicit about
> > that before.
> 
> I agree that it should be, but this, too, should be documented (in
> autoconf.texi
>
Yes, I've already agreed about this before.

> and maybe also NEWS)
>
I disagree on this, as that behaviour was already in place before.

> and tested, when it works.
>
Hmmm... I thought it was already tested somewhere.  If it's not, I
agree we should add a test.

> > > I see two ways out: warnings are only switched after all options are
> > > processed.
> > >
> > This is not good IMO, as it breaks usages like the the one in your
> > example above.
> 
> Well, as an alternative for the requirement above would be to only
> process warnings in a separate function compute_warning_set, once
> all options coming from a given source (AM_INIT_AUTOMAKE) have been
> set.
> That would be a bit more flexible.
>
Again, agreed -- but in the long run.
 
> > > Or: process_option_list is documented to require *all*
> > > options.  The latter seems fragile.
> > >
> > Still, currently this is the best option IMVHO.  And in fact, the later
> > [PATCH 7/9] works exactly by ensuring that `handle_options()' calls
> > `process_option_list()' only once (this is the reason the preliminary
> > PATCH [6/9] "Change signature of Automake::Options::_process_option_list"
> > is required).
> 
> See above.
> 
> > > Either way though, we need a consistent definition (and documentation)
> > > of intended semantics, both internally of process_option_list and user
> > > side of option handling semantics.
> > >
> > Agreed on the user-side documentation (which can be done in a follow-up
> > patch though).  But I don't think that internal documentation is really
> > required; nor can I think about how to formulate it (suggestions?)
> 
> See above.
>
Agreed.


Regards,
   Stefano