Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 194436
Summary: | Review Request: wormux - 2D Kill 'em all game | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Wart <wart> |
Component: | Package Review | Assignee: | Christopher Stone <chris.stone> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | ||
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-06-14 20:05:26 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Wart
2006-06-08 06:01:23 UTC
[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 |