Bug 191582 - Review Request: xgalaxy - Galaga clone for X11
Review Request: xgalaxy - Galaga clone for X11
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christopher Stone
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-05-13 05:09 EDT by Hans de Goede
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-06-01 04:29:14 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2006-05-13 05:09:53 EDT
Spec URL: http://home.zonnet.nl/jwrdegoede/xgalaga.spec
SRPM URL: http://home.zonnet.nl/jwrdegoede/xgalaga-2.0.34-1.src.rpm
Description:
A clone of the classic game Galaga for the X Window System. Xgalaga is a space-
invader like game with additional features to produce a more interesting game.
Comment 1 Christopher Stone 2006-05-21 16:28:39 EDT
I'm getting a build error in mock during configure:

checking for gcc... gcc
checking whether the C compiler (gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2
-fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
-fsigned-char -DXF86VIDMODE -lXxf86vm) works... no
configure: error: installation or configuration problem: C compiler cannot
create executables.
error: Bad exit status from /var/tmp/rpm-tmp.65804 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.65804 (%build)
Comment 2 Hans de Goede 2006-05-21 17:01:07 EDT
Well it works fine for me can you lift the actual gcc error from config.log that
might help.
Comment 3 Christopher Stone 2006-05-21 17:55:41 EDT
$ cat
/var/lib/mock/fedora-5-x86_64-core/root/builddir/build/BUILD/xgalaga-2.0.34/config.log
This file contains any messages produced by compilers while
running configure, to aid debugging if configure makes a mistake.

configure:564: checking host system type
configure:588: checking for gcc
configure:701: checking whether the C compiler (gcc -O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
-m64 -mtune=generic -fsigned-char -DXF86VIDMODE -lXxf86vm) works
configure:717: gcc -o conftest -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2
-fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
-fsigned-char -DXF86VIDMODE  -lXxf86vm conftest.c  1>&5
configure:714: warning: return type defaults to 'int'
/usr/bin/ld: cannot find -lXxf86vm
collect2: ld returned 1 exit status
configure: failed program was:

#line 712 "configure"
#include "confdefs.h"

main(){return(0);}
Comment 4 Christopher Stone 2006-05-21 19:32:33 EDT
* rpmlint output clean
* Package meets Package Naming Guidelines
* Spec filename matches base package %{name}
* Package meets Packaging Guidelines
* Package licensed with open source compatible license
* License in spec matches actual license
* License text included in %doc
* Spec file written in American English
* Spec file is legible
* Sources match upstream
9f7ee685e9c4741b5f0edc3f91df9510  xgalaga_2.0.34.orig.tar.gz
9f7ee685e9c4741b5f0edc3f91df9510  xgalaga_2.0.34.orig.tar.gz
* Package successfully compiles and builds on FC5 x86_64

O Package has all BR except libXxf86vm-devel which I needed to add for it to compile

* Package does not have any locales
* Package does not contain any shared library files
* Package is not relocatable
* Package owns all directories it creates
* Package does not contain any duplicate files in %files
* File permissions are set properly
* Package contains proper %clean section
* Macro usage consistant enough
- I notice you use %{__sed}, but don't bother using %{__make} or %{__rm} etc..
* Package contains permissble content
* Package does not contain large documentation to warrent a seperate package
* Package does not contain header files, libraries or .pc files
* Package does not contain any .so files
* Package does not require or use a -devel package
* Package does not contain any .la files
* Package adds an appropriate .desktop entry
* Package does not own any files or directories owned by other packages


*** MUST ***

- You MUST figure out why FC5 needs to add a BuildRequires of libXxf86vm-devel
and why this is not needed for your build (presumably FC6)


Non-blocking SHOULDs:

- Be more consistant with macro usage, for example %{__sed}, but no %{__rm} etc.
- I also prefer %{buildroot} instead of $RPM_BUILD_ROOT, but that is a matter of
preference.  I just think spec files look cleaner when everything consistantly
uses %{} format.  So basically I'm saying you should use a clean more legible
consistant style in your spec files, but I'm not going to say this is a blocker
or should be fixed, just a suggestion.
- Let me know that the name xgalaga isn't going to be a problem with Namco. 
I've heard the Lgames are not allowed because the names are too close to the
original, is this going to be a problem?
- Return the favor by reviewing some of my packages ;-)
Comment 5 Christopher Stone 2006-05-21 20:00:15 EDT
One other minor thing I noticed:

cat > README.fedora << EOF

The latest Fedora xgalaga package also includes fullscreen support, start
xgalaga with -window to get the old windowed behaviour. You can switch on the
fly between window and fullscreen mode with alt+enter .
EOF

The word "behaviour" is not American English.  It should be "behavior".  In
addition there should not be a space before the final period.
Comment 6 Hans de Goede 2006-05-23 03:59:00 EDT
Chris and I had a private discussion about this by email because BZ was down,
copy and pasting it here for future reference:

---

Hi Chris,

Bugzilla is down so I'm doing it this way. Thanks for the review.

About the missing BR I failed to add that its needed for the devel branch too,
things just worked on my system because I already had the needed devel-package
installed.

About the name, I wans't sure about this myself, so now I've changed the name to
xgalaxy (googled, not taken already).

New SRPM and spec are at:

http://people.atrpms.net/~hdegoede/

Regards,

Hans

---

Christopher Stone wrote:
> okay ill take a look at this tomorrow, been really busy today and
> didnt get the chance to look at it.
>
> Do you think the name is going to be a problem?  I'd prefer xgalaga,
> but then again, it's probably better to be safe than sorry.
>

The name is most likely not a problem, because the people with the rights to the
original name probably don't care. xgalaga has existed under this name for a
long time without trouble.

Then again the name had both me and you worried and those are valid worries the
name is a legal problem. Even if the other party _probably_ doesn't care it
still is a legal issue. It is the _probably_ that scares me and untill the "upto
now" part of upto now this hasn't been a problem. If the people with the rights
to the name one day all of a sudden do start caring, or get a grudge against OSS
we've a problem, which I would rather avoid. Since I've already done the hard
work of renaming (and recreating the "logo") I think its best / safest to stick
with the new name.

Regards,

Hans

Comment 7 Christopher Stone 2006-05-23 21:39:14 EDT
New rpm is STILL missing libXxf86vm-devel.
Comment 8 Hans de Goede 2006-05-24 01:02:17 EDT
Oops you're right, I did put adding it in the changelog, but I didn't actually
do this.

Fixed SRPM and spec are at:

http://people.atrpms.net/~hdegoede/

Comment 9 Hans de Goede 2006-06-01 04:29:14 EDT
Imported & Build,

Thanks!
Comment 10 Christopher Stone 2006-06-01 15:33:58 EDT
Fixing bug report summary.

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