[Bug 2037] Review request: acoustid-fingerprinter - Music AcoustID fingerprinting application

[RPM Fusion Bugzilla]

https://bugzilla.rpmfusion.org/show_bug.cgi?id=2037

--- Comment #10 from Andrea Musuruane <musuruan at gmail.com> 2012-01-04 15:41:57 CET ---
(In reply to comment #7)
> > 2. Buildroot, rm -rf %{buildroot} in %install, are not needed unless you want
> > to build for EL5:
> > http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
> 
> done

The Buildroot is still present:
BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

> > Update your %files section with:
> > %{_datadir}/icons/hicolor/*/apps/%{name}.png
> 
> I've created and svg file and opted using it instead bitmaps

This is fine for me - altough Fedora Guidelines suggest to stay more close to
upstream. Please consider submitting the SVG icon upstream.

> > 5. You must have a 'Requires: hicolor-icon-theme', which owns the
> > %{_datadir}/icons/hicolor directory.
> 
> Added, but I don't understand why it could need the application owning it, I
> mean, it can cause any technical trouble I can remember now?

Packages must own all directories they put files in, except for:
* any directories owned by the filesystem, man, or other explicitly created
-filesystem packages
* any directories owned by other packages in your package's natural dependency
chain 
https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

The %{_datadir}/icons/hicolor directory is owned by hicolor-icon-theme so you
must require it.

> > 12. Anyway -DCMAKE_INSTALL_PREFIX=/usr/ is useless. It's already the default,
> > just strip it.
> 
> striping it doesn't work, so I left the _prefix as suggested

When I tried yesterday I had no problem. What Fedora version/arch are you
running? 

Moreover (and I missed this yesterday) cmake should be called in %build not in
%prep.

> > 13. The sources contain two upstream ffmpeg header files in the /ffmpeg
> > directory. Please patch the source to use system header files.
> 
> both files are strongly modified from their upstream so I kept them.

Header files commonly contain forward declarations of classes, subroutines,
variables, and other identifiers. These are contained in the library you are
linking to. If you use a strongly modified version (different from upstream)
compatibility problems could arise.

> > 14. The package doesn't work properly on F-16+ (with qt 4.8). You cannot submit
> > fingerprints. An upstream patch is here:
> > https://github.com/lalinsky/acoustid-fingerprinter/commit/7577c13c02bcbf4f82f6dff0ab083f2933c7e06d.diff
> 
> I missed that. Added.

Patch name is not compliant. It should start with the package name followed by
a comment. 
E.g.: %{name}-fix-contenttypeheader.patch or
%{name}-0.5-fix-contenttypeheader.patch.

Moreover you should link to an upstream bug:
https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

The upstream bug is the following:
https://github.com/lalinsky/acoustid-fingerprinter/issues/3

> > 15. %{optflags} are partly overwritten. On x86_64 I can see:
> > "-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
> > --param=ssp-buffer-size=4  -m64 -mtune=generic  -O3"
> > 
> > Please notice the -O3 at the end that overwrites the -O2 at the beginning.
> 
> Yep, but after examining CMakeLists.txt seems to be in some way intentional and
> as I can't imagine any collateral efect I left it pristine.

Overriding these flags for performance optimizations (for instance, -O3 instead
of -O2) is generally discouraged:
https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

Therefore, please patch the sources to get rid of "-O3".

16. This is not required (only build-required):
Requires:       desktop-file-utils

Why did you add it?

17. You edited the changelog entry for 0.5-1 adding "- svg desktop icon". This
is not true (there was no svg icon then) and you should not change previous
entries.

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