Bug 210183 - Review Request: ballbuster - Move the paddle to bounce the ball and break all the bricks
Review Request: ballbuster - Move the paddle to bounce the ball and break all...
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: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-10-10 13:26 EDT by Hans de Goede
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-10 17:06:57 EDT
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 Hans de Goede 2006-10-10 13:26:26 EDT
Spec URL: http://people.atrpms.net/~hdegoede/ballbuster.spec
SRPM URL: http://people.atrpms.net/~hdegoede/ballbuster-1.0-1.src.rpm
Description:
Game inspired by one of the great classics. The purpose of the game is to
remove all the bricks on the screen, by hitting them with a ball. You can
control the ball by bouncing it back at the bricks with a paddle which you
control with your mouse. The game features: A built in level editor, 20 power
ups and special effects (particle, alpha, rotating, and zooming).
Comment 1 Jima 2006-10-10 14:28:28 EDT
Hmm, never reviewed a game before.  Worth a shot...
Comment 2 Michał Bentkowski 2006-10-10 14:36:05 EDT
Huh, I had a completed review to this package, but Jima was faster ;)
Comment 3 Jima 2006-10-10 14:43:31 EDT
By all means, if you've got it done, yank ownership from me and do it.  I
haven't even started the full review yet; I was looking at some minor issues
first.  Just make sure you don't approve it without taking good look at the
dependencies (I'll elaborate in a few minutes, I just want to commit this
hopefully before you delete your review :-).
Comment 4 Michał Bentkowski 2006-10-10 14:53:14 EDT
(In reply to comment #3)
> By all means, if you've got it done, yank ownership from me and do it.  I
> haven't even started the full review yet; I was looking at some minor issues
> first.  Just make sure you don't approve it without taking good look at the
> dependencies (I'll elaborate in a few minutes, I just want to commit this
> hopefully before you delete your review :-).

Well, in that case I won't takie over your ownership.
Package is easy to review and I think you'll make it fast.
Á propos dependencies, build dependencies may be simple checked by using
mock to build packages.
The only thing is dubious is hicolor-icon-theme require. I wanted to try run
the game after deleting this package, but if I wanted to remove hicolor, I
would have to remove 212 other packages. In my opinion, it is very likely
that hicolor-icon-theme require is unncessary.
Comment 5 Jima 2006-10-10 15:01:17 EDT
I think (and I could be wrong) that you might need to change

Requires:       hicolor-icon-theme

 to:

Requires(post):   hicolor-icon-theme
Requires(postun): hicolor-icon-theme

 as I believe that package explicitly needs to be installed during both the
post/postun commands.
 Out of curiosity, does `touch --no-create %{_datadir}/icons/hicolor` need to be
run if gtk2 isn't installed?  If not, you might want to move that command to
within the if block; if so, disregard. :-)

