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

[Stefano Lattarini]

Hi Paolo.

Since I still have some gripes with the preparatory [PATCH 1/2], I'm
thinking about reworking this patch to make is independent from that.
Find my ideas below.  Do you think they would be a good move?  If
yes, would you mind re-working you patch accordingly?

On 08/22/2012 12:44 PM, Paolo Bonzini wrote:
> The old API for dist formats can be supported easily, by parsing the
> AUTOMAKE_OPTIONS and generating AM_DIST_FORMATS from it, if not defined
> otherwise.
> 
> * NG-NEWS: Document that the old distribution format API is obsolete
> but available.
> * lib/Automake/Options.pm: Degrade error to warning.
> * lib/am/distcheck.mk: Generate AM_DIST_FORMATS from AUTOMAKE_OPTIONS
> * t/dist-obsolete-opts.sh: Add backwards-compatibility tests.
> ---
>  NG-NEWS                 |  2 +-
>  lib/Automake/Options.pm |  8 ++++----
>  lib/am/distcheck.mk     |  4 +++-
>  t/dist-obsolete-opts.sh | 24 +++++++++++++++---------
>  4 file modificati, 23 inserzioni(+), 15 rimozioni(-)
> 
> diff --git a/NG-NEWS b/NG-NEWS
> index 129da68..8ee1aae 100644
> --- a/NG-NEWS
> +++ b/NG-NEWS
> @@ -228,7 +228,7 @@ Distribution
>    have been removed; and with them the targets 'dist-shar' and 'dist-tarZ'.
>  
>  * The API to specify the formats of distribution tarballs has been changed
> -  completely.
> +  completely; the old API is still available, but gives warnings.
>  
>    Instead of using the various 'dist-*' automake options, the developer is
>    now expected to specify the default formats of its distribution tarballs
> diff --git a/lib/Automake/Options.pm b/lib/Automake/Options.pm
> index 8dd8176..abf488c 100644
> --- a/lib/Automake/Options.pm
> +++ b/lib/Automake/Options.pm
> @@ -322,11 +322,11 @@ sub _process_option_list (\%@)
>          {
>            error $where, "support for Cygnus-style trees has been removed";
>          }
> -      elsif (/^(?:no-)?dist-.*/)
> +      elsif ($_ eq 'no-dist-gzip' || /^dist-.*/)
>          {
> -          error ($where,
> -                 "'$_' option and the like are no more supported;\n" .
> -                 "use AM_DIST_FORMATS in top-level Makefile.am instead");
> +          msg ('obsolete', $where,
> +                 "'$_' option and the like are obsolete; use\n" .
> +                 "AM_DIST_FORMATS in top-level Makefile.am instead");
>          }
>
Since we are still processing these options explicitly, we can easily know
which dist formats the user is requesting.  So, instead of having to recover
them from the 'dist-*' options at make runtime, we could register them right
away in an internal make variable, say '$(am.dist.formats-from-opts)';
and then, at make runtime, default AM_DIST_FORMATS to that.  Something like
this:

    sub _process_option_list (\%@)
    {
       ...
       my %dist_formats = (gzip => 1);
       ...
      elsif ($_ eq 'no-dist-gzip' || /^dist-(.*)/)
         {
           msg ('obsolete', $where,
                "'$_' option and the like are obsolete; use\n" .
                 "AM_DIST_FORMATS in top-level Makefile.am instead");
           if ($_ eq 'no-dist-gzip')
             {
               delete $dist_formats{'gzip'};
             }
           else
             {
               $dist_formats{$1} = 1;
             }
         }
    }

Then we should find a way to pass the keys of '%dist_formats' back
to Automake, and use it to define '$(am.dist.formats-from-opts)'.
Then ...

