Re: [PATCH] dist-xz, dist-bzip2: don't hard-code -9: honor envvar settings

[Jim Meyering]

Stefano Lattarini wrote:
> There are few problems with it though, that I've fixed.
> More details below.

Hi Stefano,

Thanks for the thorough review and for correcting my errors.

> On Friday 09 December 2011, Jim Meyering wrote:
>> Today I noticed that automake-generated Makefile's dist-xz rule
>> used a hard-coded xz -9.  That is a problem because on this front, xz
>> differs from gzip and bzip2.  While the latter two incur no run-time
>> *decompression* penalty for using a higher compression level, with xz
>> if you specify -9, that imposes a potentially fatal virtual-memory
>> requirement on any client that wants to decompress your tar.xz file.
>> People have complained that a tarball compressed with -9 cannot
>> be uncompressed in a low-memory environment (wrt-based embedded).
>>
>> Hence, instead of defaulting to -9, which is useful only
>> for very large tarballs, it defaults to -e (equivalent to -6e).
>> This limits the default memory requirements imposed on decompressors,
>> yet still gives very good compression ratios.
>>
> Thanks for the detailed explanation.  Unfortunately, you have forgotten
> to put it into the ChangeLog entry and git commit message, where IMO it
> belongs; I've taken the liberty of doing so on your behalf.

Habits, habits.
I'm too used to *not* duplicating git log information in a VC'd
ChangeLog file that for the few projects that do require the
duplication I tend to forget that -- esp. when it's late and
I'm out of time.

>> Subject: [PATCH] dist-xz, dist-bzip2: don't hard-code -9: honor envvar
>>  settings
>>
>> This is a cherry-pick of commit 6da46f31, with additional changes to
>> reflect that the xz compression level should default to -e, not -9.
>>
>> * lib/am/distdir.am (dist-xz): Do not hard-code xz's -9: that made
>> it impossible to override.  Actually don't default to -9, either,
>> since that induced inordinately large virtual memory usage when
>> merely decompressing.  Instead, use its XZ_OPT envvar, defaulting
>> to -e if not defined.  Suggested by Lasse Collin.
>> (dist-bzip2): Similarly, do not hard-code -9, but do continue to
>> use -9 by default.  Honor the BZIP2 envvar.
>> * NEWS (Miscellaneous changes): Mention it.
>> * doc/automake.texi (The Types of Distributions): Describe the
>> newly enabled environment variables.
>> ---
>>  ChangeLog         |   13 +++++++++++++
>>  NEWS              |    5 +++++
>>  doc/automake.texi |    9 +++++++++
>>  lib/am/distdir.am |    4 ++--
>>  4 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/ChangeLog b/ChangeLog
>> index 1aba97a..41fa3bb 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,16 @@
>> +2010-10-05  Jim Meyering  <address@hidden>
>> +
> Oops, wrong date.  Fixed.

Another reason not to VC ChangeLog files.

> Also, I've added myself as secondary author, since I've contributed few
> non-trivial amendings (see below).  I hope you're ok with that.

Yes, of course.

>> +    dist-xz, dist-bzip2: don't hard-code -9: honor envvar settings
>> +    * lib/am/distdir.am (dist-xz): Do not hard-code xz's -9: that
>> +    made it impossible to override.  Instead, use its XZ_OPT envvar,
>> +    defaulting to -9 if not defined.  Thus no change in behavior
>> +    when XZ_OPT is not set, and now, this rule honors the setting
>> +    of that envvar when it is set.  Suggested by Lasse Collin.
>> +    (dist-bzip2): Likewise for it's corresponding envvar: BZIP2.
>> +    * NEWS (Miscellaneous changes): Mention it.
>> +    * doc/automake.texi (The Types of Distributions): Describe the
>> +    newly enabled environment variables.
>> +
>>
> Oops, you haven't update the ChangeLog entry, and it is not out-of-sync
> with the code changes and the (correct) git commit message.  I've fixed
> this.

I have not taken the time to write a commit hook to warn me when
a git log fails to match  the corresponding ChangeLog delta.
It doesn't seem worthwhile.

...
> You have forgotten to update the recipes for `dist' and `dist-all':
>
>   $ grep -e -9 lib/am/distdir.am
>           tardir=$(distdir) && $(am__tar) | BZIP2=$${BZIP2--9} bzip2 -c 
> >$(distdir).tar.bz2
>           tardir=$(distdir) && $(am__tar) | lzma -9 -c >$(distdir).tar.lzma
>   ?BZIP2? tardir=$(distdir) && $(am__tar) | bzip2 -9 -c >$(distdir).tar.bz2
>   ?LZMA?  tardir=$(distdir) && $(am__tar) | lzma -9 -c >$(distdir).tar.lzma
>   ?XZ?    tardir=$(distdir) && $(am__tar) | xz -9 -c >$(distdir).tar.xz

Good catch.

> I've fixed this "the dumb way", i.e., by copying & pasting.  Extra
> points will go to anyone which comes up with a refactoring patch that
> removes this annoying code duplication.

That looks fine for now.

> And a final nit: you have forgotten to run ./boostrap to regenerate
> Automake's own Makefile.in (sorry, theis is still version-controlled
> for now!).  I've fixed that as well.
>
> Attached is what I've pushed to maint (and merged into branch-1.11).
>
> Thanks,
>   Stefano
> From c8e01d581a7e7c2445a139def46939a547951746 Mon Sep 17 00:00:00 2001
> Message-Id: <address@hidden>
> From: Jim Meyering <address@hidden>
> Date: Fri, 9 Dec 2011 23:17:18 +0100
> Subject: [PATCH] dist-xz, dist-bzip2: don't hard-code -9, honor envvar 
> settings
>
> Before the present change, automake-generated `dist-xz' rule used
> a hard-coded `xz -9'.  That was a problem because on this front,
> xz differs from gzip and bzip2.  While the latter two don't incur
> any run-time decompression penalty for using a higher compression
> level, specifying -9 with xz imposes a potentially fatal virtual
> memory requirement on any client that wants to decompress your
> tar.xz file.
> People have complained that a tarball compressed with -9 cannot
> be uncompressed in a low-memory environment (wrt-based embedded).
> Hence, instead of defaulting to -9, which is useful only for very
> large tarballs, it defaults to -e (equivalent to -6e).  This
> limits the default memory requirements imposed on decompressors,
> yet still gives very good compression ratios.
>
> * lib/am/distdir.am (dist-xz): Do not hard-code xz's -9: that made
> it impossible to override.  Actually don't default to -9, either,
> since that induced inordinately large virtual memory usage when
> merely decompressing.  Instead, use its XZ_OPT envvar, defaulting
> to -e if not defined.  Suggested by Lasse Collin.
> (dist, dist-all) [?XZ?]: Likewise
> (dist-bzip2): Similarly, do not hard-code -9, but do continue to
> use -9 by default.  Honor the BZIP2 envvar.
> (dist, dist-all) [?BZIP2?]: Likewise
> * NEWS: Update.
> * doc/automake.texi (The Types of Distributions): Describe the
> newly enabled environment variables.
>
> This is inspired to commit v1.11-389-g6da46f3, with additional

s/inspired to/inspired by/

And in commit logs I like to use the 8-hex-digit SHA1
representation (perhaps in addition to something like you've
provided) because gitk automatically detects that and renders
it as a clickable link.

I admit that in a non-interactive rendering of the log,
your longer form is much more useful.

> changes to reflect that the xz compression level should default
> to -e, not -9.