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

[RPM Fusion Bugzilla]

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

--- Comment #7 from Ismael Olea <ismael at olea.org> 2012-01-04 13:51:45 CET ---
(In reply to comment #6)

> === Issues ===
> 1. Package doesn't consistently use macros:
> %{_bindir}/acoustid-fingerprinter

done

> 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

> 4. %{_datadir}/icons/hicolor is owned by "hicolor-icon-theme". You must not own
> it too.

done

> 
> Update your %files section with:
> %{_datadir}/icons/hicolor/*/apps/%{name}.png

I've created and svg file and opted using it instead bitmaps

> 
> 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?

> 6. %defattr in %files are not needed:
> https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
done

> 7. Why are you using an external desktop instead of the one provided upstream
> included in the tarball? They are identical.

I contributed it to upstream and forgot to remove. Done too.

> Verifying the desktop file will show the following warning: 
> key "Encoding" in group "Desktop Entry" is deprecated

done and contributed patch to upstream

> Your scriptlet is not the one provided in the wiki page. It misses the
> %posttrans section

ups! done

> 10. Icons could be installed with a for loop (really NOT mandatory at all but
> the spec file would be more readable though):

as said, substituted by svg file.

> 11. The spec file should not use an hard-coded directory name and it must not
> mix hard-coded and macro paths:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Macros
> 
> Therefore:
> -DCMAKE_INSTALL_PREFIX=/usr/ 
> 
> Should be:
> -DCMAKE_INSTALL_PREFIX=%{_prefix}  

fixed


> 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

> 
> 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.

> 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.


> 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.

Updated files:


http://olea.org/paquetes-rpm/fedora-16/acoustid-fingerprinter-0.5-2.fc16.src.rpm
http://olea.org/tmp/acoustid-fingerprinter.spec

-- 
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.