Bug #98276 for Compress-Snappy: Differences between bundled library and the current version?

Sat Aug 23 02:01:35 2014 DDICK [...] cpan.org - Ticket created

Subject:

Differences between bundled library and the current version?

Hi, I'm trying to package this library for Fedora. In order to do so, i need to remove the bundled c library and rely on the Fedora provided version of snappy (v1.1.0). The only way i could get that to work was with the attached patch. Could you check this out and let me know if this is the correct way to unbundle your version of the snappy library for Fedora ? Any suggestions/explanations you might have to explain the differences would be most welcome. Cheers Dave

Subject:

compress_snappy_unbundle.patch

diff -Naur old/Makefile.PL new/Makefile.PL --- old/Makefile.PL 2013-10-13 18:26:49.000000000 +1100 +++ new/Makefile.PL 2014-08-23 14:18:22.085946140 +1000 @@ -31,6 +31,8 @@ }, DEFINE => $ctz, + LIBS => '-lsnappy', + dist => { COMPRESS => 'gzip -9f', SUFFIX => 'gz', }, clean => { FILES => 'Compress-Snappy-*' }, diff -Naur old/Snappy.xs new/Snappy.xs --- old/Snappy.xs 2012-09-09 03:23:49.000000000 +1000 +++ new/Snappy.xs 2014-08-23 15:37:46.416968105 +1000 @@ -7,8 +7,7 @@ #define NEED_sv_2pvbyte #include "ppport.h" -#include "src/csnappy_compress.c" -#include "src/csnappy_decompress.c" +#include <snappy-c.h> MODULE = Compress::Snappy PACKAGE = Compress::Snappy @@ -20,8 +19,7 @@ PREINIT: char *src, *dest; STRLEN src_len; - uint32_t dest_len; - void *working_memory; + size_t dest_len; CODE: if (SvROK(sv) && ! SvAMAGIC(sv)) sv = SvRV(sv); @@ -30,19 +28,15 @@ src = SvPVbyte(sv, src_len); if (! src_len) XSRETURN_NO; - dest_len = csnappy_max_compressed_length(src_len); + dest_len = snappy_max_compressed_length(src_len); if (! dest_len) XSRETURN_UNDEF; - Newx(working_memory, CSNAPPY_WORKMEM_BYTES, void *); - if (! working_memory) - XSRETURN_UNDEF; RETVAL = newSV(dest_len); dest = SvPVX(RETVAL); if (! dest) XSRETURN_UNDEF; - csnappy_compress(src, src_len, dest, &dest_len, working_memory, - CSNAPPY_WORKMEM_BYTES_POWER_OF_TWO); - Safefree(working_memory); + if (snappy_compress(src, src_len, dest, &dest_len) != SNAPPY_OK) + XSRETURN_UNDEF; SvCUR_set(RETVAL, dest_len); SvPOK_on(RETVAL); OUTPUT: @@ -56,8 +50,7 @@ PREINIT: char *src, *dest; STRLEN src_len; - uint32_t dest_len; - int header_len; + size_t dest_len; CODE: PERL_UNUSED_VAR(ix); /* -W */ if (SvROK(sv)) @@ -67,15 +60,13 @@ src = SvPVbyte(sv, src_len); if (! src_len) XSRETURN_NO; - header_len = csnappy_get_uncompressed_length(src, src_len, &dest_len); - if (0 > header_len || ! dest_len) + if (snappy_uncompressed_length(src, src_len, &dest_len) != SNAPPY_OK) XSRETURN_UNDEF; RETVAL = newSV(dest_len); dest = SvPVX(RETVAL); if (! dest) XSRETURN_UNDEF; - if (csnappy_decompress_noheader(src + header_len, src_len - header_len, - dest, &dest_len)) + if (snappy_uncompress(src, src_len, dest, &dest_len) != SNAPPY_OK) XSRETURN_UNDEF; SvCUR_set(RETVAL, dest_len); SvPOK_on(RETVAL);

Sat Aug 23 09:23:28 2014 gray [...] cpan.org - Correspondence added

On Sat Aug 23 02:01:35 2014, DDICK wrote: Show quoted text

> Hi, > > I'm trying to package this library for Fedora. In order to do so, i > need to remove the bundled c library and rely on the Fedora provided > version of snappy (v1.1.0). > > The only way i could get that to work was with the attached patch. > > Could you check this out and let me know if this is the correct way to > unbundle your version of the snappy library for Fedora ? > > Any suggestions/explanations you might have to explain the differences > would be most welcome. > > Cheers > Dave

This module uses the csnappy library [1], not the official google snappy library. It looks like the debian folks are doing similar work [2]. But if you make substantial changes to my code, I obviously can't support it. If you package csnappy in Fedora, I can update the Makefile.PL to test for a system-installed csnappy library first and fallback to the bundled library otherwise. See the attached patch for what I propose. [1] https://github.com/zeevt/csnappy [2] https://lists.debian.org/debian-wnpp/2014/07/msg01910.html

Subject:

a.patch

