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

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)

trac_10094_add_cplus_command_line_option.patch (1.0 KB) - added by vbraun 10 years ago.
Initial patch
trac_10094_fix_givaro.patch (677 bytes) - added by vbraun 10 years ago.
Initial patch
trac_10094_fix_doctests.patch (1.9 KB) - added by vbraun 10 years ago.
Initial patch
10094_fix_doctests.2.patch (1.1 KB) - added by jdemeyer 10 years ago.
Updated doctest patch

Download all attachments as: .zip

Change History (25)

Changed 10 years ago by vbraun

Initial patch

comment:1 Changed 10 years ago by vbraun

  • 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 vbraun

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

Changed 10 years ago by vbraun

Initial patch

comment:3 Changed 10 years ago by vbraun

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.

Changed 10 years ago by vbraun

Initial patch

comment:4 Changed 10 years ago by jason

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 jason

Should we apply all three patches?

comment:7 Changed 10 years ago by jason

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 vbraun

Oh, I didn't know that the doctest fixes are already included elsewhere. So, just to be sure, apply

  1. trac_10094_add_cplus_command_line_option.patch
  2. trac_10094_fix_givaro.patch

comment:9 Changed 10 years ago by jason

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 jason

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 jason

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: Changed 10 years ago by vbraun

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 jason

Replying to vbraun:

Also, note that the patch only adds --cplus if language="c++" in module_list.py

Okay, thanks for the clarification.

comment:14 Changed 10 years ago by robertwb

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 jdemeyer

  • 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 vbraun

The givaro patch is obsoleted by #10089 which removed even more unnecessary includes.

Changed 10 years ago by jdemeyer

Updated doctest patch

comment:17 Changed 10 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_review to needs_work

comment:18 Changed 10 years ago by jdemeyer

  • 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 jdemeyer

  • Authors changed from Volker Braun to Volker Braun, Jeroen Demeyer
  • Reviewers changed from Robert Bradshaw, Jeroen Demeyer to Robert Bradshaw

comment:20 Changed 10 years ago by robertwb

  • Status changed from needs_review to positive_review

10094_fix_doctests.2.patch looks good to me.

comment:21 Changed 10 years ago by jdemeyer

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