Bug 672395 - Review Request: eigen3 - A lightweight C++ template library for vector and matrix math
Review Request: eigen3 - A lightweight C++ template library for vector and ma...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tim Niemueller
Fedora Extras Quality Assurance
:
Depends On: 691133
Blocks: 694479
  Show dependency treegraph
 
Reported: 2011-01-24 21:35 EST by Rich Mattes
Modified: 2012-02-11 14:34 EST (History)
6 users (show)

See Also:
Fixed In Version: eigen3-3.0.0-2.fc15
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-07-22 15:30:47 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
tim: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Rich Mattes 2011-01-24 21:35:26 EST
Spec URL: http://rmattes.fedorapeople.org/RPMS/eigen3/eigen3.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/eigen3/eigen3-3.0-0.1.beta2.fc14.src.rpm
Description: 
A lightweight C++ template library for vector and matrix math

Since the package is a template library and doesn't include any compiled code, it only includes a noarch -devel package.  I based it off of the specfile for the eigen2 package.

rpmlint output:
$ rpmlint eigen3.spec ../RPMS/noarch/eigen3*
eigen3-devel.noarch: E: zero-length /usr/include/eigen3/Eigen/src/Sparse/SparseAssign.h
1 packages and 1 specfiles checked; 1 errors, 0 warnings.

I think this error is alright, since another header file references it and removing the file would cause missing header errors.  I could also edit the header that references the empty file and remove the reference.

Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2740673
Comment 1 Susi Lehtola 2011-01-25 05:22:36 EST
Don't build against reference BLAS. Use ATLAS instead.
Comment 2 Rich Mattes 2011-01-25 18:03:36 EST
I modified the package to use the ATLAS BLAS instead of the reference one.  It shouldn't make much of a difference, since the BLAS libraries are only used in the unit tests (all this package installs are headers).  After switching to atlas I'm also getting intermittent unit test failures, there might be some kind of race condition going on.  Anyway, here's the updated spec and srpm.

Spec URL: http://rmattes.fedorapeople.org/RPMS/eigen3/eigen3.spec
SRPM URL:
http://rmattes.fedorapeople.org/RPMS/eigen3/eigen3-3.0-0.2.beta2.fc14.src.rpm

scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2742381
Comment 3 Tim Niemueller 2011-03-26 13:04:27 EDT
Hi Rich, would you mind updating to the final release?
Comment 4 Rich Mattes 2011-03-26 20:14:23 EDT
No problem.  3.0.0 release can be found at:

Spec URL: http://rmattes.fedorapeople.org/RPMS/eigen3/eigen3.spec
SRPM URL:
http://rmattes.fedorapeople.org/RPMS/eigen3/eigen3-3.0.0-1.fc15.src.rpm

A couple of issues come with the 3.0.0 release:
1. Some of the test cases are broken.  I've disabled 'make check' for now.
2. gcc experiences an internal compiler error on f15.  I've reported it with bug 691133.  We'll probably need to wait for a resolution before this review can continue.
Comment 5 Tim Niemueller 2011-04-06 07:42:57 EDT
I'll review this as far as possible on F-14.
Comment 6 Tim Niemueller 2011-04-17 19:41:12 EDT
REVIEW:

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

- rpmlint is not silent, some messages can be ignored:
eigen3.src: E: no-description-tag
eigen3.src:49: W: macro-in-comment %{_libdir}
eigen3.src:63: W: macro-in-comment %check
eigen3.src:64: W: macro-in-comment %{_target_platform}
eigen3.src: W: patch-not-applied Patch0: eigen3-fixdso.patch
eigen3-devel.noarch: E: zero-length /usr/include/eigen3/Eigen/src/Sparse/SparseAssign.h
  - The description tag should be set even though the main package is not installable. It can just be a copy of the -devel package
  - The macro warnings can be silenced by removing the %
  - The patch should be removed if not used
  - The zero size error should be discussed with upstream. It seems it is included in the core headers, but it does not appear to be auto-generated.

+ 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 (GPLv2+ or LGPLv3+).
  - The website lists the given licenses, while the LICENSE.* files in the package mention GPLv3+ and LGPLv3+.
+ The file, containing the text of the license(s) for the package, is included
in %doc.
+ 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.

