Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Review Request: wormux - 2D Kill 'em all game https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=194436 wart@xxxxxxxxxx changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution| |NEXTRELEASE AssignedTo|bugzilla-sink@xxxxxxxxxxxxx |chris.stone@xxxxxxxxx OtherBugsDependingO|163776 |163779 nThis| | ------- Additional Comments From wart@xxxxxxxxxx 2006-06-14 16:05 EST ------- [Reposting review comments due to bugzilla data loss] ------- Additional Comments From hugo@xxxxxxxxxxxx 2006-06-09 16:09 EST ------- My Review: OKS * Package's name is ok * rpmlint returns ok * Spec name is ok and legible in English * License is ok and included in %doc * Package's source matches upstream checksum * Package builds fine * BuildRequires are ok * Package's %files are ok and all directories owned * Package's %clean is ok * Use of macros and scriptlets are fine SHOULD * Currently the package doesn't have locale files and the %find_lang macro isn't necessary. But in the future this could change, so I think it should be used from now * You should create a sub-package to the data files, for example, wormux-data. This will save the users' bandwidth when minor fixes are applied to the main package. * The description field is too vague. Try describing the game better ;-) ------- Additional Comments From wart@xxxxxxxxxx 2006-06-09 17:09 EST ------- (In reply to comment #1) >> SHOULD >> >> * Currently the package doesn't have locale files and the %find_lang macro >> isn't necessary. But in the future this could change, so I think it should be >> used from now I prefer not to include %find_lang until there are actual lang files. Otherwise it leads to extra misleading cruft in the spec file. >> * You should create a sub-package to the data files, for example, >> wormux-data. This will save the users' bandwidth when minor fixes are applied >> to the main package. Good point. Done. Note that this won't actually reduce the size of the downloads, since a new -data subpackage will automatically get built when the game engine is updated. upstream should package the game data in a separate tarball for us to really benefit from the -data subpackage. >> * The description field is too vague. Try describing the game better ;-) Yeah, I kinda flaked on that. I tend to cut and paste the descriptions from the packages' home pages. In this case, the home page didn't have a decent description when I first wrote the spec. It should look better now. http://www.kobold.org/~wart/fedora/wormux-0.7.2-2.src.rpm http://www.kobold.org/~wart/fedora/wormux.spec ------- Additional Comments From chris.stone@xxxxxxxxx 2006-06-09 18:06 EST ------- * rpmlint output okay: W: wormux-data no-documentation * Package is named according to package naming guidelines * spec file matches package %{name} * Package meets packaging guidelines * Package licensed with open source compatible license * License in spec file matches actual license * License text file included in %doc * Spec file written in American english * Spec file is legible * Sources match upstream source 08d897a89f06cb855709be2904308cac wormux-0.7.2.tar.gz * Package successfully compiles and builds on x86_64 FC-5 * All build dependencies are listed in BuildRequires - Except for pkgconfig which will be needed for FC-6 * No locales in package * Package does not make a shared library * Package is not relocatable * Package owns all directories it creates * Package does not contain duplicate files in %files * Permissions on files set properly * Package has appropriate %clean section - Macro usage is not consistant * Package contains permissible content * Package does not contain large documentation to warrent a -doc subpackage * Files in %doc do not affect runtime of application * Package does not contain header files or static libraries * Package does not contain any .pc files * Package does not contain any .so files * Package does not need a devel subpackage * Package does not contain any .la files * Package contains a nearly appropriate .desktop file - .desktop file missing Encoding section * Package does not own files or directories owned by other packages MUST: - Add pkgconfig to BuildRequires for FC6 builds - Add "Encoding" field to .desktop file - Use %{buildroot} consistantly, there is an $RPM_BUILD_ROOT adn %{buildroot} - Remove INSTALL from %files Notes: - Credits button doesn't seem to do anything, not sure if its broken or just not implemented yet. ------- Additional Comments From wart@xxxxxxxxxx 2006-06-09 18:44 EST ------- (In reply to comment #3) >> MUST: >> - Add pkgconfig to BuildRequires for FC6 builds This shouldn't be necessary. There is already a BR: libxml++-devel which itself requires pkgconfig >> - Add "Encoding" field to .desktop file Fixed >> - Use %{buildroot} consistantly, there is an $RPM_BUILD_ROOT adn %{buildroot} Fixed >> - Remove INSTALL from %files Fixed >> Notes: >> - Credits button doesn't seem to do anything, not sure if its broken or just not >> implemented yet. I'll take a closer look at that. Here's a new package that fixes all of the above except for the broken credits button: http://www.kobold.org/~wart/fedora/wormux-0.7.2-3.src.rpm http://www.kobold.org/~wart/fedora/wormux.spec ------- Additional Comments From chris.stone@xxxxxxxxx 2006-06-09 18:58 EST ------- ** APPROVED ** Noticed that "exerminate" is missing a "t". Non-blocker, but please fix. ------- Additional Comments From wart@xxxxxxxxxx 2006-06-09 19:02 EST ------- I also discovered a bad icon path in the -3 release. I'll fix that as well before checking in. Thanks for the review! ------- Additional Comments From wart@xxxxxxxxxx 2006-06-09 22:50 EST ------- Imported and built on -devel -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA contact for the bug, or are watching the QA contact. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review