Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 672440 - Review Request: flann - Fast Library for Approximate Nearest Neighbors
Summary: Review Request: flann - Fast Library for Approximate Nearest Neighbors
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tim Niemueller
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 674008
TreeView+ depends on / blocked
 
Reported: 2011-01-25 04:49 UTC by Rich Mattes
Modified: 2011-02-23 01:31 UTC (History)
4 users (show)

Fixed In Version: flann-1.6.7-4.el5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-02-22 04:53:06 UTC
Type: ---
Embargoed:
tim: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Rich Mattes 2011-01-25 04:49:25 UTC
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.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 15:56:36 UTC
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 16:08:25 UTC
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 23:54:40 UTC
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 21:53:30 UTC
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 22:17:26 UTC
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 14:44:08 UTC
Looks good now.

APPROVED.

Comment 7 Rich Mattes 2011-02-03 16:21:12 UTC
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 16:28:43 UTC
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 16:52:18 UTC
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 16:52:47 UTC
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 21:17:51 UTC
Git done (by process-git-requests).

Comment 12 Fedora Update System 2011-02-04 05:26:03 UTC
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 05:26:10 UTC
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 05:26:16 UTC
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 05:26:23 UTC
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 18:27:32 UTC
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-22 04:53:01 UTC
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-22 04:54:34 UTC
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 10:58:08 UTC
/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-23 01:31:25 UTC
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-23 01:31:43 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.