Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 823873 - Review Request: ThePowderToy - falling sand physics sandbox game
Summary: Review Request: ThePowderToy - falling sand physics sandbox game
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2012-05-22 11:02 UTC by Nazar Mishturak
Modified: 2013-01-22 19:11 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-01-22 19:11:02 UTC


Attachments (Terms of Use)

Description Nazar Mishturak 2012-05-22 11:02:02 UTC
Spec URL: https://docs.google.com/open?id=0B_CR4N27LxsuM2NjU3JCci1wYk0
SRPM URL: https://docs.google.com/open?id=0B_CR4N27LxsuWGs3aDN4a2tfX3c
Description: The Powder Toy is a desktop version of the classic 'falling sand' physics sandbox game, it simulates air pressure and velocity as well as heat!
Fedora Account System Username: blasterblade
Thats my first package, and I don't know where to host my files. Game is not mine, I am just a packager. Visit http://powdertoy.co.uk/ for details

Comment 1 Michael Cronenworth 2012-05-22 13:54:33 UTC
A few concerns:

Could you add the git checkout command used to create the tarball instead of "last git revision"?

Instead of using the git HEAD, could you ask upstream to put .tar.xz files on their Download Page for the stable releases? You could then use that tarball for your source tarball.

There is no .desktop file and most users would expect to find this listed in the "Games" menu. You should include one or ask upstream to create one.

Comment 2 Nazar Mishturak 2012-05-22 14:02:33 UTC
Latest stable release is 78.1. I used git clone git://github.com/FacialTurd/The-Powder-Toy.git from https://github.com/FacialTurd/The-Powder-Toy(their project page). So, I will now recompile for its stable version(it has less features :()

Comment 3 Nazar Mishturak 2012-05-22 14:11:51 UTC
Ok, I found the commit is 60e31ac78d1d59927a60c67d8d3e17929671dc18, version is 78.1(stable). I will now recompile. They don't use version numbers in git tag :(

Comment 4 Michael Cronenworth 2012-05-22 14:20:57 UTC
I'm not saying you are required to use the stable release, but I would recommend it if you are going to include this game in stable Fedora releases.

In any case, what we need is the git commit hash, or the checkout date, to reliably get the same source tarball that you created.

See the guidelines:
http://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control

Comment 5 Nazar Mishturak 2012-05-22 14:26:34 UTC
Well, I gave you the hash, but I made some minor changes to the code. Added a few defines(include file path) for it to compile and changed the Makefile(library name). Also I fixed parts in code where game saves it data. Now all of user data is stored in ~/.powder, not in current working directory. Do I need to post these changes? I can give you a diff to patch. Sorry If I am doing something wrong, it is my first time posting a package.

Comment 7 Michael Cronenworth 2012-05-22 17:09:58 UTC
Any changes made to upstream's source should be kept as patches and applied during RPM build time instead of inside your source tarball. You should also send the patches to upstream in hope that they apply them so it benefits everyone, and so you do not have to maintain them.

Ex:
Patch0: settings.patch

%prep
%setup -q
%patch0 -p1

FYI: Since this is your first package and I am not a sponsor, I cannot review your package, but these comments will help speed up the process once a sponsor sees this bug. I am looking forward to this being in Fedora. :)

Comment 8 Gwyn Ciesla 2012-07-05 19:00:57 UTC
I am a sponsor and would be interesting in reviewing this and sponsoring you, have you looked at Michael's feedback from #7?  Also, have you done any practice package reviews?

Comment 9 Nazar Mishturak 2012-07-06 19:28:58 UTC
Yes I looked at 7th comment. The patch is distro specific(changing compiler CFLAGS). You can see this at http://powdertoy.co.uk/Wiki/W/Compiling_for_Linux.html in Lua errors section. I haven't done any package reviews before, I am new here:). I don't know where I should put my desktop file? It is not in package git repo.

Comment 10 Gwyn Ciesla 2012-07-06 19:38:22 UTC
Disto-specificness is exactly why it should be seperate.  Source0 should be what you get from upstream.  Then make a patch of your changes, and put Michael's Path0 line under the Source0 line, and the %patch0 line after %setup, as indicated.  For the desktop file, you can use:

Source1: powder.desktop

And then at install, replace powder.desktop with %{SOURCE1}

Then have a look at:

https://fedoraproject.org/wiki/Packaging:Guidelines

You'll find lots of useful information.

Comment 11 Nazar Mishturak 2012-07-07 07:47:56 UTC
It's finished, but I dont know how I where I should upload my Source? files and patches?

Comment 12 Nazar Mishturak 2012-07-07 07:59:12 UTC
Nervermind that, I put them on my website. Package updated to version 81.2.
All of Sources and Patch files are in http://blasterblade.x10.mx/packages/.
SPEC file downloads them(from my website)
SPEC URL: http://blasterblade.x10.mx/packages/ThePowderToy.spec
SRPM URL: http://blasterblade.x10.mx/packages/ThePowderToy-81.2-1.fc17.src.rpm

Comment 13 Gwyn Ciesla 2012-07-24 14:42:56 UTC
Ok, better.  If the Source and Patch files are generated by you, you can leave off the URL, so it should be:

Source0: ThePowderToy-81.2.tar.xz
Source1: Powder.desktop
Source2: powder.png
Patch0: PowderMakefile.patch
Patch1: mainc.patch


Also, there's nothing in the changelog.  Make a changelog entry and increment Release every time you make a change.

Additionally, GPLv3+ is not the only license present.  Run:

find . | xargs licensecheck

over the expanded tarball, there's also BSD, MIT, and Apache.

Comment 14 Michael Cronenworth 2013-01-22 04:08:11 UTC
Per e-mail when I asked if he was going to continue this review:

"Sorry, I have to study now. Feel free to delete request.

Best regards,
Nazar"

Comment 15 Gwyn Ciesla 2013-01-22 19:11:02 UTC
Ok, thanks!


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