Pádraig Brady wrote:
> Latest gnulib is failing coreutils CI because of a mismatch in mkdir && cd
> due to prefixes.
> In the generated Makefile I'm seeing: $(MKDIR_P) lib/crc-tmp
> I.e. MKDIR_P has been processed to add in the lib prefix.
> However the subsequent `cd crc-tmp` doesn't have the pre
On 31/10/2024 13:51, Bruno Haible via Gnulib discussion list wrote:
Simon Josefsson wrote:
I merged this now, thank you!
And here's the cross-compilation handling that I promised to add.
2024-10-31 Bruno Haible
crc: Don't attempt to run a compiled C program when cross-compiling.
gzip just took my patch to initialise MAINTAINERCLEANFILES, just built from
HEAD with gnulib pulled from HEAD, everything looking good
make check
Testsuite summary for gzip 1.13.36-46e9-dirty
I wrote:
> And here's the cross-compilation handling that I promised to add.
>
>
> 2024-10-31 Bruno Haible
>
> crc: Don't attempt to run a compiled C program when cross-compiling.
Here's an improvement:
2024-10-31 Bruno Haible
crc: Support generating the tables also when
On 31/10/2024 18:35, Sam Russell wrote:
> `cksum -a crc32` could be added I suppose to select the current
implementation in gnulib
both versions are CRC32 though, and then if you look at the iSCSI/SCTP version
they use CRC32-C which uses a totally different polynomial, not just a reversed
or
On Thu, Oct 31, 2024 at 5:15 PM Simon Josefsson via Gnulib discussion
list wrote:
>
> Pádraig Brady writes:
>
> > I'd be reluctant to complicate the gnulib implementations with variants
> > unless needed.
> > I'd be especially reluctant to add the existing cksum crc variant to gnulib,
> > since
On 31/10/2024 21:15, Simon Josefsson wrote:
Pádraig Brady writes:
I'd be reluctant to complicate the gnulib implementations with variants unless
needed.
I'd be especially reluctant to add the existing cksum crc variant to gnulib,
since I don't see anything but cksum itself using that.
Is th
Pádraig Brady writes:
> I'd be reluctant to complicate the gnulib implementations with variants
> unless needed.
> I'd be especially reluctant to add the existing cksum crc variant to gnulib,
> since I don't see anything but cksum itself using that.
Is there some performance optimization in cor
Simon Josefsson wrote:
> While CROSS_COMPILING is a common variable used for this purpose, this
> is bad namespace usage -- how about GL_CRC_CROSS_COMPILING,
> GL_CROSS_COMPILING or GNULIB_CROSS_COMPILING instead?
Indeed, a search on codesearch.debian.net reveals that a number of
packages use this
> `cksum -a crc32` could be added I suppose to select the current
implementation in gnulib
both versions are CRC32 though, and then if you look at the iSCSI/SCTP
version they use CRC32-C which uses a totally different polynomial, not
just a reversed order one like gzip
> I'd be especially reluct
On 31/10/2024 16:46, Sam Russell wrote:
I can extend coreutils cksum if you like, what are your thoughts on adding some
parameters to the commandline? Even if it's just an extra flag for --rfc1952 or
--gzip to change all the parameters just between these two variants?
Well cksum already has t
I can extend coreutils cksum if you like, what are your thoughts on adding
some parameters to the commandline? Even if it's just an extra flag for
--rfc1952 or --gzip to change all the parameters just between these two
variants?
On Thu, Oct 31, 2024, 17:41 Sam Russell wrote:
> These are all solv
These are all solvable:
Polynomial: technically the same but one could argue it's the reversed
version (we reverse the polynomial so we can read LSB first). We now
generate the tables at compile time so this could be a parameter to table
generation
Initial/final value: easy enough to configure at
On 31/10/2024 12:18, Simon Josefsson via Gnulib discussion list wrote:
I merged this now, thank you!
FYI, I looked at using this from coreutils cksum,
but unfortunately that uses a different CRC-32 variant.
For my reference...
coreutils cksum parameters:
Polynomial: 0
Bruno Haible via Gnulib discussion list writes:
> Simon Josefsson wrote:
>> I merged this now, thank you!
>
> And here's the cross-compilation handling that I promised to add.
Both this and the other one looks fine, thank you!
> configure.ac:
> AC_REQUIRE([gl_CRC_SLICE_BY_8])
> +CROSS_COMPILI
Bruno Haible via Gnulib discussion list writes:
> Sam Russell wrote:
>> Patch attached to mark functions in crc.c and crc-generate-tables.c as
>> static, GZIP now builds
>
> The patch looks good to me. Simon?
Pushed, thank you!
/Simon
signature.asc
Description: PGP signature
Simon Josefsson wrote:
> > the MAINTAINERCLEANFILES variable isn't set when building gzip so there's
> > an error when using += with it.
>
> I think you should add 'MAINTAINERCLEANFILES =' to gzip instead.
Yes, this is a documented Gnulib requirement:
https://www.gnu.org/software/gnulib/manual/ht
Sam Russell writes:
> Thanks for merging Simon.
>
> I've fixed a couple of issues with building gzip, marking the functions as
> static fixes the -Werror=missing-declarations error, and
> the MAINTAINERCLEANFILES variable isn't set when building gzip so there's
> an error when using += with it.
Sam Russell wrote:
> Patch attached to mark functions in crc.c and crc-generate-tables.c as
> static, GZIP now builds
The patch looks good to me. Simon?
> $ time ./gzip_old -d -k large_file.gz -c > /dev/null
>
> real0m0.491s
> user0m0.491s
> sys 0m0.000s
>
> $ time ./gzip_new -d -k
> I believe this should be fixed through the second patch of mine.
Yes, thanks
> I think you should add 'MAINTAINERCLEANFILES =' to gzip instead.
Adding to lib/Makefile.am on line 24 fixes this
Patch attached to mark functions in crc.c and crc-generate-tables.c as
static, GZIP now builds
$ ti
Sam Russell wrote:
> /usr/bin/ld: /tmp/cc06ZLWr.o: in function `main':
> /home/sam/code/gzip/lib/./crc-generate-table.c:139: undefined reference to
> `rpl_fopen'
> /usr/bin/ld: /tmp/cc06ZLWr.o: in function `print_copyright_notice':
> /home/sam/code/gzip/lib/./crc-generate-table.c:103: undefined ref
Simon Josefsson wrote:
> I merged this now, thank you!
And here's the cross-compilation handling that I promised to add.
2024-10-31 Bruno Haible
crc: Don't attempt to run a compiled C program when cross-compiling.
* lib/crc-generate-table.c: Don't include config.h.
*
Simon Josefsson wrote:
> I added another patch (attached)
Time for me to add another tweak:
We prepend a "DO NOT EDIT" line to generated files. Some editors react to this
line by asking the user whether they really want to edit the file. At the same
time, this is less annoying than doing a "chmod
I merged this now, thank you!
I added another patch (attached) to make the decision to use the
slice-by-8 implementation a maintainer option rather than something
visible to end-users. That also fixes the ./configure stdout output,
you forgot the corresponding AC_MSG_RESULT to the AC_MSG_CHECKING
Thanks for merging Simon.
I've fixed a couple of issues with building gzip, marking the functions as
static fixes the -Werror=missing-declarations error, and
the MAINTAINERCLEANFILES variable isn't set when building gzip so there's
an error when using += with it. There's another issue with the way
Sam Russell wrote:
> Bruno, can you please confirm whether you're happy with my implementation
> of the AC_ARG_ENABLE invocation?
Yes, it looks correct.
The serial number 3 is correct too, since there was a crc.m4 serial 2
in the past.
Bruno
I know you guys have other work to do, and I appreciate your work on this
so far. I would like to get this merged soon if possible.
Bruno, can you please confirm whether you're happy with my implementation
of the AC_ARG_ENABLE invocation? If so, I think we should be good to merge.
If there's anyt
Is there anything else we're missing on this patch? From what I can tell
the configure flag is just the standard behaviour of AC_ARG_ENABLE, so with
this setup the new slice-by-8 implementation will be enabled by default,
but maintainers have the option to disable it
On Sun, Oct 27, 2024, 23:21 Si
Thanks - I am happy with this version, or even without the user-visible logic too (not sure if this is something users should ever tune). My question about selection logic was more for maintainers rather than end-users./Simon27 okt. 2024 kl. 23:04 skrev Sam Russell :I had a play with autoconf (ins
I had a play with autoconf (inspired heavily by m4/debug.m4), can confirm
the behaviour works as follows:
# build with slice-by-8 (default enabled)
./configure
make
gltests/bench-crc 100
real 1.714850
user 1.715
sys0.000
# build without slice-by-8
./configure --disable-crc-slice-by-8
m
Simon Josefsson wrote:
> I am not certain about the method to enable/disable the optimization,
> is the configure.ac variable the best method we have?
An AC_ARG_ENABLE invocation can be added. Since Sam said that he's not familiar
with Autoconf, it would be your turn to add it...
Bruno
Looks good to me too - although I am not certain about the method to
enable/disable the optimization, is the configure.ac variable the best method
we have?
/Simon
> 27 okt. 2024 kl. 14:20 skrev Bruno Haible :
>
> Looks good to me now.
>
> Simon?
>
>
>
Looks good to me now.
Simon?
thanks, moved the xors to the front of the lines
On Sun, Oct 27, 2024, 13:41 Bruno Haible wrote:
> Sam Russell wrote:
> > > Also, in GNU coding style, when line breaking is needed within
> > expressions,
> > > we do the line break before the operator, not after the operator. [1]
> > > This affec
Sam Russell wrote:
> > Also, in GNU coding style, when line breaking is needed within
> expressions,
> > we do the line break before the operator, not after the operator. [1]
> > This affects lib/crc.c lines 73..80, 102..103.
>
> Done.
I meant this expression:
crc = crc32_sliceby8_table[0][(lo
> Unfortunately, 'indent' introduced tabs. In Gnulib, we indent with spaces,
> not tabs (except in Makefiles and ChangeLog). Can you please untabify:
Done.
> Also, in GNU coding style, when line breaking is needed within
expressions,
> we do the line break before the operator, not after the opera
Addendum to:
> Unfortunately, 'indent' introduced tabs. In Gnulib, we indent with spaces,
> not tabs (except in Makefiles and ChangeLog).
We have some documentation about this:
https://www.gnu.org/software/gnulib/manual/html_node/Indent-with-spaces-not-TABs.html
Bruno
Sam Russell wrote:
> I used Simon's indent trick on both files
Unfortunately, 'indent' introduced tabs. In Gnulib, we indent with spaces,
not tabs (except in Makefiles and ChangeLog). Can you please untabify:
$ expand < lib/crc.c > lib/crc.cx && mv lib/crc.clib/crc.c
$ expand < lib/crc-generat
> You can fix most of these by simply running 'indent' on your *.h and *.c
Thanks, this is super helpful. I'm happy to follow the coding style but a
linter would make it easier for me to track down these whitespace issues.
> Regarding the Automake stuff, please try the attached patch, on top of
y
Thanks Sam, I think we are closing in on a good patch, and Bruno's
feedback seems good. Just one small piece of hint:
Bruno Haible via Gnulib discussion list writes:
> 3) Use GNU coding style:
You can fix most of these by simply running 'indent' on your *.h and *.c
files, and then manually rev
[Please keep the mailing list in CC.]
Sam Russell wrote:
> > 1) Add into comments references to the research papers that are used in
> >or are useful to understand the code. Like the "Intel paper" that you
> >showed us in the beginning.
>
> done
>
> > 2) Generate the tables at build time
Hi Sam,
> Sorry for the multiple bumps, but this patch offers a 77% reduction in
> compute time for CRC an a 35% reduction in gzip time,
Thanks for following through!
> if this patch is not compliant then can some please let me know
I would suggest three things:
1) Add into comments references
Hi team,
Sorry for the multiple bumps, but this patch offers a 77% reduction in
compute time for CRC an a 35% reduction in gzip time, if this patch is not
compliant then can some please let me know and I am happy to do whatever it
take to make it compliant so we can all benefit from the speedups.
Sorry for the bump, confirming that I've tested on s390x locally (WSL amd64
with docker-qemu running s390x)
root@3d86c908e4f5:/gnulib# ./gnulib-tool --with-tests --test crc
...
## -- ##
## Gnulib tests
Tests pass with GL_CRC_SLICE_BY_8 set
On Wed, 16 Oct 2024 at 13:52, Simon Josefsson wrote:
> Sam Russell writes:
>
> >> People using the crc module who want to disable the faster/larger CRC32
> >> implementation can add 'gl_crc_slice_by_8=yes' to their configure.ac
> >> before invoking gnulib
Sam Russell writes:
>> People using the crc module who want to disable the faster/larger CRC32
>> implementation can add 'gl_crc_slice_by_8=yes' to their configure.ac
>> before invoking gnulib.
>
> I've been running tests with `./gnulib-tool --with-tests --test crc` so I'm
> interested in a way o
> How about adding something like this to modules/crc:
Done
> People using the crc module who want to disable the faster/larger CRC32
> implementation can add 'gl_crc_slice_by_8=yes' to their configure.ac
> before invoking gnulib.
I've been running tests with `./gnulib-tool --with-tests --test
Sam Russell writes:
> This is my implementation of the slice-by-8 algorithm for CRC32 generation.
Thanks!
> I've added a flag CRC_ENABLE_SLICE_BY_8, I'd appreciate if someone can give
> me a hint on how to set this up in the makefile config. I get the
> impression that we want this to be on by
48 matches
Mail list logo
