Bug 393041 - (libzip) Review Request: libzip - C library for reading, creating, and modifying zip archives
Review Request: libzip - C library for reading, creating, and modifying zip a...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-20 15:53 EST by Sebastian Vahl
Modified: 2012-07-10 14:36 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-17 06:31:43 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
rdieter: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Sebastian Vahl 2007-11-20 15:53:12 EST
Spec URL: http://svahl.fedorapeople.org/libzip/libzip.spec
SRPM URL: http://svahl.fedorapeople.org/libzip/libzip-0.8-2.fc9.src.rpm
Description: 
libzip is a C library for reading, creating, and modifying zip archives. Files
can be added from data buffers, files, or compressed data copied directly from
other zip archives. Changes made without closing the archive can be reverted.
The API is documented by man pages.

Additional note: This is a build requirement of the upcoming version of kdeutils for KDE4.
Comment 1 Rex Dieter 2007-11-20 16:12:25 EST
fyi, scratch build kicked off:
http://koji.fedoraproject.org/koji/taskinfo?taskID=251238

resulting x86_64 pkgs:
$ $rpmlint *.rpm
libzip.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/zipcmp ['/usr/lib64']
libzip.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/zipmerge 
['/usr/lib64']
libzip-devel.x86_64: W: no-documentation

crap.

Well, before tackling that:
1.  MUST: -devel needs
Requires: pkgconfig
Comment 2 Rex Dieter 2007-11-20 16:16:49 EST
I'll review this.
Comment 3 Patrice Dumas 2007-11-20 16:18:18 EST
The man pages that correspond with API description should better
be in -devel. In main package:
%{_mandir}/man1/*zip*
in -devel:
%{_mandir}/man3/*zip*

I think it is better to keep timestamps, this should be achieved with
make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p'

I think it is a bit unfortunate to have the header file
named simply zip.h. Maybe it could be in a libzip subdir? This maybe 
could be achieved by passing
--includedir=%{_includedir}/libzip

the license looks like a BSD one (3 clauses) to me.
Comment 4 Rex Dieter 2007-11-20 16:31:32 EST
> --includedir=%{_includedir}/libzip

let's not go inventing problems that don't exist (yet).  When/if it ever 
becomes an issue, we can address it then.

Otherwise, I concur with Patrice's comments wrt manpages.  MUST item 2.

I tried patching in %setup
# Avoid lib64 rpaths
%if "%{_libdir}" != "/usr/lib"
sed -i -e 's|"/lib /usr/lib|"/%{_lib} %{_libdir}|' configure
%endif
(a common rpath-munching trick), and it seems to work, see
http://koji.fedoraproject.org/koji/taskinfo?taskID=251282
call this MUST item 3.

So, otherwise, the rest is simple and looks good, address items 1,2,3, and 
I'll APPROVE this.
Comment 5 Rex Dieter 2007-11-20 16:32:44 EST
(almost forgot), consider also the suggesion:
make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p'
as MUST item 4.
Comment 6 Sebastian Vahl 2007-11-20 17:34:10 EST
Thanks. I've addressed all MUST items.

Spec URL: http://svahl.fedorapeople.org/libzip/libzip.spec
SRPM URL: http://svahl.fedorapeople.org/libzip/libzip-0.8-3.fc9.src.rpm

Changelog:
- require pkgconfig in devel subpkg
- move api description to devel subpkg
- keep timestamps in %%install
- avoid lib64 rpaths
Comment 7 Ralf Corsepius 2007-11-21 02:26:08 EST
Pardon, but I consider this not to be acceptable:

# Avoid lib64 rpaths
%if "%{_libdir}" != "/usr/lib"
sed -i -e 's|"/lib /usr/lib|"/%{_lib} %{_libdir}|' configure
%endif

This is kills configure scripts.

Comment 8 Rex Dieter 2007-11-21 07:53:33 EST
Ralf, It works.  If you don't like it, offer a better solution, please.

Otherwise, looks good to me Sebastian, APPROVED.


Comment 9 Sebastian Vahl 2007-11-21 13:52:22 EST
Thanks, Rex.

New Package CVS Request
=======================
Package Name: libzip
Short Description: C library for reading, creating, and modifying zip archives
Owners: svahl
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: no
Comment 10 Kevin Fenzi 2007-11-22 14:59:03 EST
cvs done.
Comment 11 Ralf Corsepius 2007-11-23 10:58:11 EST
(In reply to comment #8)
> Ralf, It works.
It only looks as if. What you recommend is worst hack ever having made in
Fedora-  Next time you're going to launch a hexeditor or the like?

>  If you don't like it, offer a better solution, please.
Run autoreconf
cut diff
and patch.

> Otherwise, looks good to me Sebastian, APPROVED.
A sad day for Fedora ...

Comment 12 Rex Dieter 2007-11-23 12:19:54 EST
Ralf, you must understand, we're under tremendous pressure to get kde4 into
fedora rawhide asap, so I hope you can understand our not wanting to block on a
cosmetic (and currently) nonproblem.
Comment 13 Patrice Dumas 2007-11-23 13:32:20 EST
Although I don't disagree with hacks working around upstream issues
I think that we should never ever sacrifice quality for deadlines.

If the reason for using this hack is deadlines, this is a very bad
idea, in my opinion. It adds the possibility for 'blitz' reviews of 
dubious quality and I have seen too much reviews like that, we should 
really be especially watchful and not let anything go that way. For a 
recent example, you can see how the ltsp related reviews begun, 
we should not go down that road. And for merge reviews there is also
a lot of similar issues (although some come more from ignorance than
something else).

Here it is much less extreme since the review was quite correct,
however if the hack is there because of the time it is wrong.
Comment 14 Rex Dieter 2008-01-11 15:40:10 EST
In rawhide now, closing.

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