Bug 459705 (eigen2)

Summary: Review Request: eigen2 - A lightweight C++ template library for vector and matrix math
Product: [Fedora] Fedora Reporter: Rex Dieter <rdieter>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, kevin, notting
Target Milestone: ---Flags: j: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-09-12 16:48:39 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Rex Dieter 2008-08-21 14:26:46 UTC
Spec URL: http://rdieter.fedorapeople.org/pkg-reviews/eigen2/eigen2.spec
SRPM URL: http://rdieter.fedorapeople.org/pkg-reviews/eigen2/eigen2-2.0-0.1.alpha7.fc10.src.rpm
Description:
A lightweight C++ template library for vector and matrix math

Needed for koffice2 (in rawhide).  Parallel-installable with eigen(1), which is (still) needed by kdeedu-4.x

Comment 1 Jason Tibbitts 2008-08-21 19:00:21 UTC
I guess the only real question is if it's worth enabling the tests.  Unfortunately I know so little of cmake that I can't figure out the proper makefile target for actually running the tests.

* source files match upstream:
   167eb73ae79a55c957188e4c80659c03ef786773ddb7286abfca637bda3e8c73  
   eigen-2.0-alpha7.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint is silent.
* final provides and requires are sane:
   eigen2 = 2.0-0.1.alpha7.fc10
   eigen2-devel = 2.0-0.1.alpha7.fc10
  =
   (nothing)
? %check is not present, but a test suite exists.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package (since that's all there is).

Comment 2 Kevin Kofler 2008-08-21 22:22:28 UTC
This is a headers-only library, it can't get miscompiled, so I don't see what we'd really gain from running tests.

Comment 3 Jason Tibbitts 2008-08-21 23:03:35 UTC
Pretty obvious to me: it can uncover issues relating to our compiler's interpretation of the headers.

Comment 4 Rex Dieter 2008-08-21 23:34:16 UTC
I agree with tibbs mostly (as discussed a bit on irc).  At best, the additional checks will uncover breakage, at worst (should) be harmless.

Tibbs, are there any other remaining issues you see?

Comment 5 Jason Tibbitts 2008-08-22 00:35:08 UTC
I don't see anything else.  Do you think it difficult to enable the tests?

Comment 6 Rex Dieter 2008-08-22 01:26:26 UTC
Nope, it's just enabling a single define in the specfile.

Comment 7 Jason Tibbitts 2008-08-22 01:35:34 UTC
I had originally done the naive thing and enabled that define but I didn't see any output that indicated that any of the tests was actually being run.

Comment 8 Rex Dieter 2008-08-22 15:00:15 UTC
Good spot, previously we only built the tests, but didn't actually run them (which required using ctest).  I'll upload a new pkg that enables these properly.
Turns out that I'm seeing 1 failed test, but I'm hoping that's ok for now.

Comment 9 Rex Dieter 2008-08-22 15:10:43 UTC
Spec URL: http://rdieter.fedorapeople.org/pkg-reviews/eigen2/eigen2.spec
SRPM URL:
http://rdieter.fedorapeople.org/pkg-reviews/eigen2/eigen2-2.0-0.2.alpha7.fc10.src.rpm


* Fri Aug 22 2008 Rex Dieter <rdieter> 2.0-0.2.alpha7
- add working %check

Comment 10 Rex Dieter 2008-08-22 20:06:10 UTC
scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=778975

Comment 11 Jason Tibbitts 2008-08-24 20:06:29 UTC
I guess you know that scratch build failed.  I can't really approve a package that doesn't actually build, but I went ahead and tested it out myself.  Oddly enough, it builds on x86_64 but not on i386 (even though the result is, of course, a noarch package).  I believe the buildsys will build noarch packages in a 32-bit chroot even if it builds on an x86_64 (or ppc64) machine.

In any case, it's kind of a stretch to approve a package that won't actually build.  Failing tests aren't inherently a problem as long as you know about them, understand the failure and are convinced that it won't cause a problem for the distro if the test is disabled until upstream can fix the issue.  However, failing builds probably should block the process.

Comment 12 Rex Dieter 2008-08-24 20:28:52 UTC
Joy, no, I didn't notice the failure (scratch builds don't send email notification I guess, and all my local builds succeeded).

