bug-gzip
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#34918: [PATCH] Add support for IBM Z hardware-accelerated deflate


From: Ilya Leoshkevich
Subject: bug#34918: [PATCH] Add support for IBM Z hardware-accelerated deflate
Date: Wed, 3 Apr 2019 11:35:59 +0200

Hello Paul,

thanks for improving the patch!

> * Would it be better to have --enable-dfltc be the default on develoment
> platforms that have the support?

The current plan is to enable this on a per-distro basis.

> * Why was the change to tests/znew-k needed?

If I understand correctly, znew -K compares whether the new file is
smaller than the old one, and keeps the old one if this is not the
case.  The znew-k test expects that the replacement will happen, 
however, when DFLTCC is used, it does not.  The description of the
test states: "Check that znew -K works without compress(1)“, so I assume
whether or not the file is replaced is irrelevant w.r.t. the main point
of the test.  Therefore I changed it to accept both outcomes.

> * What is the significance of the SOURCE_DATE_EPOCH environment
> variable? Should that getenv be removed?

Unfortunately DFLTCC can produce different (but valid!) outputs for the
same input when called multiple times.  This may pose a problem for
reproducible builds, so ideally when gzip is used for building software,
it should use software compression.  SOURCE_DATE_EPOCH is an environment
variable that instructs build tools to use a given timestamp instead
of the current time [1] and I think nowadays it’s reasonable to expect
that whenever reproducibility is expected, then this variable will be
set.  For example, Python looks at it when building the bytecode [2].

> * Should the other new environment variables be documented?

Yes, I think so.  Should this go to gzip.1 file?

> * For new code it's better to use GNU style and assume now-ubiquitous
> C99 constructs so I installed the attached further patch to do that. I
> haven't tested it since I don't have access to the relevant hardware;
> please give it a try when you have the chance.

I have verified that the test suite passes with your changes.

> * Anonymous structures and unions are a C11 feature. Currently we are
> assuming only C99, so the attached patch redoes 'struct context' to omit
> the anonymous union.
> 
> * It's more portable to use C11-style 'alignas (8)' instead of
> GCC-specific '__attribute__ ((aligned (8)))'. The Gnulib stdalign module
> supports this under C99. I gave that a whirl in the attached patch.
> 
> * Should have a NEWS entry; I added one in the attached patch.
> 
> * I didn't understand why that '(void)blocksize;' needed to be there, so
> I removed it; if it's needed please explain.

This must be a leftover that I have missed when cleaning up the code.

[1] https://reproducible-builds.org/docs/source-date-epoch/
[2] https://docs.python.org/3/library/py_compile.html#py_compile.compile

Best regards,
Ilya





reply via email to

[Prev in Thread] Current Thread [Next in Thread]