Bug 366841

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

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.

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.

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.

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.

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.

(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.

Erm, Peter I spend a lot of time writing that patch and I would really like to
see this in Fedora soon, so ... ping?

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.

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. 

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.

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

(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.

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.

[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 :)

(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?

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?

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.

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

Yes, it is same as my changed script except that I've used "-1" instead of "-n 1" :)

Otherwise, it works fine.

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.

Hans, please file a new bug so I can close this one.

(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.

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.

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

Sorry if I haven't made it evident, the version nogravity-2.00-5.fc9.src.rpm
works fine for me (two-head config).

Ah, previous reviewer didn't set status back to NEW when he left :(

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

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

cvs done

Hans in the future please leave the InitialCC list empty for empty.  it saves 
having to edit your request.

(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.