[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