Bug 371411

Spec URL should be:
http://www.auroralinux.org/people/spot/review/new/alienarena.spec

The description could really do with some clarification.

It is rather strange to ask you, however are the files
below legally okay?

---------------------------------------------------
./game/acesrc/*.c
---------------------------------------------------

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!

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,-)

As promised some time ago, I'm doing a full review now.

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.

Created attachment 262271 [details]
Patch to not look for .so files under /usr/share

Spot, I understand you're probably busy with other stuff, but still ... ping?

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

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.

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

Looks good now, approved!

cvs done, builds going now. Thanks for the review.