Red Hat Bugzilla – Bug 371411
Review Request: alienarena - 3d Deathmatch game
Last modified: 2007-11-30 17:12:21 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
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.