> diff --git a/lib/am/distcheck.mk b/lib/am/distcheck.mk
> index 421b15b..cbde58b 100644
> --- a/lib/am/distcheck.mk
> +++ b/lib/am/distcheck.mk
> @@ -69,7 +69,9 @@ am.dist.extract-cmd.zip = \
>  
>  # This is namespace-safe, so it's OK to accept values from
>  # the environment.
> -AM_DIST_FORMATS ?= gzip
> +AM_DIST_FORMATS ?= \
> +   $(patsubst dist-%, %, $(filter dist-%, $(am.automake-options))) \
> +   $(if $(filter no-dist-gzip, $(am.automake-options)),,gzip)
> 
... here we could simply do:

    AM_DIST_FORMATS ?= $(am.dist.formats-from-opts)

How does that sound?

-*-*-*-

Now, to some style/minor nits ...

>  am.dist.bad-targets := \
>    $(filter-out $(am.dist.all-formats),$(AM_DIST_FORMATS))
> diff --git a/t/dist-obsolete-opts.sh b/t/dist-obsolete-opts.sh
> index 06bea8d..018fc1c 100644
> --- a/t/dist-obsolete-opts.sh
> +++ b/t/dist-obsolete-opts.sh
> @@ -22,9 +22,9 @@ $ACLOCAL
>  
>  for fmt in gzip bzip2 xz lzip zip tarZ lzma shar; do
>    echo AUTOMAKE_OPTIONS = dist-$fmt > Makefile.am
> -  AUTOMAKE_fails -Wnone -Wno-error
> -  grep "^Makefile\\.am:1:.* 'dist-$fmt' option.* no more supported" stderr
> -  grep "^Makefile\\.am:1:.* use AM_DIST_FORMATS .*instead" stderr
> +  $AUTOMAKE -Wobsolete -Wno-error 2>stderr
> +  grep "^Makefile\\.am:1:.* 'dist-$fmt' option.* obsolete" stderr
> +  grep "^Makefile\\.am:1:.* AM_DIST_FORMATS .*instead" stderr
>
Here is the usual idiomatic way to check for warnings:

    AUTOMAKE_fails -Wnone -Wobsolete
    grep "^Makefile\\.am:1:.* 'dist-$fmt' option.* obsolete" stderr
    grep "^Makefile\\.am:1:.* AM_DIST_FORMATS .*instead" stderr
    $AUTOMAKE -Wno-obsolete

>  done
>  
>  rm -rf autom4te*.cache
> @@ -33,12 +33,18 @@ cat > configure.ac << 'END'
>  AC_INIT([foo], [1.0])
>  AM_INIT_AUTOMAKE([no-dist-gzip dist-xz])
>  AC_CONFIG_FILES([Makefile])
> +AC_OUTPUT
>  END
> -: > Makefile.am
> +: > foo
> +echo EXTRA_DIST = foo > Makefile.am
>  $ACLOCAL
> -AUTOMAKE_fails -Wnone -Wno-error
> -grep "^configure\\.ac:2:.* 'no-dist-gzip' option.* no more supported" stderr
> -grep "^configure\\.ac:2:.* 'dist-xz' option.* no more supported" stderr
> -grep "^configure\\.ac:2:.*use AM_DIST_FORMATS .*instead" stderr
> +$AUTOMAKE -Wobsolete -Wno-error 2>stderr
>
Please use:

   AUTOMAKE_run -Wobsolete -Wno-error

instead.  It will do the stder r capture for you.

> +grep "^configure\\.ac:2:.* 'no-dist-gzip' option.* obsolete" stderr
> +grep "^configure\\.ac:2:.* 'dist-xz' option.* obsolete" stderr
> +grep "^configure\\.ac:2:.* AM_DIST_FORMATS .*instead" stderr
>  
> -:
> +$AUTOCONF
> +./configure
> +$MAKE dist
>
This will require a working 'xz' program, so you should ensure is is present
before proceeding, to avoid spurious failures on systems lacking it.

    if xz --version; then
      $MAKE dist
      test -f foo-1.0.tar.xz
      test -f foo-1.0.tar.gz && exit 1
      : For shells with busted 'set -e'.
    fi

    :

Thanks,
  Stefano