Opened 7 years ago
Closed 7 years ago
#18323 closed enhancement (wontfix)
switch to C++11 when compiling some cython files
Reported by: | rws | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | build | Keywords: | c++ gcc cxx gcc5 |
Cc: | Merged in: | ||
Authors: | Reviewers: | Jeroen Demeyer | |
Report Upstream: | N/A | Work issues: | |
Branch: | u/rws/18323-1 (Commits, GitHub, GitLab) | Commit: | 7fd25bc1eea60abb8f686d611bd40f69996d343f |
Dependencies: | #18450, C++11 compiler | Stopgaps: |
Description (last modified by )
C++11 is needed by a development version of Pynac, and GCC 5.x's g++
again got stricter w.r.t. C++ standard conformance (while it still defaults to -std=gnu++98
).
Sage needs -std=c++11
for at least all files that import ginac headers, that is, for
src/sage/symbolic/constants_c.pyx src/sage/symbolic/expression.pyx src/sage/symbolic/function.pyx src/sage/symbolic/getitem.pyx src/sage/symbolic/pynac.pyx src/sage/symbolic/ring.pyx src/sage/symbolic/substitution_map.pyx
Change History (44)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
P.S.: While the GCC 4.6 and 4.7 series reached "end of life", there are still (LTS) distros using these.
comment:3 follow-up: ↓ 5 Changed 7 years ago by
Remember also that gcc 5.1 is defaulting to -std=gnu11
for C so there may be other surprises but you would already have noticed them I am guessing?
comment:4 follow-up: ↓ 6 Changed 7 years ago by
We should list all the other tickets for C++11
or have a meta ticket somewhere.
comment:5 in reply to: ↑ 3 Changed 7 years ago by
comment:6 in reply to: ↑ 4 Changed 7 years ago by
Replying to fbissey:
We should list all the other tickets for
C++11
or have a meta ticket somewhere.
We've already done a lot in that direction in the past, see for example the closed metaticket on GCC 4.7.0 (#12751), or search for C++11 on trac.
Related: #12426 (Metaticket for Clang; at least some issues are related to C++ or GNUisms)
comment:7 follow-up: ↓ 8 Changed 7 years ago by
I like meta tickets (but not abusing them). I guess I should rather look for a port to gcc 5.1 meta ticket, this one would just be a consequence. Looking at #18301now...
comment:8 in reply to: ↑ 7 Changed 7 years ago by
Replying to fbissey:
I like meta tickets (but not abusing them). I guess I should rather look for a port to gcc 5.1 meta ticket, this one would just be a consequence. Looking at #18301 now...
There's a thread "Issues with GCC 5.x" on sage-devel. Besides MPIR, Lcalc, and ncurses (all fixed), I'm having problems with gf2x on x86_64 (work-around: -O0
or GF2X_CONFIGURE=--disable-hardware-specific-code
).
comment:9 Changed 7 years ago by
I just confirmed that you simply cannot compile a package with C++11 and expect the .so
file to match all references generated by libsage compilation using a pre-C++11 compile. Specifically the template class std::vector<GiNaC::ex>
will create incompatible code.
Okay, you knew that beforehand, but I had to prove it to myself.
comment:10 Changed 7 years ago by
- Description modified (diff)
- Summary changed from switch to C++11 to switch to C++11 when compiling some cython files
Sorry for specializing but only this guarantees that I will be working on it.
comment:11 Changed 7 years ago by
- Branch set to u/rws/switch_to_c__11_when_compiling_some_cython_files
comment:12 Changed 7 years ago by
- Commit set to 9875e951d9f29e18b1dadef70989e92a27afbfc9
Branch pushed to git repo; I updated commit sha1. New commits:
9875e95 | 18323: add extra C++11 compile flags for symbolics because of Pynac-0.4
|
comment:13 Changed 7 years ago by
- Status changed from new to needs_review
comment:14 follow-up: ↓ 37 Changed 7 years ago by
I'll be picky.
1) I would have put that patch in the same ticket that starts the use of
C++11
in pynac. That's where it belongs, it's an associated change. 2) cosmetic, the indentation is wrong. 3) do you need to define a variable with an extra long name to replace what is just-std=c++11
in only three different places? It feels overkill. I would have tolerated a variable with a shorter name, to switchC++11
on - we'll see more of that certainly. Unless you plan to add more flags in the near future, I don't see why it should be symbolics specific
comment:15 Changed 7 years ago by
- Milestone changed from sage-6.7 to sage-pending
Thanks for the input. We could as well wait until gcc5 is mandatory in Sage, or even earlier, as soon as all C++ packages compile with C++11, and then switch globally. It's not that there is pressing interest in Pynac right now.
comment:16 Changed 7 years ago by
Just for the record: g++
5.x doesn't default to -std=c++11
, not even -std=gnu++11
or -std=gnu++0x
, but -std=gnu++98
.
comment:17 Changed 7 years ago by
- Description modified (diff)
comment:18 Changed 7 years ago by
... and the first g++
version said to be C++11 feature-complete is 4.8.1.
Since we already have 4.9.2 in Sage, we'd "just" have to SAGE_INSTALL_GCC
on systems with 4.8.0 (if such exist at all), 4.7, 4.6, and earlier.
Ubuntu 12.04 with 5-year LTS still ships with "Ubuntu/Linaro? GCC" 4.6 for example.
comment:19 Changed 7 years ago by
If you have time, you could try building Sage with -Werror=c++11-compat
in CXXFLAGS
(CFLAGS
for modules built by distutils, or added to extra_compile_args
)... :P
comment:20 follow-up: ↓ 21 Changed 7 years ago by
- Status changed from needs_review to needs_work
You should leave module_list.py
alone and move those flags to the appropriate .pxd
file.
comment:21 in reply to: ↑ 20 Changed 7 years ago by
Replying to jdemeyer:
You should leave
module_list.py
alone and move those flags to the appropriate.pxd
file.
How do I do that? The manual says nothing of it:
"A definition file can contain:
- Any kind of C type declaration.
- extern C function or variable declarations.
- Declarations of C functions defined in the module.
- The definition part of an extension type (see below)."
comment:22 Changed 7 years ago by
See #18450 (note that this ticket might also conflict with that)
comment:23 Changed 7 years ago by
- Branch changed from u/rws/switch_to_c__11_when_compiling_some_cython_files to u/rws/18323
comment:24 follow-up: ↓ 28 Changed 7 years ago by
- Commit changed from 9875e951d9f29e18b1dadef70989e92a27afbfc9 to cf808d3d0d72233edb693e930b007eea1913c7ca
- Dependencies set to C++11 compiler
- Status changed from needs_work to needs_review
This works now but still relies on a C++11 compiler. It was discussed above what efforts were already done to make the rest of Sage gcc5 compatible. My mistake was to assume that with gcc5 we would automatically get C++11. So what are the chances that whatever shipped gcc, which is at 4.9x I understand, will be made mandatory (it need not be switched to C++11 globally, it just needs to be there when the seven files are compiled)?
New commits:
cf808d3 | 18323: switch to C++11 when compiling some cython files
|
comment:25 follow-up: ↓ 30 Changed 7 years ago by
If those flags are only needed to support pynac, they should only be added to pynac declarations, not other random files like ring.pxd
.
comment:26 Changed 7 years ago by
- Dependencies changed from C++11 compiler to #18450, C++11 compiler
- Status changed from needs_review to needs_work
comment:27 Changed 7 years ago by
This needs https://github.com/cython/cython/pull/381: therefore, it needs to depend on #18450 or you need to wait for the next Cython release.
comment:28 in reply to: ↑ 24 Changed 7 years ago by
Replying to rws:
So what are the chances that whatever shipped gcc, which is at 4.9x I understand, will be made mandatory (it need not be switched to C++11 globally, it just needs to be there when the seven files are compiled)?
I don't think we should ever make GCC 4.9.x
mandatory. What could be discussed is making C++0x
or C++11
support mandatory.
comment:29 Changed 7 years ago by
- Branch changed from u/rws/18323 to u/rws/18323-1
comment:30 in reply to: ↑ 25 ; follow-ups: ↓ 31 ↓ 34 Changed 7 years ago by
- Commit changed from cf808d3d0d72233edb693e930b007eea1913c7ca to 7fd25bc1eea60abb8f686d611bd40f69996d343f
- Dependencies changed from #18450, C++11 compiler to C++11 compiler
- Status changed from needs_work to needs_review
Replying to jdemeyer:
If those flags are only needed to support pynac, they should only be added to pynac declarations, not other random files like
ring.pxd
.
Indeed, it boils down to expression.pxd
.
it needs to depend on #18450 or you need to wait for the next Cython release.
It's pending, so just wait. I expect some friction with mandatory C++11, anyway.
New commits:
7fd25bc | 18323: switch to C++11 when compiling some cython files
|
comment:31 in reply to: ↑ 30 ; follow-up: ↓ 32 Changed 7 years ago by
comment:32 in reply to: ↑ 31 Changed 7 years ago by
comment:33 Changed 7 years ago by
- Status changed from needs_review to needs_work
comment:34 in reply to: ↑ 30 ; follow-up: ↓ 35 Changed 7 years ago by
- Dependencies changed from C++11 compiler to #18450, C++11 compiler
Replying to rws:
I expect some friction with mandatory C++11, anyway.
Personally, I don't think it would be such a problem. The most annoying part will be testing with older GCC versions (for example, GCC 4.6.x supports some C++11 features, are these enough for Pynac?).
I put back the dependency on #18450 since otherwise this patch won't work properly.
comment:35 in reply to: ↑ 34 Changed 7 years ago by
Replying to jdemeyer:
Replying to rws:
I expect some friction with mandatory C++11, anyway.
Personally, I don't think it would be such a problem. The most annoying part will be testing with older GCC versions (for example, GCC 4.6.x supports some C++11 features, are these enough for Pynac?).
From a table I have here 4.6 is missing override
. I could hold back usage of override
, no problem.
comment:36 Changed 7 years ago by
Coming back to this... We could check for the feature that are necessary. If you need override
we can first check for c++11
as suggested by Jeroen, once we know that we have a c++11
switch we can check for particular features like override
it is just a matter of writing a test in configure.ac
.
But making a decision on what we should want as minimal features for the compiler should be matter of debate on sage-devel.
comment:37 in reply to: ↑ 14 ; follow-up: ↓ 38 Changed 7 years ago by
- Milestone changed from sage-pending to sage-duplicate/invalid/wontfix
- Reviewers set to Jeroen Demeyer
- Status changed from needs_work to needs_review
Replying to fbissey:
I would have put that patch in the same ticket that starts the use of
C++11
in pynac. That's where it belongs, it's an associated change.
I totally agree. Close as "wontfix"?
comment:38 in reply to: ↑ 37 Changed 7 years ago by
- Status changed from needs_review to positive_review
Replying to jdemeyer:
Replying to fbissey:
I would have put that patch in the same ticket that starts the use of
C++11
in pynac. That's where it belongs, it's an associated change.I totally agree. Close as "wontfix"?
I am fine with closing. We should open a new ticket when we have decided exactly what we want on sage-devel.
comment:39 Changed 7 years ago by
I disagree. Holding back on allowing C++11 just means
- taking on more technical debt as authors can't take advantage of the new features, writing code that may have to be changed later
- prevent contributors from being exposed to new C++11 features, slowing down their learning
- online C++ resources increasingly include C++11 features, making it even more difficult to learn how to not use them
- delaying the inevitable, we'll have to shitlist broken compilers anyways
comment:40 follow-ups: ↓ 41 ↓ 43 Changed 7 years ago by
This ticket is about adding std=c++11
for some ginac files. It's not about C++11 in general.
Besides, why do you think that people are prevented from using C++11? It's not like we prevent people from using their own C++11 compiler. Once their code gets in Sage, we make the switch in Sage.
comment:41 in reply to: ↑ 40 ; follow-up: ↓ 42 Changed 7 years ago by
Replying to jdemeyer:
This ticket is about adding
std=c++11
for some ginac files. It's not about C++11 in general.
And a separate ticket wasn't the right place for it.
Besides, why do you think that people are prevented from using C++11? It's not like we prevent people from using their own C++11 compiler. Once their code gets in Sage, we make the switch in Sage.
This ticket is not the right forum for the discussion.
Nevertheless which minimum gcc
would we be aiming for?
comment:42 in reply to: ↑ 41 Changed 7 years ago by
Replying to fbissey:
This ticket is not the right forum for the discussion.
Right, there is already a thread on sage-devel. Let's just close this discussion here.
comment:43 in reply to: ↑ 40 Changed 7 years ago by
comment:44 Changed 7 years ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
Well, we don't have to compile everything in C++ (implicitly or explicitly) with
-std=c++11
, but Lcalc's special anyway (cf. http://trac.sagemath.org/ticket/12437#comment:15 for example). See also #12426 and many (already closed) tickets dealing with C++11/GCC 4.7, 5.x.