Comment 13 Rex Dieter 2008-08-24 20:30:27 UTC
Heh,
/builddir/build/BUILD/eigen2/Eigen/src/Core/Functors.h:274: internal compiler error: in convert_move, at expr.c:371

NOTOURBUG. :)

Comment 14 Kevin Kofler 2008-08-24 20:43:43 UTC
Not building the testsuite will get the package built, however client programs building against it may run into it.

It's a g++ bug, it will have to be filed against GCC.

I'll look into it to see if it can be worked around, maybe I can even figure out what is going wrong in GCC.

Comment 15 Rex Dieter 2008-08-24 20:48:16 UTC
I can at least confirm that koffice2 builds fine against eigen2 on F-9/i386 and F-9/x86_64

Comment 16 Kevin Kofler 2008-08-24 21:24:11 UTC
Looks pretty clearly like a GCC bug.

The offending line in Eigen looks reasonable:
  inline ei_scalar_quotient1_impl(const Scalar& other) : m_other(static_cast<Scalar>(1) / other) {}
No inline assembly or anything suspicious.

GCC crashes in void convert_move (rtx to, rtx from, int unsignedp). The function is documented as:
/* Copy data from FROM to TO, where the machine modes are not the same.
   Both modes may be integer, or both may be floating, or both may be
   fixed-point.
   UNSIGNEDP should be nonzero if FROM is an unsigned type.
   This causes zero-extension instead of sign-extension.  */
The assertion which fails is gcc_assert (to_real == from_real);. This checks for the constraint that "Both modes may be integer, or both may be floating", here we have a mix, so GCC is calling convert_move with invalid arguments.

A backtrace might help, as might GCC internal representation dumps (both tree and RTL dumps), but in any case a bug needs to be filed against GCC.

I'd suggest disabling building the testsuite for now if that gets the package to build, but filing a bug against GCC to get GCC fixed.

Comment 17 Rex Dieter 2008-08-25 14:56:51 UTC
Spec URL: http://rdieter.fedorapeople.org/pkg-reviews/eigen2/eigen2.spec
SRPM URL:
http://rdieter.fedorapeople.org/pkg-reviews/eigen2/eigen2-2.0-0.3.alpha7.fc10.src.rpm


%changelog
* Mon Aug 25 2008 Rex Dieter <rdieter> 2.0-0.3.alpha7
- disable buildtime tests, which tickle gcc bugs

scratch build:
 http://koji.fedoraproject.org/koji/taskinfo?taskID=784715

Comment 18 Jason Tibbitts 2008-08-26 15:05:52 UTC
OK, this builds fine.  If you think it reasonable to skip the test suite then I'll go along, although I hope that the compiler issue can be fixed soon and the tests turned back on.

APPROVED

Comment 19 Kevin Kofler 2008-09-11 15:35:12 UTC
Hmmm, this was approved, why hasn't it been imported yet?

Comment 20 Rex Dieter 2008-09-11 16:57:41 UTC
Maybe cause I hadn't gotten round to doing the cvsadmin request yet? :)

New Package CVS Request
=======================
Package Name: eigen2
Short Description: A lightweight C++ template library for vector and matrix math
Owners: rdieter
Branches: F-9

Comment 21 Kevin Fenzi 2008-09-11 19:40:44 UTC
cvs done.

Comment 22 Kevin Kofler 2008-09-11 20:55:08 UTC
I can confirm that the testsuite build crashes g++ during the build of lu.o in a F9 32-bit chroot whereas the build succeeds in a 64-bit chroot on the same machine. I have the preprocessed source now and can reproduce the crash with "g++ -m32 -O2 -g -S -fpreprocessed eigen2-lu.i", I'm working on figuring out a minimal testcase now.

Comment 23 Kevin Kofler 2008-09-11 21:41:00 UTC
It's actually the build of inverse.cpp which crashes, unfortunately it's hard to distill a reasonable-sized testcase as the GCC folks like it from the big mess of templates. :-(

Comment 24 Kevin Kofler 2008-09-11 22:09:41 UTC
I just filed https://bugzilla.redhat.com/show_bug.cgi?id=462015 against gcc.

Comment 25 Rex Dieter 2008-09-12 16:48:39 UTC
imported, building.