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

[Paolo Bonzini]

Il 22/08/2012 13:47, Stefano Lattarini ha scritto:
> 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?

I prefer to wait for 1/2 to be sorted out.

> 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:

I wanted to minimize the Automake changes.

I'm a bit confused as to where to draw the line between Automake and GNU
make...

Paolo

>> 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
>