Bug 773313 - Review Request: ZipArchive - The ZipArchive library
Review Request: ZipArchive - The ZipArchive library
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dan Horák
Fedora Extras Quality Assurance
: 782823 (view as bug list)
Depends On:
Blocks: 772362
  Show dependency treegraph
Reported: 2012-01-11 08:51 EST by Dan Horák
Modified: 2012-01-23 17:10 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2012-01-23 17:10:41 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dan: fedora‑review+
limburgher: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Dan Horák 2012-01-11 08:51:47 EST
Spec URL: http://fedora.danny.cz/ZipArchive.spec
SRPM URL: http://fedora.danny.cz/ZipArchive-4.1.1-3.fc17.src.rpm

The ZipArchive library provides C++ interface to zlib.
Comment 1 Susi Lehtola 2012-01-12 08:39:08 EST
I'd improve on the description, e.g. (adapted from http://www.artpol-software.com/ )

The ZipArchive Library can be used to add compression functionality to your software. It is written in C++ and offers the following features:
* Compression, decompression and modification of zip archives.
* Segmented archives support (splitting and spanning).
* Zip64 format - practically no limits for sizes and the number of files in archives.
* Unicode support in archives compatible with WinZip.
* AES encryption - in accordance with the WinZip format.
* Standard zip encryption.
* BZIP2 compression algorithm - provides a better compression ratio.  

Since bzip2 is bundled, you need to remove the relevant sources to make sure the package uses the system version of bzip2. And you need to add BR: bzip2-devel.
Comment 2 Susi Lehtola 2012-01-12 08:40:33 EST
The summary also needs improvement.

And I think this is incorrect:
 install -p -m 644 %{SOURCE1} $RPM_BUILD_ROOT%{_libdir}/cmake/%{name}
the installed file name should be FindZipArchive.cmake .
Comment 3 Dan Horák 2012-01-12 09:37:14 EST
Jussi, 3 notes to your comments
- the Zip64, AES and bzip2 features are available only in the commercial version of the library. The GPLed version provides only zlib interface
- both zlib and bzip2 (in fact completely useless here) are removed in %prep
- the cmake file is installed as %{_libdir}/cmake/ZipArchive/FindZipArchive.cmake, but I'll probably drop it as the pkgconfig file can be used easier in sigil/flightcrew
Comment 4 Robin Lee 2012-01-18 21:19:18 EST
*** Bug 782823 has been marked as a duplicate of this bug. ***
Comment 5 Hans de Goede 2012-01-19 03:35:13 EST
As discussed in the sigil review I filed, see bug 772362. I'm also interested in getting sigil (and thus ZipArchive and FlightCrew) into Fedora. But I did not know that Dan had already filed a review for ZipArchive, hence I filed a review request myself in bug 782823.

Luckily I was made aware of Dan's work by Dan in the sigil review so my ZipArchive work is based on top of his.

Here is my version of Dan's ZipArchive package with a few small fixes:
Spec URL: http://people.fedoraproject.org/~jwrdegoede/ZipArchive.spec
Comment 6 Hans de Goede 2012-01-19 09:31:36 EST
I've got the entire sigil dep chain finished, as a result of that here is a new version:

Spec URL: http://people.fedoraproject.org/~jwrdegoede/ZipArchive.spec

* Thu Jan 19 2012 Hans de Goede <hdegoede@redhat.com> - 4.1.1-5
- Drop custom cmake module, cmake using apps can use the .pc file
- Fix the .pc file to properly return -lziparch for --libs

Don't be fooled by the fc15 in the filenames, that is because I'm using the old makefiles from the distcvs era to build and test wip packages. The sigil packages will only work with F-16 and newer due to needing xerces-c 3.1
Comment 7 Dan Horák 2012-01-19 10:12:18 EST
For the records, Hans will be the submitter and me the reviewer.
Comment 8 Dan Horák 2012-01-19 10:35:02 EST
formal review is here, see the notes explaining OK* and BAD statuses below:

OK	source files match upstream:
		b81560f504917137bfa6927bb55c7eed02166fa3  ziparchive_src.zip
OK	package meets naming and versioning guidelines.
OK*	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
OK	license field matches the actual license.
OK	license is open source-compatible (GPLv2+). License text included in package.
OK	latest version is being packaged.
OK	BuildRequires are proper.
OK	compiler flags are appropriate.
OK	package builds in mock (Rawhide/i386).
OK	debuginfo package looks complete.
OK*	rpmlint is silent.
OK	final provides and requires look sane.
N/A	%check is present and all tests pass.
OK	shared libraries are added to the regular linker search paths with correct scriptlet
OK	owns the directories it creates.
OK	doesn't own any directories it shouldn't.
OK	no duplicates in %files.
OK	file permissions are appropriate.
OK	correct scriptlets present.
OK	code, not content.
OK	documentation is small, so no -docs subpackage is necessary.
OK	%docs are not necessary for the proper functioning of the package.
OK	headers in devel subpackage
OK	pkgconfig files in devel subpackage
OK	no libtool .la droppings.
OK	not a GUI app.

- description needs improvement and I'm almost sure I made one based on Jussi's comment, but can't find it now ...
- rpmlint complains about incorrect FSF address, upstream will be informed

The package is APPROVED.
Comment 9 Hans de Goede 2012-01-20 04:41:34 EST
New Package SCM Request
Package Name: ZipArchive
Short Description: The ZipArchive library
Owners: jwrdegoede sharkcz
Branches: f16

Danny, thanks for the review! I'll improve the description after the initial import into git.
Comment 10 Gwyn Ciesla 2012-01-20 08:53:45 EST
Git done (by process-git-requests).
Comment 11 Hans de Goede 2012-01-23 17:10:41 EST
ZipArchive has been imported and build for Rawhide and F-16. I don't intend to do a F-16 update in bodhi until we've the entire chain (ZipArchive, FlightCrew and Sigil) and then I'll push them all as one update -> closing.

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