package# sha256sum ../SOURCES/3.0.0.tar.bz2 
e60efc5b18331b2e6c23ac5a8180a13b987f0aeb6fc6dca316ae338fa0513931  ../SOURCES/3.0.0.tar.bz2
downloaded # sha256sum ~/Downloads/eigen-eigen-65ee2328342f.tar.bz2 
e60efc5b18331b2e6c23ac5a8180a13b987f0aeb6fc6dca316ae338fa0513931  /home/tim/Downloads/eigen-eigen-65ee2328342f.tar.bz2

I strongly recommend renaming the source file to contain the project name!

+ The package successfully compiles and builds into binary rpms on at least one
primary architecture.
  - No problems on F-14
  - On F-15 I needed to add "#include <cstddef>" to Eigen/src/StlSupport/details.h to define ptrdiff_t.

+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files.
+ 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.
  - You use both, $RPM_BUILD_ROOT and %{...} style macros. Since rpmdev-newspec creates spec files this way by default I deem this is acceptable, but you might consider deciding for one consistent style.

+ 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 -devel subpackage
0 No static libraries.
+ pkg-config files are in -devel subpackage
  - If you intent to build the package for EPEL 5 the -devel package must depend on pkg-config
0 The package doesn't contain library files with a suffix (e.g. libfoo.so.1.1).
+ 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.

The package looks good. For this review to be approved please
- consider fixing or give reasons for not fixing the rpmlint warnings
- rename the source file to contain the package base name
- Add the required patch to get it to build on F-15, recheck #691133.
Comment 7 Rich Mattes 2011-04-24 00:27:57 EDT
Spec URL: http://rmattes.fedorapeople.org/RPMS/eigen3/eigen3.spec
SRPM URL:
http://rmattes.fedorapeople.org/RPMS/eigen3/eigen3-3.0.0-2.fc15.src.rpm

rpmlint: 
eigen3.spec: W: invalid-url Source0: eigen-3.0.0.tar.bz2
eigen3-devel.noarch: E: zero-length /usr/include/eigen3/Eigen/src/Sparse/SparseAssign.h
1 packages and 1 specfiles checked; 1 errors, 1 warnings.

The invalid source URL is because of the tarball rename.

The zero-length file was addressed in the first comment: I can remove it, but I would have to patch some other files to get rid of references to it.

I checked and the bug that was blocking this bug is no longer an issue as you pointed out.  I went ahead and closed it, so this review can continue.

Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3022117
Comment 8 Mario Ceresa 2011-07-04 08:23:08 EDT
Hello! what is the current status of this review? it seems almost finished, isn't it?

Thanks,

Mario
Comment 9 Rex Dieter 2011-07-05 12:49:28 EDT
tim is currently the active reviewer...
Comment 10 Tim Niemueller 2011-07-08 07:36:25 EDT
My blockers have been fixed, therefore:

APPROVED
Comment 11 Rich Mattes 2011-07-09 16:39:05 EDT
New Package SCM Request
=======================
Package Name: eigen3
Short Description: A lightweight C++ template library for vector and matrix math
Owners: rmattes
Branches: f14 f15
InitialCC:
Comment 12 Jon Ciesla 2011-07-09 19:14:50 EDT
Git done (by process-git-requests).
Comment 13 Fedora Update System 2011-07-09 21:02:00 EDT
eigen3-3.0.0-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/eigen3-3.0.0-2.fc14
Comment 14 Fedora Update System 2011-07-09 21:02:09 EDT
eigen3-3.0.0-2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/eigen3-3.0.0-2.fc15
Comment 15 Fedora Update System 2011-07-12 00:58:47 EDT
eigen3-3.0.0-2.fc15 has been pushed to the Fedora 15 testing repository.
Comment 16 Fedora Update System 2011-07-22 15:30:40 EDT
eigen3-3.0.0-2.fc14 has been pushed to the Fedora 14 stable repository.
Comment 17 Fedora Update System 2011-07-22 15:41:19 EDT
eigen3-3.0.0-2.fc15 has been pushed to the Fedora 15 stable repository.
Comment 18 Rich Mattes 2012-02-10 18:58:45 EST
Package Change Request
======================
Package Name: eigen3
New Branches: el6
Owners: rmattes

Required to build the pcl package on rhel6
Comment 19 Jon Ciesla 2012-02-11 14:34:57 EST
Git done (by process-git-requests).

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