Bug 672395 - Review Request: eigen3 - A lightweight C++ template library for vector and matrix math
Summary: Review Request: eigen3 - A lightweight C++ template library for vector and ma...
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tim Niemueller
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On: 691133
Blocks: 694479
TreeView+ depends on / blocked
 
Reported: 2011-01-25 02:35 UTC by Rich Mattes
Modified: 2012-02-11 19:34 UTC (History)
6 users (show)

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 19:30:47 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tim: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Rich Mattes 2011-01-25 02:35:26 UTC
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 10:22:36 UTC
Don't build against reference BLAS. Use ATLAS instead.

Comment 2 Rich Mattes 2011-01-25 23:03:36 UTC
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 17:04:27 UTC
Hi Rich, would you mind updating to the final release?

Comment 4 Rich Mattes 2011-03-27 00:14:23 UTC
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 11:42:57 UTC
I'll review this as far as possible on F-14.

Comment 6 Tim Niemueller 2011-04-17 23:41:12 UTC
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 04:27:57 UTC
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 12:23:08 UTC
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 16:49:28 UTC
tim is currently the active reviewer...

Comment 10 Tim Niemueller 2011-07-08 11:36:25 UTC
My blockers have been fixed, therefore:

APPROVED

Comment 11 Rich Mattes 2011-07-09 20:39:05 UTC
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 Gwyn Ciesla 2011-07-09 23:14:50 UTC
Git done (by process-git-requests).

Comment 13 Fedora Update System 2011-07-10 01:02:00 UTC
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-10 01:02:09 UTC
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 04:58:47 UTC
eigen3-3.0.0-2.fc15 has been pushed to the Fedora 15 testing repository.

Comment 16 Fedora Update System 2011-07-22 19:30:40 UTC
eigen3-3.0.0-2.fc14 has been pushed to the Fedora 14 stable repository.

Comment 17 Fedora Update System 2011-07-22 19:41:19 UTC
eigen3-3.0.0-2.fc15 has been pushed to the Fedora 15 stable repository.

Comment 18 Rich Mattes 2012-02-10 23:58:45 UTC
Package Change Request
======================
Package Name: eigen3
New Branches: el6
Owners: rmattes

Required to build the pcl package on rhel6

Comment 19 Gwyn Ciesla 2012-02-11 19:34:57 UTC
Git done (by process-git-requests).


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