Opened 10 years ago
Closed 10 years ago
#10094 closed defect (fixed)
cython and C++
Reported by: | vbraun | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6.1 |
Component: | packages: standard | Keywords: | |
Cc: | robertwb, leif, jason, craigcitro, timdumol, mpatel | Merged in: | sage-4.6.1.alpha1 |
Authors: | Volker Braun, Jeroen Demeyer | Reviewers: | Robert Bradshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
I still can't build C++ cython modules, even though it should be possible with the cython-0.13.
python `which cython` --embed-positions --directive cdivision=True,autotestdict=False -I/home/vbraun/opt/sage-4.5.3/devel/sage-main -o sage/geometry/ppl.cpp sage/geometry/ppl.pyx Error converting Pyrex file to C: ------------------------------------------------------------ ... """ cdef Variable *thisptr def __cinit__(self, dimension_type i): thisptr = new Variable(i) ^ ------------------------------------------------------------ /home/vbraun/opt/sage-4.5.3/devel/sage-main/sage/geometry/ppl.pyx:19:22: Operation only allowed in c++
The problem seems to be that cython expects --cplus
to enable the new language features. Cython does not derive this from the output file name extension ('cpp').
I've written the obvious patch to setup.py
, see attachment.
Apply trac_10094_add_cplus_command_line_option.patch and 10094_fix_doctests.2.patch.
Attachments (4)
Change History (25)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Cc robertwb leif jason craigcitro timdumol mpatel added
python `which cython` --cplus --embed-positions --directive cdivision=True,autotestdict=False -I/home/vbraun/opt/sage-4.5.3/devel/sage-main -o sage/rings/finite_rings/element_givaro.cpp sage/rings/finite_rings/element_givaro.pyx Error converting Pyrex file to C: ------------------------------------------------------------ ... modulus.append(0) if is_Polynomial(modulus): modulus = modulus.list() if PY_TYPE_CHECK(modulus, list) or PY_TYPE_CHECK(modulus, tuple): ^ ------------------------------------------------------------ /home/vbraun/opt/sage-4.5.3/devel/sage-main/sage/rings/finite_rings/element_givaro.pyx:264:24: ambiguous overloaded method
Givaro, one of the C++ extension modules, is very unhappy about cython --cplus
.
comment:2 Changed 10 years ago by
- Status changed from new to needs_review
The problem is this snippet from element_givaro.pyx
, where PY_TYPE_CHECK
is declared a second time. It is then ambiguous with its first declaration.
cdef extern from "stdsage.h": [...] int PY_TYPE_CHECK(object o, object t)
Removing this one line works, patch attached. I did a sage -ba
and had no more troubles.
comment:3 Changed 10 years ago by
Final patch for minor doctest breakage. The two doctests fixes to sageinspect.py
also need to be applied to local/lib/python2.6/site-packages/sagenb-0.8.2-py2.6.egg/sagenb/misc/sageinspect.py
. I'll put a corresponding comment on #10036.
comment:4 Changed 10 years ago by
The colormaps doctest is fixed in the patch on the matplotlib upgrade ticket, and shouldn't be included in this patch (it is already merged).
comment:5 Changed 10 years ago by
Should we apply all three patches?
comment:6 Changed 10 years ago by
I think the sagenb issue is already fixed as well: http://trac.sagemath.org/sage_trac/attachment/ticket/9828/trac_9828-sagenb_cython_0_13.patch
comment:7 Changed 10 years ago by
So I guess my point is that trac_10094_fix_doctests.patch is not needed, as those fixes are already merged on #10036 (the notebook issue) and the matplotlib upgrade to 1.0.
comment:8 Changed 10 years ago by
Oh, I didn't know that the doctest fixes are already included elsewhere. So, just to be sure, apply
trac_10094_add_cplus_command_line_option.patch
trac_10094_fix_givaro.patch
comment:9 Changed 10 years ago by
robertwb: can you comment on this ticket? Does --cplus switch on the capability, or does it force c++ generation (i.e., should --cplus be enabled for all files??)
comment:10 Changed 10 years ago by
Can you not just declare the language to be c++ in the module_list.py file, instead of modifying the command-line switch for everything?
comment:11 Changed 10 years ago by
In fact, it seems that lots of modules in module_list.py already switch on the C++ features by doing language="c++"
comment:12 follow-up: ↓ 13 Changed 10 years ago by
Initially I of course used language="c++"
, this does not work. If you look at the original patch, all that does is switch the output file name extension to cpp
. And cython then still relies on the --cplus
switch. I don't know if thats a cython bug or intentional design. Robert, you should be able to tell us?
Also, note that the patch only adds --cplus
if language="c++"
in module_list.py
comment:13 in reply to: ↑ 12 Changed 10 years ago by
Replying to vbraun:
Also, note that the patch only adds
--cplus
iflanguage="c++"
inmodule_list.py
Okay, thanks for the clarification.
comment:14 Changed 10 years ago by
The --cplus is needed to use C++ features in the Cython file, and language="c++" is needed to invoke the C++ compiler. We should not enable --cplus for all files, as it may (now or in the future) emit C++ code. The reason we need to specify the flag manually is that we're manually invoking Cython (for good reason) rather than trying to let distutils do it for us. I thought, however, that this setting would be inferred from the output file extension, apparently not (yet/anymore?).
In any case, positive review to trac_10094_add_cplus_command_line_option.patch
comment:15 Changed 10 years ago by
- Reviewers set to Robert Bradshaw, Jeroen Demeyer
Patches trac_10094_fix_givaro.patch
and trac_10094_fix_doctests.patch
give patch errors. I will check to see whether they are still needed.
comment:16 Changed 10 years ago by
The givaro patch is obsoleted by #10089 which removed even more unnecessary includes.
comment:17 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_review to needs_work
comment:18 Changed 10 years ago by
- Status changed from needs_work to needs_review
Can somebody review 10094_fix_doctests.2.patch, then we have a full positive_review.
comment:19 Changed 10 years ago by
- Reviewers changed from Robert Bradshaw, Jeroen Demeyer to Robert Bradshaw
comment:20 Changed 10 years ago by
- Status changed from needs_review to positive_review
10094_fix_doctests.2.patch looks good to me.
comment:21 Changed 10 years ago by
- Merged in set to sage-4.6.1.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
Initial patch