python-cython C++ support patch

python-cython C++ support patch

[Masamichi Hosoda]

Hi,

I've found that modules built by python-cython with C++ could not be loaded.
If I understand correctly, the following patch fixes it.
Would you like to apply this patch to the package?

```
--- a/Cython-0.29.21/Cython/Utility/ModuleSetupCode.c
+++ b/Cython-0.29.21/Cython/Utility/ModuleSetupCode.c
@@ -709,7 +709,11 @@
 /////////////// PyModInitFuncType.proto ///////////////
 
 #ifndef CYTHON_NO_PYINIT_EXPORT
+#ifdef __cplusplus
+#define __Pyx_PyMODINIT_FUNC  extern "C" PyObject *
+#else
 #define __Pyx_PyMODINIT_FUNC  PyObject *
+#endif
 
 #elif PY_MAJOR_VERSION < 3
 // Py2: define this to void manually because PyMODINIT_FUNC adds __declspec(dllexport) to it's definition.
```

Thanks.

Re: python-cython C++ support patch

[Marco Atzeri]

On 29.01.2021 11:31, Masamichi Hosoda wrote:
> Hi,
> 
> I've found that modules built by python-cython with C++ could not be loaded.
> If I understand correctly, the following patch fixes it.
> Would you like to apply this patch to the package?
> 
> ```
> --- a/Cython-0.29.21/Cython/Utility/ModuleSetupCode.c
> +++ b/Cython-0.29.21/Cython/Utility/ModuleSetupCode.c
> @@ -709,7 +709,11 @@
>   /////////////// PyModInitFuncType.proto ///////////////
>   
>   #ifndef CYTHON_NO_PYINIT_EXPORT
> +#ifdef __cplusplus
> +#define __Pyx_PyMODINIT_FUNC  extern "C" PyObject *
> +#else
>   #define __Pyx_PyMODINIT_FUNC  PyObject *
> +#endif
>   
>   #elif PY_MAJOR_VERSION < 3
>   // Py2: define this to void manually because PyMODINIT_FUNC adds __declspec(dllexport) to it's definition.
> ```
> 
> Thanks.

Thanks Masamichi,

it seems in line with similar patches that I have applied to some
other python sub packages

Have you a test case for this one ?

I will look to rebuilt python-cython after some extra check

Marco

Re: python-cython C++ support patch

[Marco Atzeri]

On 29.01.2021 15:18, Marco Atzeri wrote:
> On 29.01.2021 11:31, Masamichi Hosoda wrote:
>> Hi,
>>
>> I've found that modules built by python-cython with C++ could not be 
>> loaded.
>> If I understand correctly, the following patch fixes it.
>> Would you like to apply this patch to the package?
>>
>> ```
>> --- a/Cython-0.29.21/Cython/Utility/ModuleSetupCode.c
>> +++ b/Cython-0.29.21/Cython/Utility/ModuleSetupCode.c
>> @@ -709,7 +709,11 @@
>>   /////////////// PyModInitFuncType.proto ///////////////
>>   #ifndef CYTHON_NO_PYINIT_EXPORT
>> +#ifdef __cplusplus
>> +#define __Pyx_PyMODINIT_FUNC  extern "C" PyObject *
>> +#else
>>   #define __Pyx_PyMODINIT_FUNC  PyObject *
>> +#endif
>>   #elif PY_MAJOR_VERSION < 3
>>   // Py2: define this to void manually because PyMODINIT_FUNC adds 
>> __declspec(dllexport) to it's definition.
>> ```
>>

have you considered that you just need to define
CYTHON_NO_PYINIT_EXPORT ?

the portion of the code below your change has already the
ifdef __cplusplus semantic

Have you proposed it upstream ? It does not seem
a change restricted to Cygwin

Any way I see no "wrongness" to add it on the Cython rebuild

Regards
Marco

Re: python-cython C++ support patch

[Masamichi Hosoda]

> have you considered that you just need to define
> CYTHON_NO_PYINIT_EXPORT ?
>
> the portion of the code below your change has already the
> ifdef __cplusplus semantic
>
> Have you proposed it upstream ? It does not seem
> a change restricted to Cygwin
>
> Any way I see no "wrongness" to add it on the Cython rebuild

