Opened 10 years ago

Closed 10 years ago

#12761 closed defect (fixed)

Givaro's (3.2.13.rc1) C++ headers don't conform to C++11

Reported by: leif Owned by: leif
Priority: major Milestone: sage-5.0.1
Component: packages: standard Keywords: C++11 GCC 4.7.0 CXXFLAGS -fpermissive spkg sd40.5
Cc: cpernet, malb, mariah Merged in: sage-5.0.1.rc1
Authors: Leif Leonhardy, Jeroen Demeyer Reviewers: Karl-Dieter Crisman, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

This breaks the build of its test suite as well as of the Sage library with (e.g.) GCC 4.7.x.

I already have an spkg fixing this, i.e., the offending headers.

See #12751 for the GCC-4.7.0 metaticket.

Closely related: #12444 (which I unfortunately wasn't aware of until now) for the clang port


New spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/givaro-3.2.13.p0.spkg

givaro-3.2.13.p0 (Jeroen Demeyer, 25 May 2012)

  • #12761: Restore upstream sources to vanilla 3.2.13 (the previous src/ directory was some never-released CVS version between givaro-3.2.13.rc1 and givaro-3.2.13, but bootstrapped with a different automake).
  • Remove gmp++.h.patch which is upstreamed (the old diff was wrong).
  • Use patch to apply all patches.
  • Fix patch for givtablelimits.h such that it can be applied on all systems, not only Cygwin.
  • Merged all GCC-4.7.0 patches into one: cplusplus_scoping.patch
  • Don't touch .pyx files, instead fix module_list.py (also on #12761).

givaro-3.2.13.rc1.p4 (Leif Leonhardy, March 27th 2012)

  • #12761: Fix headers not conforming to C++11 to make Sage (especially the Sage library) build with GCC 4.7.0 (and without -fpermissive). Same for Givaro's test suite, which uses / instantiates much more! (These headers get installed into $SAGE_LOCAL/include/givaro/.) New patches:
    • patches/src.kernel.integer.givintnumtheo.inl.patch
    • patches/src.kernel.integer.givintrsa.inl.patch
    • patches/src.library.poly1.givpoly1factor.inl.patch
    • patches/src.library.poly1.givpoly1padic.h.patch
    • patches/src.library.poly1.givpoly1proot.inl.patch
  • Remove the obsolete Debian dist/ directory.
  • Remove obsolete GCC 4.3 patch.
  • Rename diffs of prepatched files that are (still) copied over to *.diff (rather than *.patch) such that they don't get "automatically" applied by the patch -p1 loop, which I added.
  • Fix permissions of SPKG.txt and spkg-install, and two upstream files.
  • Add "Special Update/Build? Instructions" section.
  • Clean up spkg-check and spkg-install.
  • Also set up environment variables in spkg-check, as make check involves compilation. (Although configure should have put them into the generated Makefiles.)
  • Use $MAKE in spkg-check as well.
  • Exit in case the build failed!
  • Only touch extension modules (*.pyx) if they (already) exist.

Attachments (3)

givaro-3.2.13.rc1.p3-p4.diff (44.3 KB) - added by leif 10 years ago.
Diff between the previous spkg in Sage and my new p4 spkg. For reference / review only.
12761_givaro_depends.patch (1.4 KB) - added by jdemeyer 10 years ago.
givaro-3.2.13.p0.diff (74.0 KB) - added by jdemeyer 10 years ago.
Diff between leif's 3.2.13.rc1.p4 and my 3.2.13.p0 spkg. For reference / review only.

Download all attachments as: .zip

Change History (25)

Changed 10 years ago by leif

Diff between the previous spkg in Sage and my new p4 spkg. For reference / review only.

comment:1 Changed 10 years ago by leif

  • Authors set to Leif Leonhardy
  • Cc cpernet malb mariah added
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by jdemeyer

  • Description modified (diff)

comment:3 follow-up: Changed 10 years ago by jdemeyer

This is something which should be reported upstream (sending them the patch of course).

comment:4 follow-up: Changed 10 years ago by jdemeyer

If adding the -I flag is not necessary, then why add it while saying it is not needed?

# It shouldn't be necessary to add Sage's include directory here, 
# since we configure with '--with-gmp=...'. 
# Also, '-I...' should normally be added to (just) CPPFLAGS. 
CFLAGS="$CFLAGS -fPIC -I\"$SAGE_LOCAL/include\"" 
CXXFLAGS="$CXXFLAGS -fPIC -I\"$SAGE_LOCAL/include\"" 

comment:5 Changed 10 years ago by jdemeyer

Don't Cython modules have proper dependency checking these days? Then the "touching" part might not be needed.

comment:6 in reply to: ↑ 4 Changed 10 years ago by leif