hicolor-icon-theme is required because it owns the directory
/usr/share/icons/hicolor/32x32/apps/ballbuster.png (one of this package's files)
lives in.  (Well, strictly speaking, so do four other packages in Core, and
seven in Extras, but I think it might be the directory's rightful owner.)

Michał (ack, does that even paste sanely?): As I said, I haven't even started a
proper review, so you're welcome to take over.
Comment 6 Michał Bentkowski 2006-10-10 15:26:37 EDT
(In reply to comment #5)
> I think (and I could be wrong) that you might need to change
> 
> Requires:       hicolor-icon-theme
> 
>  to:
> 
> Requires(post):   hicolor-icon-theme
> Requires(postun): hicolor-icon-theme
> 
>  as I believe that package explicitly needs to be installed during both the
> post/postun commands.

I don't think so. hi-color-icon-theme need to be installed because if you
removed that package, it would cause removing of ballbuster.png too
(theoretically). So using it as Requires is good. Now it doesn't make doubt for
me.

>  Out of curiosity, does `touch --no-create %{_datadir}/icons/hicolor` need to 
be
> run if gtk2 isn't installed?  If not, you might want to move that command to
> within the if block; if so, disregard. :-)

It will be run if gtk2 isn't installed, but it won't cause any errors
due to use of " || ;".
Read http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?#head-
7103f6c38d1b5735e8477bdd569ad73ea2c49bda for more informations
> hicolor-icon-theme is required because it owns the directory
> /usr/share/icons/hicolor/32x32/apps/ballbuster.png (one of this package's 
files)
> lives in.  (Well, strictly speaking, so do four other packages in Core, and
> seven in Extras, but I think it might be the directory's rightful owner.)

Yeah, you're right, thanks to point that

> Michał (ack, does that even paste sanely?): As I said, I haven't even 
started a
> proper review, so you're welcome to take over.

My first name should be proper written as "Michał" ;) So okay, I'll review it 
in next comment.
Comment 7 Michał Bentkowski 2006-10-10 15:28:49 EDT
MUST items:
 * rpmlint doesn't show anything
 * package is named well
 * spec file name is good
 * package meets Packaging Guidelines
 * package is licensed with an LGPL open-source compatible license
 * License field in spec file matches actual license
 * license file is included in %doc
 * md5sums are matching (792c7ed1b8c62177ed043eb4955491fe)
 * package successfully compiles on x86_64
 * BuildRequires listed well (mock builds successfully)
 * Requires good
 * no locales
 * proper %post and %postun sections
 * not relocatable
 * package owns directories well
 * duplicates in %files
 * every %files section includes %defattr
 * proper %clean section
 * macros used well
 * no need to -devel and -doc subpackages
 * .desktop file is present

Everything looks good.

Approved.
Comment 8 Jima 2006-10-10 15:41:23 EDT
(In reply to comment #6)
> It will be run if gtk2 isn't installed, but it won't cause any errors
> due to use of " || ;".
> Read http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?#head-
> 7103f6c38d1b5735e8477bdd569ad73ea2c49bda for more informations

I was more concerned with "is it necessary?" than "will it error out?" (which,
right, it won't).  Ultimately it doesn't matter, due to the --no-create (it
won't unnecessarily create the file).

> I don't think so. hi-color-icon-theme need to be installed because if you
> removed that package, it would cause removing of ballbuster.png too
> (theoretically). So using it as Requires is good. Now it doesn't make doubt
> for me.

 Yes, but if hicolor-icon-theme gets removed before ballbuster, it won't take
the directory with it (as it's non-empty); when ballbuster gets removed
(assuming it's the last thing with anything in that directory), the directory
will remain on the system, unowned by anything (in an ideal world where it isn't
improperly owned by 11 other packages, anyway).  But, I'll readily admit that
I'm not positive about that. :-)

 Also, I built the package successfully on devel/i386, FC-5/i386, and FC-5/ppc;
it looks like it'll build across all the supported platforms.
Comment 9 Michał Bentkowski 2006-10-10 16:19:38 EDT
(In reply to comment #8)
> I was more concerned with "is it necessary?" than "will it error out?" (which,
> right, it won't).  Ultimately it doesn't matter, due to the --no-create (it
> won't unnecessarily create the file).

These scripts are certainly good, as they're used in many other packages
and has their place in guideliens (in ScriptletSnippets)

>  Yes, but if hicolor-icon-theme gets removed before ballbuster, it won't take
> the directory with it (as it's non-empty); when ballbuster gets removed
> (assuming it's the last thing with anything in that directory), the directory
> will remain on the system, unowned by anything (in an ideal world where it 
isn't
> improperly owned by 11 other packages, anyway).  But, I'll readily admit that
> I'm not positive about that. :-)

It looks like a really questionable thing. As far as I know, many packages
contain their files in hicolor-icon-theme dirs and they don't have it as
dependency. In fact, I'm still not sure if it's necessary at all.
Comment 10 Jima 2006-10-10 16:25:56 EDT
'K, fair enough. Consider my objections dropped. :-)
Comment 11 Michał Bentkowski 2006-10-10 16:48:31 EDT
Hm... So if "Requires: hicolor-icon-theme" is used then described by you
case won't take place (do I understand it good?)
Comment 12 Hans de Goede 2006-10-10 16:59:22 EDT
Hi guys,

Phew lots of comments. Anyways thanks for the review I'll import it and get it
build.

To answerboth your questions:
1) "Requires: hicolor-icon-theme", must be present in any package which installs
files under /usr/share/icons/hicolor/XxX/apps. I know many packages
don't Require it but that could lead to an empty unowned directory staying
behind on package removal. About the problem of this package getting removed
after hicolor-icon-theme and thus still leaving an empty unowned directory
behind. That isn't possible because rpm (should) order erases to first erase
this package and only then erase any package requiring it. In practice this
doesn't happen due to an rpm bug hence the "(should)", but this bug has been on
the to fix list for a while so maybe it is fixed now. Either it was decided to
ignore this bug and create packages which work with a proper rpm, as avoiding
this problem with the rpm bug present is impossible.

2) The "touch --nocreate" is correct and nescesarry and must be outside the if
   the desktop.org standard /usr/share/icons icons and /usr/share/applications
   are used by other non GTK desktop environments such as KDE too, these don't
   have an in tree icon cache like GTK has, but instead check the timestamp of
   the upper directory of the iocn hierarchy.
Comment 13 Hans de Goede 2006-10-10 17:06:57 EDT
Imported and build, closing

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