Bug 309061 - Review Request: adanaxisgpl - FPS game in 4 spatial dimensions
Review Request: adanaxisgpl - FPS game in 4 spatial dimensions
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-27 10:02 EDT by Andy Southgate
Modified: 2008-01-11 17:16 EST (History)
4 users (show)

See Also:
Fixed In Version: 1.2.4-1.fc8
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-11 16:59:23 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Andy Southgate 2007-09-27 10:02:55 EDT
Spec URL: http://www.mushware.com/files/adanaxisgpl.spec
SRPM URL: http://www.mushware.com/files/adanaxisgpl-1.2.1-1.src.rpm
Description:
Adanaxis is a fast-moving first person shooter set in deep space, where the
fundamentals of space itself are changed.  By adding another dimension to
space this game provides an environment with movement in four directions
and six planes of rotation.  Initially the game explains the 4D control
system via a graphical sequence, before moving on to 30 levels of gameplay
with numerous enemy, ally, weapon and mission types.  Features include
simulated 4D texturing, mouse and joystick control, and original music.
Information, reviews and movies can be found at:

http://www.mushware.com/ (Developer site - me)
http://www.linuxgamingworld.com/2007/07/adanaxis
http://happypenguin.org/show?Adanaxis

This is the GPL'd version of the full commercial game.  It's stripped of
material that can't be GPL'd, but it's not a demo - it does contain all of
the levels and has no artificial limitations.  You will need hardware 3D
acceleration to meaningfully test the game.

This is my first Fedora package an I am seeking a sponsor.
Comment 1 Mamoru TASAKA 2007-10-12 10:52:20 EDT
Well, I must say that there are some issues to fix
on 1.2.1-1 spec/srpm. Please check the following URLs 
for general packaging procedure.

http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/DistTag

A. Description stage
* Please explain why you want to introduce seemingly-redundant
  macros like %name, %version, ..... etc
* Please consider to use %?dist tag.
* Please follow the BuildRoot format requested for Fedora.
* Remove redundant BuildRequires
  - For example, libGLU-devel requires libGL-devel, so
    "BuildRequires: libGL-devel" can be removed
  - Remove BuildRequires listed on "Exceptions" list
    on "Guidelines" wiki.
  - Also I suggest to write one BuildRequires per one line
    because
    * it makes easier to read
    * it makes easier to find out the difference when BuildRequiers
      changes.
* Check the missing BuildRequires
  - This srpm cannot be rebuilt.
    http://koji.fedoraproject.org/koji/taskinfo?taskID=191544
    At least libogg-devel is missing from BuildRequires.

B. %prep/%build/%install stage
* Support parallel make if possible, otherwise write as
  a comment which tells that this package cannot support
  parallel make.
* Don't use %makeinstall unless it cannot be avoided.
* Desktop files must be installed by "desktop-file-install"
  (BuildRequires: desktop-file-utils is needed).
  - I don't think the Category 
    "X-MandrivaLinux-MoreApplications-Games-Arcade" is needed.
  - The category "MoreApplications" is not valid.
-----------------------------------------------------------
error: value
"Game;ActionGame;MoreApplications;X-MandrivaLinux-MoreApplications-Games-Arcade;"
for key "Categories" in group "Desktop Entry" contains an unregistered value
"MoreApplications"
-----------------------------------------------------------
* When using "cp" or "install" command, add "-p" option
  to keep timestamps.
