Bug 238562 - Review Request: machineball - A futuristic ball game with simple rules
Review Request: machineball - A futuristic ball game with simple rules
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michał Bentkowski
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-01 10:00 EDT by Hans de Goede
Modified: 2007-11-30 17:12 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-12 04:51:17 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mr.ecik: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2007-05-01 10:00:25 EDT
Spec URL: http://people.atrpms.net/~hdegoede/machineball.spec
SRPM URL: http://people.atrpms.net/~hdegoede/machineball-1.0.1-1.fc7.src.rpm
Description:
Machine Ball is a futuristic sport with amazing 3D graphics and realistic
physics with very simple rules: Get the ball into your opponents goal. You can
use your machine to push the ball in, or you can collect powerups such as
missiles and blast the ball into the goal. Be creative.
Comment 1 Bernard Johnson 2007-05-01 14:58:25 EDT
Missing call to update-desktop-database in %post and %postun.

Missing Requires(post), Requires(postun) on desktop-file-utils

Doesn't build in mock:
g++ -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables   -c -o intro.o intro.cpp
intro.cpp:13:20: error: GL/glu.h: No such file or directory
make: *** [intro.o] Error 1
Comment 2 Bernard Johnson 2007-05-01 22:19:55 EDT
(In reply to comment #1)
> Missing Requires(post), Requires(postun) on desktop-file-utils

Someone reminded me today that this is deprecated, so never mind on this one.
Comment 3 Michał Bentkowski 2007-05-02 17:18:07 EDT
(In reply to comment #1)
> Missing call to update-desktop-database in %post and %postun.

According to ScripletSnippets we only need update-desktop-database only if a 
desktop entry has a MimeType key. This package's desktop file doesn't have a 
MimeType so we don't need update-... at all :)

> Doesn't build in mock:

BuildRequires:  mesa-libGLU-devel fixes this issue.

Package seems quite interesting so I'm probably going to review it. However, 
feel free to take it as long as it's not assigned to me.
Comment 4 Hans de Goede 2007-05-03 02:23:39 EDT
(In reply to comment #3)
> > Doesn't build in mock:
> 
> BuildRequires:  mesa-libGLU-devel fixes this issue.
> 
> Package seems quite interesting so I'm probably going to review it. However, 
> feel free to take it as long as it's not assigned to me.

Thanks,

I'll at that BR together with any other necessary changes once a full review is
done, if you want a new version with the BR added before starting the review let
me know.
Comment 5 Michał Bentkowski 2007-05-08 17:34:01 EDT
Sorry that you had to wait a long time. I'll make a full review within 24 hours.
Comment 6 Michał Bentkowski 2007-05-09 11:57:16 EDT
REVIEW:
 * rpmlint is quiet
!* missing BR: mesa-libGLU-devel
 * licensed under GPL license and its text's included
 * required %post(un) supplied
 * dist tag present
!* no macros in Source0 tag
 * buildroot's fine
 * package doesn't create any new directories
 * proper permissions of all files
 * desktop file seems fine
 * %clean section is fine
 * source's md5sum is good, but
!* curiously enough, its name is different than the offical one. Adding
 %{version} tag should fix that problem

THINGS TO DO:
 - add missing BR: mesa-libGLU-devel
 - add at least %{version} to Source0 tag
 - upload proper source code file to srpm (current one has a wrong name)

The last issue forces change of versioning. Probably you can just replace "-" 
with ".", but if aby other (better) ideas are very welcome.
Comment 7 Michał Bentkowski 2007-05-09 12:05:19 EDT
(In reply to comment #6)
> The last issue forces change of versioning. Probably you can just replace "-" 
> with ".", but if aby other (better) ideas are very welcome.

I should have written "but any other (better) ideas are very welcome" ;)
Comment 8 Hans de Goede 2007-05-10 15:34:29 EDT
(In reply to comment #6)
> THINGS TO DO:
>  - add missing BR: mesa-libGLU-devel
Done
>  - add at least %{version} to Source0 tag
>  - upload proper source code file to srpm (current one has a wrong name)
Source0 url fixed and changed to use %{name} and %{version}. I started this
packaged based on a srpm send to me by someone who had read on my wikipage that
I was interested on this, he renamed the tarbal (and I didn't check).


> The last issue forces change of versioning. Probably you can just replace "-" 
> with ".", but if aby other (better) ideas are very welcome.

The fix is easy, the real version is 1.0, the added -1 in the src tarbal is
because upstream uses binary rpms as their primary distribution method, and they
don't know what the release field is for, so they add it to their source tarbals
too. I've seen this confused behaviour by other upstreams too, for example with
blobwars.


New version with all this fixed here:
Spec URL: http://people.atrpms.net/~hdegoede/machineball.spec
SRPM URL: http://people.atrpms.net/~hdegoede/machineball-1.0-2.fc7.src.rpm
Comment 9 Michał Bentkowski 2007-05-11 11:22:02 EDT
(In reply to comment #8)
> The fix is easy, the real version is 1.0, the added -1 in the src tarbal (...)

Right. The easiest solutions are the best ones ;)

I like the package now and nothing stands in the way to approve it.
The game looks quite nice, but it's a pity that there's no AI mode ;)
Comment 10 Hans de Goede 2007-05-11 11:39:55 EDT
New Package CVS Request
=======================
Package Name:      machineball
Short Description: A futuristic ball game with simple rules
Owners:            j.w.r.degoede@hhs.nl
Branches:          FC-6 devel
InitialCC:         <empty>


Comment 11 Hans de Goede 2007-05-12 04:51:17 EDT
Thanks for the review!

Imported and build, closing.

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