Bug 672440

Summary: Review Request: flann - Fast Library for Approximate Nearest Neighbors
Product: [Fedora] Fedora Reporter: Rich Mattes <richmattes>
Component: Package ReviewAssignee: Tim Niemueller <tim>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, kryzhev, notting, tim
Target Milestone: ---Flags: tim: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: flann-1.6.7-4.el5 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-02-21 23:53:06 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 674008    

Description Rich Mattes 2011-01-24 23:49:25 EST
Spec URL: http://rmattes.fedorapeople.org/RPMS/flann/flann.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/flann/flann-1.6.7-1.fc14.src.rpm
Description: 
FLANN is a library for performing fast approximate nearest neighbor searches 
in high dimensional spaces. It contains a collection of algorithms found 
to work best for nearest neighbor search and a system for automatically 
choosing the best algorithm and optimum parameters depending on the data sets.

rpmlint:
$ rpmlint flann.spec ../RPMS/x86_64/flann*
flann.x86_64: W: shared-lib-calls-exit /usr/lib64/libflann.so.1.6.7 exit@GLIBC_2.2.5
flann-devel.x86_64: W: no-documentation
flann-python.x86_64: W: no-documentation
flann-static.x86_64: W: no-documentation
5 packages and 1 specfiles checked; 0 errors, 4 warnings.

The shared-lib-calls-exit issue is something to take up with upstream. The package is pretty sparse on documentation, but the base package includes a pdf manual and all the subpackages require the base package in one way or another.

scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2740732
Comment 1 Tim Niemueller 2011-01-31 10:56:36 EST
Ok, I'm taking this review. Before even looking, can you patch out the call to exit meaningful before upstream reacts?
Comment 2 Rich Mattes 2011-01-31 11:08:25 EST
I emailed the upstream developer about the LICENSE text and the exit(0) problems.  He replied very quickly, and said that he has removed the call to exit(0) in SVN for the next release.  It doesn't look like he's using a public SVN, but I should be able to dig in and fix things later tonight.  He also said he would include a BSD LICENSE text in his next release, in case you stumble upon the fact that it's missing while you're reviewing.
Comment 3 Rich Mattes 2011-01-31 18:54:40 EST
Alright, exit(1) has been fixed.  Flann includes its own implementation of malloc(), and it was calling exit(1) when it couldn't allocate enough memory.  I patched it to return NULL instead.  Once the next version is released, I will be able to drop the patch.

Spec URL: http://rmattes.fedorapeople.org/RPMS/flann/flann.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/flann/flann-1.6.7-2.fc14.src.rpm

$ rpmlint flann.spec ../RPMS/x86_64/flann*flann-devel.x86_64: W: no-documentation
flann-python.x86_64: W: no-documentation
flann-static.x86_64: W: no-documentation
5 packages and 1 specfiles checked; 0 errors, 3 warnings.

scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2753897
Comment 4 Tim Niemueller 2011-02-02 16:53:30 EST
REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

(+) rpmlint is not silent, some messages can be ignored:
  - No documentation for sub-packages ok
+ The package is named according to the Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format
%{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the
Licensing Guidelines.
+ The License field in the package spec file matches the actual license (BSD).
(-) The file, containing the text of the license(s) for the package, is included
in %doc.
  - issue is known and packager has contacted upstream about a fix. A license file will be included in the next release, therefore acceptable. Source code files carry the license tag, so license can be verified.

+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided
in the spec URL.

# sha256sum ../SOURCES/flann-1.6.7-src.zip 
c96feb000e7ce430bec4a03fb53ce0fb82c0bda9c475d93691916101a0c6c137  ../SOURCES/flann-1.6.7-src.zip
# sha256sum ~/Downloads/flann-1.6.7-src.zip 
c96feb000e7ce430bec4a03fb53ce0fb82c0bda9c475d93691916101a0c6c137  /home/tim/Downloads/flann-1.6.7-src.zip

+ The package successfully compiles and builds into binary rpms on at least one
primary architecture.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
+ Main package calls ldconfig in %post/%postun
+ The package does NOT bundle copies of system libraries.
+ The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files
listings.
+ Permissions on files are set properly.
- The package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
(-) The package consistently uses macros.
  - The package uses $RPM_BUILD_ROOT as variable, and everything else as macro. Since this is what rpmdev-newspec provides by default, I figure this is acceptable. Consider changing for consistency, but if you don't it is not a blocker (I'm using that format in many packages myself).
