Bug 517466 - Review Request: lbreakout2 - A breakout-style arcade game for Linux
Summary: Review Request: lbreakout2 - A breakout-style arcade game for Linux
Keywords:
Status: CLOSED DUPLICATE of bug 435514
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
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:
TreeView+ depends on / blocked
 
Reported: 2009-08-14 08:20 UTC by Stjepan Gros
Modified: 2009-09-26 08:39 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-08-28 06:00:38 UTC


Attachments (Terms of Use)

Description Stjepan Gros 2009-08-14 08:20:48 UTC
Spec URL: http://www.zemris.fer.hr/~sgros/stuff/fedora/lbreakout2/lbreakout2.spec
SRPM URL: http://www.zemris.fer.hr/~sgros/stuff/fedora/lbreakout2/lbreakout2-2.6-0.1.beta7.fc11.src.rpm

Description:
A breakout-style arcade game for Linux. I guess all of you know how
to play breakout basically. Ball bounces around --> paddle keeps ball
in game -> all bricks destroyed --> next level ;-D

Comment 1 Susi Lehtola 2009-08-14 08:38:54 UTC
A few notes:

- The description is nonsense. Use e.g.
"The successor to LBreakout offers you a new challenge in more than 50 levels with loads of new bonuses (goldshower, joker, explosive balls, bonus magnet ...), maluses (chaos, darkness, weak balls, malus magnet ...) and special bricks (growing bricks, explosive bricks, regenerative bricks ...). If you are still hungry for more after that you can create your own levelsets with the integrated level editor."

- The comments to your patches are missing. Add them. Send the patches upstream.

- Add INSTALL="install -p" to make install to preserve time stamps. Use 
'cp -p' instead of 'cp' for the same reason.

- Don't mix %{name} and lbreakout2 in %files - use one or the other and stick with it.

- Remove the docdir created by install; just list the necessary files as %doc.

