Bug 371411 - Review Request: alienarena - 3d Deathmatch game
Review Request: alienarena - 3d Deathmatch game
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On: 371421
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-08 11:19 EST by Tom "spot" Callaway
Modified: 2007-11-30 17:12 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-11-28 17:24:38 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
tcallawa: fedora‑cvs+


Attachments (Terms of Use)
Patch to not look for .so files under /usr/share (888 bytes, patch)
2007-11-17 09:31 EST, Hans de Goede
no flags Details | Diff

  None (edit)
Description Tom "spot" Callaway 2007-11-08 11:19:56 EST
Spec URL: http://www.auroralinux.org/people/spot/review/new/alienarena.sped
SRPM URL: http://www.auroralinux.org/people/spot/review/new/alienarena-6.10-2.fc8.src.rpm
Description: 
Alien Arena 2007 is the latest version of an online deathmatch game that was
first introduced to the public in October, 2004. Since that initial release,
nearly every aspect of the game has been revamped, in fact, all of the content
and code from the November 2005 release of Alien Arena 2006 has been redone as
well.  It's like an entirely new game, and it may shock people just how much
it has improved in less than a year's time. With over 30 levels, seven modes
of play, loads of mutators, built-in bots, 11 player characters, 9 weapons
(with alt-fire modes), the game has an endless supply of replayability. With
so many new features, AA2K7 is nearly an entirely new game when held in
comparison to its predecessor. With the trials and tribulations of software
development, endless hours of playing, gathering feedback, COR Entertainment
has been able to not only fine tune and perfect it's flagship game, but add
completely new dimensions to it.

Note: Depends on alienarena-data
Comment 1 Tom "spot" Callaway 2007-11-08 11:22:28 EST
Spec URL should be:
http://www.auroralinux.org/people/spot/review/new/alienarena.spec
Comment 2 Bastien Nocera 2007-11-11 22:10:01 EST
The description could really do with some clarification.
Comment 3 Mamoru TASAKA 2007-11-16 08:52:53 EST
It is rather strange to ask you, however are the files
below legally okay?

