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