Hi Marco,

The relevant upstream source looks like this.
https://github.com/cython/cython/blob/0.29.21/Cython/Utility/ModuleSetupCode.c#L712

```
#define __Pyx_PyMODINIT_FUNC PyMODINIT_FUNC
```
Perhaps `PyMODINIT_FUNC` is defined differently
depending on whether it is in C++ or not.

If I understand correctly, python-cython-0.29.21-2.src.patch
in the Cygwin python-cython package changes `PyMODINIT_FUNC` to `PyObject *`.
Here is the part of python-cython-0.29.21-2.src.patch.

```
--- origsrc/Cython-0.29.21/Cython/Utility/ModuleSetupCode.c	2020-07-08 23:44:39.000000000 +0200
+++ src/Cython-0.29.21/Cython/Utility/ModuleSetupCode.c	2021-01-04 22:10:21.641515500 +0100
@@ -709,7 +709,7 @@ static CYTHON_INLINE void * PyThread_tss
 /////////////// PyModInitFuncType.proto ///////////////
 
 #ifndef CYTHON_NO_PYINIT_EXPORT
-#define __Pyx_PyMODINIT_FUNC PyMODINIT_FUNC
+#define __Pyx_PyMODINIT_FUNC  PyObject *
 
 #elif PY_MAJOR_VERSION < 3
 // Py2: define this to void manually because PyMODINIT_FUNC adds __declspec(dllexport) to it's definition.
```

Thanks.

Re: python-cython C++ support patch

[Marco Atzeri]

On 30.01.2021 14:28, Masamichi Hosoda wrote:
>> have you considered that you just need to define
>> CYTHON_NO_PYINIT_EXPORT ?
>>
>> the portion of the code below your change has already the
>> ifdef __cplusplus semantic
>>
>> Have you proposed it upstream ? It does not seem
>> a change restricted to Cygwin
>>
>> Any way I see no "wrongness" to add it on the Cython rebuild
> 
> Hi Marco,
> 
> The relevant upstream source looks like this.
> https://github.com/cython/cython/blob/0.29.21/Cython/Utility/ModuleSetupCode.c#L712
> 
> ```
> #define __Pyx_PyMODINIT_FUNC PyMODINIT_FUNC

PyMODINIT_FUNC unfortunately dos not work and my changes try
to overcome it, see similar on

https://sourceware.org/pipermail/cygwin/2021-January/247211.html

but your portion is for additional C++ case
than I am adding in asimilar way

Regards
Marco

Re: python-cython C++ support patch

[Marco Atzeri]

On 30.01.2021 14:58, Marco Atzeri wrote:

> 
> PyMODINIT_FUNC unfortunately dos not work and my changes try
> to overcome it, see similar on
> 
> https://sourceware.org/pipermail/cygwin/2021-January/247211.html
> 
> but your portion is for additional C++ case
> than I am adding in asimilar way
> 
> Regards
> Marco

a test package has been just uploaded.
Try it and tell me if it solves your problem.

Otherwise point me to your C++ code and I will look on it.

Regards
Marco

Re: python-cython C++ support patch

[Masamichi Hosoda]

>> PyMODINIT_FUNC unfortunately dos not work and my changes try
>> to overcome it, see similar on
>> 
>> https://sourceware.org/pipermail/cygwin/2021-January/247211.html
>> 
>> but your portion is for additional C++ case
>> than I am adding in asimilar way
>> 
>> Regards
>> Marco
>
> a test package has been just uploaded.
> Try it and tell me if it solves your problem.
>
> Otherwise point me to your C++ code and I will look on it.
>
> Regards
> Marco

python38-cython-0.29.21-3 works fine with following test code.

```hello.pyx
print('Hello World!')
```

```setup.py
from distutils.core import setup
from Cython.Build import cythonize

