Two minute review:

-BR is missing allegro-devel
-rpmlint errors/warnings:
W: shippy no-documentation
E: shippy non-standard-executable-perm /usr/bin/shippy 02755
W: shippy-allegro no-documentation
E: shippy-allegro non-standard-executable-perm /usr/bin/shippy-allegro 02755

I guess these are okay to be ignored.  I tried running shippy and fullscreen
mode does not work properly on my LCD monitor.  The screen is shifted to the
right and down so I only see the upper-left portion of the screen.  Probablly
due to a bad X11 config on my part.

-package name okay
-If I only install shippy-allegro I do not get a desktop entry
-Shouldnt shippy-data be noarch?
-Could not find:
http://download.sourceforge.net/ship84/shipv1.3.3.7UNIX.zip
-Could not verify source due to error above
-Illegal use of elite version numbers
-The angry Fez won!

(In reply to comment #1)
> Two minute review:
> 
> -BR is missing allegro-devel

ditto.

> -rpmlint errors/warnings:
> W: shippy no-documentation

Perhaps the docs from shippy-data should move to the base shippy package. 
rpmlint will warn about no docs for shippy-data, but that's fine.

> E: shippy non-standard-executable-perm /usr/bin/shippy 02755
> W: shippy-allegro no-documentation

Either make shippy-allegro require shippy, or duplicate the docs in
shippy-allegro.  Based on my .desktop file comments below, I would recommend the
former

> E: shippy-allegro non-standard-executable-perm /usr/bin/shippy-allegro 02755

This is allowed per the Games SIG packaging guidelines for shared scoreboard files.

> I guess these are okay to be ignored.  I tried running shippy and fullscreen
> mode does not work properly on my LCD monitor.  The screen is shifted to the
> right and down so I only see the upper-left portion of the screen.  Probablly
> due to a bad X11 config on my part.
> 
> -package name okay
> -If I only install shippy-allegro I do not get a desktop entry

Since there are two executables (shippy shippy-allegro), I would suggest
changing the name of the executable 'shippy' to 'shippy-std' and providing a
wrapper script that invokes shippy-allegro, if present, or shippy-std if not. 
This wrapper should be in the base 'shippy' package and called by the .desktop
file.  This will allow the .desktop file to launch the best available frontend
found, but would also force shippy-allegro to Requires: shippy.

> -Shouldnt shippy-data be noarch?

It should, but you can't provide arch and noarch packages from a single spec
file.  Unless upstream provides a separate tarball for the -data files, the use
of a single spec that generates an arch-specific -data package is fine.

> -Could not find:
> http://download.sourceforge.net/ship84/shipv1.3.3.7UNIX.zip
> -Could not verify source due to error above
> -Illegal use of elite version numbers

??  As far as I'm concerned, if upstream uses 1.3.3.7 as the version number, the
spec file should as well.  I wonder what they'll change it to if they ever
release a new version...

> -The angry Fez won!

No kidding!  Curse the angry Fez!

> I would suggest changing the name of the executable 'shippy' to 
> 'shippy-std' and providing a wrapper script that invokes
> shippy-allegro, if present, or shippy-std if not.

Or perhaps make a shippy-allegro, shippy-sdl, and shippy, where shippy is the
wrapper script that calls which ever version it finds.

>> -Illegal use of elite version numbers

> ??  As far as I'm concerned, if upstream uses 1.3.3.7 as the version number

Yes, I just wanted the point out the humor in the version number incase anyone
else missed it ;-)

About the missing BR allegro-devel, no it is not dumb-devel requires
allegro-devel. Erm that is dumb-devel should require allegro-devel. I'll fix
this in dumb right away.

About the 2 versions mess, I suggest (and lets make this game SIG policy for
similar cases, which will happen):
-rename shippy-data to shippy-common leave the docs there
-put a wrapper which wille executa any shippy found prefering the SDL version
 in shippy-common
-mv desktop and icon to shippy-common, make desktop call the wrapper
-make -common require shippy-engine
-make both shippy and shippy-allegro provide shippy-engine and require
 shippy-common
-a regular user and/or one using comps will install shippy which will suck in 
 -common, result everything ok
-someone who want the allegro version can install shippy-allegro which will suck
 in -common, result everything ok
-someone who insists on doing a "yum install shippy-common" will get shippy
 (shortest name) resulting in a working setup too.