Replying to jdemeyer:

If adding the -I flag is not necessary, then why add it while saying it is not needed?

# It shouldn't be necessary to add Sage's include directory here, 
# since we configure with '--with-gmp=...'. 
# Also, '-I...' should normally be added to (just) CPPFLAGS. 
CFLAGS="$CFLAGS -fPIC -I\"$SAGE_LOCAL/include\"" 
CXXFLAGS="$CXXFLAGS -fPIC -I\"$SAGE_LOCAL/include\"" 

I haven't added this, nor have I checked that it works, i.e., builds, without that (although I'm pretty sure it would) -- that's why I wrote "shouldn't be necessary", not "isn't needed".

One side effect of adding it is that the test suite is built with the headers installed into Sage, which isn't all bad, but I can try to remove it later, although quite unrelated to the purpose of this ticket.

W.r.t. touching: It doesn't hurt. Moreover, the installed headers might have timestamps earlier than those of the Python extension modules, such that Cython probably won't notice that they're "new".

comment:7 in reply to: ↑ 3 Changed 10 years ago by leif

Replying to jdemeyer:

This is something which should be reported upstream (sending them the patch of course).

I cc'ed the spkg maintainers, which include Clément Pernet, according to SPKG.txt also the upstream contact.

Note that (as already mentioned elsewhere) the version currently in Sage is a release candidate, dated 2008 IIRC.

comment:8 Changed 10 years ago by leif

  • Description modified (diff)

Added a reference to #12444.

comment:9 follow-up: Changed 10 years ago by ddrake

The new spkg actually builds and passes tests (such as they are) on skynet/cicero. I can give a positive review to everything except your new header patches; they seem simple enough (almost all of it is just casting things to other types, right?) but I don't know enough C++ to be confident about that.

comment:10 in reply to: ↑ 9 Changed 10 years ago by leif

Replying to ddrake:

The new spkg actually builds and passes tests (such as they are) on skynet/cicero. I can give a positive review to everything

Fine.

except your new header patches; they seem simple enough (almost all of it is just casting things to other types, right?) but I don't know enough C++ to be confident about that.

Nope, I haven't added casts, but (roughly speaking) qualified the names of (mostly, I think) member functions used in template class member functions, i.e.,

template <class Foo>
void Bar<Foo>::bar() // member function bar() of class Bar, which is parameterized by a class Foo
{
    Bar<Foo>::baz(); // call with qualified [function] name; 
                     // baz() here is [also] a member function of [template] class Bar
}

instead of

...
{
    baz(); // call with unqualified [function] name
}

I could have used

...
{
    this->baz();
}

instead in most (if not all) cases as well; cf. Andrew Ohana's changes at #12444 (which I unfortunately found only after I made my own changes).

comment:11 Changed 10 years ago by leif

(Note that just using baz() wouldn't be a problem if it was already declared as a "direct" member function of Bar; problems arise if baz() isn't yet known at the point of the template definition, e.g. because it is inherited from a class which is a template parameter. I rather wanted to explain the Bar<Foo>:: prefix above.)

comment:12 Changed 10 years ago by Snark

givaro-3.2.13.rc1.p3 from 5.0.rc1 built well already, but I still gave a try to : a84996518e39a1197eaf63562d8fe734 givaro-3.2.13.rc1.p4.spkg and it built well too on my debian x86_64 box using gcc 4.7.0.

comment:13 Changed 10 years ago by Snark

Upstream latest version 3.5.0 doesn't compile with gcc 4.7 ; but their trunk does. I have written them to know when a new release will be available.

Did you discuss your patch with upstream?

comment:14 Changed 10 years ago by Snark

Upstream just released 3.6.0 today -- just upgrade!

Changed 10 years ago by jdemeyer

comment:15 Changed 10 years ago by jdemeyer

  • Authors changed from Leif Leonhardy to Leif Leonhardy, Jeroen Demeyer
  • Description modified (diff)

comment:16 Changed 10 years ago by jdemeyer

  • Keywords sd40.5 added

Changed 10 years ago by jdemeyer

Diff between leif's 3.2.13.rc1.p4 and my 3.2.13.p0 spkg. For reference / review only.

comment:17 Changed 10 years ago by kcrisman

This works and passes tests on sage.math with GCC-4.7.0 and with some 4.6.x on Mac OS X, and all the changes make sense, so I think this is ready to go as well.

comment:18 Changed 10 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman, Jeroen Demeyer

comment:19 Changed 10 years ago by kcrisman

  • Status changed from needs_review to positive_review

comment:20 Changed 10 years ago by Snark

Why not just upgrade to upstream 3.6.0 ?

comment:21 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.0.1

comment:22 Changed 10 years ago by jdemeyer

  • Merged in set to sage-5.0.1.rc1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.