Bug 249725 - Review Request: bombardier - The GNU bombing utility
Review Request: bombardier - The GNU bombing utility
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Wart
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-26 13:45 EDT by Gwyn Ciesla
Modified: 2007-11-30 17:12 EST (History)
3 users (show)

See Also:
Fixed In Version: 0.8.2.2-5.fc7
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-30 13:04:13 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wart: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Gwyn Ciesla 2007-07-26 13:45:39 EDT
Spec URL: http://zanoni.jcomserv.net/fedora/bombardier/bombardier.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/bombardier/bombardier-0.8.2.2-1.fc7.src.rpm
Description: Fly an ncurses plane over an ncurses city, and try to level the buildings.
Comment 1 Wart 2007-07-26 13:58:56 EDT
It would be good to add a .desktop file that launches the game in a terminal window.
Comment 2 Gwyn Ciesla 2007-07-26 14:06:56 EDT
I can do that, but then what do I use for an icon?  Should I roll my own?  I've
done that for other ncurses games before. :)
Comment 3 Wart 2007-07-26 14:09:36 EDT
For some curses based games, I made a graphic of the character that represents
the player ('@' in rogue and ularn).  You could do the same with the airplane in
bombardier.
Comment 4 Gwyn Ciesla 2007-07-26 14:18:37 EDT
That's what I had in mind.  I'm polishing an ncurses homage to defender that
I've not even released yet, and it's icon is the player's ship, <_==>. :) 

I'll do that and re-upload.
Comment 5 Gwyn Ciesla 2007-07-26 14:45:21 EDT
Spec URL: http://zanoni.jcomserv.net/fedora/bombardier/bombardier.spec
SRPM URL:
http://zanoni.jcomserv.net/fedora/bombardier/bombardier-0.8.2.2-2.fc7.src.rpm

Done, but the terminal opens 80x24, and bombardier needs 80x25.  Is there a way
to set that in the .desktop file?  I've dug around and can't find anything.
Comment 6 Wart 2007-07-26 15:07:06 EDT
One possibility to solve the size problem is to change the HEIGHT setting in
values.h from 24 to 23.  The game still seems to play fine with this smaller
setting.

GOOD
====
 * Sources matches upstream:
    1a557bdfc61a7d53963a7637624a99e4  bombardier_0.8.2.2.tar.gz
 * Packages runs without crashing
 * Package named appropriately
 * GPL license ok, license file included
 * Spec file legible and in Am. English
 * Compiles and builds on F7-x86_64
 * No locales
 * No shared libs
 * Not relocatable
 * Does not create directories that it should own
 * No need for -devel or -static subpackages
 * Filenames are UTF-8

MUSTFIX
=======
 * Include the manpage
 * Package does not honor RPM_OPT_FLAGS
 * desktop-file-utils should be part of BuildRequires, not Requires

SHOULD
======
 * Include a .desktop file that launches the game in a terminal window
 * Enable the shared highscore file via a setgid executable.  Ideally this
   highscore file would be moved out of its own directory from
   /var/games/bombardier/bdscore to /var/games/bdscore, but that's just a minor
   nit.

NOTES
=====
 * You can further simplify the %install section by creating the destination
   directory and installing the executable in one step:
   install -pD -m 755 bombardier %{buildroot}%{_bindir}/bombardier
Comment 7 Gwyn Ciesla 2007-07-26 15:32:10 EDT
Spec URL: http://zanoni.jcomserv.net/fedora/bombardier/bombardier.spec
SRPM URL:
http://zanoni.jcomserv.net/fedora/bombardier/bombardier-0.8.2.2-3.fc7.src.rpm

Added man page, rpm_opt_flags, moves d-f-u to br.  Used single install line. 
Might address shared high score file in the future.
Comment 8 Gwyn Ciesla 2007-07-26 15:32:35 EDT
Oh, and added patch for height.
Comment 9 Wart 2007-07-26 17:57:29 EDT
* The 'Version' field in the desktop file refers to the .desktop file
  specification version, not the application version.  Should be '1.0'.

* Use the %{_mandir} macro for the man page directory.  You also don't
  need to compress it yourself; rpm will do that for you:

install -pD -m 644 bombardier.6 %{buildroot}%{_mandir}/man6/bombardier.6

* RPM_OPT_FLAGS is still not being used on the compile line.  You can
  add a single colon in the Makefile to get it to pick up CFLAGS from the
  command line:

Makefile:
  CFLAGS:=-Wall -g -Werror -O2

...and in %build:
make CFLAGS="$RPM_OPT_FLAGS"
Comment 11 Wart 2007-07-27 11:39:44 EDT
Almost there...

/usr/share/man still needs to be macroized in the %files section.  You also
don't need to explicitly create the /usr/share/man/man6 directory in %install
anymore, since this is done by the 'install -pD ...' line now.

Mixed use of $RPM_BUILD_ROOT and %{buildroot}.  Either is ok, but you should
consistently use one or the other.

rpmlint out out is clean (good!)
Comment 13 Wart 2007-07-27 15:38:10 EDT
Looks good.  APPROVED

I'll spend some time this weekend to try to come up with a usable setgid
solution to allow the shared scoreboard file.
Comment 14 Gwyn Ciesla 2007-07-27 15:49:18 EDT
Excellent. Thank you!

New Package CVS Request
=======================
Package Name: bombardier
Short Description: The GNU bombing utility
Owners: limb@jcomserv.net
Branches: F-7 FC-6
InitialCC: 
Comment 15 Fedora Update System 2007-07-30 13:04:07 EDT
bombardier-0.8.2.2-5.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.

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