Re: [Automake-NG] [PATCH 0/3] dist: add back support for obsolete dist-* options

[Stefano Lattarini]

On 08/22/2012 11:41 AM, Paolo Bonzini wrote:
> Hi Stefano,
>
Hi Paolo, and thanks for the patches.

> these patches add back support for obsolete dist-* options, by parsing
> the AUTOMAKE_OPTIONS variable and distilling AM_DIST_FORMATS out of it.
> 
> The first two patches are required for this.  They provide a merger of
> configure.ac and Makefile.am options in the Makefile.in's AUTOMAKE_OPTIONS.
>
I find the idea of making the options given in AM_INIT_AUTOMAKE available
also at make runtime very good, and much in the spirit of Automake-NG ("let
make, not automake, do the processing"), even independently from the use
case at hand.  So thanks for brining this up.

But while I like the general direction of the series, I don't like the
proposed implementation very much  So I have outlined a counter-proposal
below.  Let me know if you can re-roll your series to take it into
account, or if I should do that (the latter will of course take more
time, but should be done in a week or so).

> They could be suited for Automake mainline too, though the use case is
> specific to Automake-NG.
>
I'd rather do the enhancement to Automake-NG only; no need to tinker
with the mainline codebase for a change is not going to offer any real
improvement there, and whose lack in mainline is not going to cause
extra troubles or conflicts when we merge back into Automake-NG.

On 08/22/2012 11:41 AM, Paolo Bonzini wrote:
>
> [PATCH 1/3] var: add VAR_COMPUTED source
>
> We want AUTOMAKE_OPTIONS in Makefile.in to be the combination of
> configure.ac and Makefile.am options.
>
>  Define a new variable owner
> for this, because we need to override the Makefile.am value
> unconditionally and never emit warnings.
>
> * lib/Automake/VarDef.pm (VAR_COMPUTED): New.
> (dump): Print it.
> * lib/Automake/Variable.pm (define): Check that VAR_COMPUTED variables
> were not conditionally defined, and always allow them to override
> previous definitions.
>
Hmm... So, with this patch, we would define a new variable type (quite
bad IMHO, as it adds new complexity); and then ...

> [PATCH 2/3] var: format all options in the Makefile.in output
>
> Make sure that Makefile.in includes all options, both from
> configure.ac and from Makefile.am.  This is useful to further
> process options at Make execution time via GNU Make functions.
>
> * automake.in (handle_options): Redefine AUTOMAKE_OPTIONS as a
> VAR_COMPUTED variable.
> * lib/Automake/Options.pm ($_version_regex): New global.
> (_process_option_list): Use it.
> (format_options): New function.
>
... with this later patch, we would munge and rewrite a user-defined
variable.  Which I'd usually see as very bad.  OK, not so much in
this case, because I don't see a user ever using the expansion of
$(AUTOMAKE_OPTIONS); but still suspicious and bad-looking.

So here is my counter-proposal: we simply define a new internal variable
'am.automake-options' instead; it will contain the merged version
of the AUTOMAKE_OPTIONS and AM_INIT_AUTOMAKE entries.  Not only this
would simplify the implementation (removing the need for a new variable
type), but would also avoid adding extra differences between mainline
Automake (which doesn't rewrite AUTOMAKE_OPTIONS) and Automake-NG (which
would start rewriting it with your unmodified patches).

WDYT?

BTW, I have other minor nits, but I'd rather not conflate them with
this more serious objection; I will bring them up once (and if) a
re-roll is ready.

Thanks,
  Stefano