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...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  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:
Environment:
Last Closed: 2008-09-12 12:48:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
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
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 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:
   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 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
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@fedoraproject.org> 2.0-0.2.alpha7
- add working %check
Comment 10 Rex Dieter 2008-08-22 16:06:10 EDT
scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=778975
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
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 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
   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 10:56:51 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.3.alpha7.fc10.src.rpm


%changelog
* Mon Aug 25 2008 Rex Dieter <rdieter@fedoraproject.org> 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 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.

APPROVED
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.