Comment 2 Stjepan Gros 2009-08-14 16:16:39 UTC
(In reply to comment #1)

> - The description is nonsense. Use e.g.
> "The successor to LBreakout offers you a new challenge in more than 50 levels
> with loads of new bonuses (goldshower, joker, explosive balls, bonus magnet
> ...), maluses (chaos, darkness, weak balls, malus magnet ...) and special
> bricks (growing bricks, explosive bricks, regenerative bricks ...). If you are
> still hungry for more after that you can create your own levelsets with the
> integrated level editor."

Fixed.

> - The comments to your patches are missing. Add them. Send the patches
> upstream.

I sent them upstream, but I doubt anything will happen as the maintainter leaved message in January on the sourceforge page that he's taking break the next few months/years...

Comments should go where?

> - Add INSTALL="install -p" to make install to preserve time stamps. Use
> 'cp -p' instead of 'cp' for the same reason.

Fixed (I hope).

> - Don't mix %{name} and lbreakout2 in %files - use one or the other and stick
> with it.

Changed two lines: '%{_datadir}/%{name}' and '%doc %{_docdir}/%{name}'. Hope that's it?

> - Remove the docdir created by install; just list the necessary files as %doc.

You mean by issuing 'rm -rf' on that directory? Otherwise, I have to generate the patch to prevent doc installation by 'make install'. That probably wont be accepted upstream...

Spec URL:
http://www.zemris.fer.hr/~sgros/stuff/fedora/lbreakout2/lbreakout2.spec
SRPM URL:
http://www.zemris.fer.hr/~sgros/stuff/fedora/lbreakout2/lbreakout2-2.6-0.2.beta7.fc11.src.rpm

Comment 3 Stjepan Gros 2009-08-14 16:36:51 UTC
I corrected some warnings when running rpmlint on binary package.

Spec URL:
http://www.zemris.fer.hr/~sgros/stuff/fedora/lbreakout2/lbreakout2.spec
SRPM URL:
http://www.zemris.fer.hr/~sgros/stuff/fedora/lbreakout2/lbreakout2-2.6-0.3.beta7.fc11.src.rpm

Comment 4 Ed Brand 2009-08-15 18:43:30 UTC
The output of rpmlint looks clean. 

Giving your spec file a once over I see this:
Use %global instead of %define, unless you really need only locally defined submacros within other macro definitions (a very rare case)
See: https://fedoraproject.org/wiki/PackagingGuidelines#.25global_preferred_over_.25define

Comment 5 Susi Lehtola 2009-08-15 20:27:31 UTC
(In reply to comment #2)
> > - The comments to your patches are missing. Add them. Send the patches
> > upstream.
> 
> I sent them upstream, but I doubt anything will happen as the maintainter
> leaved message in January on the sourceforge page that he's taking break the
> next few months/years...
> 
> Comments should go where?

Some people put the comments to the apply phase, i.e.

# Enable bar support
%patch0 -p1

but I usually put them just before listing them, e.g.

# This patch enables bar support
Patch0: foo-bar.patch

and so on, since normally one looks first at the Patch(N) lines.


> > - Don't mix %{name} and lbreakout2 in %files - use one or the other and stick
> > with it.
> 
> Changed two lines: '%{_datadir}/%{name}' and '%doc %{_docdir}/%{name}'. Hope
> that's it?

No, not really.

%{_bindir}/lbreakout2
%{_bindir}/lbreakout2server
%{_datadir}/%{name}
#there is no high scores for now
%config(noreplace) %attr(664, root, games) %{_var}/games/lbreakout2.hscr
%doc AUTHORS ChangeLog COPYING README
%doc %{_docdir}/%{name}
%{_datadir}/applications/%{name}.desktop
%{_datadir}/pixmaps/lbreakout48.gif

What bugs me is that you mix lbreakout2 and %{name}, both of which mean the same thing. I'd replace all occurrences of %{name} in %files with lbreakout2.

> > - Remove the docdir created by install; just list the necessary files as %doc.
> 
> You mean by issuing 'rm -rf' on that directory? Otherwise, I have to generate
> the patch to prevent doc installation by 'make install'. That probably wont be
> accepted upstream...

Yes, exactly: remove the documentation installed by 'make install' at the end of %install with 'rm -rf' and just list the necessary files in %doc, which will install them in the correct directory. The current listing

%doc AUTHORS ChangeLog COPYING README
%doc %{_docdir}/%{name}

looks like you end up with the directories %{_docdir}/%{name} and %{_docdir}/%{name}-%{version}.

PS. You can probably drop the
 --with-docdir=/usr/share/doc
argument from %configure. (I haven't looked at the build at all, though.)

Comment 6 Stjepan Gros 2009-08-18 20:32:07 UTC
(In reply to comment #4)
> Giving your spec file a once over I see this:
> Use %global instead of %define, unless you really need only locally defined
> submacros within other macro definitions (a very rare case)
> See:
> https://fedoraproject.org/wiki/PackagingGuidelines#.25global_preferred_over_.25define  

Changed.

(In reply to comment #5)
> Some people put the comments to the apply phase, i.e.
> 
> # Enable bar support
> %patch0 -p1
> 
> but I usually put them just before listing them, e.g.
> 
> # This patch enables bar support
> Patch0: foo-bar.patch
> 
> and so on, since normally one looks first at the Patch(N) lines.

I've added comments to PatchN lines.

> What bugs me is that you mix lbreakout2 and %{name}, both of which mean the
> same thing. I'd replace all occurrences of %{name} in %files with lbreakout2.

Replaced.
 
> Yes, exactly: remove the documentation installed by 'make install' at the end
> of %install with 'rm -rf' and just list the necessary files in %doc, which will
> install them in the correct directory. The current listing

Removed and added to %doc the whole subdirectory with manual files, i.e. docs/.

> PS. You can probably drop the
>  --with-docdir=/usr/share/doc
> argument from %configure. (I haven't looked at the build at all, though.)  

The default location to install the documentation is /usr/lbreakout2/docs so I used this switch to change the location. Now I removed it since it is not necessary any more.

New files:

Spec URL:
http://www.zemris.fer.hr/~sgros/stuff/fedora/lbreakout2/lbreakout2.spec
SRPM URL:
http://www.zemris.fer.hr/~sgros/stuff/fedora/lbreakout2/lbreakout2-2.6-0.4.beta7.fc11.src.rpm

Comment 7 Stjepan Gros 2009-08-28 06:00:38 UTC
I just found out that lbreakout2 was rebranded as lbrickbuster2, so I'm closing this ticket.

*** This bug has been marked as a duplicate of bug 435514 ***


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