Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1149649 - Review Request: tuxfootball - funny soccer game
Summary: Review Request: tuxfootball - funny soccer game
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
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: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2014-10-06 10:59 UTC by Raphael Groner
Modified: 2014-10-25 11:37 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-10-23 22:22:08 UTC


Attachments (Terms of Use)

Description Raphael Groner 2014-10-06 10:59:21 UTC
Spec URL: https://raphgro.fedorapeople.org/review/tuxfootball/tuxfootball.spec
SRPM URL: https://raphgro.fedorapeople.org/review/tuxfootball-0.3.1-0.1.fc20.src.rpm
Description: funny soccer game
Fedora Account System Username: raphgro

koji build rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=7774452

Comment 1 Andrea Musuruane 2014-10-16 15:49:30 UTC
Correct SRPM URL seem to be:
https://raphgro.fedorapeople.org/review/tuxfootball/tuxfootball-0.3.1-0.1.fc20.src.rpm

I'm using this one to perform the review.

Release tag is wrong. You are using a pre-release tag but tuxfootball-0.3.1 has been released.
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag

Better, more descriptive, summary: "Great 2D soccer (sometimes called football) game"

License is wrong. It is "GPLv2+" (note the +) not "GPLv2".

Source0 URL is wrong. It should be:
http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
https://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net

In this way you can get rid of the %{ver} macro.

Guidelines require to "Requires:" a package when you install a file into a directory that the package does not own. You install icons into /usr/share/icons/hicolor and therefore you MUST "Requires: hicolor-icon-theme".
https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

You must use trademarks in a way that is not ambiguous. Avoid phrasing like "similar to" or "like". Therefore avoid using "Amco's Kick Off" and "Sensible Software's Sensible Soccer" in description.
https://fedoraproject.org/wiki/Packaging:Guidelines#summary

You can improve the description adding:
The gameplay is designed to be quick, responsive and fun. You are always
in control of the player closest to the ball. The ball is controlled via
two different kick buttons - one for pass, and one for shoot. Aftertouch
can be applied to shots by quickly pressing and holding the direction you
want the ball to bend towards. Pushing in the opposite direction to what
you kicked the ball makes it raise into the air, pushing in the same
direction as the ball makes it dip towards the ground.
 
You MUST use scriptlets to update the icon cache to ensure that the Desktop files are able to load the included icon files. 
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

Because the data package is build from the same source as the main package, there is no real gain in splitting them apart.

You can use %{_pkgdocdir} instead of %{_docdir}/%{name}

Remove ChangeLog from docs. It is irrelevant documentation (it is about source code commits) and it should not be packaged.
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

The package uses gettext for translations so you should add "BuildRequires: gettext". 

Moreover, you MUST use the %find_lang macro.
https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files

Compilation is not verbose. Therefore it is not possible to check the compiler flag used. Invoke make like "make %{?_smp_mflags} V=1"

Incorrect FSF addess should be reported upstream.
https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address

tuxfootball-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/tuxfootball-0.3.1/src/SFont.c
tuxfootball-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/tuxfootball-0.3.1/src/SFont.h
tuxfootball-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/tuxfootball-0.3.1/src/Font.hpp

I'll perform a deeper analysis after these issues are fixed.

Comment 2 Raphael Groner 2014-10-18 19:21:37 UTC
Thanks a lot for your interest. I'll give some thought and come back later to the open issues. Sorry for the delay.

Comment 3 Raphael Groner 2014-10-23 18:12:10 UTC
If you notice some issues that need to be solved before you want to start a formal review, add these issues in a comment and set the Whiteboard of the bug to contain NotReady. This helps other possible reviewers to notice that the review request is not yet ready for further review action. 
https://fedoraproject.org/wiki/Package_Review_Process#Reviewer

Comment 4 Andrea Musuruane 2014-10-23 19:53:28 UTC
The NotReady is only used not to make other possible reviewer lose time by looking at a reviews that are not yet ready.

I already assigned this package review to myself (so you already had found a reviewer!) and I already started the review process. The review process is an interative process. I asked you, the packager, to fix some items and I expect you to do so to continue the review process.

I reassign the review to myself. 

Anyway if you are not happy with my review, please speak up.

Comment 5 Raphael Groner 2014-10-23 21:07:21 UTC
Andrea, I am sorry to have to speak that out, but your review is not of good quality. I don't get the connection to the official review guidelines. Therefore I would like to find another reviewer, I think to have the right to not have to accept any first coming random reviewer, cause there could be several reviewers doing just informal review hints. Of course, the approval could be done only by one reviewer that thinks all review issues are fixed.


