Re: [Automake-NG] [PATCH v2 1/2] var: format all options in the Makefile.in output

[Stefano Lattarini]

Hi Paolo.

After reading this patch carefully, I realize the issue is more complicated
and tricky than it appeared at first.  To avoid getting stuck here, I think
we should re-work your other patch to be independent from this one (it can
be done quite easily, see my reply to that patch).  Once that is done, and
the feature we care about is thus implemented, we can tackle this issue
with all the due carefulness.

Preliminary observations, nits and objections follow.

On 08/22/2012 12:44 PM, Paolo Bonzini wrote:
>
> 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 not an accurate description of what the patch does IMHO.
I'd suggest something like this instead:

    vars: new internal make variable $(am.automake-options)

    It will contain a "merged" version of the list of options from both
    AUTOMAKE_OPTIONS an AM_INIT_AUTOMAKE.  This will be useful to further
    process options at Make execution time via GNU Make functions.

> * automake.in (handle_options): Define am.automake-options.
> * lib/Automake/Options.pm ($_version_regex): New global.
> (_process_option_list): Use it.
> (format_options): New function.
> ---
>  automake.in             |  1 +
>  lib/Automake/Options.pm | 43 +++++++++++++++++++++++++++++++--
>  t/option-out.sh         | 63 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 file modificati, 105 inserzioni(+), 2 rimozioni(-)
>  create mode 100755 t/option-out.sh
> 
> diff --git a/automake.in b/automake.in
> index 1799875..96873f4 100644
> --- a/automake.in
> +++ b/automake.in
> @@ -1190,6 +1190,7 @@ sub handle_options
>        set_option ('check-news', INTERNAL);
>      }
>  
> +  define_variable ('am.automake-options', INTERNAL, format_options ());
>    return 0;
>  }
>  
> diff --git a/lib/Automake/Options.pm b/lib/Automake/Options.pm
> index 77e9cbd..8dd8176 100644
> --- a/lib/Automake/Options.pm
> +++ b/lib/Automake/Options.pm
> @@ -28,7 +28,7 @@ use vars qw (@ISA @EXPORT);
>  @ISA = qw (Exporter);
>  @EXPORT = qw (option global_option
>                set_option set_global_option
> -              unset_option unset_global_option
> +              unset_option unset_global_option format_options
>                process_option_list process_global_option_list
>                set_strictness $strictness $strictness_name
>                &FOREIGN &GNU &GNITS);
> @@ -81,6 +81,7 @@ use vars '$_options_processed';
>  # Whether process_global_option_list has already been called.
>  use vars '$_global_options_processed';
>  
> +use vars '$_version_regex';
>  =head2 Constants
>  
>  =over 4
> @@ -245,6 +246,14 @@ Return 1 on error, 0 otherwise.
>  
>  =cut
>  
> +=item C<format_options ()>
> +
> +Formats the combination of the current global and local options,
> +returning the result as a string.  The version options are
> +replaced by the version of Automake that produced the Makefile.
> +
> +=cut
> +
>  # _option_must_be_from_configure ($OPTION, $WHERE)
>  # ----------------------------------------------
>  # Check that the $OPTION given in location $WHERE is specified with
> @@ -336,7 +345,7 @@ sub _process_option_list (\%@)
>                last;
>              }
>          }
> -      elsif (/^\d+\.\d+(?:\.\d+)?[a-z]?(?:-[A-Za-z0-9]+)?$/)
> +      elsif ($_ =~ $_version_regex)
>          {
>            # Got a version number.
>            if (Automake::Version::check ($VERSION, $&))
> @@ -370,6 +379,35 @@ sub _process_option_list (\%@)
>    return 0;
>  }
>  
> +sub format_options()
> +{
> +  my @out = ($VERSION, 'ng');
> +
> +  foreach (keys %_options)
> +    {
> +      if ($_ eq 'filename-length-max')
> +        {
> +          push @out, $_options{$_}->[1];
> +        }
>
Hmm... shouldn't this be

    push @out, "$_=$_options{$_}->[1]";

instead?  Surely something to be covered in the test case ...

> +      elsif ($_ =~ $_version_regex || $_ eq 'ng' )
> +        {
> +          # AUTOMAKE_OPTIONS in Makefile.in includes the actual version
> +          # number, not the required one.
> +          next;
> +        }
> +      elsif (/^(?:--warnings=|-W)(.*)$/)
> +        {
> +          next;
>
OK.  If we decide to make them accessible at Make runtime, they should go
in another variable, like 'am.enabled-warnings' or so.  This is something
for another patch series though.

> +        }
> +      else
> +        {
> +          push @out, $_;
> +        }
> +    }
> +
> +  return join (' ', @out);
> +}
> +
>  sub process_option_list (@)
>  {
>    prog_error "local options already processed"
> @@ -418,6 +456,7 @@ sub set_strictness ($)
>      }
>  }
>  
> +$_version_regex = qr/^\d+\.\d+(?:\.\d+)?[a-z]?(?:-[A-Za-z0-9]+)?$/;
>
Don't you think this would be better moved in the Automake::Version
module instead?  Not a prerequisite for an ACK though, more a "note
to self", and something to consider for a follow-up patch.

