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