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 Review | Assignee: | Jason Tibbitts <j> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | 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
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). This is a headers-only library, it can't get miscompiled, so I don't see what we'd really gain from running tests. Pretty obvious to me: it can uncover issues relating to our compiler's interpretation of the headers. 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? I don't see anything else. Do you think it difficult to enable the tests? Nope, it's just enabling a single define in the specfile. 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. 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. 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 scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=778975 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. Joy, no, I didn't notice the failure (scratch builds don't send email notification I guess, and all my local builds succeeded). Heh, /builddir/build/BUILD/eigen2/Eigen/src/Core/Functors.h:274: internal compiler error: in convert_move, at expr.c:371 NOTOURBUG. :) 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. I can at least confirm that koffice2 builds fine against eigen2 on F-9/i386 and F-9/x86_64 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. 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 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 Hmmm, this was approved, why hasn't it been imported yet? 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 cvs done. 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. 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. :-( I just filed https://bugzilla.redhat.com/show_bug.cgi?id=462015 against gcc. imported, building. |