Bug 248301 - Review Request: lzma - lzma compression tools
Review Request: lzma - lzma compression tools
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-15 13:44 EDT by Patrice Bouchand
Modified: 2007-11-30 17:12 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-09 03:30:45 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Patrice Bouchand 2007-07-15 13:44:59 EDT
Spec URL: http://patrice.bouchand.free.fr/rpm/lzma.spec
SRPM URL: http://patrice.bouchand.free.fr/rpm/lzma-4.32.0-0.1.beta3.src.rpm
Description:

* This is my first package, and I'm seeking a sponsor. *

LZMA provides very high compression ratio and fast decompression. The
core of the LZMA utils is Igor Pavlov's LZMA SDK containing the actual
LZMA encoder/decoder. LZMA utils add a few scripts which provide
gzip-like command line interface and a couple of other LZMA related
tools. Also provides:

- Average compression ratio 30% better than that of gzip and 15%
  better than that of bzip2.

- Decompression speed is only little slower than that of gzip, being
  two to five times faster than bzip2.

- In fast mode, compresses faster than bzip2 with a comparable
  compression ratio.

- Achieving the best compression ratios takes four to even twelve
  times longer than with bzip2. However. this doesn't affect
  decompressing speed.

- Very similar command line interface than what gzip and bzip2 have.
Comment 1 Marc Bradshaw 2007-07-15 23:24:03 EDT
Hi Patrice,

I am also seeking sponsorship so these are some informal suggestions based on a
first look at the spec rather than an actual review.

Make use of standard macros in Source:
http://tukaani.org/%{name}/%{name}-%{version}beta3.tar.gz

lzma-devel should have lzma as a dependency

The changelog lacks a version

Some RPMLint errors need addressing.

E: lzma-libs library-without-ldconfig-postin /usr/lib/liblzmadec.so.0.0.0
E: lzma-libs library-without-ldconfig-postun /usr/lib/liblzmadec.so.0.0.0
W: lzma-libs non-standard-group System/Libraries
W: lzma-devel non-standard-group Development/C

Man pages should be installed as %doc

-M
Comment 2 Patrice Bouchand 2007-07-16 13:04:47 EDT
Hello Marc,

 Thanks for your help !
 
> Make use of standard macros in Source:
> http://tukaani.org/%{name}/%{name}-%{version}beta3.tar.gz

Done

> lzma-devel should have lzma as a dependency

Done

> The changelog lacks a version

Done
 
> E: lzma-libs library-without-ldconfig-postin /usr/lib/liblzmadec.so.0.0.0

ldconfig is now called in %post

> E: lzma-libs library-without-ldconfig-postun /usr/lib/liblzmadec.so.0.0.0

ldconfig is now called in %postun

> W: lzma-libs non-standard-group System/Libraries

Group set to System Environment/Libraries

> W: lzma-devel non-standard-group Development/C

Group set to Development/Libraries

> Man pages should be installed as %doc

Done


Should I increase the release number in such a case ?

  Best regards

           Patrice



Comment 3 Marc Bradshaw 2007-07-16 18:35:23 EDT
Yes, so any potential reviewers can be sure they are reviewing the latest
version you should increment the release and post any new SRPM or SPEC urls.
Comment 4 Patrice Bouchand 2007-07-17 12:14:02 EDT
Ok, here's the new links.

Spec URL: http://patrice.bouchand.free.fr/rpm/lzma.spec
SRPM URL: http://patrice.bouchand.free.fr/rpm/lzma-4.32.0-0.2.beta3.src.rpm

Thanks again.

     Patrice

Comment 5 Adel Gadllah 2007-07-19 12:50:44 EDT
use %{?dist} as a suffix to the release
add %{?_smp_mflags} to make.
Comment 6 Patrice Bouchand 2007-07-19 13:18:15 EDT
Ok, this is fixed in the following links:

Spec URL: http://patrice.bouchand.free.fr/rpm/lzma.spec
SRPM URL: http://patrice.bouchand.free.fr/rpm/lzma-4.32.0-0.3.beta3.fc7.src.rpm

Thanks for your help.

     Patrice
Comment 7 Adel Gadllah 2007-07-19 16:41:29 EDT
ok some more issues:
1) description to long.
2) don't package .a/.la files
3) package the COPYING file
Comment 8 Adel Gadllah 2007-07-19 18:35:27 EDT
Correction:
You can package the static libs but in a different package (-static) but the .la
files need to be removed.
Comment 9 Patrice Bouchand 2007-07-25 12:26:19 EDT
> 1) description to long.

Description is five lines long now.

> 2) don't package .a/.la files

Done