Please ack if this is ok, then I can make it so.

About the not showing properly on an LCD thats my fault. I've modified it so
that it starts fullscreen by default (it used to be in a window and only in a
window)
I've also made it so that alt+enter toggles fullscreen<->window.

The LCD problem is because by default it uses (I didn't change this part) a
funky resolution of 480x320 (which gives nice oldscool scanlines on my CRT).
I've uploaded a new SRPM (sorry I didn't bump release) with an extra patch which
does the following:
-make the default res 640x480
-adds a -w cmdline option to start in a window instead of fullcreen
-adds a -a (arcade) option to get the 480x320 resolution instead of 640x480
-allegro-version: be nice (sleep when idle) (sdl already had this).

So this new SRPM should (once compiled) fix things on your LCD.

New version (same location as previous) at:
Spec Name or Url: http://home.zonnet.nl/jwrdegoede/shippy.spec
SRPM Name or Url: http://home.zonnet.nl/jwrdegoede/shippy-1.3.3.7-1.src.rpm

tkmame, adding you to CC because you seem interested and didn't do so yourself,
please read my previous comments (replies to your comments).

Michael, adding you to CC because you seem interested and didn't do so yourself,
please read my previous comments (replies to your comments).

(In reply to comment #7)
> Michael, adding you to CC because you seem interested and didn't do so yourself,
> please read my previous comments (replies to your comments).

Oops.  You're right, I forgot to cc myself.  I've been following the thread in
f-e-l, though.

tkmame was the first to comment on this package, so I'll let him take the review
if he wants it. Otherwise I'll assign it to myself.

(In reply to comment #8)
> tkmame was the first to comment on this package, so I'll let him take the review
> if he wants it. Otherwise I'll assign it to myself.
> 

Ok,

Any chance you could comment on comment 4, this needs some kinda concensus
before I can move forward.

(In reply to comment #4)
> About the 2 versions mess, I suggest (and lets make this game SIG policy for
> similar cases, which will happen):
> -rename shippy-data to shippy-common leave the docs there
> -put a wrapper which wille executa any shippy found prefering the SDL version
>  in shippy-common
> -mv desktop and icon to shippy-common, make desktop call the wrapper
> -make -common require shippy-engine
> -make both shippy and shippy-allegro provide shippy-engine and require
>  shippy-common
> -a regular user and/or one using comps will install shippy which will suck in 
>  -common, result everything ok
> -someone who want the allegro version can install shippy-allegro which will suck
>  in -common, result everything ok
> -someone who insists on doing a "yum install shippy-common" will get shippy
>  (shortest name) resulting in a working setup too.
> 
> Please ack if this is ok, then I can make it so.

This sounds fine to me.  Can you generalize the rule and add it to the Games SIG
page?  This affects xpilot-ng, as it simply packages two .desktop files in the
client package instead of using a wrapper script.

Hi, I can take the review, but I dont know if I have the authority to approve
something yet.  But I can atleast do the tedious dirty work for you.

Hans:  I tested your latest fullscreen patch and it works great with both SDL
and allegro versions!

(In reply to comment #11)
> Hi, I can take the review, but I dont know if I have the authority to approve
> something yet.  But I can atleast do the tedious dirty work for you.

If you have already received sponsorship and approval for your first package
then you have authority to approve other packages.  I don't see you in my
(2-week old) copy of owners.list, so you might not have approval rights yet.  If
that's the case then I'll take ownership of this, but I'll refrain from giving
it a final approval until you say it's ok.  :)

I just received sponsorship today, but not approval of my first package yet.  I
did talk to my sponsor and he said I can approve other packages, so I'll go
ahead and review this one when it's ready.

(In reply to comment #10)
> (In reply to comment #4)
> > About the 2 versions mess, I suggest (and lets make this game SIG policy for
> > similar cases, which will happen):
> 
> This sounds fine to me.  Can you generalize the rule and add it to the Games SIG
> page?
Sure will do.

> This affects xpilot-ng, as it simply packages two .desktop files in the
> client package instead of using a wrapper script.
> 

I'm not sure if xpilot-ng is similar judging by the name and .desktop comments
this really are 2 different versions. Whereas with shippy both version are 100%
identical to the user except that the titlebar says SDL or allegro.

The only difference between the 2 versions of shippy is the display library they
use. I've also contemplated on just choosing one version and only including
that, but hey choice is everything right?

About the broken download URL, it seems all download.sf.ne tis completly broken,
any ideas?

If you're referring to the URL in comment 1, it works for me in Firefox.  Wget
seems to be eventually (I have timeout = 10 in ~/.wgetrc) redirected to
belnet.dl.sf.net which doesn't work, but heanet.dl.sf.net does.

I've uploaded a new version, doing "things" as described in comment 4. I didn't
know what to put in a changelog entry for all these changes, so I just left the
release field at 1 :)

Go get it (and review it) from:
Spec Name or Url: http://home.zonnet.nl/jwrdegoede/shippy.spec
SRPM Name or Url: http://home.zonnet.nl/jwrdegoede/shippy-1.3.3.7-1.src.rpm

New version:
* Thu Mar 30 2006 Hans de Goede <j.w.r.degoede> 1.3.3.7-2
- Fix reversed joy directions in allegro version
- Improve support for analog joysticks in the allegro version
- Add support for joysticks to the SDL version

Go get it (and review it) from:
Spec Name or Url: http://home.zonnet.nl/jwrdegoede/shippy.spec
SRPM Name or Url: http://home.zonnet.nl/jwrdegoede/shippy-1.3.3.7-2.src.rpm

I haven't seen any comments from tkmame lately, so here's a full review. 
tkmame:  please assign this to yourself if you would still like to give the
final approval.

MUST
====
rpmlint warnings:
W: shippy no-documentation
W: shippy-allegro no-documentation

Perhaps put at least the license file in each of these?  The rest of the
docs are in shippy-common, which is fine.

E: shippy non-standard-executable-perm /usr/bin/shippy-sdl 02755
E: shippy-allegro non-standard-executable-perm /usr/bin/shippy-allegro 02755

This is allowed per the Games SIG guidelines for shared scoreboard files.

E: shippy-common score-file-must-not-be-conffile /var/lib/games/shippy.hs

This will go away if you move it to /var/games per the FHS.

E: shippy-common zero-length /var/lib/games/shippy.hs

Empty initial scoreboard file.  This is fine.

* Package and spec named appropriately
* License (GPL) ok, license file included
* Spec file legible, in Am. English
* Source matches upstream
  06df2ae060fe4a076d7fa17a57205348  shipv1.3.3.7UNIX.zip
* compiles and builds on FC5 i386
* No excessive or offensive BR:
* No locales
* No shared libraries
* Not relocatable
* Owns directories that it creates (/usr/share/shippy)
* Permissions look ok.  setgid binary acceptable (see rpmlint warnings above)
* %install and %clean both clean $RPM_BUILD_ROOT
* %doc does not affect runtime
* Contains code and permissible content
* No -devel package
* .desktop file included and installed properly
* Runs without crashing

RECOMMENDED
===========
* Compiler warning:

shipall.c: In function 'SYSTEM_INIT':
shipall.c:264: warning: 'set_window_close_hook' is deprecated (declared at
/usr/include/allegro/alcompat.h:198)

This is probably safe for now, but you might want to consider not using the
deprecated function to avoid problems with future versions of allegro that
might remove it.

* Move the high score file from /var/lib/games to /var/games per FHS.  This will
also clean up one of the rpmlint warnings.

I don't consider either of the RECOMMENDED items blockers, but it would be nice
if Christopher could verify the joystick patch (no game port on my desktop,
unfortunately).

- MUST: rpmlint must be run on every package. The output should be posted in the
review.

W: shippy no-documentation
OKAY: documentation is in shippy-common which is required by shippy

E: shippy non-standard-executable-perm /usr/bin/shippy-sdl 02755
OKAY: needed to save high scores

W: shippy-allegro no-documentation
OKAY: documentation is in shippy-common which is required by shippy-allegro

E: shippy-allegro non-standard-executable-perm /usr/bin/shippy-allegro 02755
OKAY: needed to save high scores

E: shippy-common score-file-must-not-be-conffile /var/lib/games/shippy.hs
????: I have no idea what this means and rpmlint -I didn't have any idea either.

E: shippy-common zero-length /var/lib/games/shippy.hs
OKAY: Safe to ignore.

package name OKAY
spec file name OKAY
package meets package guidelines OKAY
package OSI license OKAY
package matches license in source OKAY
license in %doc OKAY
spec file in english OKAY
spec file legible OKAY
sources match upstream sources OKAY
package successfully builds on x86_64 for FC5 OKAY
package does not contain any extra BRs OKAY
all dependencies in BR OKAY [1]
no locales in spec OKAY
package does not contain shared libraries OKAY
package is not relocatable OKAY
package owns directories it creates OKAY
package contains no duplicate files OKAY
file permissions set correctly OKAY
package contains good %clean section OKAY
macro use is consistant OKAY
pacakge contains permissible content OKAY
package does not contain large docs OKAY
%doc files do not affect runtime OKAY
package does not contain any files that should go in -devel OKAY
package does not contain any .la files OKAY
package contains a %{name}.desktop OKAY
package does not own files owned by other packages OKAY

One other thing I noticed is when I try to install shippy-common I get:
# rpm -ivh shippy-common-1.3.3.7-2.x86_64.rpm
error: Failed dependencies:
        shippy-engine = 1.3.3.7 is needed by shippy-common-1.3.3.7-2.x86_64

however, there is no shippy-engine package, just shippy.

APPROVED! [2][3]

[1] dumb-devel requires allegro-devel must be fixed!  This still fails in mock
for FC5.
[2] Since this is my first ever official approval perhaps wart or someone might
want to take a quick 2nd look
[3] Please review that rpmlint error which I couldn't understand, and look into
why installing shippy-common asks for shippy-engine when the package name is
just "shippy".

Whew!  P.S.  Joystick works great in both versions now!

Seems we did the reviews at the same time.  I tried to assign this bug to me,
but I do not see where this is done?  Perhaps I don't have permissions yet to
assign myself to bugs?  Not sure why I can't do that yet.

(In reply to comment #20)

> One other thing I noticed is when I try to install shippy-common I get:
> # rpm -ivh shippy-common-1.3.3.7-2.x86_64.rpm
> error: Failed dependencies:
>         shippy-engine = 1.3.3.7 is needed by shippy-common-1.3.3.7-2.x86_64


> however, there is no shippy-engine package, just shippy.

shippy-engine is provided by both the base shippy package and shippy-allegro. 
You need to install shippy-common with one of these to get the full game. 
Fortunately, yum will make sure that the dependencies are satisfied.

> APPROVED! [2][3]
> 
> [1] dumb-devel requires allegro-devel must be fixed!  This still fails in mock
> for FC5.

If I'm not mistaken, Hans has (or is in the process of) fixing this in the dumb
package.

> [2] Since this is my first ever official approval perhaps wart or someone might
> want to take a quick 2nd look

Already did.  :)

> [3] Please review that rpmlint error which I couldn't understand,

rpmlint does not like high score files in /var/lib/games being marked as
configuration files.  Perhaps we need to discuss this in the Games SIG.  Should
scoreboard files be marked %config(noreplace) or not?  They aren't really
configuration files, but we also don't want to have them replaced during an upgrade.

> and look into
> why installing shippy-common asks for shippy-engine when the package name is
> just "shippy".

See comment 4 for the explanation of shippy-engine.

(In reply to comment #21)
> Seems we did the reviews at the same time.  I tried to assign this bug to me,
> but I do not see where this is done?  Perhaps I don't have permissions yet to
> assign myself to bugs?  Not sure why I can't do that yet.

There is a "Bug Reassignment" area at the bottom of the bug page.  Enter your
email address next to "Reassign bug to ___" and hit "Save Changes".  It seems
that Ignacio has done this for you already.  :)

One more thing:  Once you APPROVE a package, you should change the blocking bug
from FE-REVIEW to FE-ACCEPT.

Thanks for the reviewS guys!

Michael, thanks for answering most of Christophers (tkmame's) questions.

About dumb-devel, this is already fixed in devel, but not for FC-5, I'll fix
this for FC-5 too, good piont.

About the score file should not be config warning I took a look how gnome-games
handles this and it uses:
%config(noreplace) %attr(664, games, games) /var/lib/games/*
So I guess this is ok and the rpmlint warning should be removed or ignored.
This still leaves the /var/games vs /var/lib/games issue, see bug 165425 . I'm
sticking with /var/lib/games for now as thats in filesystem, this will however
cause migration issues when moving to /var/games <sigh> .

Okay, sounds good.  Moving bug over to FE-ACCEPT.

Imported and Build, thanks!