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

[nexttime]

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)

• Givaro's (3.2.13.rc1) C++ headers don't conform to C++11 #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 Givaro's (3.2.13.rc1) C++ headers don't conform to C++11 #12761).

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

• Givaro's (3.2.13.rc1) C++ headers don't conform to C++11 #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.

CC: @ClementPernet @malb @sagetrac-mariah

Component: packages: standard

Keywords: C++11 GCC 4.7.0 CXXFLAGS -fpermissive spkg sd40.5

Author: Leif Leonhardy, Jeroen Demeyer

Reviewer: Karl-Dieter Crisman, Jeroen Demeyer

Merged: sage-5.0.1.rc1

Issue created by migration from https://trac.sagemath.org/ticket/12761

[nexttime]

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

[nexttime]

comment:1

Attachment: givaro-3.2.13.rc1.p3-p4.diff.gz

[nexttime]

Author: Leif Leonhardy

[jdemeyer]

comment:3

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

[jdemeyer]

comment:4

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\"" 

[jdemeyer]

comment:5

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

[nexttime]

comment:6

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

[nexttime]

comment:7

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.

[nexttime]

comment:8

Added a reference to #12444.

[nexttime]

comment:10

Replying to @dandrake:

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

[nexttime]

comment:11

(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.)

[SnarkBoojum]

comment:12

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.

[SnarkBoojum]

comment:13

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?

[SnarkBoojum]

comment:14

Upstream just released 3.6.0 today -- just upgrade!

[jdemeyer]

Attachment: 12761_givaro_depends.patch.gz

[jdemeyer]

Changed author from Leif Leonhardy to Leif Leonhardy, Jeroen Demeyer

[jdemeyer]

Changed keywords from C++11 GCC 4.7.0 CXXFLAGS -fpermissive spkg to C++11 GCC 4.7.0 CXXFLAGS -fpermissive spkg sd40.5

[jdemeyer]

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

[kcrisman]

comment:17

Attachment: givaro-3.2.13.p0.diff.gz

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.

[kcrisman]

Reviewer: Karl-Dieter Crisman, Jeroen Demeyer

[SnarkBoojum]

comment:20

Why not just upgrade to upstream 3.6.0 ?

[jdemeyer]

Merged: sage-5.0.1.rc1