This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 178568 - Review Request: lacewing Asteroid like game with may different ships
Review Request: lacewing Asteroid like game with may different ships
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Wart
David Lawrence
http://users.olis.net.au/zel/
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-01-21 14:31 EST by Hans de Goede
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-01-31 03:19:37 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
New specfile (2.90 KB, text/plain)
2006-01-30 02:14 EST, Hans de Goede
no flags Details
diff between last and new specfile (1.51 KB, patch)
2006-01-30 02:16 EST, Hans de Goede
no flags Details | Diff

  None (edit)
Description Hans de Goede 2006-01-21 14:31:40 EST
Spec Name or Url: http://home.zonnet.nl/jwrdegoede/lacewing.spec
SRPM Name or Url: http://home.zonnet.nl/jwrdegoede/lacewing-1.10-1.src.rpm
Description:
Asteroid like game where you can choose a type of ship and depending on the
type of ship can pickup a number of upgrades during the game.

Quoting from the webpage: "Lacewing is an arcade-style shoot-em-up which plays
a little bit like a cross between Spacewar and Centipede. It has a decidedly
retro style to it. It has a single-player mode, and also co-operative and duel
modes for two players (split-screen)".
Comment 1 Mike McGrath 2006-01-21 15:33:24 EST
Couple of things

-paths used where macro's should be %{_usr} and %{_bindir}
-go ahead and move lacew.cfg to /etc
-I would create a different rpm for the lwdata.zip
-What license is lacewing.png released under?  Where did it come from?
-Would it make more sense just use the quote from the webpage as the entire
description?
-add || : to the end of your post and postun commands.
Comment 2 Hans de Goede 2006-01-21 17:04:45 EST
Mike wrote:
-paths used where macro's should be %{_usr} and %{_bindir}
I could replace /usr with %{prefix} in the make commands, yes, but where
do you want to use %{_bindir) ?
-go ahead and move lacew.cfg to /etc
I don't want to polute /etc with this its not a garbage-bin, I'm working on
several packages like this one, which all have a similar scheme.
-I would create a different rpm for the lwdata.zip
Why, this is the only game which uses it and it won't run without it.
-What license is lacewing.png released under?  Where did it come from?
I created it with gthumb from a bmp in lwdata.zip, license is thus GPL
-Would it make more sense just use the quote from the webpage as the entire
description?
Not in my opinion
-add || : to the end of your post and postun commands.
Will do.

Comment 3 Mike McGrath 2006-01-21 17:13:31 EST
Under post and postun you have :

/usr/bin/gtk-update-icon-cache

I would change this to

%{_bindir}/gtk-update-icon-cache
Comment 4 Wart 2006-01-21 21:13:27 EST
rpmlint output:
E: lacewing file-in-usr-marked-as-conffile /usr/share/lacewing/lacew.cfg
W: lacewing wrong-file-end-of-line-encoding /usr/share/doc/lacewing-1.10/readme.txt
W: lacewing wrong-file-end-of-line-encoding /usr/share/doc/lacewing-1.10/licence.txt