> 3) package the COPYING file

Done

The new files:

Spec URL: http://patrice.bouchand.free.fr/rpm/lzma.spec
SRPM URL: http://patrice.bouchand.free.fr/rpm/lzma-4.32.0-0.4.beta3.fc7.src.rpm


Comment 10 Adel Gadllah 2007-07-25 12:40:05 EDT
package looks good now and also builds fine in mock. ;)
(I am not a sponsor so I can't sponsor you)
Comment 11 Mamoru TASAKA 2007-07-25 13:40:24 EDT
Some comments as one of sponsor members:

* If -devel subpackage is licensed under LGPL, the corresponding
  documents should be installed as a %doc of -devel subpackage
  (i.e. add LICENSE.LIB to %doc of -devel subpackage).
* Add ChangeLog to %doc
* Usually the dependency against main or subpackage should be
  version-release specific.
  i.e. main package should have:
  Requires: %{name}-libs = %{version}-%{release}
  and -devel package should have:
  Requires: %{name} = %{version}-%{release}
* Provides: %{name}-devel = ... on -devel subpackage is not needed
* Explain why -devel package *must* have 
  Provides: lzmadec-devel = %{version}-%{release}
  (I suggest to remove this)
* Please use the following to keep timestamps on header files
  and man pages.
----------------------------------------------------
make install DESTDIR=%{buildroot} INSTALL="%{_install} -p"
-----------------------------------------------------
* We now recommend %defattr(-,root,root,-)
* and %defattr(0644,root,root,-755) on -devel package is not needed.
* You define %{src_version} and it seems that it can be
  used also for source tarball name.
* Files under %{_mandir} is automatically tagged as %doc
Comment 12 Mamoru TASAKA 2007-07-31 15:57:24 EDT
ping?
Comment 13 Patrice Bouchand 2007-08-01 01:31:03 EDT
> ping?

I get your remarks and I will work on it next week during my vacation.

Comment 14 Patrice Bouchand 2007-08-07 05:47:35 EDT
> * If -devel subpackage is licensed under LGPL, the corresponding
>   documents should be installed as a %doc of -devel subpackage
>   (i.e. add LICENSE.LIB to %doc of -devel subpackage).

COPYING.LIB has been added to -devel subpackage.

> * Add ChangeLog to %doc

Done.

> * Usually the dependency against main or subpackage should be
>   version-release specific.
>   i.e. main package should have:
>   Requires: %{name}-libs = %{version}-%{release}

Main package seems to not require -libs to work.

>   and -devel package should have:
>   Requires: %{name} = %{version}-%{release}

-devel does not require main package to work, but if I remove it rpmlint complains ?

> * Provides: %{name}-devel = ... on -devel subpackage is not needed

This has been removed.

> * Explain why -devel package *must* have 
>   Provides: lzmadec-devel = %{version}-%{release}
>   (I suggest to remove this)

The devel package only provides a way to decompress lzma archive, not to
compress. As you suggest, I remove it. Should lzma-libs become lzmadec-libs ? (
and lzma-devel -> lzmadec-devel )

> * Please use the following to keep timestamps on header files
>   and man pages.
> ----------------------------------------------------
> make install DESTDIR=%{buildroot} INSTALL="%{_install} -p"
> -----------------------------------------------------

Done, but I have to use __install instead of _install ?

> * We now recommend %defattr(-,root,root,-)

Done.

> * and %defattr(0644,root,root,-755) on -devel package is not needed.

Done.

> * You define %{src_version} and it seems that it can be
>   used also for source tarball name.

Done.

> * Files under %{_mandir} is automatically tagged as %doc

%doc has been removed for %{_mandir}

The new files:

Spec URL: http://patrice.bouchand.free.fr/rpm/lzma.spec
SRPM URL: http://patrice.bouchand.free.fr/rpm/lzma-4.32.0-0.5.beta3.fc7.src.rpm

   Thanks for your time.

        Patrice

Comment 15 Mamoru TASAKA 2007-08-07 09:23:41 EDT
First for your comment 14:
(In reply to comment #14)
> > * If -devel subpackage is licensed under LGPL, the corresponding
> >   documents should be installed as a %doc of -devel subpackage
> >   (i.e. add LICENSE.LIB to %doc of -devel subpackage). 
> COPYING.LIB has been added to -devel subpackage.
  - Well, -libs package is also licensed under LGPL and
    it is more proper that LICENSE.LIB be owned by -libs, not -devel
    (or you can have LICENSE.LIB owned by both -libs and -devel).

> > * Usually the dependency against main or subpackage should be
> >   version-release specific.
> >   i.e. main package should have:
> >   Requires: %{name}-libs = %{version}-%{release}
> 
> Main package seems to not require -libs to work.
   - Umm... this is undesirable. I checked the build log and for example:
-------------------------------------------------------
/bin/sh ../../libtool --tag=CC --mode=link gcc -I../../src/liblzmadec -O2 -g
-pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables -D_FILE_OFFSET_BITS=64 -static  -o lzmadec 
lzmadec.o ../../src/liblzmadec/liblzmadec.la 
mkdir .libs
gcc -I../../src/liblzmadec -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2
-fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386
-mtune=generic -fasynchronous-unwind-tables -D_FILE_OFFSET_BITS=64 -o lzmadec
lzmadec.o  ../../src/liblzmadec/.libs/liblzmadec.a
make[3]: Leaving directory `/builddir/build/BUILD/lzma-4.32.0beta3/src/lzmadec'
--------------------------------------------------------
     Here /usr/bin/lzmadec needs liblzmadec.la but liblzmadec.la is
     used staticly, which must not be and /usr/bin/lzmadec should be
     linked against liblzmadec.so

     For this package you can disable static link against liblzmadec.so
     by passing "--diable-static" to configure. You also have to kill
     unneeded rpath. 
     (check "Removing Rpath" of
      http://fedoraproject.org/wiki/Packaging/Guidelines)
     The following seems to work.
--------------------------------------------------------
%build
CFLAGS="%{optflags} -D_FILE_OFFSET_BITS=64" \
CXXFLAGS="%{optflags} -D_FILE_OFFSET_BITS=64" \
%configure --disable-static

# kill rpath
sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool

make %{?_smp_mflags}
--------------------------------------------------------

> >   and -devel package should have:
> >   Requires: %{name} = %{version}-%{release}
> 
> -devel does not require main package to work, but if I remove it 
> rpmlint complains ?
  - The reason I said so is because your -4 spec file had
---------------------------------------------------------
Requires:       %{name} = %{version}
---------------------------------------------------------
    for -devel package. However actually -devel package does not
    require main package so you can remove the line.

> The devel package only provides a way to decompress lzma archive, not to
> compress. As you suggest, I remove it. Should lzma-libs become lzmadec-libs ?
  - Just okay with lzma-libs.

> Done, but I have to use __install instead of _install ?
  - Sorry, my typo.

Then another issues are:
* Source
  - The newest seems to be beta5.

* License
  - License tag policy is recently changed and we must follow
    http://fedoraproject.org/wiki/Packaging/LicensingGuidelines
    http://fedoraproject.org/wiki/Licensing

    As far as I checked this source code,
    - -libs and -devel package should be tagged as "LGPLv2+"
      (this means that License field in spec file
       should be tagged as "LGPLv2+")
    - For main package, I cannot find the phrase "and any later" on
      LzmaDecode.c so I guess this code is licensed under strict LGPL
      version 2.
      However (according to the license matrix) LGPLv2 can be
      relicensed to GPLv2+ (GPL version 2 and any later), and other
      parts are licensed under LGPLv2+ and GPLv2+,
      so the main package can be tagged with "GPLv2+".  

* -devel package still has %defattr(-,root,root)  
Comment 16 Patrice Bouchand 2007-08-07 10:49:17 EDT
>   - Well, -libs package is also licensed under LGPL and
>     it is more proper that LICENSE.LIB be owned by -libs, not -devel
>     (or you can have LICENSE.LIB owned by both -libs and -devel).

COPYING.LIB has been moved to -libs package.
 
>      For this package you can disable static link against liblzmadec.so
>      by passing "--diable-static" to configure. You also have to kill
>      unneeded rpath. 

Done. I checked it using ldd and chrpath.

>   - The reason I said so is because your -4 spec file had
> ---------------------------------------------------------
> Requires:       %{name} = %{version}
> ---------------------------------------------------------
>     for -devel package. However actually -devel package does not
>     require main package so you can remove the line.

Done.
 
> Then another issues are:
> * Source
>   - The newest seems to be beta5.

SRPM use the latest release now.
 
>     As far as I checked this source code,
>     - -libs and -devel package should be tagged as "LGPLv2+"
>       (this means that License field in spec file
>        should be tagged as "LGPLv2+")

License has been set to LGPLv2+ for -libs and -devel.

>     - For main package, I cannot find the phrase "and any later" on
>       LzmaDecode.c so I guess this code is licensed under strict LGPL
>       version 2.
>       However (according to the license matrix) LGPLv2 can be
>       relicensed to GPLv2+ (GPL version 2 and any later), and other
>       parts are licensed under LGPLv2+ and GPLv2+,
>       so the main package can be tagged with "GPLv2+".  

License has been set to GPLv2+ for main package.

 * -devel package still has %defattr(-,root,root)

Sorry, I think it's ok now.

The new files:

Spec URL: http://patrice.bouchand.free.fr/rpm/lzma.spec
SRPM URL: http://patrice.bouchand.free.fr/rpm/lzma-4.32.0-0.6.beta5.fc7.src.rpm

    Thanks

         Patrice

Comment 17 Mamoru TASAKA 2007-08-07 12:04:40 EDT
Well
* Now lzma is okay
* As this is sponsor needed issue, we usually ask the submitter to
  do a pre-review of other persons' review requests or submit another
  review request .
  http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored
  Now you have another review request (bug 248778) and although
  I have not checked gtkperf in detail, it looks (at least almost) good.

Okay.
---------------------------------------------------------
    This package (lzma) is APPROVED by me
---------------------------------------------------------

Please follow the procedure according to:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor (at the stage, please also write on
this bug for confirmation that you requested for sponsorship)
Then I will sponsor you.

If you want to import this package into Fedora 7, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on Fedora rebuilding system).

If you have questions, please ask me.
Comment 18 Patrice Bouchand 2007-08-07 14:37:13 EDT
> Well
> * Now lzma is okay
> * As this is sponsor needed issue, we usually ask the submitter to
>   do a pre-review of other persons' review requests or submit another
>   review request .
>   http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored
>   Now you have another review request (bug 248778) and although
>   I have not checked gtkperf in detail, it looks (at least almost) good.
> 
> Okay.
> ---------------------------------------------------------
>     This package (lzma) is APPROVED by me
> ---------------------------------------------------------

Thanks a lot !
 
> Please follow the procedure according to:
> http://fedoraproject.org/wiki/PackageMaintainers/Join
> from "Get a Fedora Account".
> At a point a mail should be sent to sponsor members which notifies
> that you need a sponsor (at the stage, please also write on
> this bug for confirmation that you requested for sponsorship)
> Then I will sponsor you.

I think it's done.

> If you want to import this package into Fedora 7, you also have
> to look at
> http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
> (after once you rebuilt this package on Fedora rebuilding system).
> 
> If you have questions, please ask me.

Ok, thanks again for your time and for your help.

Comment 19 Mamoru TASAKA 2007-08-07 14:59:34 EDT
Now I am sponsoring you.
Comment 20 Patrice Bouchand 2007-08-08 03:12:08 EDT
New Package CVS Request
=======================
Package Name: lzma
Short Description: lzma compression tools
Owners: patriceb
Branches: F-7
InitialCC:
Comment 21 Mamoru TASAKA 2007-08-08 03:56:48 EDT
Ah.. Owners must be your bugzilla email.
Please rewrite CVS request and again set cvs flag to ? .

(Note: I don't have the right to do the needed CVS admin procedure.
       Someone else will do it and then will set CVS flag to +)
Comment 22 Patrice Bouchand 2007-08-08 04:02:31 EDT
New Package CVS Request
=======================
Package Name: lzma
Short Description: lzma compression tools
Owners: patrice.bouchand.fedora@gmail.com
Branches: F-7
InitialCC:

Comment 23 Toshio Kuratomi 2007-08-08 12:43:58 EDT
(In reply to comment #21)
> Ah.. Owners must be your bugzilla email.
> Please rewrite CVS request and again set cvs flag to ? .
> 
> (Note: I don't have the right to do the needed CVS admin procedure.
>        Someone else will do it and then will set CVS flag to +)

This has just changed.  Fedora Account System Username is what is needed now
because we're in the process of switching to the Package Database.  I'm sending
an announcement about needing FAS username today.  Sorry for the confusion
during the transition.
Comment 24 Kevin Fenzi 2007-08-08 17:09:33 EDT
cvs done. 
Comment 25 Ralf Corsepius 2007-10-08 09:54:55 EDT
Why is the package still not available for FC-7?

CVS has it but it doesn't seem to be available from the repos.
Comment 26 Mamoru TASAKA 2007-10-08 10:55:36 EDT
(In reply to comment #25)
> Why is the package still not available for FC-7?
> CVS has it but it doesn't seem to be available from the repos.

Well, F-7 lzma is still in testing. I added a comment on bodhi
to ask Patrice to request to move from testing to stable.
Comment 27 Ralf Corsepius 2007-10-11 06:42:57 EDT
Ping ^2 - What needs to happen to make this package finally be released?

Keeping a package "> 2" months in testing is non-reasonable.

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