Bug 366841 - Review Request: nogravity - Space shooter in 3D
Review Request: nogravity - Space shooter in 3D
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-05 09:25 EST by Peter Lemenkov
Modified: 2008-01-30 08:36 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-30 08:36:04 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
michel: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)
Make nogravity work on 64 bit machines (30.46 KB, patch)
2007-11-18 17:24 EST, Hans de Goede
no flags Details | Diff
PATCH: some fixes to the 64bit patch (5.13 KB, patch)
2007-11-19 06:10 EST, Hans de Goede
no flags Details | Diff
improved 64bit patch (32.54 KB, patch)
2007-11-20 11:31 EST, Hans de Goede
no flags Details | Diff
Wrapper script fixed for dual head usage (169 bytes, text/plain)
2008-01-08 16:23 EST, Hans de Goede
no flags Details

  None (edit)
Description Peter Lemenkov 2007-11-05 09:25:29 EST
Spec URL: http://peter.fedorapeople.org/nogravity.spec
SRPM URL: http://peter.fedorapeople.org/nogravity-2.00-1.src.rpm
Description: No Gravity is a fantastic and futuristic universe made of five
intergalactic worlds. An arcade type game with great playability,
where it is easy to plunge into space battles against spacefighters,
space stations and more!

OK, this is my 3rd try to submit this game. Previous attempts are here:

https://bugzilla.redhat.com/show_bug.cgi?id=166389
http://bugzilla.livna.org/show_bug.cgi?id=621

Looks like package in good shape but still there are some things to discuss. First of all I'm in doubts about splitting it into main package and data, 2nd - I forced to disable OpenGL because of garbled colors in case of using it instead of software rendering. 

I can't use openal as sound output - someone should try it also.

I created quick and dirty patch to start game windowed instead of fullscreen and applied it.
Comment 1 Aurelien Bompard 2007-11-05 15:26:37 EST
The advantage of splitting the packages is that when you do a bugfix upgrade,
the download size is greatly reduced.
- Do not use /usr/games/%{name}, use /usr/share/%{name} instead
- in the desktop file, the exec command should not end in .sh (looks like a
leftover)

In general, please try to follow the guidelines here:
http://fedoraproject.org/wiki/SIGs/Games#head-367e7208aacfec8f8aff1b34c1795e43c0967ecd
Comment 2 Andy Southgate 2007-11-15 18:46:42 EST
This is a pre-review as part of my own quest for sponsorship, so don't take 
too much notice, but here goes:

In the spec file:

o  Consider using dist tag (http://fedoraproject.org/wiki/Packaging/DistTag)
o  Redundant BuildRequires SDL-devel and libogg-devel (already brought in by 
SDL_mixel-devel and libvorbis-devel respectively)
o  Missing BuildRequires, at least aclocal for the bootstrap script (mock 
nogravity-2.00-1.src.rpm fails)
o  Avoid %makeinstall or comment as to why it's necessary (see 
http://fedoraproject.org/wiki/Packaging/Guidelines)
o  Consider installing icon pixmaps under %_datadir/icons/hicolor/??x??/apps 
and calling gtk-update-icon-cache, as per the section "GTK+ icon cache" of 
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
o  rpmlint nogravity-2.00-1.src.rpm
nogravity.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 10)
nogravity.src: W: strange-permission nogravity--bootstrap 0755
o  rpmlint nogravity-2.00-1.i386.rpm
nogravity.i386: W: 
wrong-file-end-of-line-encoding /usr/share/doc/nogravity-2.00/GNU.TXT
nogravity.i386: W: 
wrong-file-end-of-line-encoding /usr/share/doc/nogravity-2.00/README.TXT
o  rpmlint nogravity-data-2.00-1.i386.rpm
nogravity-data.i386: W: no-documentation
o  Should the game binary package Require: the data package as a dependency?
o  Data directory needs owner (%dir directive in %files)
o  Spelling mistake Necesssary in %package data

Apart from that, it builds and runs fine on my desktop.
Comment 3 Hans de Goede 2007-11-17 17:02:15 EST
Interesting game, I'll do a review of this.

First of all all issues mentioned in comment 2 and 3 must be resolved.

For starters please make 2 packages (and with that I mean 2 srpms) which require
each other, one data package and one engine. This way bugfixes to the engine can
be done easily without the user needing redownload all the data. The general
rule of thumb is: If there are seperate upstream tarbals, make seperate srpms.

While making this new packages please also take the other points from comment 2
and 3 into account.

