[Reposting review comments due to bugzilla data loss]
------- Additional Comments From hugo.br  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  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  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  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  2006-06-09 18:58 EST -------
** APPROVED **

Noticed that "exerminate" is missing a "t".  Non-blocker, but please fix.

------- Additional Comments From wart  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  2006-06-09 22:50 EST -------
Imported and built on -devel