Bug 1074129 - Review Request: perl-Compress-LZF - Extremely light-weight Lempel-Ziv-Free compression
Summary: Review Request: perl-Compress-LZF - Extremely light-weight Lempel-Ziv-Free co...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Petr Pisar
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1075911
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-03-08 04:58 UTC by David Dick
Modified: 2014-05-01 18:30 UTC (History)
2 users (show)

Fixed In Version: perl-Compress-LZF-3.7-1.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-04-26 09:16:50 UTC
ppisar: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description David Dick 2014-03-08 04:58:58 UTC
Spec URL: http://ddick.fedorapeople.org/packages/perl-Compress-LZF.spec
SRPM URL: http://ddick.fedorapeople.org/packages/perl-Compress-LZF-3.7-1.fc20.src.rpm
Description: Extremely light-weight Lempel-Ziv-Free compression
Fedora Account System Username: ddick

Comment 1 David Dick 2014-03-08 05:18:02 UTC
koji build at http://koji.fedoraproject.org/koji/taskinfo?taskID=6611436

Comment 2 Petr Pisar 2014-03-11 14:25:30 UTC
URL and Source0 are usable. Ok.
Source archive is original (SHA-256: 571389c9ab62d9d0dbae479460d8c2b5132de7467990198532b9f4845b9ecfe4). Ok.
XS code presents, architecture dependent package is Ok.
Summary verified from LZF.pm. Ok.
Description verified from LZF.pm.Ok.

TODO: The description is non-descriptive. It talks about bundling (that will not be true in Fedora) and about free for commercial usage (that should go into license). I'd like to see instead of these statements something useful like `this is Perl binding to LZF compression library'.

Perl code license verified from COPYING.

FIX: Bundled LZF sources (lzf_c.c, lzfP.h, lzf_c_best.c) are licensed under (BSD or GPLv2+). This must be declared in the License tag too. Or unbundle the code.

FIX: Unbundle the LZF library (lzf_c.c, lzfP.h, lzf_c_best.c) and use system implementation (liblzf). Or obtain exception from Fedora Packaging Committee <https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries>.

Build-time dependencies are Ok.

All tests pass. Ok.

$ rpmlint perl-Compress-LZF.spec ../SRPMS/perl-Compress-LZF-3.7-1.fc21.src.rpm ../RPMS/x86_64/perl-Compress-LZF-3.7-1.fc21.x86_64.rpm 
perl-Compress-LZF.src: W: spelling-error %description -l en_US memcpy -> memory
perl-Compress-LZF.x86_64: W: spelling-error %description -l en_US memcpy -> memory
perl-Compress-LZF.x86_64: E: incorrect-fsf-address /usr/share/doc/perl-Compress-LZF/COPYING.GNU
2 packages and 1 specfiles checked; 1 errors, 2 warnings.
rpmlint is Ok.

$ rpm -q -lv -p ../RPMS/x86_64/perl-Compress-LZF-3.7-1.fc21.x86_64.rpm
drwxr-xr-x    2 root    root                        0 Mar 11 14:59 /usr/lib64/perl5/vendor_perl/Compress
-rw-r--r--    1 root    root                     4777 Aug 25  2013 /usr/lib64/perl5/vendor_perl/Compress/LZF.pm
drwxr-xr-x    2 root    root                        0 Mar 11 14:59 /usr/lib64/perl5/vendor_perl/auto/Compress
drwxr-xr-x    2 root    root                        0 Mar 11 14:59 /usr/lib64/perl5/vendor_perl/auto/Compress/LZF
-rwxr-xr-x    1 root    root                    23696 Mar 11 14:59 /usr/lib64/perl5/vendor_perl/auto/Compress/LZF/LZF.so
drwxr-xr-x    2 root    root                        0 Mar 11 14:59 /usr/share/doc/perl-Compress-LZF
-rw-r--r--    1 root    root                      244 Jun  1  2010 /usr/share/doc/perl-Compress-LZF/COPYING
-rw-r--r--    1 root    root                     6111 Jun  1  2010 /usr/share/doc/perl-Compress-LZF/COPYING.Artistic
-rw-r--r--    1 root    root                    17998 Jun  1  2010 /usr/share/doc/perl-Compress-LZF/COPYING.GNU
-rw-r--r--    1 root    root                     4650 Aug 25  2013 /usr/share/doc/perl-Compress-LZF/Changes
-rw-r--r--    1 root    root                     4691 Aug 25  2013 /usr/share/doc/perl-Compress-LZF/README
-rw-r--r--    1 root    root                     3758 Mar 11 14:59 /usr/share/man/man3/Compress::LZF.3pm.gz
File layout and permissions are Ok.