I've just done a testbuild of this on my 64 bit machine, and it is not 64 bit
clean (it crashes). I've already fixed 2 crashes, so no I can get upto the start
of the game (at first it crashed in the menus). I'll try to finish making this
64 bit clean tomorrow and attach a patch.
Comment 4 Hans de Goede 2007-11-18 17:24:43 EST
Created attachment 263141 [details]
Make nogravity work on 64 bit machines

As promised. Was a lot harder then I initially thought dough, see the patch.
Especially the fact that the V3KKEY struct needs to be at the same offset in
several structs as those are crammed into a union, and the code expects to be
able to access this struct without taking into account which struct actually
sits inside the union.
Comment 5 Hans de Goede 2007-11-19 06:10:42 EST
Created attachment 263391 [details]
PATCH: some fixes to the 64bit patch

I had an awake moment this morning and thought of the fact that pointers on
x86_64 will get 64 bits aligned. The attached incremental patch takes this into
account, untested yet, will test tonight.
Comment 6 Hans de Goede 2007-11-20 11:31:26 EST
Created attachment 265081 [details]
improved 64bit patch

Ok, this is the final version of the 64 bit patch, I've first been hunting one
last 64 bit graphical glitch for half a day (solved now) and then been busy
another half day testing and cleaning up the patch.

BTW I've tested both with opengl and without and with openal and sdlmixer, all
work fine. I think building with opengl and openal enabled is best, or
alternatively building both an opengl + openal and a software + sdl_mixer
binary.

This was all done on a clean x86_64 F-8 install.
Comment 7 Peter Lemenkov 2007-11-26 05:29:43 EST
(In reply to comment #2)
> This is a pre-review as part of my own quest for sponsorship, so don't take 
> too much notice, but here goes:
> 
> In the spec file:
> 
> o  Consider using dist tag (http://fedoraproject.org/wiki/Packaging/DistTag)
> o  Redundant BuildRequires SDL-devel and libogg-devel (already brought in by 
> SDL_mixel-devel and libvorbis-devel respectively)
> o  Missing BuildRequires, at least aclocal for the bootstrap script (mock 
> nogravity-2.00-1.src.rpm fails)
> o  Avoid %makeinstall or comment as to why it's necessary (see 
> http://fedoraproject.org/wiki/Packaging/Guidelines)
> o  Consider installing icon pixmaps under %_datadir/icons/hicolor/??x??/apps 
> and calling gtk-update-icon-cache, as per the section "GTK+ icon cache" of 
> http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
> o  rpmlint nogravity-2.00-1.src.rpm
> nogravity.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 10)
> nogravity.src: W: strange-permission nogravity--bootstrap 0755
> o  rpmlint nogravity-2.00-1.i386.rpm
> nogravity.i386: W: 
> wrong-file-end-of-line-encoding /usr/share/doc/nogravity-2.00/GNU.TXT
> nogravity.i386: W: 
> wrong-file-end-of-line-encoding /usr/share/doc/nogravity-2.00/README.TXT
> o  rpmlint nogravity-data-2.00-1.i386.rpm
> nogravity-data.i386: W: no-documentation
> o  Should the game binary package Require: the data package as a dependency?
> o  Data directory needs owner (%dir directive in %files)
> o  Spelling mistake Necesssary in %package data
> 
> Apart from that, it builds and runs fine on my desktop.

OK, updated:

http://peter.fedorapeople.org/nogravity.spec
http://peter.fedorapeople.org/nogravity-2.00-2.fc8.src.rpm

I'll split this package and add Hans's patch asap.
Comment 8 Hans de Goede 2007-12-23 17:14:39 EST
Erm, Peter I spend a lot of time writing that patch and I would really like to
see this in Fedora soon, so ... ping?
Comment 9 Peter Lemenkov 2007-12-25 03:34:23 EST
Hello All.
My computer from which I worked with fedora-related stuff (which has all
necessary certificates, software, files etc) encountered some hardware failures.
As a result I won't be able to do something with Fedora-related stuff (except of
bugreporting) for a week or two.