setup(
    ext_modules=cythonize(
        "hello.pyx",
        language="c++",
    )
)
```

```test.py
import hello
```

Here's the result by python38-cython-0.29.21-2 (failed).

```
$ python setup.py build_ext --inplace
[...snip...]
$ python test.py
Traceback (most recent call last):
  File "test.py", line 1, in <module>
    import hello
ImportError: dynamic module does not define module export function (PyInit_hello)
```

Here's the result by python38-cython-0.29.21-3 (succeeded).

```
$ python setup.py build_ext --inplace
[...snip...]
$ python test.py
Hello World!
```

Thanks.

Re: python-cython C++ support patch

[Marco Atzeri]

On 31.01.2021 04:53, Masamichi Hosoda wrote:

> python38-cython-0.29.21-3 works fine with following test code.
> 
> ```hello.pyx
> print('Hello World!')
> ```
> 
> ```setup.py
> from distutils.core import setup
> from Cython.Build import cythonize
> 
> setup(
>      ext_modules=cythonize(
>          "hello.pyx",
>          language="c++",
>      )
> )
> ```
> 
> ```test.py
> import hello
> ```
> 
> Here's the result by python38-cython-0.29.21-2 (failed).
> 
> ```
> $ python setup.py build_ext --inplace
> [...snip...]
> $ python test.py
> Traceback (most recent call last):
>    File "test.py", line 1, in <module>
>      import hello
> ImportError: dynamic module does not define module export function (PyInit_hello)
> ```
> 
> Here's the result by python38-cython-0.29.21-3 (succeeded).
> 
> ```
> $ python setup.py build_ext --inplace
> [...snip...]
> $ python test.py
> Hello World!
> ```
> 
> Thanks.

Thanks to you

I still think that the change you proposed should go upstream.

Have you tested your example on a NOT cygwin system ?

Regards
Marco

Re: python-cython C++ support patch

[Masamichi Hosoda]

>> python38-cython-0.29.21-3 works fine with following test code.
>> 
>> ```hello.pyx
>> print('Hello World!')
>> ```
>> 
>> ```setup.py
>> from distutils.core import setup
>> from Cython.Build import cythonize
>> 
>> setup(
>>      ext_modules=cythonize(
>>          "hello.pyx",
>>          language="c++",
>>      )
>> )
>> ```
>> 
>> ```test.py
>> import hello
>> ```
>> 
>> Here's the result by python38-cython-0.29.21-2 (failed).
>> 
>> ```
>> $ python setup.py build_ext --inplace
>> [...snip...]
>> $ python test.py
>> Traceback (most recent call last):
>>    File "test.py", line 1, in <module>
>>      import hello
>> ImportError: dynamic module does not define module export function (PyInit_hello)
>> ```
>> 
>> Here's the result by python38-cython-0.29.21-3 (succeeded).
>> 
>> ```
>> $ python setup.py build_ext --inplace
>> [...snip...]
>> $ python test.py
>> Hello World!
>> ```
>> 
>> Thanks.
>
> Thanks to you
>
> I still think that the change you proposed should go upstream.
>
> Have you tested your example on a NOT cygwin system ?
>
> Regards
> Marco

Here's the result by Ubuntu 20.04 with python-cython 0.29.21

```
$ lsb_release -d
Description:    Ubuntu 20.04.2 LTS
$ pip3 list | grep "Cython"
Cython                 0.29.21
$ python3 setup.py build_ext --inplace
[...snip...]
$ python3 test.py
Hello World!
```

The export name is different for the successful and unsuccessful cases.

Here's Cygwin python38-cython-0.29.21-2 (failed).

```
$ nm hello.cpython-38-x86_64-cygwin.dll | grep "PyInit"
000000040b8a22af T _Z12PyInit_hellov
```

Here's Cygwin python38-cython-0.29.21-3 (succeeded).

```
$ nm hello.cpython-38-x86_64-cygwin.dll | grep "PyInit"
000000040b8a22af T PyInit_hello
```

Here's Ubuntu python-cython 0.29.21 (succeeded).

```
$ nm hello.cpython-38-x86_64-linux-gnu.so | grep "PyInit"
00000000000030b7 T PyInit_hello
```

Thanks.