$ rpm -q --requires -p ../RPMS/x86_64/perl-Compress-LZF-3.7-1.fc21.x86_64.rpm | sort | uniq -c
      1 libc.so.6()(64bit)
      1 libc.so.6(GLIBC_2.14)(64bit)
      1 libc.so.6(GLIBC_2.2.5)(64bit)
      1 libc.so.6(GLIBC_2.4)(64bit)
      1 libperl.so.5.18()(64bit)
      1 perl(DynaLoader)
      1 perl(Exporter)
      1 perl(:MODULE_COMPAT_5.18.2)
      1 rpmlib(CompressedFileNames) <= 3.0.4-1
      1 rpmlib(FileDigests) <= 4.6.0-1
      1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
      1 rpmlib(PayloadIsXz) <= 5.2-1
      1 rtld(GNU_HASH)
Binary requires are Ok.

$ rpm -q --provides -p ../RPMS/x86_64/perl-Compress-LZF-3.7-1.fc21.x86_64.rpm | sort | uniq -c
      1 perl(Compress::LZF) = 3.7
      1 perl-Compress-LZF = 3.7-1.fc21
      1 perl-Compress-LZF(x86-64) = 3.7-1.fc21
Binary provides are Ok.

$ resolvedeps rawhide ../RPMS/x86_64/perl-Compress-LZF-3.7-1.fc21.x86_64.rpm 
Binary dependencies resolvable. Ok.