diff --git a/Makefile.PL b/Makefile.PL index 7716c6e..9bc33d1 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -1,14 +1,22 @@ use strict; use warnings; +use Config; use ExtUtils::MakeMaker 6.52; use Devel::CheckLib; -my $ctz = check_lib( +my $have_ctz = check_lib( lib => 'c', function => 'return (__builtin_ctzll(0x100000000LL) != 32);' ) ? '-DHAVE_BUILTIN_CTZ' : ''; +my $have_csnappy = check_lib( + lib => 'csnappy', + libpath => [ split ' ', $Config{loclibpth} ], + header => [ 'csnappy_compress.c', 'csnappy_decompress.c' ], + incpath => [ split ' ', $Config{locincpth} ], +); + my %conf = ( NAME => 'Compress::Snappy', AUTHOR => 'gray <gray@cpan.org>', @@ -30,7 +38,10 @@ my %conf = ( }, }, - DEFINE => $ctz, + $have_csnappy + ? (LIBS => '-lcsnappy') + : (INC => '-Isrc'), + DEFINE => $have_ctz, dist => { COMPRESS => 'gzip -9f', SUFFIX => 'gz', }, clean => { FILES => 'Compress-Snappy-*' }, diff --git a/Snappy.xs b/Snappy.xs index 707e026..ed5b377 100644 --- a/Snappy.xs +++ b/Snappy.xs @@ -7,8 +7,8 @@ #define NEED_sv_2pvbyte #include "ppport.h" -#include "src/csnappy_compress.c" -#include "src/csnappy_decompress.c" +#include <csnappy_compress.c> +#include <csnappy_decompress.c> MODULE = Compress::Snappy PACKAGE = Compress::Snappy

Sat Aug 23 09:23:29 2014 The RT System itself - Status changed from 'new' to 'open'

Sun Aug 24 04:57:26 2014 DDICK [...] cpan.org - Correspondence added

On Sat Aug 23 23:23:28 2014, GRAY wrote: Show quoted text

> This module uses the csnappy library [1], not the official google > snappy library. It looks like the debian folks are doing similar work > [2]. But if you make substantial changes to my code, I obviously can't > support it. If you package csnappy in Fedora, I can update the > Makefile.PL to test for a system-installed csnappy library first and > fallback to the bundled library otherwise. See the attached patch for > what I propose. > > [1] https://github.com/zeevt/csnappy > [2] https://lists.debian.org/debian-wnpp/2014/07/msg01910.html

Thanks for the speedy reply. Naturally, i'm not at all keen on packaging this module in a state that you don't want to support. I'm also not keen (at least for the moment) on packaging the csnappy library for Fedora. Reasons follow; 1) Fedora already has the snappy library packaged which afaik offers identical functionality as far as compressing/decompressing data is concerned. The differences (once again, afaik) are entirely to do with the API and the linked libraries required at runtime. 2) The csnappy library (according to [1] above, appears to be a redesign to suit embedding into the Linux kernel. It is not intended as a shared library (unlike snappy), and the author admits the entire shared library concept for csnappy is a hack. 3) The csnappy library ability to build on multiple architectures appears to be confined to a single Makefile, which doesn't fill me with confidence with the authors interest in supporting me if i go back to him with problems. Please let me know if the above concerns are incorrect, or if i have missed out anything you think would encourage me to package csnappy for Fedora. If not, is there any possibility that you could use the snappy library instead of csnappy?

Sun Aug 24 11:54:59 2014 gray [...] cpan.org - Correspondence added

On Sun Aug 24 04:57:26 2014, DDICK wrote: Show quoted text

> On Sat Aug 23 23:23:28 2014, GRAY wrote:

> > This module uses the csnappy library [1], not the official google > > snappy library. It looks like the debian folks are doing similar work > > [2]. But if you make substantial changes to my code, I obviously > > can't > > support it. If you package csnappy in Fedora, I can update the > > Makefile.PL to test for a system-installed csnappy library first and > > fallback to the bundled library otherwise. See the attached patch for > > what I propose. > > > > [1] https://github.com/zeevt/csnappy > > [2] https://lists.debian.org/debian-wnpp/2014/07/msg01910.html

> > Thanks for the speedy reply. Naturally, i'm not at all keen on > packaging this module in a state that you don't want to support. I'm > also not keen (at least for the moment) on packaging the csnappy > library for Fedora. Reasons follow; > > 1) Fedora already has the snappy library packaged which afaik offers > identical functionality as far as compressing/decompressing data is > concerned. The differences (once again, afaik) are entirely to do > with the API and the linked libraries required at runtime. > 2) The csnappy library (according to [1] above, appears to be a > redesign to suit embedding into the Linux kernel. It is not intended > as a shared library (unlike snappy), and the author admits the entire > shared library concept for csnappy is a hack. > 3) The csnappy library ability to build on multiple architectures > appears to be confined to a single Makefile, which doesn't fill me > with confidence with the authors interest in supporting me if i go > back to him with problems. > > Please let me know if the above concerns are incorrect, or if i have > missed out anything you think would encourage me to package csnappy > for Fedora.

I chose csnappy because it focuses on portability across architectures and the author is responsive. The Sereal project also uses it. Show quoted text

> If not, is there any possibility that you could use the snappy library > instead of csnappy?

No, I don't think that's a reasonable request. Compress::Snappy works everywhere now: http://www.cpantesters.org/distro/C/Compress-Snappy

Mon Aug 25 04:17:01 2014 DDICK [...] cpan.org - Correspondence added

On Mon Aug 25 01:54:59 2014, GRAY wrote: Show quoted text

> On Sun Aug 24 04:57:26 2014, DDICK wrote:

> > Please let me know if the above concerns are incorrect, or if i have > > missed out anything you think would encourage me to package csnappy > > for Fedora.

> > I chose csnappy because it focuses on portability across architectures > and the author is responsive. The Sereal project also uses it.

Okay. I'll see if i can get csnappy into Fedora/CentOS.

Mon Aug 25 04:17:03 2014 DDICK [...] cpan.org - Status changed from 'open' to 'resolved'