* Consider to install pixmaps image data under
  %_datadir/icons/hicolor/??x??/apps and call gtk-update-icon-cache
  (please check the section "GTK+ icon cache" of
   http://fedoraproject.org/wiki/Packaging/ScriptletSnippets )

C. %files section
* Make it sure that all directories created by this package
  are owned by this package.
  For example, %_datadir/adanaxisgpl/ itself is not owned by
  any package.
Comment 2 Andy Southgate 2007-10-12 15:09:12 EDT
Thanks for the review.  I'll get on to those fixes as soon as I can.
Comment 3 Andy Southgate 2007-10-18 20:07:06 EDT
Here are the updated .spec and SRPM:

Spec URL: http://www.mushware.com/files/adanaxisgpl-1.2.3-1.fc7.spec
SRPM URL: http://www.mushware.com/files/adanaxisgpl-1.2.3-1.fc7.src.rpm

Thanks again.
Comment 4 Mamoru TASAKA 2007-10-19 04:56:38 EDT
For 1.2.3-1:

* Source tarball
  - Tarball in your srpm does not match the tarball downloaded
    from upstream:
-------------------------------------------------------
20015891 2007-10-19 08:24 adanaxisgpl-1.2.3-1.fc7/adanaxisgpl-1.2.3.tar.gz
20015932 2007-10-19 09:32 adanaxisgpl-1.2.3.tar.gz

4a199ac0da9468d6b622af53d2189015  adanaxisgpl-1.2.3-1.fc7/adanaxisgpl-1.2.3.tar.gz
2a0b49cb7c2b13b6272420983279288f  adanaxisgpl-1.2.3.tar.gz
-------------------------------------------------------

* Redundant BuildRequires
  - Still some BuildRequires are redundant.
    autoconf, automake - Please explain why these are needed.
                         This spec file does not call autoconf or automake.
    binutils          <- gcc requires this
    glibc-devel       <- gcc requires this
    libGLU-devel      <- freeglut-devel requires this
    SDL-devel         <- SDL_mixer-devel requires this

* Timestamp
  - To keep timestamps, please use the following
--------------------------------------------------------
make DESTDIR=%{buildroot} INSTALL="install -p" CPPROG="cp -p" install
--------------------------------------------------------
    For recent Makefiles, adding 'INSTALL="install -p"' will usually
    save timestamps on installed files. However as this Makefile
    uses install-sh, 'CPPROG="cp -p"' is also needed.

Then:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 5 Mamoru TASAKA 2007-10-25 12:54:01 EDT
ping?
Comment 6 Andy Southgate 2007-10-25 15:39:22 EDT
I've been a bit busy recently, but I should be able to find some time to
repackage this weekend.  I will also do some pre-reviews as you suggest when
time permits, but it might take a week or two.  Will get back to you soon...
Comment 8 Mamoru TASAKA 2007-11-01 03:17:50 EDT
For 1.2.4-1:

* ICE dependency
  - Please check if the following result in build.log is
    correct.
----------------------------------------------------------------
   193  checking for shmat... yes
   194  checking for IceConnectionNumber in -lICE... no
   195  checking for main in -lm... yes
----------------------------------------------------------------

* Ruby dependency
  - By the way, is ruby not needed for this package?

! Still I am waiting for your pre-review or another review request
  submission.
Comment 9 Andy Southgate 2007-11-13 17:19:18 EST
I've had a look a the ICE dependency and I don't think it's a problem.  It looks
like one of the macros - maybe AC_PATH_XTRA - is looking for X libraries, but
the app doesn't use anything from libICE directly.  I've tried the binary from
the mock build (i.e. without libICE) and it runs fine.

There's no ruby dependency because the game source contains a modified version
of the ruby source code, and doesn't refer to installed extensions.  The game
runs happily without it.  I'll get the pre-review done as soon as I can.
Comment 10 Andy Southgate 2007-11-15 18:48:13 EST
I have done a pre-review:

https://bugzilla.redhat.com/show_bug.cgi?id=366841

Comment 11 Hans de Goede 2007-11-16 03:11:06 EST
(In reply to comment #10)
> I have done a pre-review:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=366841
> 

Looks good, and thanks I had that on my todo list to review. Note that I'm a
potential candidate to sponsor you to, just like Mamoru, but before doing so I
would like to see one more pre-review, or another package submitted by you and
reviewed.

About your last version, you say:
"There's no ruby dependency because the game source contains a modified version
of the ruby source code, and doesn't refer to installed extensions.  The game
runs happily without it.  I'll get the pre-review done as soon as I can."

Note that its much prefered to always use the system version of any libs / tools
instead of your own private copy, that way there is only one place to take into
account when adding bugfixes / security fixes.

What exactly has been modified, and is it possible (with a reasonable effort) to
use the system version of ruby instead?
Comment 12 Andy Southgate 2007-11-16 16:01:12 EST
Hi,

I agree that it's best to use the installed ruby .so if you can, but in this
case normal ruby isn't ideal.  The philosophical problem is that the security
model is wrong for level scripting (where the scripts might be hostile) as it's
aimed at web sites (where everything except the scripts might be hostile).  The
practical problem is that game relies on the way this version of ruby does
things a bit too much.  From a developer point of view it's useful to know
exactly what code you're dealing with on all platforms (Windows, Mac OS X,
Linux, etc.) to reduce the number of test configurations.  Security problems are
less of an issue because the game never opens a socket.  In practice what I'll
do is pull later versions of ruby into the upstream as necessary, test it, and
then re-release.
Comment 13 Hans de Goede 2007-11-16 16:12:09 EST
Ok, thats a reasonable explanation, in that case using an included copy of ruby
is fine. So that only leaves one more package reviewed, or one other good pre
review to get you sponsored.
Comment 14 Andy Southgate 2007-11-20 20:19:33 EST
I've done another pre-review:

https://bugzilla.redhat.com/show_bug.cgi?id=361701

Comment 15 Mamoru TASAKA 2007-11-21 04:56:12 EST
Well,
- When I launch adanaxisgpl from xterm, I get the following warning.
  Is this okay?
--------------------------------------------------------
mkdir /usr/share/adanaxisgpl/mushware-cache failed: Permission denied
--------------------------------------------------------

- Umm.. Currently rawhide mesa seems a heavy mess and when I launch
  adanaxisgpl no characters are displayed correctly...
Comment 16 Andy Southgate 2007-11-21 17:50:01 EST
Yep, the mkdir message is normal, and if fully-featured transparency isn't
working in your mesa you will get blocks instead of letters.
Comment 17 Mamoru TASAKA 2007-11-22 05:28:34 EST
Okay.

------------------------------------------------------------
    This package (adanaxisgpl) is APPROVED by me
------------------------------------------------------------

Please follow the procedure according to:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor (at the stage, please also write on
this bug for confirmation that you requested for sponsorship)
Then I will sponsor you.

If you want to import this package into Fedora 7 or Fedora 8, 
you also have to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on Fedora rebuilding system).

