Bug 823873 - Review Request: ThePowderToy - falling sand physics sandbox game
Review Request: ThePowderToy - falling sand physics sandbox game
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2012-05-22 07:02 EDT by Nazar Mishturak
Modified: 2013-01-22 14:11 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-01-22 14:11:02 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Nazar Mishturak 2012-05-22 07:02:02 EDT
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 09:54:33 EDT
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 10:02:33 EDT
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 10:11:51 EDT
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 10:20:57 EDT
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 10:26:34 EDT
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 13:09:58 EDT
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 Jon Ciesla 2012-07-05 15:00:57 EDT
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 15:28:58 EDT
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 Jon Ciesla 2012-07-06 15:38:22 EDT
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 03:47:56 EDT
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 03:59:12 EDT
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 Jon Ciesla 2012-07-24 10:42:56 EDT
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-21 23:08:11 EST
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 Jon Ciesla 2013-01-22 14:11:02 EST
Ok, thanks!

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