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).
Hmm, never reviewed a game before. Worth a shot...
Huh, I had a completed review to this package, but Jima was faster ;)
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 :-).
(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.
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.
(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.
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.
(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.
(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.
'K, fair enough. Consider my objections dropped. :-)
Hm... So if "Requires: hicolor-icon-theme" is used then described by you case won't take place (do I understand it good?)
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.
Imported and build, closing