+ The package contains code, or permissible content.
+ No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the
application.
+ Header files are in -devel package
+ Static libraries are in -static package
+ pkg-config files are in -devel package
+ .so (no suffix) are in -devel package
+ -devel package requires base package, -static package requires -devel package
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other
packages.
- At the beginning of %install, the package runs rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
+ All filenames in rpm packages are valid UTF-8.

Package looks good, things that MUST be changed:
- Add %clean section with rm -rf $RPM_BUILD_ROOT
- Add rm -rf $RPM_BUILD_ROOT at beginning of %install section
- Add license file in a future release (current state acceptable transiently and does not stop the package from being accepted)
Comment 5 Rich Mattes 2011-02-02 17:17:26 EST
Alright added the %clean section, and the rm -rf %{buildroot} at the beginning of install.  I also switched all of the $RPM_BUILD_ROOT variables to %{buildroot}; I think it makes the specfile look a lot cleaner.

The licensing guidelines mention that the license text is required if (and only if) the text ships with the source, but as you note, the issue has been resolved with upstream and should be a non-issue for 1.6.8

Spec URL: http://rmattes.fedorapeople.org/RPMS/flann/flann.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/flann/flann-1.6.7-3.fc14.src.rpm

$ rpmlint ../RPMS/x86_64/flann* flann.spec
flann-devel.x86_64: W: no-documentation
flann-python.x86_64: W: no-documentation
flann-static.x86_64: W: no-documentation
Comment 6 Tim Niemueller 2011-02-03 09:44:08 EST
Looks good now.

APPROVED.
Comment 7 Rich Mattes 2011-02-03 11:21:12 EST
New Package SCM Request
=======================
Package Name: flann
Short Description: Fast Library for Approximate Nearest Neighbors
Owners: rmattes
Branches: f13 f14
InitialCC:
Comment 8 Tim Niemueller 2011-02-03 11:28:43 EST
Would you mind maintaining it in el5/el6? Maybe we can have OpenRAVE there, then. Some teams use CentOS or the like as a long-term stable robot platform.
Comment 9 Rich Mattes 2011-02-03 11:52:18 EST
Sure, I can maintain them there too.  I just ran some mockbuilds, hdf5 and gtest aren't available in EPEL.  el6 is ok without them, but I have to track down a build error for el5.
Comment 10 Rich Mattes 2011-02-03 11:52:47 EST
New Package SCM Request
=======================
Package Name: flann
Short Description: Fast Library for Approximate Nearest Neighbors
Owners: rmattes
Branches: f13 f14 el5 el6
InitialCC:
Comment 11 Kevin Fenzi 2011-02-03 16:17:51 EST
Git done (by process-git-requests).
Comment 12 Fedora Update System 2011-02-04 00:26:03 EST
flann-1.6.7-4.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/flann-1.6.7-4.el5
Comment 13 Fedora Update System 2011-02-04 00:26:10 EST
flann-1.6.7-4.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/flann-1.6.7-4.fc14
Comment 14 Fedora Update System 2011-02-04 00:26:16 EST
flann-1.6.7-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/flann-1.6.7-4.el6
Comment 15 Fedora Update System 2011-02-04 00:26:23 EST
flann-1.6.7-4.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/flann-1.6.7-4.fc13
Comment 16 Fedora Update System 2011-02-04 13:27:32 EST
flann-1.6.7-4.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update flann'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/flann-1.6.7-4.el5
Comment 17 Fedora Update System 2011-02-21 23:53:01 EST
flann-1.6.7-4.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 18 Fedora Update System 2011-02-21 23:54:34 EST
flann-1.6.7-4.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 19 Dmitrij S. Kryzhevich 2011-02-22 05:58:08 EST
/usr/include/flann/flann_mpi.hpp both require some mpi implementation and boost.
1) Second could (but not need, see 2) be fixed by adding Requires: boost.
2) There is no mpi support in current fedora flann at all.

May be better to not provide flann_mpi.hpp?
Comment 20 Fedora Update System 2011-02-22 20:31:25 EST
flann-1.6.7-4.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 21 Fedora Update System 2011-02-22 20:31:43 EST
flann-1.6.7-4.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.