You should run
%{__sed} -i 's/\r//'
on these two files to fix the line endings.
Comment 5 Paul Howarth 2006-01-22 04:12:50 EST
(In reply to comment #4)
> rpmlint output:
> E: lacewing file-in-usr-marked-as-conffile /usr/share/lacewing/lacew.cfg

Given that this file basically just provides a set of default options, I would
either:
(a) not mark it as a config file, or
(b) move it to /etc
the idea being that /usr should be able to be mounted read-only. So if it's a
file that's expected to be edited at some time, it should go in /etc, and if not
it can stay in /usr but not be marked %config.

(In reply to comment #3)
> Under post and postun you have :
> 
> /usr/bin/gtk-update-icon-cache
> 
> I would change this to
> 
> %{_bindir}/gtk-update-icon-cache

I would advise against this. I used to do that sort of thing myself but was
convinced not to do so. The reason is that by keeping %{_bindir} etc. for use
only as targets for installation of files, someone can rebuild the package from
the SRPM and specify different destination directories, e.g.

$ rpmbuild --rebuild --define '_bindir /myapps' package.spec

and get the binaries installed where they want them. If you make changes like
the one suggested in comment #3, this will fail unless the package builder has
also installed gtk-update-icon-cache in the same place that they want to install
this package to. So I'd leave it as /usr/bin/gtk-update-icon-cache

Comment 6 Hans de Goede 2006-01-22 07:14:58 EST
I've created a new version with all above comments taken into account:
http://home.zonnet.nl/jwrdegoede/lacewing.spec
http://home.zonnet.nl/jwrdegoede/lacewing-1.10-2.src.rpm

See the changelog in the specfile for all the changes as thunderbird in rawhide
currently has broken cut and paste support.

Regarding %{_bindir} and comment 5 , I agree with comment 5, the wiki scriplets
page however suggests using %{_bindir} so I have done that.

One last note if you try to build this on Rawhide, allegro-devel is currently
broken on rawhide. It is fixed in CVS but can't be build because of buildsys
trouble. So todo a testbuild of this package on rawhide, first checkout allegro
from CVS, build that locally and install it.

Also note that rawhide mockbuilds will also fail because of this and because
rawhide has broken deps internally.
Comment 7 Paul Howarth 2006-01-22 09:23:17 EST
(In reply to comment #6)
> Regarding %{_bindir} and comment 5 , I agree with comment 5, the wiki scriplets
> page however suggests using %{_bindir} so I have done that.

Normally I would just fix the wiki scriptlets page but it appears that Ville
Skyttä was the person that changed /usr/bin/gtk-update-icon-cache to
%{_bindir}/gtk-update-icon-cache there (revision 17), and I respect Ville's
opinions so it'd be interesting if he could expand on why he made that change.
Comment 8 Wart 2006-01-29 21:04:15 EST
MUST items:
* rpmlint output is clean
* package and spec name matches upstream
* GPL license valid, matches upstream, license file included
* Meets packaging guidelines
* Spec file is legible and in Am. English.
* Source matches upstream (md5sum ok)
* Builds cleanly on FC5 i386
* Valid BR; none are redundant
* No lang files; no shlibs.
* Package not relocatable
* 0wns all directories that it creates
* File permissions ok
* %clean looks good
* code, not content
* minimal doc files, do not affect runtime
* no -devel package necessary
* desktop file installed correctly

SHOULD items:
* package includes license fie
- mock build not tested, but did build fine on FC5.
* Package runs and causes loss of productivity.  :)

MUSTFIX:
* typo in the Summary:  'may' -> 'many'
* Leave out the phrase "Quoting from the webpage" from the description.  It
  seems excessive.

SHOULDFIX (won't block approval):
* consider splitting the data files and the program into separate packages.
  This will allow you to make smaller updates to fix problems with the code
  without requiring users to download the unchanged data files again (~135k
  vs. 635k download)

Since there have been no addtional comments about the use of %{_bindir} in the
pre/post scripts, and since they match the guidelines, I'm willing to leave them
as-is.
Comment 9 Hans de Goede 2006-01-30 02:14:03 EST
Thanks for reviewing.

MUSTFIX: fixed

SHOULDFIX: I see your rationale, but make install depends on the data being
there, so the only way todo this is make this a subpackage and if I then rebuild
it to fix a bug in the engine the sub-package will get a version bump also. Or I
should /could change the Makefile. Concidering the amount of work to create a
seperate date src.rpm, the little gain (only 0.5 Mb on many Mb's of updates /
day) and taking into account that I don't plan todo updates frequently I'll just
keep it as one big package for now.

I've attached a the new spec and a diff to the previous version, I can't upload
because I'm not behind my home PC and my ISP only allows ftp access to my
homepage from my home PC (GRRR).

<shameless plug to lure you in doing another review)
If you like lacewing you might also like the "sequel":
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=178625
Comment 10 Hans de Goede 2006-01-30 02:14:53 EST
Created attachment 123858 [details]
New specfile
Comment 11 Hans de Goede 2006-01-30 02:16:00 EST
Created attachment 123859 [details]
diff between last and new specfile
Comment 12 Wart 2006-01-30 15:40:18 EST
I'll accept your reasoning for not splitting the data and source packages. 
Changes look good, except for the extra " at the end of %description.  Fix that
before you check it in.

APPROVED
Comment 13 Hans de Goede 2006-01-31 03:19:37 EST
Imported & Build.

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