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: |
Description (last modified by )
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 thepatch -p1
loop, which I added. - Fix permissions of
SPKG.txt
andspkg-install
, and two upstream files. - Add "Special Update/Build? Instructions" section.
- Clean up
spkg-check
andspkg-install
. - Also set up environment variables in
spkg-check
, asmake check
involves compilation. (Althoughconfigure
should have put them into the generated Makefiles.) - Use
$MAKE
inspkg-check
as well. - Exit in case the build failed!
- Only
touch
extension modules (*.pyx
) if they (already) exist.
Attachments (3)
Change History (25)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Cc cpernet malb mariah added
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Description modified (diff)
comment:3 follow-up: ↓ 7 Changed 10 years ago by
This is something which should be reported upstream (sending them the patch of course).
comment:4 follow-up: ↓ 6 Changed 10 years ago by
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
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
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. touch
ing: 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
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:9 follow-up: ↓ 10 Changed 10 years ago by
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
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
(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
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
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
Upstream just released 3.6.0 today -- just upgrade!
Changed 10 years ago by
comment:15 Changed 10 years ago by
- Description modified (diff)
comment:16 Changed 10 years ago by
- Keywords sd40.5 added
Changed 10 years ago by
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
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
- Reviewers set to Karl-Dieter Crisman, Jeroen Demeyer
comment:19 Changed 10 years ago by
- Status changed from needs_review to positive_review
comment:20 Changed 10 years ago by
Why not just upgrade to upstream 3.6.0 ?
comment:21 Changed 10 years ago by
- Milestone changed from sage-5.1 to sage-5.0.1
comment:22 Changed 10 years ago by
- Merged in set to sage-5.0.1.rc1
- Resolution set to fixed
- Status changed from positive_review to closed
Diff between the previous spkg in Sage and my new p4 spkg. For reference / review only.