Spec URL: http://www.devin.com.br/eitch/rpm/fedora/SPECS/netpanzer-data.spec SRPM URL: http://www.devin.com.br/eitch/rpm/fedora/5/SRPMS/netpanzer-data-0.8-1.src.rpm Description: This package contains the graphics and sounds for netpanzer, an online multiplayer tactical warfare game designed for FAST ACTION combat. This is my first set of packages, so I need a sponsor please :)
Good: * proper naming * spec file name matches %{name} * package meets packaging guidelines * License is GPL, License meets packaged COPYING * Spec file written in American English * Spec file is understandable * Package succesfully builds in mock on devel x86_64 and FC-5 x86 * No locales/shared libraries to worry about * No static/libtool files * Package not relocatable * Package owns all directories it creates * No duplicate files * Proper file permissions, proper %defattr(...) in spec file * Package contains necessary game-data for netpanzer, conforming to the Packaging Guidelines * No need for separate doc package * %doc files not needed for runtime * No header/other devel package files to worry about * Desktop File not needed * Package is correctly set to noarch. Approved, pending sponsorship and successfull review of #190396
Updated package: Spec URL: http://www.devin.com.br/eitch/rpm/fedora/SPECS/netpanzer-data.spec SRPM URL: http://www.devin.com.br/eitch/rpm/fedora/5/SRPMS/netpanzer-data-0.8-2.src.rpm Changes: - Changed Package's RPM Group - Fixed Changelog entries to specify versions - Stripped '\r' EOL from RELNOTES file
Thanks for updating the package with my suggestions as well as the other suggestions on IRC. Gonna take a look at the package tomorrow though.
I can sponsor and you seem worthy of sponsering concedering your quick and correct reactions to this review and your other opensource and Fedora (translation / writing) work. Assigning to me. I'll do a Review as time permits.
I'll keep my review short (again) as Andreas has already done most of the work. I only see 1 problem, why all the BuildReqs? I see no need for desktop-file-utils and most of the others seem superficial too.
While contents of this packages includes only data-files, they are included with a ./configure script and jam (make substitute). This is really strange indeed, but this configure searchs for these dependencies, just like the main netpanzer package (Bug #190396). As I like to work in the ways upstream works, I thought that it would be good to follow this scheme. But you're right about desktop-file-utils, it was a copy and paste error of mine, sorry :) I'm updating the spec and rebuilding the package after you comment about my opinion above. Thanks!
I already thought it might be such a thing (a strange configure script) in that case leaving the BR's in is fine. A comment in the .spec about this might be a good idea though.
Updated package: Spec URL: http://www.devin.com.br/eitch/rpm/fedora/SPECS/netpanzer-data.spec SRPM URL: http://www.devin.com.br/eitch/rpm/fedora/5/SRPMS/netpanzer-data-0.8-3.src.rpm Changes: - Removed desktop-file-utils BuildReq entry Notes: Commenting the BuildReq indeed is good, so I did it ;)
Looking good, _approved_! One request though, could you remove the %{?dist} from the release field before importing (don't forget to rebuild the .src.rpm)? There has been some discussion about large .noarch content packages like this eating up a lot of diskspace on the mirrors. So the idea is to build it once and then copy it over to the other branches. So you also won't need to request an FC-5 branch for this on the CVS-admin page. Instead once its build for devel (GRRR building on devel i still broken), mail Warren Togami <wtogami> with a request to copy it to the FC-5 repo. This was disccused here: https://www.redhat.com/archives/fedora-games-list/2006-May/msg00001.html Currently this doesn't help the mirrors, but in the future the copy may be replaced by a link.
Updated package: Spec URL: http://www.devin.com.br/eitch/rpm/fedora/SPECS/netpanzer-data.spec SRPM URL: http://www.devin.com.br/eitch/rpm/fedora/5/SRPMS/netpanzer-data-0.8-3.src.rpm Notes: Like Comment #9 said, removed the %{?dist} var from the Release field.
Package built for devel, and will be copied by Warren Togami for the other branches. I think the work is completed, closing ;)
Change owner to limb (orphaned)
Add lxtnow as co-maintainer.