Hi Jakub, Thanks for taking the time to review my work. On Thu, Dec 29, 2011 at 11:22:08AM +0100, Jakub Wilk wrote: > * Simon Chopin <chopin.simon@gmail.com>, 2011-12-26, 15:19: > > http://mentors.debian.net/package/chromaprint > > > >Alternatively, one can download the package with dget using this command: > > > > dget -x http://mentors.debian.net/debian/pool/main/c/chromaprint/chromaprint_0.6-1.dsc > > Here's my review: > > + - The python bindings have been moved to the pyacoustid package, > + available in Debian as python-acoustid. > > s/python/Python/. I'd explicitly write here that python-chromaprint > is not being built from this source package anymore. Done. > > + * Add fpcalc binary to libchromaprint0 (Closes: #649292) > > That'd be violation of Policy 8.2 if you actually did that. But in > fact, the binary was added to a new binary package > libchromaprint-bin. (BTW, the Policy suggests <libraryname>-tools > for such package name.) Yep, Stefano Rivera already pointed me to this policy violation and suggested to create the -bin package. I find the -tools name more explicit, I've switched to it (and updated the changelog) > > + * Enable multiarch > > Please be specific about what changes were required to enable > multi-arch. Done. > > +libchromaprint.so.0 libchromaprint0 #MINVER# [c++ symbols] > + > It's slightly worrisome that these symbols are exported in the first > place. OTOH I don't know if there's a simple and upstreamable way to > hide them. (Upstream is using cmake.) Yep, I've looked around to find a solution to strip all the template cruft, but so far the only solution I've found would be to do a `strip -x libchromaprint.so.0.1.4`. If I understand correctly, this approach would not only risk breaking the ABI, but could also be problematic for the exceptions. I'm not sure the latter is relevant given the symbols posing problem, but the former worries me. > But anyway, on i386 I get this lintian error: > E: libchromaprint0: symbols-file-contains-current-version-with-debian-revision on symbol _ZNSt6vectorIdSaIdEE14_M_fill_insertEN9__gnu_cxx17__normal_iteratorIPdS1_EEjRKd@Base and 2 others I added a specific symbol file for i386, but I fear the problem will also occur with other architectures. > + chromaprint_dealloc@Base 0.6 > + chromaprint_decode_fingerprint@Base 0.6 > + chromaprint_encode_fingerprint@Base 0.6 > + chromaprint_feed@Base 0.6 > + chromaprint_finish@Base 0.6 > + chromaprint_free@Base 0.6 > + chromaprint_get_fingerprint@Base 0.6 > + chromaprint_get_raw_fingerprint@Base 0.6 > + chromaprint_get_version@Base 0.6 > + chromaprint_new@Base 0.6 > + chromaprint_start@Base 0.6 > > Why 0.6? As far as I can tell, there were no interface changes in > this release, to this should be 0.5 or even lower. Correct me if I'm wrong, but since the previous versions didn't have a symbols file, the shlib system cannot pick a older version, right? Anyway, I've checked, the last time the interface changed was for the 0.2. I highly doubt it is worth it to dig deeper than that, I've set the minimal version to 0.2. > [pieces of the patch] > > You changed tabs to spaces (here, and in another place). I dropped the patch. It is all documented in the git history, or at https://github.com/lalinsky/chromaprint/pull/19 > > At least the following changes are not documented in the changelog: > - Addition of python-docutils to B-D; addition of manual page. (BTW, > rst2man was added in docutils 0.6; please consider using versioned > B-D.) > - Changes to Vcs-* fields. > - Changes to debian/copyright. > - Disabling DH_VERBOSE. All done. Thanks again for the review. Cheers, Simon
Attachment:
signature.asc
Description: Digital signature