(In reply to Andrea Musuruane from comment #1)
> Correct SRPM URL seem to be:
> https://raphgro.fedorapeople.org/review/tuxfootball/tuxfootball-0.3.1-0.1.
> fc20.src.rpm

I don't understand why you do a review for an URL I did not post. Maybe it was a typo of mine, maybe not. It's only an assumption from your side that may be correct. So that's the first point why you should not accept a formal review.

> I'm using this one to perform the review.
> 
> Release tag is wrong. You are using a pre-release tag but tuxfootball-0.3.1
> has been released.
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag

Where's written that 0.1 is not allowed for a pre-release of the spec file itself? My idea was to use 0.x for preofficial spec files, 1 will then be when it's allowed to go into SCM.

> Better, more descriptive, summary: "Great 2D soccer (sometimes called
> football) game"

Why is this more descriptive?

> License is wrong. It is "GPLv2+" (note the +) not "GPLv2".

License GPLv2 includes any upstream compatibility. But I can use + additionally, if you think this is so important.

> Source0 URL is wrong. It should be:
> http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
> https://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net

Should or must? My link is documented upstream and works for a correct download of the source tarball.

> In this way you can get rid of the %{ver} macro.

Why to get rid of it?

> Guidelines require to "Requires:" a package when you install a file into a
> directory that the package does not own. You install icons into
> /usr/share/icons/hicolor and therefore you MUST "Requires:
> hicolor-icon-theme".
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#File_and_Directory_Ownership

Wrong. My package does not need any icons from hicolor-icon-theme. The folder could be owned by several packages, like it's also so for any other folders.

> You must use trademarks in a way that is not ambiguous. Avoid phrasing like
> "similar to" or "like". Therefore avoid using "Amco's Kick Off" and
> "Sensible Software's Sensible Soccer" in description.
> https://fedoraproject.org/wiki/Packaging:Guidelines#summary
> 
> You can improve the description adding:
> The gameplay is designed to be quick, responsive and fun. You are always
> in control of the player closest to the ball. The ball is controlled via
> two different kick buttons - one for pass, and one for shoot. Aftertouch
> can be applied to shots by quickly pressing and holding the direction you
> want the ball to bend towards. Pushing in the opposite direction to what
> you kicked the ball makes it raise into the air, pushing in the same
> direction as the ball makes it dip towards the ground.

The original description was taken from upstream.

> You MUST use scriptlets to update the icon cache to ensure that the Desktop
> files are able to load the included icon files. 
> https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

ok

> Because the data package is build from the same source as the main package,
> there is no real gain in splitting them apart.

The data content is noarch. I think doing that is generally a good practice, and I have already done that for other approved packages, too.

> You can use %{_pkgdocdir} instead of %{_docdir}/%{name}

What will be the benefit? My other packages are not using %{_pkgdocdir}.

> Remove ChangeLog from docs. It is irrelevant documentation (it is about
> source code commits) and it should not be packaged.
> https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

The question is not if it's irrelevant. It's more about if upstream wishes to have ChangeLog shipped as official documentation. So better include it than being punished for not distributing it. I don't feel responsible as a package for cleaning up the tarball I got from upstream.

> The package uses gettext for translations so you should add "BuildRequires:
> gettext". 

I have tested with help from mock (koji build). It does not complain.

> Moreover, you MUST use the %find_lang macro.
> https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files

Unclear why this is MUST.

> Compilation is not verbose. Therefore it is not possible to check the
> compiler flag used. Invoke make like "make %{?_smp_mflags} V=1"

The right compiler flags are ensured by using %make macro.

> Incorrect FSF addess should be reported upstream.
> https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address

That should not block the approval.

> I'll perform a deeper analysis after these issues are fixed.

As I can not fulfill all of your wishes, I don't see why you want to and think to be still able to do a formal review.

