Spec URL: http://kraxel.fedorapeople.org/osm/pngcrush/pngcrush.spec SRPM URL: http://kraxel.fedorapeople.org/osm/pngcrush/pngcrush-1.6.10-1.fc9.src.rpm Description: pngcrush is an optimizer for PNG (Portable Network Graphics) files. It can be run from a commandline in an MSDOS window, or from a UNIX or LINUX commandline. Its main purpose is to reduce the size of the PNG IDAT datastream by trying various compression levels an PNG filter methods. It also can be used to remove unwanted ancillary chunks, or to add certain chunks including gAMA, tRNS, iCCP, and textual chunks. Note 1: used by openstreetmap.org map rendering software. Note 2: binary packages: http://kraxel.fedorapeople.org/osm/fedora9/$basearch/
I'll take this. Before I do a full review, I see a few problems that need to be corrected. First, RPM_OPT_FLAGS are not respected during compilation. Change the make invocation in the spec file to read: %{__make} %{?_smp_mflags} CFLAGS="$RPM_OPT_FLAGS" Second, the source tar file includes all of the sources of cexcept, zlib, and libpng. This leads directly to the next two problems. Third, the program should be linked against -lz, and -lpng to pick up the latest and greatest security fixes, but isn't. This is required by the Packaging Guideliness. (See "Duplication of system libraries".) I realize that this package is shipping source for later versions of libpng and zlib than Fedora currently ships, but I don't see that as changing the situation. Fourth, there is no Fedora package of cexcept. It is a compile-time-only artifact, so that might be okay, but it has a different license, so I'm not sure. If you know of an official Fedora statement that this kind of thing is okay, please point me to it. Otherwise, you'll probably need to package cexcept first. Fifth, the license is not the same text as the zlib license. The intent seems to be the same, but I am not a lawyer. Please check with fedora-legal that this license can be called "zlib". Sixth, when I tried to compile with the system versions of zlib and libpng, I got this: pngcrush.c: In function ‘main’: pngcrush.c:2891: error: ‘chunk_name’ undeclared (first use in this function) pngcrush.c:2891: error: (Each undeclared identifier is reported only once pngcrush.c:2891: error: for each function it appears in.) This appears to be a pngcrush bug. Starting on line 2821, there is an #if expression that is missing "|| !defined(PNG_sTER_SUPPORTED)". Please report this bug upstream. Finally, please consider whether ChangeLog.txt ought be included as a %doc file.
cexcept licence text --------------- cut here --------------- Copyright (c) 2000-2008 Adam M. Costello and Cosmin Truta. This software may be modified only if its author and version information is updated accurately, and may be redistributed only if accompanied by this unaltered notice. Subject to those restrictions, permission is granted to anyone to do anything with this software. The copyright holders make no guarantees regarding this software, and are not responsible for any damage resulting from its use. --------------- cut here --------------- It's just a header file, defining a few macros which play tricks with setjmp and longjmp. ~10k size, 80% of that being documentation in comments. Making that a package of its own looks like overkill to me, especially as there is no shared library or similar which can be shared at runtime ...
Created attachment 322038 [details] pngcrunch license text
Uploaded updated package (release 2) to the same location.
Cexcept is "Copyright Only". pngcrush is a verbose zlib variant, but the restrictions are the same, so it is fine to call it "zlib". Lifting FE-Legal.
Sorry to be paranoid about the license, but reviewers are supposed to be paranoid, aren't they? :-) Speaking of paranoia, would you mind also removing crc32.h, deflate.h, inf*.h, and trees.h before compiling? It probably doesn't matter, but it will give me warm fuzzies. Also, it doesn't appear necessary to link with -lm, since that gets pulled in by libpng. And Patch0 is now unnecessary since you aren't using make any more. Did you consider making Changelog.txt a %doc file? Finally, according to https://fedoraproject.org/wiki/Packaging/SourceURL the source URL should refer to downloads.sourceforge.net. MUST items: - rpmlint output: pngcrush.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 0 errors, 1 warnings. - package name: OK - spec file name: OK - Packaging Guidelines: OK, except for the source URL and Patch0 - approved license: OK - license field: OK - license file: OK - spec in American English: OK - legible spec: OK - package sources match upstream: OK - successful compile on one arch: OK - ExcludeArch: OK - BuildRequires: OK - locales: OK - library files: OK - relocatable: OK - own all directories: OK - no duplicates files: OK - file permissions: OK - %clean section: OK - consistent use of macros: OK - code or permissible content: OK - large documentation files: OK - no runtime files in %doc: OK - header files: OK - static libraries: OK - pkgconfig files: OK - library files with no suffix in devel: OK - devel package Requires: OK - no libtool archives: OK - desktop files: OK - do not own files/dirs owned by other packages: OK - clean at start of %install: OK - filenames are UTF-8: OK SHOULD items: - query upstream for a license file: have you done this? - description and summary translations: OK - builds in mock: OK - builds into packages on all supported arches: did not check - package functions as described: OK with light testing - sane scriptlets: OK - subpackages require main package: OK - pkgconfig files in devel: OK - file dependencies: OK I believe the source URL must be changed, because it is referred to from the Packaging Guidelines, and so falls under a MUST item. The unnecessary Patch0 also falls afoul of the Packaging Guidelines, I believe, due to the section entitled "All patches should have an upstream bug link or comment". Just remove it. Please also consider the following (but I won't block approval for these): - Also remove crc32.h, deflate.h, inf*.h, and trees.h before compiling - Drop "-lm" from the link line - Make Changelog.txt a %doc file
Revision #3 uploaded http://kraxel.fedorapeople.org/osm/pngcrush/pngcrush.spec http://kraxel.fedorapeople.org/osm/pngcrush/pngcrush-1.6.10-3.fc10.src.rpm Headers are removed completely now. The patch holds the build fix for "‘chunk_name’ undeclared" now, thus it isn't obsolete. I'm using pkg-config now for cflags & ldlibs. Source0 URL has been fixed. ChangeLog.txt is now %doc. Being picky on the license is perfectly fine, better safe than sorry ;)
Looks good. APPROVED.
New Package CVS Request ======================= Package Name: pngcrush Short Description: Optimizer for PNG files Owners: kraxel Branches: F-9
cvs done.
committed & built now.
irqbalance-0.55-12.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/irqbalance-0.55-12.fc10
(In reply to comment #0) > Note 1: used by openstreetmap.org map rendering software. I noticed this review because of a mail on fedora devel. Are you talking about "tiles at home" / osmarender here? Last time I used it, the developers added support for optipng, a fork of pngcrush, which is also available in Fedora. But I do not know, which one is better for which usecase.
Yes, this referes to osmarender / tiles-at-home.
Package Change Request ====================== Package Name: pngcrush New Branches: el6 epel7 Owners: fcami
Package Change Request ====================== Package Name: pngcrush New Branches: el6 Owners: fcami Please ignore the request for the epel7 branch as pngcrush is already in RHEL7.
Git done (by process-git-requests).
Thank you Jon.