If anyone wants to take care of this title - feel free to resubmit this review
request (I'll close this one after that). Otherwise we'll need to wait until I
recover my computer.
Comment 10 Rahul Sundaram 2007-12-28 16:38:09 EST
I was looking at this package with an eye towards temporarily continuing with
the review till you return back and the livna review claims that the data is
under GPLv2 but the Readme file in the nogravity package has a different wording. 

"All of the No Gravity data files remain copyrighted and licensed by RealTech
under the original terms. You can use our data for personal entertainment or
educational purposes.

Please do not contact us for possible commercial exploitation of No Gravity --
we will not be interested"

This suggests that the data file isn't suitable for commercial distribution and
such a restriction is against the licensing guidelines of Fedora. Do you have
any clarification from upstream that the data file is indeed GPlv2? If so the
readme in the upstream tarball and SCM should be updated to reflect this
information. 
Comment 11 Hans de Goede 2007-12-28 16:53:06 EST
That text in the readme is a left over from the past, later on they have also
released the datafiles under the GPL. They had a news item about this on their
website. Spot has looked this over and approved it, this was discussed on a
mailinglist and should be archived, it was probably the games list, but I'm not
sure.

We may want to patch the readme to not confuse users.
Comment 12 Rahul Sundaram 2007-12-28 19:06:13 EST
Hans, 

Can you take a quick look at this? The 64-bit patch doesn't seem to apply on my
system so I have commented it out for now. It is probably because I am not
applying it correctly. I have split the data into a separate spec (should the
review be separate too?) but rpmbuild fails with "error: Package has no
%description: nogravity-data" and the spec does have a description. 

Anyway the spec files are at

http://sundaram.fedorapeople.org/nogravity.spec
http://sundaram.fedorapeople.org/nogravity-data.spec
Comment 13 Hans de Goede 2007-12-30 05:23:58 EST
(In reply to comment #12)
> Hans, 
> 
> Can you take a quick look at this? The 64-bit patch doesn't seem to apply on my
> system so I have commented it out for now. It is probably because I am not
> applying it correctly. I have split the data into a separate spec (should the
> review be separate too?) but rpmbuild fails with "error: Package has no
> %description: nogravity-data" and the spec does have a description. 
> 
> Anyway the spec files are at
> 
> http://sundaram.fedorapeople.org/nogravity.spec
> http://sundaram.fedorapeople.org/nogravity-data.spec

Thanks,

They do still need some work though (missing icon cache scriptlets, and more
importantly, we should build the binary twice, once with the opengl renderer and
once with the software renderer).

I'm working on this. Expected an updated package later today.
Comment 14 Hans de Goede 2007-12-30 17:52:14 EST
Oof, well that took longer then expected, I encountered a bug while testing,
then another one, etc. All fixed now, here is a new version:

http://people.atrpms.net/~hdegoede/nogravity.spec
http://people.atrpms.net/~hdegoede/nogravity-2.00-4.fc9.src.rpm

A review would be much appreciated. I've filed a seperate review request for the
data see bug 427077.
Comment 15 Andrew Zabolotny 2008-01-03 12:34:57 EST
[2|zap@zap|~]nogravity
/usr/bin/nogravity: line 3: [: too many arguments

[2|zap@zap|~]glxinfo | grep "direct rendering: "
direct rendering: Yes
direct rendering: Yes

I think something like a 'head -1' would help :)
Comment 16 Hans de Goede 2008-01-03 12:41:16 EST
(In reply to comment #15)
> [2|zap@zap|~]nogravity
> /usr/bin/nogravity: line 3: [: too many arguments
> 
> [2|zap@zap|~]glxinfo | grep "direct rendering: "
> direct rendering: Yes
> direct rendering: Yes
> 
> I think something like a 'head -1' would help :)

Thats very strange the same shell code is in opengl-games-utils and thus is
quite widely used, why on earth would the output of your glxinfo say that twice,
can you attach the full output?

Dual head using 2 cards perhaps?
Comment 17 Hans de Goede 2008-01-04 04:25:09 EST
Repeating my last comment, this time with Andrew in the CC so he will see it :)

(In reply to comment #15)
> [2|zap@zap|~]nogravity
> /usr/bin/nogravity: line 3: [: too many arguments
> 
> [2|zap@zap|~]glxinfo | grep "direct rendering: "
> direct rendering: Yes
> direct rendering: Yes
> 
> I think something like a 'head -1' would help :)

Thats very strange the same shell code is in opengl-games-utils and thus is
quite widely used, why on earth would the output of your glxinfo say that twice,
can you attach the full output?

Dual head using 2 cards perhaps?

Comment 18 Andrew Zabolotny 2008-01-04 10:41:06 EST
Of course dual-head card (most cards are two-head today, you just have to enable
the second head). Sorry I haven't mentioned this, I thought it is obvious. I
have connected the second head to my TV but it doesn't matter much.

glxinfo displays info for both heads (:0.0 and :0.1) successively no matter of
the value of the DISPLAY env variable or the -display option. I tried and I
can't force it to display the information exactly for :0.0, it always displays
info also for :0.1.
Comment 19 Hans de Goede 2008-01-08 16:23:21 EST
Created attachment 291089 [details]
Wrapper script fixed for dual head usage

Andrew Zabolotny,

Can you give this slightly modified (using head -n 1 as suggested) nogravity.sh
a try, I can't test this myself.

Thanks,

Hans
Comment 20 Andrew Zabolotny 2008-01-10 04:56:29 EST
Yes, it is same as my changed script except that I've used "-1" instead of "-n 1" :)

Otherwise, it works fine.
Comment 21 Hans de Goede 2008-01-11 15:22:59 EST
Okay, a new version with the dual head fix in is here:

http://people.atrpms.net/~hdegoede/nogravity.spec
http://people.atrpms.net/~hdegoede/nogravity-2.00-4.fc9.src.rpm

Now can someone please review this? To be clear I've taken over as review
submitter, so I'm no longer reviewing this. I'm updating the bug status to
reflect this.
Comment 22 Peter Lemenkov 2008-01-11 15:50:41 EST
Hans, please file a new bug so I can close this one.
Comment 23 Hans de Goede 2008-01-11 16:06:26 EST
(In reply to comment #22)
> Hans, please file a new bug so I can close this one.

Why exactly, I don'y see any reason for closing this one, if you want it closed
you can review it then I'll import and close :) Or we can swap roles again.
Comment 24 Andrew Zabolotny 2008-01-13 06:02:00 EST
nogravity-2.00-4.fc9.src.rpm does not contain the fix.

[3|zap@zap|~/rpm]cat SOURCES/nogravity.sh
#!/bin/bash

if [ `glxinfo | grep "direct rendering: " | cut -d " " -f 3` != Yes ]; then
  exec nogravity-software "$@"
else
  exec nogravity-opengl "$@"
fi

P.S. Tried intuitively
http://people.atrpms.net/~hdegoede/nogravity-2.00-5.fc9.src.rpm
and it's the one with the fix.
Comment 25 Hans de Goede 2008-01-13 06:28:17 EST
Bah, copy and paste error, indeed the last version and the one to review is:

http://people.atrpms.net/~hdegoede/nogravity.spec
http://people.atrpms.net/~hdegoede/nogravity-2.00-5.fc9.src.rpm

Comment 26 Andrew Zabolotny 2008-01-13 16:09:47 EST
Sorry if I haven't made it evident, the version nogravity-2.00-5.fc9.src.rpm
works fine for me (two-head config).
Comment 27 Michel Alexandre Salim 2008-01-27 09:59:56 EST
Ah, previous reviewer didn't set status back to NEW when he left :(
Comment 28 Michel Alexandre Salim 2008-01-27 18:35:25 EST
APPROVED

Checklist:

MUST

Passed:
• rpmlint
• package name
• spec file name
• package guideline-compliant
• license complies with guidelines
• license field accurate
• license file not deleted
• spec in US English
• spec legible
• source matches upstream
• builds under >= 1 archs, others excluded
Tested with Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=376265
• build dependencies complete
see above
• own all directories
• no dupes in %files
• permission
• %clean RPM_BUILD_ROOT
• macros used consistently
• Package contains code
• desktop file uses desktop-file-install
• clean buildroot before install
• filenames UTF-8

SHOULD
Passed:
• package build in mock on all architectures
• package functioned as described
• scriplets are sane
• require package not files
Comment 29 Hans de Goede 2008-01-28 04:08:22 EST
Thanks for the review!

New Package CVS Request
=======================
Package Name:      nogravity
Short Description: Space shooter in 3D
Owners:            jwrdegoede
Branches:          F-7 F-8 devel
InitialCC:         <empty>
Cvsextras Commits: yes
Comment 30 Dennis Gilmore 2008-01-28 10:37:22 EST
cvs done

Hans in the future please leave the InitialCC list empty for empty.  it saves 
having to edit your request.
Comment 31 Hans de Goede 2008-01-30 08:36:04 EST
(In reply to comment #30)
> cvs done
> 
> Hans in the future please leave the InitialCC list empty for empty.  it saves 
> having to edit your request.

Will do, didn't know things were scripted to that level :)

--

Imported and build for devel, F-8 and F-7, updates for F-7 F-8 requested through
bodhi.

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