> diff --git a/t/option-out.sh b/t/option-out.sh
> new file mode 100755
> index 0000000..89784df
> --- /dev/null
> +++ b/t/option-out.sh
> @@ -0,0 +1,63 @@
> +#! /bin/sh
> +# Copyright (C) 2012 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/>.
> +
> +# Automake-NG should set Makefile.in's am.automake-options variable to
> +# the full set of options, from both configure and Makefile.am.  It
> +# should also include the current version and the "ng" token.
> +
> +. ./defs || exit 1
> +
> +cat > configure.ac <<END
> +AC_INIT([$me], [1.0])
> +AM_INIT_AUTOMAKE([gnu foreign])
> +AC_CONFIG_FILES([Makefile])
> +END
> +
> +: > Makefile.am
> +
> +$ACLOCAL
> +$AUTOCONF
> +$AUTOMAKE
>
> +grep automake-options.*=.*ng Makefile.in
> +grep automake-options.*=.*gnu Makefile.in
> +grep automake-options.*=.*foreign Makefile.in
>
Today, we strongly prefer proper semantic checks over grepping ones.
That is true for mainline Automake, but even more for Automake-NG,
as it has been changing the form and contents of the generated
Makefile.in dramatically.

So, here you would do something like this:

    cat > Makefile.am << 'END'
    test:
            is $(am.automake-options) == 'ng gnu foreign'
    END

    $ACLOCAL
    $AUTOCONF
    $AUTOMAKE
    ./configure
    $MAKE test

Refer to the e.g., tests 't/cond31.sh' and 't/cond32.sh' for
examples.

Also, and an aside, since the 'gnu' and 'foreign' options cancel one
another (with the last one "winning"), shouldn't only that winning
one be listed in $(am.automake-options)?  Otherwise, we have no easy
way to decide which the strictness actually is ...

Maybe the safest and more future-proof solution would be to set aside
a "namespace" rather than a single variable for the options, and store
each option in a dedicated variable.  So that, upon an input like:

    configure.ac:
      AM_INIT_AUTOMAKE([gnu])
    Makefile.am:
      AUTOMAKE_OPTIONS = filename-length-max=120 no-installman
    sub/Makefile.am:
      AUTOMAKE_OPTIONS = serial-tests foreign

in Makefile.in we would have the following definitions:

    am.opts.parallel-tests = yes
    am.opts.strictness = gnu
    am.opts.installman =

and in sub/Makefile.in we would have the following ones:

    am.opts.serial-tests = yes
    am.opts.strictness = foreign
    am.opts.installman = yes

You get the gist I guess ...

> +
> +# Test combination of configure and Makefile.am values
> +echo AUTOMAKE_OPTIONS = serial-tests > Makefile.am
> +$AUTOMAKE
> +grep automake-options.*=.*ng Makefile.in
> +grep automake-options.*=.*gnu Makefile.in
> +grep automake-options.*=.*foreign Makefile.in
> +grep automake-options.*=.*serial-tests Makefile.in
> +
> +# Test concatenation of Makefile.am values
> +echo AUTOMAKE_OPTIONS += color-tests >> Makefile.am
> +$AUTOMAKE
> +grep automake-options.*=.*ng Makefile.in
> +grep automake-options.*=.*gnu Makefile.in
> +grep automake-options.*=.*foreign Makefile.in
> +grep automake-options.*=.*serial-tests Makefile.in
> +grep automake-options.*=.*color-tests Makefile.in
> +


> +# Conditional assignments must be rejected.
> +echo 'AM_CONDITIONAL([FOO], [true])' >> configure.ac
> +cat > Makefile.am <<END
> +if FOO
> +AUTOMAKE_OPTIONS = serial-tests
> +endif
> +END
> +
> +AUTOMAKE_fails
>
No need to: this is already tested in 't/amopt.sh'.

Regards,
  Stefano