Bug 1020839

First thing I see, no check on the .desktop file is run:

"It is not simply enough to just include the .desktop file in the package, one MUST run desktop-file-install (in %install) OR desktop-file-validate (in %check or %install) and have BuildRequires: desktop-file-utils, to help ensure .desktop file safety and spec-compliance."

https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files

You can drop the Buildroot:

"Fedora (as of F-10) does not require the presence of the BuildRoot tag in the spec and if one is defined it will be ignored. The provided buildroot will automatically be cleaned before commands in %install are called."

https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

The provided .src.rpm doesn't seem to have been built from the provided .spec: its Source0 is http://blaskovic.fedorapeople.org/fedora-gooey-karma/0.1.tar.gz , which doesn't match the .spec.

The source tree clearly specifies the license to be GPLv3+, but the License: tag in the spec file states GPLv2+ .

Thank you for review!

(In reply to Adam Williamson from comment #1)
> First thing I see, no check on the .desktop file is run:

Added.

(In reply to Adam Williamson from comment #2)
> You can drop the Buildroot:

Removed.

(In reply to Adam Williamson from comment #3)
> The provided .src.rpm doesn't seem to have been built from the provided
> .spec

My bad. I've just uploaded the correct one.

(In reply to Adam Williamson from comment #4)
> The source tree clearly specifies the license to be GPLv3+, but the License:
> tag in the spec file states GPLv2+ .

Fixed in spec file.

I might change the Source0 to https://github.com/blaskovic/fedora-gooey-karma/archive/%{name}-%{version}.tar.gz , as that's what the file is actually called once it gets through github's magic.

fedora-gooey-karma.noarch: E: script-without-shebang /usr/share/fedora-gooey-karma/mainwindow_gui.py

if that script isn't meant to be called directly it should not have executable permissions; if it is, then it should have a shebang.

Why is the .desktop file marked "OnlyShowIn=GNOME;" ? Is there some reason it won't work on other desktops?

The app not having an icon is kinda bad, you could ask if the art team could provide you with one.

un-taking this one as I'm not a sponsor (yet), but I'll still do an informal review and look at becoming a sponsor.

Going through the review process, consider all 'MUST' and 'SHOULD' elements as OK unless posted here...

rpmlint output:

[adamw@vaioz noarch]$ rpmlint fedora-gooey-karma-0.1-1.fc20.noarch.rpm 
fedora-gooey-karma.noarch: W: no-documentation
fedora-gooey-karma.noarch: E: script-without-shebang /usr/share/fedora-gooey-karma/mainwindow_gui.py
fedora-gooey-karma.noarch: W: no-manual-page-for-binary fedora-gooey-karma
1 packages and 0 specfiles checked; 1 errors, 2 warnings.
[adamw@vaioz noarch]$ rpmlint ../../SRPMS/fedora-gooey-karma-0.1-1.fc20.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings.

script-without-shebang noted above, the entire lack of documentation may or may not be a problem for a fairly 'trivial' app.

MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc - this is not done, the package includes a COPYING file (GPLv3) but it is not packaged.

MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. - see note about python file above

SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense. - there aren't any, as noted by rpmlint. But it probably doesn't make sense to have one for a simple command/app which probably takes no or few arguments.

I'm assuming it's permissible to simply ignore the Python guidelines when you're not actually packaging a Python module, but just a little app which happens to be a pile of Python code, otherwise this would be completely out of line with the requirements.

Some comments:

- spec file mixes $RPM_BUILD_ROOT and %{buildroot}, pick one form only.

- %clean section is not needed: https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#.25clean

- %defattr is not needed:

https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#File_Permissions

Add leading space when using \ :

desktop-file-install \
    --dir=%{buildroot}%{_datadir}/applications \
    %{buildroot}/%{_datadir}/applications/fedora-gooey-karma.desktop

- Most of empty lines seems unneeded.

- Summary is cryptic, please improve.

> I might change the Source0 to
> https://github.com/blaskovic/fedora-gooey-karma/archive/%{name}-%{version}.
> tar.gz , as that's what the file is actually called once it gets through
> github's magic.

I would leave it as is. It's creating (after unpacking) fedora-gooey-karma-fedora-gooey-karma-0.1 folder when used with your url.

> fedora-gooey-karma.noarch: E: script-without-shebang
> /usr/share/fedora-gooey-karma/mainwindow_gui.py
> if that script isn't meant to be called directly it should not have
> executable permissions; if it is, then it should have a shebang.

Fixed. This script should not be executable.

$ rpmlint fedora-gooey-karma-0.1-1.fc20.noarch.rpm
fedora-gooey-karma.noarch: W: no-manual-page-for-binary fedora-gooey-karma
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

$ rpmlint fedora-gooey-karma-0.1-1.fc20.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

> Why is the .desktop file marked "OnlyShowIn=GNOME;" ? Is there some reason
> it won't work on other desktops?

Removed. I recycled that .desktop file from some app which had this. My bad.

> The app not having an icon is kinda bad, you could ask if the art team could
> provide you with one.

I will try to create one or contact somebody with some art-skill.

> MUST: If (and only if) the source package includes the text of the
> license(s) in its own file, then that file, containing the text of the
> license(s) for the package must be included in %doc - this is not done, the
> package includes a COPYING file (GPLv3) but it is not packaged.

Fixed. It's packaged (via %doc) now.

> MUST: Permissions on files must be set properly. Executables should be set
> with executable permissions, for example. - see note about python file above

Fixed as mentioned above.

> SHOULD: your package should contain man pages for binaries/scripts. If it
> doesn't, work with upstream to add them where they make sense. - there
> aren't any, as noted by rpmlint. But it probably doesn't make sense to have
> one for a simple command/app which probably takes no or few arguments.

I can create a simple man page for this. But it's not required in my opinion because this app has 0 arguments.

> - spec file mixes $RPM_BUILD_ROOT and %{buildroot}, pick one form only.

I've picked %{buildroot}.

> - %clean section is not needed:
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#.
> 25clean

Removed.

> - %defattr is not needed:
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#File_Permissions

Removed.

> Add leading space when using \ :
> 
> desktop-file-install \
>     --dir=%{buildroot}%{_datadir}/applications \
>     %{buildroot}/%{_datadir}/applications/fedora-gooey-karma.desktop

Fixed.

> - Most of empty lines seems unneeded.

I've removed doubled empty lines.

> - Summary is cryptic, please improve.

Does somebody have some suggestions to make the summary clear?

TODO:

- Man pages for binary? (man fedora-gooey-karma)
- Graphical icon?
- Summary improve.

branislav: I think it's fine to skip the man pages, and the other two steps would be nice but probably don't block the review. We should probably try and find you a sponsor.

Created attachment 816073 [details]
Icon mockup

fedora-gooey-karma has icon!

I've asked Máirín Duffy for them and she did it. I am very thankfull!

Thanks for improvements, if you could post urls to your updated spec, srpm
and do a koji scratch build it would help a lot :-)

File at "Spec URL" differs from spec file in src.rpm at "SRPM URL". The %doc line is different.

If you kept those two lines in the review ticket up-to-date, it would become convenient to run "fedora-review -b 1020839" and let that tool perform some packaging checks.


> Summary: GUI tool for adding karma to Bodhi system.
> Similar to fedora-easy-karma

What I find surprising here is that the %summary uses two terms not used in the %description: karma, Bodhi

I would drop the insider talk and write:

  Summary: GUI for sending feedback about installed Test Update packages

The %description could expand on that and mention the "Fedora Updates System" (bodhi), explain that users/testers can vote about test-updates using "karma" points. If you consider it relevant, you could mention the CLI tool fedora-easy-karma in the description, but I think they are different enough. And in a related search, fedora-easy-karma ought to turn up as well.


> desktop-file-install
> desktop-file-validate

Using either one is sufficient. desktop-file-install also validates the specified file.


> desktop-file-install \
> --dir=%{buildroot}%{_datadir}/applications \
> %{buildroot}/%{_datadir}/applications/fedora-gooey-karma.desktop

What this does is it reinstalls (!) and validates the existing desktop file found at the path inside the %buildroot. But if the desktop file is located in the %buildroot already, you only need to validate it. Hence I suggest you only run desktop-file-validate and not desktop-file-install. Using the latter makes more sense if you need to install the file actually, or if you want to use the tool's options to modify the .desktop file on-the-fly.


Files that are not supposed to be executed should not be mode +x:

-rwxr-xr-x  /usr/share/fedora-gooey-karma/bodhiworker.py
-rwxr-xr-x  /usr/share/fedora-gooey-karma/browser.py
-rwxr-xr-x  /usr/share/fedora-gooey-karma/config.py
-rwxr-xr-x  /usr/share/fedora-gooey-karma/customgui.py
-rwxr-xr-x  /usr/share/fedora-gooey-karma/idlequeue.py
-rwxr-xr-x  /usr/share/fedora-gooey-karma/idlequeuedispatcher.py
-rwxr-xr-x  /usr/share/fedora-gooey-karma/packagesworker.py
-rwxr-xr-x  /usr/share/fedora-gooey-karma/sendkarma.py

(In reply to Terje Røsten from comment #15)
> Thanks for improvements, if you could post urls to your updated spec, srpm
> and do a koji scratch build it would help a lot :-)

It should be updated now.

(In reply to Michael Schwendt from comment #16)
> File at "Spec URL" differs from spec file in src.rpm at "SRPM URL". The %doc
> line is different.

Updated as mentioned above. I will try to keep it updated. My bad.

> If you kept those two lines in the review ticket up-to-date, it would become
> convenient to run "fedora-review -b 1020839" and let that tool perform some
> packaging checks.

You can try it now. It's working on my machine.

> > Summary: GUI tool for adding karma to Bodhi system.
> > Similar to fedora-easy-karma
> 
> What I find surprising here is that the %summary uses two terms not used in
> the %description: karma, Bodhi
> 
> I would drop the insider talk and write:
> 
>   Summary: GUI for sending feedback about installed Test Update packages
> 
> The %description could expand on that and mention the "Fedora Updates
> System" (bodhi), explain that users/testers can vote about test-updates
> using "karma" points. If you consider it relevant, you could mention the CLI
> tool fedora-easy-karma in the description, but I think they are different
> enough. And in a related search, fedora-easy-karma ought to turn up as well.

I've edited summary a little. Description is grabbed from fedora-easy-karma. 

> > desktop-file-install
> > desktop-file-validate
> 
> Using either one is sufficient. desktop-file-install also validates the
> specified file.
> 
> 
> > desktop-file-install \
> > --dir=%{buildroot}%{_datadir}/applications \
> > %{buildroot}/%{_datadir}/applications/fedora-gooey-karma.desktop
> 
> What this does is it reinstalls (!) and validates the existing desktop file
> found at the path inside the %buildroot. But if the desktop file is located
> in the %buildroot already, you only need to validate it. Hence I suggest you
> only run desktop-file-validate and not desktop-file-install. Using the
> latter makes more sense if you need to install the file actually, or if you
> want to use the tool's options to modify the .desktop file on-the-fly.

I've removed desktop-file-install as it's installed by Makefile.

> Files that are not supposed to be executed should not be mode +x:
> 
> -rwxr-xr-x  /usr/share/fedora-gooey-karma/bodhiworker.py
> -rwxr-xr-x  /usr/share/fedora-gooey-karma/browser.py
> -rwxr-xr-x  /usr/share/fedora-gooey-karma/config.py
> -rwxr-xr-x  /usr/share/fedora-gooey-karma/customgui.py
> -rwxr-xr-x  /usr/share/fedora-gooey-karma/idlequeue.py
> -rwxr-xr-x  /usr/share/fedora-gooey-karma/idlequeuedispatcher.py
> -rwxr-xr-x  /usr/share/fedora-gooey-karma/packagesworker.py
> -rwxr-xr-x  /usr/share/fedora-gooey-karma/sendkarma.py

Fixed. Thank you for bringing this up.

Icons should be installed as well. I've verified by installing in Gnome.

Taking.

Can you please post link to your latest SPEC and SRC.RPM? Oh, I see you are still updating the same files. Please do not do that. Every change - even if it is just your own and it does not reach Fedora - every change which your copy to web, please bump up release (and make entry in changelog).

Or not? Because for example the last issue from #17 (file permissions) is still not fixed.

Please include python2-devel
https://fedoraproject.org/wiki/Packaging:Python#BuildRequires
Or python3-devel -- which would be even nice, because Fedora now tries to move to python3 as default.

rm -rf %{buildroot}
is not needed unless you want to build for EPEL.

Please use full commit URL for SOURCE0. See:
http://fedoraproject.org/wiki/Packaging:SourceURL#Github

Spec URL: http://blaskovic.fedorapeople.org/fedora-gooey-karma/fedora-gooey-karma.spec
SRPM URL: http://blaskovic.fedorapeople.org/fedora-gooey-karma/fedora-gooey-karma-0.1-2.fc20.src.rpm

(In reply to Miroslav Suchý from comment #19)
> Can you please post link to your latest SPEC and SRC.RPM? Oh, I see you are
> still updating the same files. Please do not do that. Every change - even if
> it is just your own and it does not reach Fedora - every change which your
> copy to web, please bump up release (and make entry in changelog).

Ok, attaching latest SRPM, bumping release and adding note to changelog.

> Or not? Because for example the last issue from #17 (file permissions) is
> still not fixed.

I can see this fixed. If you are talking about /usr/share/fedora-gooey-karma/* files. Please check it in the newest package.

> Please include python2-devel
> https://fedoraproject.org/wiki/Packaging:Python#BuildRequires
> Or python3-devel -- which would be even nice, because Fedora now tries to
> move to python3 as default.

python2-devel included. Thank you

> rm -rf %{buildroot}
> is not needed unless you want to build for EPEL.

Removed.

> Please use full commit URL for SOURCE0. See:
> http://fedoraproject.org/wiki/Packaging:SourceURL#Github

Changed. Thank you for this Github trick. I missed it while reading wikis.

Builds from copr: http://copr.fedoraproject.org/coprs/blaskovic/fedora-gooey-karma/builds/

Miro, any progress on this?

Thank you

Issues:
- you should require package hicolor-icon-theme
- you should provide man page, I strongly encourage you to use asciidoc as
it provide very low entry barrier.
See this examples:
https://git.fedorahosted.org/cgit/copr.git/tree/man/copr-cli.1.asciidoc
you will then needs:
BuildRequires: asciidoc
BuildRequires: libxslt
and in %build:
a2x -d manpage -f manpage man/copr-cli.1.asciidoc
in %install:
install -d %{buildroot}%{_mandir}/man1
install -p -m 644 man/copr-cli.1 %{buildroot}/%{_mandir}/man1/
and in %files:
%{_mandir}/man1/copr-cli.1*
- you have non-executable-script /usr/share/fedora-gooey-karma/*
Either they are meant to be executed as standalone scripts - then add them executable bit.
Or the are not meant to be executed separately, then remove shebang at the top of files.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v3 or later)", "Unknown or generated". 1 files have unknown
     license.
[!]: Package must own all directories that it creates.
     Note: Directories without known owners:
     /usr/share/icons/hicolor/256x256/apps, /usr/share/icons/hicolor/24x24,
     /usr/share/icons/hicolor/32x32/apps, /usr/share/icons/hicolor/24x24/apps,
     /usr/share/icons/hicolor/16x16/apps, /usr/share/icons/hicolor/16x16,
     /usr/share/icons/hicolor/48x48, /usr/share/icons/hicolor/48x48/apps,
     /usr/share/icons/hicolor/256x256, /usr/share/icons/hicolor,
     /usr/share/icons/hicolor/32x32
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: gtk-update-icon-cache is invoked in %postun and %posttrans if package
     contains icons.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 40960 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or desktop-
     file-validate if there is such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build process.
[-]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[-]: Latest version is packaged.
[-]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: fedora-gooey-karma-0.1-2.fc20.noarch.rpm
          fedora-gooey-karma-0.1-2.fc20.src.rpm
fedora-gooey-karma.noarch: E: non-executable-script /usr/share/fedora-gooey-karma/config.py 0644L /usr/bin/python2
fedora-gooey-karma.noarch: E: non-executable-script /usr/share/fedora-gooey-karma/toolbox.py 0644L /usr/bin/python2
fedora-gooey-karma.noarch: E: non-executable-script /usr/share/fedora-gooey-karma/customgui.py 0644L /usr/bin/python2
fedora-gooey-karma.noarch: E: non-executable-script /usr/share/fedora-gooey-karma/sendkarma.py 0644L /usr/bin/python2
fedora-gooey-karma.noarch: E: non-executable-script /usr/share/fedora-gooey-karma/bodhiworker.py 0644L /usr/bin/python2
fedora-gooey-karma.noarch: E: non-executable-script /usr/share/fedora-gooey-karma/browser.py 0644L /usr/bin/python2
fedora-gooey-karma.noarch: E: non-executable-script /usr/share/fedora-gooey-karma/packagesworker.py 0644L /usr/bin/python2
fedora-gooey-karma.noarch: E: non-executable-script /usr/share/fedora-gooey-karma/idlequeue.py 0644L /usr/bin/python2
fedora-gooey-karma.noarch: E: non-executable-script /usr/share/fedora-gooey-karma/idlequeuedispatcher.py 0644L /usr/bin/python2
fedora-gooey-karma.noarch: W: no-manual-page-for-binary fedora-gooey-karma
2 packages and 0 specfiles checked; 9 errors, 1 warnings.

Spec URL: http://blaskovic.fedorapeople.org/fedora-gooey-karma/fedora-gooey-karma.spec
SRPM URL: http://blaskovic.fedorapeople.org/fedora-gooey-karma/fedora-gooey-karma-0.1-3.fc20.src.rpm

Copr: https://copr.fedoraproject.org/coprs/blaskovic/fedora-gooey-karma/builds/

Changes:
--------
* Fri Mar 14 2014 Branislav Blaskovic <branislav> - 0.1-3
- Fixed UnicodeEncodeError issue #28
- Require for hicolor-icon-theme added
- Man page added

$ rpmlint ~/rpmbuild/SRPMS/fedora-gooey-karma-0.1-3.fc20.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint ~/rpmbuild/RPMS/noarch/fedora-gooey-karma-0.1-3.fc20.noarch.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

I am not sure how to fix this:

[!]: Package must own all directories that it creates.
     Note: Directories without known owners:
     /usr/share/icons/hicolor/256x256/apps, /usr/share/icons/hicolor/24x24,
     /usr/share/icons/hicolor/32x32/apps, /usr/share/icons/hicolor/24x24/apps,
     /usr/share/icons/hicolor/16x16/apps, /usr/share/icons/hicolor/16x16,
     /usr/share/icons/hicolor/48x48, /usr/share/icons/hicolor/48x48/apps,
     /usr/share/icons/hicolor/256x256, /usr/share/icons/hicolor,
     /usr/share/icons/hicolor/32x32

I've added %{_datadir}/icons/hicolor/*/apps/ to %files. Is it correct solution?

fedora-review gives me:

[ ]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by: 
     /usr/share/icons/hicolor/16x16/apps(mono-tools, hicolor-icon-theme,
     fedora-logos, anaconda), /usr/share/icons/hicolor/256x256/apps(mono-
     tools, hicolor-icon-theme, fedora-logos, anaconda),
     /usr/share/icons/hicolor/24x24/apps(mono-tools, hicolor-icon-theme,
     fedora-logos, anaconda), /usr/share/icons/hicolor/32x32/apps(mono-tools,
     hicolor-icon-theme, fedora-logos, anaconda),
     /usr/share/icons/hicolor/48x48/apps(mono-tools, hicolor-icon-theme,
     fedora-logos, anaconda)

> I've added %{_datadir}/icons/hicolor/*/apps/ to %files. Is it correct solution?

No. Remove 
  %{_datadir}/icons/hicolor/*/apps/
from %files. But leave the second line there. Requiring hicolor-icon-theme is enough. If fedora-review still report it, then it is false negative.
You can do this one small change during import into dist-git.

APPROVED

And I just sponsored you in packager group. Welcome to Fedora! :)

New Package SCM Request
=======================
Package Name: fedora-gooey-karma
Short Description: GUI tool for sending feedback about installed Test Update packages
Owners: blaskovic
Branches: f19 f20
InitialCC:

Git done (by process-git-requests).

fedora-gooey-karma-0.1-4.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/fedora-gooey-karma-0.1-4.fc19

fedora-gooey-karma-0.1-4.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/fedora-gooey-karma-0.1-4.fc20

fedora-gooey-karma-0.1-4.fc20 has been pushed to the Fedora 20 testing repository.

fedora-gooey-karma-0.1-4.fc20 has been pushed to the Fedora 20 stable repository.

fedora-gooey-karma-0.1-4.fc19 has been pushed to the Fedora 19 stable repository.

We should probably write up gooey-karma in the appropriate places on the wiki, just like easy-karma...