Comment 6 Andrea Musuruane 2014-10-23 22:04:24 UTC
(In reply to Raphael Groner from comment #5)
> Andrea, I am sorry to have to speak that out, but your review is not of good
> quality. I don't get the connection to the official review guidelines.
> Therefore I would like to find another reviewer, I think to have the right
> to not have to accept any first coming random reviewer, cause there could be
> several reviewers doing just informal review hints. Of course, the approval
> could be done only by one reviewer that thinks all review issues are fixed.

Well, you are the first one to think I am not a good reviewer. But I won't go against your wishes.

> (In reply to Andrea Musuruane from comment #1)
> > Correct SRPM URL seem to be:
> > https://raphgro.fedorapeople.org/review/tuxfootball/tuxfootball-0.3.1-0.1.
> > fc20.src.rpm
> 
> I don't understand why you do a review for an URL I did not post. Maybe it
> was a typo of mine, maybe not. It's only an assumption from your side that
> may be correct. So that's the first point why you should not accept a formal
> review.

You know that it was a typo and you know that I got the right SRPM (BTW, I just browsed the correct directory in fedorapeople and got the only SRPM there). Why are you even annoying me with this?

> > Release tag is wrong. You are using a pre-release tag but tuxfootball-0.3.1
> > has been released.
> > https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag
> 
> Where's written that 0.1 is not allowed for a pre-release of the spec file
> itself? My idea was to use 0.x for preofficial spec files, 1 will then be
> when it's allowed to go into SCM.

Because version 0.3.1 is not a PRE-release. It'a RELEASE. Therefore your relase tag is wrong and it MUST NOT start with a 0.

> > Better, more descriptive, summary: "Great 2D soccer (sometimes called
> > football) game"
> 
> Why is this more descriptive?

Again. You are just trying to annoy me. I just tryed to help.

> > License is wrong. It is "GPLv2+" (note the +) not "GPLv2".
> 
> License GPLv2 includes any upstream compatibility. But I can use +
> additionally, if you think this is so important.

You don't even know what you are talking about.

> > Source0 URL is wrong. It should be:
> > http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
> > https://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net
> 
> Should or must? My link is documented upstream and works for a correct
> download of the source tarball.

I read SHOULD. Don't you?

Anyway the Guidelines suggest the other URL because it is not supposed to change over time.

> > In this way you can get rid of the %{ver} macro.
> 
> Why to get rid of it?

Because it's no longer needed?
 
> > Guidelines require to "Requires:" a package when you install a file into a
> > directory that the package does not own. You install icons into
> > /usr/share/icons/hicolor and therefore you MUST "Requires:
> > hicolor-icon-theme".
> > https://fedoraproject.org/wiki/Packaging:
> > Guidelines#File_and_Directory_Ownership
> 
> Wrong. My package does not need any icons from hicolor-icon-theme. The
> folder could be owned by several packages, like it's also so for any other
> folders.

hicolor-icon-theme owns the directory where you install the icon files. Just type 
"rpm -ql hicolor-icon-theme" to understand it.

> > You must use trademarks in a way that is not ambiguous. Avoid phrasing like
> > "similar to" or "like". Therefore avoid using "Amco's Kick Off" and
> > "Sensible Software's Sensible Soccer" in description.
> > https://fedoraproject.org/wiki/Packaging:Guidelines#summary
> > 
> > You can improve the description adding:
> > The gameplay is designed to be quick, responsive and fun. You are always
> > in control of the player closest to the ball. The ball is controlled via
> > two different kick buttons - one for pass, and one for shoot. Aftertouch
> > can be applied to shots by quickly pressing and holding the direction you
> > want the ball to bend towards. Pushing in the opposite direction to what
> > you kicked the ball makes it raise into the air, pushing in the same
> > direction as the ball makes it dip towards the ground.
> 
> The original description was taken from upstream.

Sure. Mine too. It was just a friendly suggestion.

> > You can use %{_pkgdocdir} instead of %{_docdir}/%{name}
> 
> What will be the benefit? My other packages are not using %{_pkgdocdir}.

Again, just a friendly suggestion. 

%{_pkgdocdir} expands correctly even for older versions of Fedora or RHEL where docs where installed in %{_docdir}/%{name}-%{version}.

> > Remove ChangeLog from docs. It is irrelevant documentation (it is about
> > source code commits) and it should not be packaged.
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
> 
> The question is not if it's irrelevant. It's more about if upstream wishes
> to have ChangeLog shipped as official documentation. So better include it
> than being punished for not distributing it. I don't feel responsible as a
> package for cleaning up the tarball I got from upstream.

Actually the packager is supposed to do this clean up.

> > The package uses gettext for translations so you should add "BuildRequires:
> > gettext". 
> 
> I have tested with help from mock (koji build). It does not complain.

Yes, it does not complain but the review guidelines tell you to do so because otherwise "your package could fail to generate translation files in the buildroot"

> > Moreover, you MUST use the %find_lang macro.
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files
> 
> Unclear why this is MUST.

Have you bothered to read the Guidelines? Especially the section "Why do we need to use %find_lang?"?
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Why_do_we_need_to_use_.25find_lang.3F

> > Compilation is not verbose. Therefore it is not possible to check the
> > compiler flag used. Invoke make like "make %{?_smp_mflags} V=1"
> 
> The right compiler flags are ensured by using %make macro.

No, beacuse you don't know if they are overwritten by other flags if you actually cannot read them.

> > Incorrect FSF addess should be reported upstream.
> > https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address
> 
> That should not block the approval.

And it does not!

> > I'll perform a deeper analysis after these issues are fixed.
> 
> As I can not fulfill all of your wishes, I don't see why you want to and
> think to be still able to do a formal review.

Those are not my wishes. Some of them are MUST items and you must fulfill them before any reviewer can actually approve this review. The others are SHOULD items and they are just suggestions to improve your packager's skills. You should really start listening to other experienced packagers instead of annoying people.

Good luck finding another reviewer!

Comment 7 Raphael Groner 2014-10-23 22:22:08 UTC
Thanks for killing this review request.


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