(In reply to comment #0)

> Why this package is not in Fedora:
> Because it links with ffmpeg

Is the ffmpeg a hard requirement?   I thought fftw (in Fedora) could do some of the audio decoding?

(In reply to comment #1)
> 
> Is the ffmpeg a hard requirement?   I thought fftw (in Fedora) could do some of
> the audio decoding?

For sure.

Libchromaprint can build against fftw3 as you know, but fpcalc binds with the ffmpeg API. No option here.

The acoustid-fingerprint tool fall in this same case.

I assume is obviously possible to code a new fpcalc-like tool using FFTW but I'm not aware of anyone, neither Luks, the author, had told me anything on that.

FFTW only computes fast Fourier transformations (which is one of the analysis steps done during fingerprinting). It does not do any decoding. In order to be eligible for Fedora, the actual decoding code would have to be ported to a decoder library in Fedora (or eligible for Fedora), such as libsndfile or GStreamer. (FFmpeg is NOT suitable for Fedora because it includes all the codecs, patent-free and patent-encumbered alike, in one single library. Also be warned that libsndfile doesn't support patent-encumbered formats at all. GStreamer with its plugin-based interface is definitely most suitable. Unfortunately, many upstream projects don't even consider it as a solution for decoding, which is why we're stuck with so many things with hard FFmpeg dependencies.)

Is it settled? This belongs in RPM Fusion?

If so I'll review it.

(In reply to comment #4)
> Is it settled? This belongs in RPM Fusion?

Yep, seems ffmpeg is hard requirement, because decoding is required before the Fourier analysis of the audio data.

> If so I'll review it.

Great!

Ok, quick spec review first.

1. You don't need the "-n chromaprint-tools" since it's the name of the package. You'll still need -n on %setup though since directory in the archive doesn't include "tools" in the name". 

Speaking of which, I noticed you're deleting a lot of stuff in the spec. Is there a reason for this?

2. Don't use macro versions of basic shell tools, in this case "rm". It's not necessary and just hurts readability.

3. Unless you're going to build for EL5 (unlikely) you can get rid of:
BuildRoot:
%{__rm} -rf %{buildroot} in %install
%clean entirely
%defattr(-,root,root,-) in %files

Does this Require or BuildRequire chromaprint from Fedora?  If so, it should probably be added to the .spec.

Since this is in review, changing to ASSIGNED.

hmm... I hate to say it, but if this is using the same source as the package in Fedora then it should probably be treated like other projects like this. You'll need to package it just like Fedora but use rpmdiff to generate a rpm diff file that people can install. I've never done it before so I'm a little short on details. 

It's possible that's overkill since you're just adding a binary, but at a minimum you'll need to keep to the same version as the Fedora release:

Requires: chromaprint == %{version}

or something like that.

> hmm... I hate to say it, but if this is using the same source as the package in
> Fedora then it should probably be treated like other projects like this. You'll
> need to package it just like Fedora but use rpmdiff to generate a rpm diff file
> that people can install. I've never done it before so I'm a little short on
> details. 

Huh? No package in RPM Fusion is packaged as delta/patch RPMs, our toolset doesn't even support them. What we do is that we package a -freeworld RPM in RPM Fusion which contains only the files not in the Fedora RPM (by not building them if possible, or deleting them in %install otherwise), or if that's not possible, which overrides the files from the Fedora RPM without conflicting with it.

This chromaprint-tools package is already following the recommended approach.

> It's possible that's overkill since you're just adding a binary, but at a
> minimum you'll need to keep to the same version as the Fedora release:
>
> Requires: chromaprint == %{version}
>
> or something like that.

Uhm, no, please only add '='-versioned dependencies on Fedora packages if absolutely necessary (i.e. if the tools version x are not expected to work with the library version y≠x)! '='-versioned dependencies mean the packages in Fedora and RPM Fusion need to be pushed at exactly the same time, which in practice can only be approximated, so there's always a window where the user will not be able to update. Thus, please do not gratuitously add them when not needed.

My apologies! 

Thanks for straightening me out as usual Kevin. I see now this isn't the same situation as a "freeworld" package.

One question though. As far as the version requirement. If it's not an exact match, how do we know the binary here will be compatible with the library on Fedora? Or do we just not worry about it unless it breaks?

While a simultaneous release isn't possible, doesn't the PackageKit GUI updated run with the equivalent of --skip-broken? So the base package wouldn't update until there was a matching -tools package?

Richard

We can normally assume that a library with the same soname is compatible, at least one way (>= dependency), sometimes both ways (in which case no explicit versioned dependency is necessary at all).

And yes, PackageKit does a --skip-broken, but it's still an annoyance (e.g. you have the update prompt nagging you all the time, and actually updating won't do anything).

(In reply to comment #7)
> Does this Require or BuildRequire chromaprint from Fedora?  If so, it should
> probably be added to the .spec.

Well, it IS chromaprint, except we're deleting all the library parts. From what Kevin said I think don't worry about it and let it find chromaprint through a soname dependency?

Also, I was thinking it would be easier to cherry-pick the binary and install it manually than to go through a normal "mark install" and delete all the stuff we don't want. Same end result though.

Richard

(In reply to comment #13)
> (In reply to comment #7)
> > Does this Require or BuildRequire chromaprint from Fedora?  If so, it should
> > probably be added to the .spec.
> 
> Well, it IS chromaprint, except we're deleting all the library parts. From what
> Kevin said I think don't worry about it and let it find chromaprint through a
> soname dependency?

Yes, if fpcalc dynamically links to libraries in the main Fedora chromaprint package, then no need to include the Requires, as auto requires will do it for us.  This is a similar situation as the picard-freeworld package that builds from the same tarball as the Fedora picard package that I both maintain.  picard-freeworld adds a single .so to main picard Fedora package, except that in that case, the .so file (it's essentially a plugin) doesn't link to anything in the main Fedora package, so we need a manual "Requires" on picard, and I make the picard-freeworld Require == %{version} because in this case, there are possible API issues if it isn't installed with at least the same major version of picard.   (This also means that the release tag doesn't necessarily need to match, meaning that packaging fixes for either package can be pushed independently).   I am also very careful to co-ordinate the pushes the Fedora/RPM Fusion packages at the same time (I file a bug here to prevent pushes to stable of picard-freeworld until corresponding Fedora picard has been pushed) and this has worked pretty well, generally there's at most a few hours when the deps might be broken. 

> Also, I was thinking it would be easier to cherry-pick the binary and install
> it manually than to go through a normal "mark install" and delete all the stuff we don't want. Same end result though.

Either works for me.    We do the latter in picard-freeworld:

http://cvs.rpmfusion.org/viewvc/rpms/picard-freeworld/devel/picard-freeworld.spec?revision=1.14&root=free&view=markup

Also probably don't need the subpackage nomenclature "-n chromaprint-tools" for %description and %files because the package is already named "chromaprint-tools,

Update:

Spec URL: http://olea.org/tmp/chromaprint-tools-rpms/chromaprint-tools.spec
SRPM URL:
http://olea.org/tmp/chromaprint-tools-rpms/chromaprint-tools-0.6-1.fc16.src.rpm

- fixed rpm-ism's
- updated to 0.6

about fpcalc, it links statically with chromaprint so the requires are innecesary

New paths

http://olea.org/tmp/chromaprint-tools.spec
http://olea.org/paquetes-rpm/fedora-16/chromaprint-tools-0.6-1.fc16.i686.rpm

Looks like the last SRPM link is bad. I'll try to get a full review done as soon as the link is fixed.

Richard

(In reply to comment #18)
> Looks like the last SRPM link is bad. I'll try to get a full review done as
> soon as the link is fixed.
> 
> Richard

Ups.


http://olea.org/tmp/chromaprint-tools.spec
http://olea.org/paquetes-rpm/fedora-16/chromaprint-tools-0.6-1.fc16.src.rpm

http://olea.org/tmp/chromaprint-tools.spec
http://olea.org/paquetes-rpm/fedora-16/chromaprint-tools-0.6-2.fc16.src.rpm

(In reply to comment #18)
> Looks like the last SRPM link is bad. I'll try to get a full review done as
> soon as the link is fixed.

@Richard, are you still interested?

Yes, sorry. Between home and work life and the F17 mass rebuild for gcc 4.7 I've been a little busy. Checking it out now.

Ok, just a couple of things:

1. Were you not using the "%cmake" macro for a reason? Seemed to work for me and put the libraries in the right place (/usr/lib64) which is where they need to be in order to be removed later in %{_libdir}. I'm assuming you build a 32bit package? Otherwise it would have failed for you.

I changed your cmake line to:
%cmake -DBUILD_EXAMPLES=on -DBUILD_TESTS=off -DCMAKE_BUILD_TYPE=RelWithDebInfo

The cmake build type is optional but I've gotten to where I like to set it on all my cmake packages.

It doesn't look like much but the %cmake macro does a lot for you. Too see it just do this:

$ rpm -E %cmake

  CFLAGS="${CFLAGS:--O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic}" ; export CFLAGS ; 
  CXXFLAGS="${CXXFLAGS:--O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic}" ; export CXXFLAGS ; 
  FFLAGS="${FFLAGS:--O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic -I/usr/lib64/gfortran/modules}" ; export FFLAGS ; 
  LDFLAGS="${LDFLAGS:--Wl,-z,relro }" ; export LDFLAGS ; 
  /usr/bin/cmake \
        -DCMAKE_VERBOSE_MAKEFILE=ON \
        -DCMAKE_INSTALL_PREFIX:PATH=/usr \
        -DCMAKE_INSTALL_LIBDIR:PATH=/usr/lib64 \
        -DINCLUDE_INSTALL_DIR:PATH=/usr/include \
        -DLIB_INSTALL_DIR:PATH=/usr/lib64 \
        -DSYSCONF_INSTALL_DIR:PATH=/etc \
        -DSHARE_INSTALL_PREFIX:PATH=/usr/share \
%if "lib64" == "lib64" 
        -DLIB_SUFFIX=64 \
%endif 
        -DBUILD_SHARED_LIBS:BOOL=ON

You can see it sets the cflags and install prefix among other things.

2. rpmlint output:
$ rpmlint *.rpm
chromaprint-tools.src: W: spelling-error %description -l en_US fpcalc -> calcify
chromaprint-tools.x86_64: W: no-manual-page-for-binary fpcalc
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

looks good!

I'll do the full review as soon as I can.

Ok, one more thing. I just caught that your cmake command was in %prep, it belongs in %build.

Otherwise the package looks good. Here's my review:
+: OK
-: must be fixed
=: should be fixed (at your discretion)
?: Question or clairification needed
N: not applicable

MUST:
[+] rpmlint output: shown in comment.
[+] follows package naming guidelines
[+] spec file base name matches package name
[+] package meets the packaging guidelines
[+] package uses a Fedora approved license: LGPLv2+
[+] license field matches the actual license.
[+] license file is included in %doc: COPYING
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5 (6b5a4f2685395e68d8abc40d1c2a8785)
[+] package builds on at least one primary arch: Tested F16 x86_64
[N] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires
[N] spec file handles locales properly
[N] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[N] no relocatable packages
[N] package owns all directories that it creates
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[N] large documentation in -doc
[N] no runtime dependencies in %doc
[N] header files in -devel
[N] static libraries in -static
[N] .so in -devel
[N] -devel requires main package
[+] package contains no libtool archives
[N] package contains a desktop file, uses desktop-file-install/validate
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[+] query upstream for license text
[N] description and summary contains available translations
[+] package builds in mock
[+] package builds on all supported arches: Tested x86_64
[?] package functions as described: Didn't test
[+] sane scriptlets
[N] subpackages require the main package
[N] placement of pkgconfig files
[N] file dependencies versus package dependencies
[N] package contains man pages for binaries/scripts

Once you post a new SRPM with the last few things fixed I can approve.

(In reply to comment #23)

> 1. Were you not using the "%cmake" macro for a reason? 

Yes, bc sometimes I do it wrong... :-/

fixed!

(In reply to comment #24)
> Ok, one more thing. I just caught that your cmake command was in %prep, it
> belongs in %build.

fixed.


http://olea.org/tmp/chromaprint-tools.spec
http://olea.org/paquetes-rpm/fedora-16/chromaprint-tools-0.6-3.fc16.src.rpm

Looks good to me!

*** APPROVED ***

Package CVS request
======================
Package Name: chromaprint-tools
Short Description: Chromaprint audio fingerprinting tools
Owners: olea
Branches: f15 f16 devel
InitialCC:
----------------------
License tag: free

Looks like CVS access has been completed. Closing.