---------------------------------------------------
./game/acesrc/*.c
---------------------------------------------------
Comment 4 Tom "spot" Callaway 2007-11-16 09:13:22 EST
Hehe, no worries. I am certainly far from perfect.

The code in question is GPL, here is the documented proof from the author:

http://www.aftermoon.net/?page=2002-07-30-acebot

That discussion should be included in the package, I've made that change, and
uploaded new SRPM and SPEC (also, has a slightly clarified description):

New SRPM:
http://www.auroralinux.org/people/spot/review/new/alienarena-6.10-3.fc9.src.rpm
New SPEC: http://www.auroralinux.org/people/spot/review/new/alienarena.spec

Thank you for your diligence in license checking!
Comment 5 Mamoru TASAKA 2007-11-16 11:06:30 EST
Well, for 6.10-3:

* alienarena.src: W: strange-permission alienarena.sh 0775
  - All files in src should have 0644 permission.
    Change permissons when you actually install.

* Timestamps
  - Please use "-p" option to keep timestamps when using
    "cp" or "install" commands (for SOURCE3, SOURCE1)

* Location of the binaries
  - By the way, if crded or crx.sdl is not directly called,
    it may well that these binaries are moved under %_libexecdir.

* W: unstripped-binary-or-object /usr/lib/alienarena/game.so
------------------------------------------------------
install -m0644 release/game.so $RPM_BUILD_ROOT%{_libdir}/alienarena/
------------------------------------------------------
  - find-debuginfo.sh or so won't strip binaries if they
    don't have executable permission.
    If you want 0644 permission for this, perhaps
    - once install with 0755 permission
    - later, set permission by %attr
    would work properly? (I have not checked this).

* defattr
  - We now recommend %defattr(-,root,root,-)
Comment 6 Hans de Goede 2007-11-17 06:02:38 EST
As promised some time ago, I'm doing a full review now.
Comment 7 Hans de Goede 2007-11-17 08:30:04 EST
Full review results (Thanks to Mamoru for some of these):

Must Fix:
---------
* rpmlint: W: unstripped-binary-or-object /usr/lib/alienarena/game.so
  (chmod +x ?)

* Make the Requires: alienarena-data versioned, so that if a newer version of
  the data is needed by a newer release, an someone does yum update alienarena,
  the data will get updated too.

  Usually I use a = version requirement in the engine and a >= version req on
  the engine in the data.

* '--vendor ""' argument to desktop-file-install must be '--vendor fedora'

* Add "ActionGame" to the .desktop file Categories, so that alienarena will not
  end up on the main games sub-menu when the games menu has been further 
  sub-menud through install of the games-menus package (as the live dvd does for
  example)

* Remove these non RPM_OPTFLAGS from the flags used for the compile:
  -O2 -fomit-frame-pointer -ftree-vectorize -fexpensive-optimizations -march=k8
  Notice that upstream also adds this to CFLAGS:
  -fno-strict-aliasing -fmerge-constants
  But those are ok.

* You install symlinks to %{_libdir}/%{name}/game.so under /usr/share, but
  %{_libdir} is arch dependent and files under /usr/share may not be arch 
  dependent.

  Suggestion, instead use this in %prep:
  sed -i 's|"game.so"|"%{_libdir}/%{name}/game.so"|g' unix/sys_unix.c

  Together with a patch commenting the stupid code where it first tries to
  fopen the .so in a number of different places totally irrelevant to dlopen's
  search path (will attach the patch next).


Should Fix:
-----------
* defattr
  - We now recommend %defattr(-,root,root,-)
* Timestamps
  - Please use "-p" option to keep timestamps when using
    "cp" or "install" commands (for SOURCE3, SOURCE1)
* "-n %{name}-%{version}" to %setup is redundant, remove please
* Make .desktop file use aa as Icon, not /full/path/Aa.ico
* Use just alienarena as Exec in the .desktop file not a full path
* The symlinks to the binaries under /usr/share/alienarena do not appear to be
  needed
* Since crx.sdl may not be directly called, it should be moved to %_libexecdir
* Change wrapper script from:
---
#!/bin/bash

cd /usr/share/alienarena/
./crx.sdl
---
   To:
---
#!/bin/bash

cd /usr/share/alienarena/
exec /usr/libexec/crx.sdl "$@"
---
So that bash doesn't linger in memory and cmd line parameters can be passed.
Comment 8 Hans de Goede 2007-11-17 09:31:09 EST
Created attachment 262271 [details]
Patch to not look for .so files under /usr/share
Comment 9 Hans de Goede 2007-11-27 08:12:19 EST
Spot, I understand you're probably busy with other stuff, but still ... ping?
Comment 10 Tom "spot" Callaway 2007-11-28 10:06:54 EST
Sorry for the delay here, I wanted to make sure I had all the fixes correct in
both alienarena and alienarena-data.

New SRPM:
http://www.auroralinux.org/people/spot/review/new/alienarena-6.10-4.fc9.src.rpm
New SPEC:
http://www.auroralinux.org/people/spot/review/new/alienarena.spec
Comment 11 Hans de Goede 2007-11-28 16:08:28 EST
I'm sorry, but you missed / didn't fix this MUST fix item:
* Remove these non RPM_OPTFLAGS from the flags used for the compile:
  -O2 -fomit-frame-pointer -ftree-vectorize -fexpensive-optimizations -march=k8
  Notice that upstream also adds this to CFLAGS:
  -fno-strict-aliasing -fmerge-constants
  But those are ok.

Esp the -fomit-frame-pointer (breaks debuginfo) and -march=foo (means it might
not run on certain machines) are bad.
Comment 12 Tom "spot" Callaway 2007-11-28 16:31:12 EST
Argh! Apologies, it is really fixed this time. :)

New SRPM:
http://www.auroralinux.org/people/spot/review/new/alienarena-6.10-5.fc9.src.rpm
New SPEC:
http://www.auroralinux.org/people/spot/review/new/alienarena.spec
Comment 13 Hans de Goede 2007-11-28 16:38:52 EST
Looks good now, approved!
Comment 14 Tom "spot" Callaway 2007-11-28 17:24:38 EST
cvs done, builds going now. Thanks for the review.

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