Bug 459705 - (eigen2) Review Request: eigen2 - A lightweight C++ template library for vector and matrix math
Review Request: eigen2 - A lightweight C++ template library for vector and ma...
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2008-08-21 10:26 EDT by Rex Dieter
Modified: 2009-10-20 09:57 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-09-12 12:48:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Rex Dieter 2008-08-21 10:26:46 EDT
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
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 15:00:21 EDT
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:
* 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
? %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 18:22:28 EDT
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 19:03:35 EDT
Pretty obvious to me: it can uncover issues relating to our compiler's interpretation of the headers.
Comment 4 Rex Dieter 2008-08-21 19:34:16 EDT
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-21 20:35:08 EDT
I don't see anything else.  Do you think it difficult to enable the tests?
Comment 6 Rex Dieter 2008-08-21 21:26:26 EDT
Nope, it's just enabling a single define in the specfile.
Comment 7 Jason Tibbitts 2008-08-21 21:35:34 EDT
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 11:00:15 EDT
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 11:10:43 EDT
Spec URL: http://rdieter.fedorapeople.org/pkg-reviews/eigen2/eigen2.spec

* Fri Aug 22 2008 Rex Dieter <rdieter@fedoraproject.org> 2.0-0.2.alpha7
- add working %check
Comment 10 Rex Dieter 2008-08-22 16:06:10 EDT
scratch build:
Comment 11 Jason Tibbitts 2008-08-24 16:06:29 EDT
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 16:28:52 EDT
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 16:30:27 EDT
/builddir/build/BUILD/eigen2/Eigen/src/Core/Functors.h:274: internal compiler error: in convert_move, at expr.c:371

Comment 14 Kevin Kofler 2008-08-24 16:43:43 EDT
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 16:48:16 EDT
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 17:24:11 EDT
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
   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 10:56:51 EDT
Spec URL: http://rdieter.fedorapeople.org/pkg-reviews/eigen2/eigen2.spec

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

scratch build:
Comment 18 Jason Tibbitts 2008-08-26 11:05:52 EDT
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.

Comment 19 Kevin Kofler 2008-09-11 11:35:12 EDT
Hmmm, this was approved, why hasn't it been imported yet?
Comment 20 Rex Dieter 2008-09-11 12:57:41 EDT
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 15:40:44 EDT
cvs done.
Comment 22 Kevin Kofler 2008-09-11 16:55:08 EDT
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 17:41:00 EDT
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 18:09:41 EDT
I just filed https://bugzilla.redhat.com/show_bug.cgi?id=462015 against gcc.
Comment 25 Rex Dieter 2008-09-12 12:48:39 EDT
imported, building.

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