Package builds in F21 (http://koji.fedoraproject.org/koji/taskinfo?taskID=6621625). Ok.

Otherwise the package is in line with Fedora and Perl packaging guidelines.

Please correct the `FIX' items, and provide new spec file.
Resolution: NOT approved.

Comment 3 David Dick 2014-03-12 10:18:02 UTC
Package has been reuploaded with liblzf support which removes the need for lzf_c.c and lzf_d.c.  A bug has been filed upstream to clarify the package license as lzf_c_best.c (and lzfP.h) do not contain code that is available in the liblzf library.

https://rt.cpan.org/Ticket/Display.html?id=93776

Comment 4 Petr Pisar 2014-03-12 12:25:47 UTC
Spec file changes:

--- perl-Compress-LZF.spec.old  2014-03-08 06:27:17.000000000 +0100
+++ perl-Compress-LZF.spec      2014-03-12 11:00:35.000000000 +0100
@@ -7,6 +7,7 @@
 Group:          Development/Libraries
 URL:            http://search.cpan.org/dist/Compress-LZF/
 Source0:        http://www.cpan.org/modules/by-module/Compress/Compress-LZF-%{version}.tar.gz
+Patch1:         compress_lzf_unbundle.patch
 BuildRequires:  perl
 BuildRequires:  perl(DynaLoader)
 BuildRequires:  perl(Exporter)
@@ -15,16 +16,13 @@
 Requires:       perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))
 
 %description
-LZF is an extremely fast (not that much slower than a pure memcpy)
-compression algorithm. It is ideal for applications where you want to save
-some space but not at the cost of speed. It is ideal for repetitive data as
-well. The module is self-contained and very small (no large library to be
-pulled in). It is also free, so there should be no problems incorporating
-this module into commercial programs.
+This is Perl binding to the LZF compression library
 
 %prep
 %setup -q -n Compress-LZF-%{version}
 
+%patch1
+
 %build
 %{__perl} Makefile.PL INSTALLDIRS=vendor OPTIMIZE="$RPM_OPT_FLAGS"
 make %{?_smp_mflags}


> TODO: The description is non-descriptive.
TODO: Append full-stop mark at the end of the %description.

> FIX: Bundled LZF sources (lzf_c.c, lzfP.h, lzf_c_best.c) are licensed under
> (BSD or GPLv2+). This must be declared in the License tag too. Or unbundle
> the code.
FIX: You forgot to mention the lzf_c_best.c license in the License tag (because you still use it).

> FIX: Unbundle the LZF library (lzf_c.c, lzfP.h, lzf_c_best.c) and use system
> implementation (liblzf). Or obtain exception from Fedora Packaging Committee
> <https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries>.
FIX: Build-require liblzf-devel.

TODO: I strongly recommend to delete the unbundled files in %prep section to make sure they will not be used for building.

FIX: You kept files (lzf_c_best.c) that come from libzf development sources (http://cvs.schmorp.de/liblzf/) and its dependency lzfP.h. This is still considered as a bundling. The best approach is to ask liblzf developers and maintainers to do a new release with these files. Otherwise you need to obtain exception from FPC. Alternatively you can remove support for the "best compression level" from LZF.xs.

Please correct all `FIX' items, and provide new spec file.
Resolution: Package NOT approved.

Comment 5 David Dick 2014-03-13 08:05:43 UTC
lzfP.h and lzf_c_best.c are currently present in CVS, however, an examination of the Changelog shows that lzf_c_best.c has just (in the yet-to-be-released 3.7 branch) been added.  The changelog also notes that lzf_c_best is currently considered broken by the author (line 5 of Changelog file), so it seems a bit harsh to ask the liblzf authors to support a beta, broken version.

Therefore i have asked the liblzf packagers to only include lzfP.h.  If they include lzf_best_c.c, fine, otherwise i will remove the best functions from Compress::LZF

Comment 6 David Dick 2014-04-10 11:23:32 UTC
Steve Traylon has only added the lzfP.h header file.  Therefore, as discussed, i've stripped out the _best files.  Could you please review this again?

koji builds follow;

rawhide at http://koji.fedoraproject.org/koji/taskinfo?taskID=6723429

el6 at http://koji.fedoraproject.org/koji/taskinfo?taskID=6723428

Comment 7 Petr Pisar 2014-04-15 07:19:17 UTC
Spec file changes:

--- perl-Compress-LZF.spec.old  2014-03-12 11:00:35.000000000 +0100
+++ perl-Compress-LZF.spec      2014-04-10 13:17:32.000000000 +0200
@@ -8,6 +8,7 @@
 URL:            http://search.cpan.org/dist/Compress-LZF/
 Source0:        http://www.cpan.org/modules/by-module/Compress/Compress-LZF-%{version}.tar.gz
 Patch1:         compress_lzf_unbundle.patch
+BuildRequires:  liblzf-devel
 BuildRequires:  perl
 BuildRequires:  perl(DynaLoader)
 BuildRequires:  perl(Exporter)
@@ -16,12 +17,12 @@
 Requires:       perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

 %description
-This is Perl binding to the LZF compression library
+This is Perl binding to the LZF compression library.

 %prep
 %setup -q -n Compress-LZF-%{version}

-%patch1
+%patch1 -p1

 %build
 %{__perl} Makefile.PL INSTALLDIRS=vendor OPTIMIZE="$RPM_OPT_FLAGS"


The patch is good. Ok.

> TODO: Append full-stop mark at the end of the %description.
-This is Perl binding to the LZF compression library
+This is Perl binding to the LZF compression library.
Ok.

> FIX: You forgot to mention the lzf_c_best.c license in the License tag
> (because you still use it).
The lzf_c_best.c has been removed. Ok.

> FIX: Build-require liblzf-devel.
+BuildRequires:  liblzf-devel
Ok.

> TODO: I strongly recommend to delete the unbundled files in %prep section to
> make sure they will not be used for building.
Implemented in the patch. Ok.

> FIX: You kept files (lzf_c_best.c) that come from libzf development sources
> (http://cvs.schmorp.de/liblzf/) and its dependency lzfP.h. This is still
> considered as a bundling. The best approach is to ask liblzf developers and
> maintainers to do a new release with these files. Otherwise you need to obtain
> exception from FPC. Alternatively you can remove support for the "best
> compression level" from LZF.xs.
The "best compression level" with lzf_c_best.c and lzfP.h have been removed. Ok.

All tests pass. Ok.

$ rpmlint  perl-Compress-LZF.spec ../SRPMS/perl-Compress-LZF-3.7-1.fc21.src.rpm ../RPMS/x86_64/perl-Compress-LZF-*
perl-Compress-LZF.x86_64: E: incorrect-fsf-address /usr/share/doc/perl-Compress-LZF/COPYING.GNU
3 packages and 1 specfiles checked; 1 errors, 0 warnings.
rpmlint is Ok.

Package builds in F21 (http://koji.fedoraproject.org/koji/taskinfo?taskID=6739594). Ok.

Package is in line with Fedora and Perl packaging guidelines.

Resolution: Package APPROVED.

Comment 8 Petr Pisar 2014-04-15 07:20:49 UTC
I'm glad you managed to unbundle the files.

Comment 9 David Dick 2014-04-15 07:52:56 UTC
New Package SCM Request
=======================
Package Name: perl-Compress-LZF
Short Description: Extremely light-weight Lempel-Ziv-Free compression
Owners: ddick
Branches: f20 el6 epel7
InitialCC: perl-sig

Thanks for the review Petr.  This package was a lot more trouble than i intended.

Comment 10 Gwyn Ciesla 2014-04-15 11:52:15 UTC
Git done (by process-git-requests).

Comment 11 Fedora Update System 2014-04-16 09:25:02 UTC
perl-Compress-LZF-3.7-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/perl-Compress-LZF-3.7-1.fc20

Comment 12 Fedora Update System 2014-04-16 09:31:50 UTC
perl-Compress-LZF-3.7-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/perl-Compress-LZF-3.7-1.el6

Comment 13 Fedora Update System 2014-04-16 16:26:53 UTC
perl-Compress-LZF-3.7-1.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 14 Fedora Update System 2014-04-26 09:16:50 UTC
perl-Compress-LZF-3.7-1.fc20 has been pushed to the Fedora 20 stable repository.

Comment 15 Fedora Update System 2014-05-01 18:30:00 UTC
perl-Compress-LZF-3.7-1.el6 has been pushed to the Fedora EPEL 6 stable repository.


Note You need to log in before you can comment on or make changes to this bug.