If you have questions, please ask me.
Comment 18 Mamoru TASAKA 2007-11-22 05:35:56 EST
If you have already requested cvsextras group, please write your
FAS (Fedora Account System) name in this bug.
Comment 19 Andy Southgate 2007-11-23 15:56:44 EST
Thanks!

I've created a fedora account with the username southa, and I confirm that I 
am requesting sponsorship.
Comment 20 Mamoru TASAKA 2007-11-24 02:47:06 EST
Now I should be sponsoring you.
Comment 21 Andy Southgate 2007-11-25 14:10:30 EST
New Package CVS Request
=======================
Package Name: adanaxisgpl
Short Description: FPS in 4 spatial dimensions
Owners: southa
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: yes
Comment 22 Kevin Fenzi 2007-11-26 12:20:57 EST
cvs done.
Comment 23 Andy Southgate 2007-11-27 16:42:21 EST
Seems to build without problems:

http://koji.fedoraproject.org/koji/buildinfo?buildID=25640
http://koji.fedoraproject.org/koji/buildinfo?buildID=25641
http://koji.fedoraproject.org/koji/buildinfo?buildID=25633

I've closed this bug as NEXTRELEASE.  Thanks all!
Comment 24 Fedora Update System 2007-11-28 20:35:15 EST
adanaxisgpl-1.2.4-1.fc7 has been pushed to the Fedora 7 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update adanaxisgpl'
Comment 25 Fedora Update System 2007-11-28 20:41:19 EST
adanaxisgpl-1.2.4-1.fc8 has been pushed to the Fedora 8 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update adanaxisgpl'
Comment 26 Fedora Update System 2008-01-11 16:59:21 EST
adanaxisgpl-1.2.4-1.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 27 Fedora Update System 2008-01-11 17:16:24 EST
adanaxisgpl-1.2.4-1.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.

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