Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 225130 (smashteroid) - Review Request: smashteroid - Astrosmash Remake
Summary: Review Request: smashteroid - Astrosmash Remake
Keywords:
Status: CLOSED NEXTRELEASE
Alias: smashteroid
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Christopher Stone
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2007-01-29 08:59 UTC by Hans de Goede
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-02-01 10:02:24 UTC


Attachments (Terms of Use)

Description Hans de Goede 2007-01-29 08:59:40 UTC
Spec URL: http://people.atrpms.net/~hdegoede/smashteroid.spec
SRPM URL: http://people.atrpms.net/~hdegoede/smashteroid-1.11-1.fc7.src.rpm
Description:
Smashteroid is a remake of the old Intellivision game Astrosmash. Your job is
to defend Earth from the onslaught of asteroids.  Features three game modes
with exciting new twists.

Comment 1 Jochen Schmitt 2007-01-29 17:21:51 UTC
Good:
+ License look fine.
+ Tarball matchs with upstream.
+ Nameing seams ok.
+ Local build works fine
+ Gaming is running on my machine.
+ Installing and uninstalling works fine.
+ Build on mock works fine.
+ rpmlint is quite on source and binary package.
+ rpmlint is quite on install package.

Bad:
- Please include license material to the docs.

Comment 2 Christopher Stone 2007-01-29 19:48:15 UTC
What the hell?  I was reviewing this package?  Why did you remove my name from
the assigned field?

Comment 3 Christopher Stone 2007-01-29 19:48:53 UTC
Here is my review:
==== REVIEW CHECKLIST ====
- rpmlint output clean
- package named according to package naming guidelines
- spec filename matches %{name}
- package meets packaging guidelines
- licensed with open source compatible license
- license field matches actual license
- license text included in %doc
- spec written in American english
- spec legible
- sources match upstream
fbdd2aed12da3f2e4802c629fcdd7979  astro111src.zip
- successfully compiles and builds on FC-6 x86_64
- all build dependencies listed in BR
- no locales
- no shared libraries
- not relocatable
- package owns all directories it creates
X package does not pull in all directories it uses
- no duplicates in %files
- file permissions set properly
- contains proper %clean
- macro usage consistent
- contains code
- no large documentation
- files in %doc do not affect runtime
- no header files or static libraries
- no pkgconfig files
- no library files with suffix
- no devel subpackage required
- no libtool archives
- contains proper .desktop file
- does not own files or directories owned by other packages

==== MUST FIX ====
- Shouldnt this require hicolor-icon-theme?

==== SHOULD FIX ====
- Use http://www.t3-i.com/smashteroid.htm as URL
- inform upstream of 64bit compiler warnings

==== QUESTIONS ====
- patch seems to include license-change.txt but its not included in %doc?
- patch seems littered with ^Ms, not an issue, but difficult to read

Comment 4 Christopher Stone 2007-01-29 19:49:42 UTC
Re-assigning bug to me because I was reviewing it before Jochen so rudely
removed my name from asigned feild.

Comment 5 Christopher Stone 2007-01-29 19:51:29 UTC
Readding alias ... Jochen why did you remove the alias too?

Comment 6 Christopher Stone 2007-01-29 19:59:01 UTC
I'm guessing that what happened was that Jochen got a mid-air collision and
decided to completely ignore it and overwrite my changes to this bug....

Comment 7 Christopher Stone 2007-01-31 02:49:24 UTC
Is it easy to patch this so that high scores go in /var/games/smashteroid?

Comment 8 Hans de Goede 2007-01-31 08:50:04 UTC
Jochen, Christopher, both thanks for your review.

(In reply to comment #3)
> Here is my review:
> ==== MUST FIX ====
> - Shouldnt this require hicolor-icon-theme?
> 
Fixed

> ==== SHOULD FIX ====
> - Use http://www.t3-i.com/smashteroid.htm as URL
Fixed

> - inform upstream of 64bit compiler warnings
I need to send all my changes upstream, if I get around to it I'll fix all the
warnings (also the non 64 bit ones) before doing so.
> 

> ==== QUESTIONS ====
> - patch seems to include license-change.txt but its not included in %doc?
Yes, a must-fix actually as Jochen has correctly identified.

> - patch seems littered with ^Ms, not an issue, but difficult to read
Thats what you get when you take dos source code and port it to Linux with a
patch :)


(In reply to comment #7)
> Is it easy to patch this so that high scores go in /var/games/smashteroid?

Doable yes, easy unfortunately not. Not worth the work IMHO. A patch is ofcourse
welcome.

Here is a new version:
* Wed Jan 31 2007 Hans de Goede <j.w.r.degoede@hhs.nl> 1.11-2
- Not only create but actually package license-change.txt
- Add Requires: hicolor-icon-theme
- Use: http://www.t3-i.com/smashteroid.htm as URL

Go get it here:
Spec URL: http://people.atrpms.net/~hdegoede/smashteroid.spec
SRPM URL: http://people.atrpms.net/~hdegoede/smashteroid-1.11-2.fc7.src.rpm


Comment 9 Christopher Stone 2007-01-31 15:16:01 UTC
Looks like all MUST FIX items have been fixed.

APPROVED.

Comment 10 Hans de Goede 2007-02-01 10:02:24 UTC
Imported and build, closing.



Note You need to log in before you can comment on or make changes to this bug.