Make cypari2 Python2 / Python3 compatible

[videlec]

Currently string handling in cypari2 is done via PyString_AsString which does not exist in Python 3. We rely on Cython to make proper conversions to bytes.

Depends on #22185

CC: @defeo @jdemeyer

Component: c_lib

Author: Vincent Delecroix, Jeroen Demeyer

Branch/Commit: 7096c82

Reviewer: Jeroen Demeyer, Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/22198

[videlec]

Branch: u/vdelecroix/22198

[videlec]

Last 10 new commits:

183588e	21808: fix some more .python() .sage()
8828867	Python 3 compatibility in doctests
87b6ac3	Merge branch 't/21807/21807' into HEAD
ee54f07	Python 3 compatibility in doctests
7a35c89	Merge commit '6022cab1880d6f3820e0f028671ddd2983eae42b'; commit 'ee54f071a26c63821f475d2832c7bb1fbbdd7e95' into ticket/22183
235efd3	Rename PariInstance -> Pari
6f04aba	Remove unused imports from sage
c258dca	Rename gen -> Gen
0c4433c	Fix documentation
ff6a3fd	22198: do not depend on PyString_AsString

[videlec]

Commit: ff6a3fd

[jdemeyer]

comment:2

Did you actually check that Python 3 works with this patch?

[videlec]

comment:3

How can I do that?!

At least it works in cypari2 with commit 88bf24e.

[jdemeyer]

Changed branch from u/vdelecroix/22198 to u/jdemeyer/22198

[jdemeyer]

Changed commit from ff6a3fd to 683816d

[jdemeyer]

comment:5

This is a much simpler version, relying on Cython to convert str -> bytes.

---

New commits:

683816d

22198: do not depend on PyString_AsString

[jdemeyer]

comment:6

Note: Cython converts Unicode strings to bytes by using sys.getdefaultencoding() which seems like a reasonable thing to do. In Python 2, this encoding is ASCII by default, in Python 3 it is UTF-8 by default.

[videlec]

comment:7

For me it compiles but does not work in Python 3

>>> import cypari2
>>> pari = cypari2.Pari()
>>> pari("zeta(2)")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cypari2/pari_instance.pyx", line 831, in cypari2.pari_instance.Pari.__call__ (cypari2/pari_instance.c:13408)
  File "cypari2/gen.pyx", line 4534, in cypari2.gen.objtogen (cypari2/gen.c:128337)
TypeError: expected bytes, str found

[videlec]

comment:8

Actually, in Cython for Python 3 given a string s the conversion

<bytes> s

does not do anything!! (tested with Cython version 0.25.2)

[videlec]

comment:9

In your patch, bytes do not get catch in Python 3.

[jdemeyer]

comment:10

<bytes>s is not a conversion, it's a cast! So yes, it doesn't "do" anything and that's a feature.

[videlec]

comment:11

With your patch, unicode are also not treated correctly (see [comment:7]).

[videlec]

comment:12

And conversion does fail

Python 3.5.2
>>> bytes("hello")
Traceback (most recent call last):
...
TypeError: string argument without an encoding

I don't understand where the default encoding that you mentioned in [comment:6] is intended to happen.

EDIT: this command fails in the same way in Cython

[jdemeyer]

comment:13

The default encoding is something Cython-specific when passing unicode or bytes to C as a char*. It's not about converting within Python from unicode to bytes.

[videlec]

comment:14

Replying to @jdemeyer:

The default encoding is something Cython-specific when passing unicode or bytes to C as a char*. It's not about converting within Python from unicode to bytes.

I know and I already mentioned it in [comment:12]: the code behaves the same when compiled in Cython. You can check with

Python 3.5.2
>>> import cython
>>> cython.inline('bytes("hello")')
Traceback (most recent call last):
...
TypeError: string argument without an encoding

[jdemeyer]

comment:15

Replying to @videlec:

I know and I already mentioned it in [comment:12]: the code behaves the same when compiled in Cython.

Yes, and I already mentioned that it's not about bytes(foo), it's about passing foo as a char* to C.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

819ed9e

Trac #22198: do not depend on PyString_AsString

[sagetrac-git]

Changed commit from 683816d to 819ed9e

[jdemeyer]

comment:18

Does this work?

[videlec]

comment:19

I don't think so: bytes and basestring are disjoint in Python 3. Let me try.

[videlec]

comment:20

Actually it fails at compilation

ImportError: Building module cython_string failed: ['LookupError: unknown encoding: default\n']

The encoding used seems to be the encoding of the file, not of the system. Adding # encoding: utf-8 at the top of the file make it works.

[videlec]

Attachment: cython_string.pyx.gz

[videlec]

comment:21

Replying to @videlec:

I don't think so: bytes and basestring are disjoint in Python 3. Let me try.

Indeed

Python 3.5.2
>>> import cypari2
>>> pari = cypari2.Pari()
>>> pari(b"zeta(2)")
Traceback (most recent call last):
...
PariError: syntax error, unexpected variable name, expecting $end

[videlec]

Changed branch from u/jdemeyer/22198 to u/vdelecroix/22198

[videlec]

Changed commit from 819ed9e to ff6a3fd

[videlec]

New commits:

ff6a3fd

22198: do not depend on PyString_AsString

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

819ed9e	Trac #22198: do not depend on PyString_AsString
6c55c78	Trac #22198: fix bytes for Python3

[sagetrac-git]

Changed commit from ff6a3fd to 6c55c78

[videlec]

comment:24

please see my last commit 6c55c78 that does work in the standalone cypari.

[videlec]

Reviewer: Jeroen Demeyer, Vincent Delecroix

[videlec]

Changed author from Vincent Delecroix to Vincent Delecroix, Jeroen Demeyer

[jdemeyer]

Changed branch from u/vdelecroix/22198 to u/jdemeyer/22198

[jdemeyer]

Changed commit from 6c55c78 to 7096c82

[jdemeyer]

comment:27

Does this work too? It's a slightly more efficient version of your code.

---

New commits:

7096c82

Trac #22198: fix bytes for Python3

[videlec]

comment:28

It does.

[jdemeyer]

comment:29

Positive review then?

[vbraun]

Changed branch from u/jdemeyer/22198 to 7096c82