[Bug 517466] Review Request: lbreakout2 - A breakout-style arcade game for Linux

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=517466





--- Comment #5 from Jussi Lehtola <jussi.lehtola at iki.fi>  2009-08-15 16:27:31 EDT ---
(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.)

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.