.pxd files should not use PY_MAJOR_VERSION compile-time variable

[jdemeyer]

sage: cython('''from sage.cpython.string cimport str_to_bytes''')
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-1-217de50fbb18> in <module>()
----> 1 cython('''from sage.cpython.string cimport str_to_bytes''')

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3756)()
    352             True
    353         """
--> 354         return self.get_object()(*args, **kwds)
    355 
    356     def __repr__(self):

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython_compile(code, **kwds)
   1009     with open(tmpfile,'w') as f:
   1010         f.write(code)
-> 1011     return cython_import_all(tmpfile, get_globals(), **kwds)

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython_import_all(filename, globals, **kwds)
    899       code
    900     """
--> 901     m = cython_import(filename, **kwds)
    902     for k, x in iteritems(m.__dict__):
    903         if k[0] != '_':

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython_import(filename, **kwds)
    874     - the module that contains the compiled Cython code.
    875     """
--> 876     name, build_dir = cython(filename, **kwds)
    877 
    878     oldpath = sys.path

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/misc/cython.pyc in cython(filename, verbose, compile_message, use_cache, create_local_c_file, annotate, sage_namespace, create_local_so_file)                                                                                                                                        
    636                         You can fix your code by adding "from {} cimport {}".
    637                         """.format(pxd, name))
--> 638         raise RuntimeError(cython_messages.strip())
    639 
    640     if verbose >= 0:

RuntimeError: Error compiling Cython file:
------------------------------------------------------------
...
    # Missing from cpython.unicode in Cython 0.27.3
    char* PyUnicode_AsUTF8(object s)


cdef inline str char_to_str(const char* c, encoding=None, errors=None):
    IF PY_MAJOR_VERSION <= 2:
      ^
------------------------------------------------------------

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/cpython/string.pxd:25:7: Compile-time name 'PY_MAJOR_VERSION' not defined

Error compiling Cython file:
------------------------------------------------------------
...
        TypeError: expected bytes, list found
    """
    if not isinstance(b, bytes):
        raise TypeError(f"expected bytes, {type(b).__name__} found")

    IF PY_MAJOR_VERSION <= 2:
      ^
------------------------------------------------------------

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/cpython/string.pxd:70:7: Compile-time name 'PY_MAJOR_VERSION' not defined

Error compiling Cython file:
------------------------------------------------------------
...
        TypeError: expected str ... list found
    """
    cdef const char* err
    cdef const char* enc

    IF PY_MAJOR_VERSION <= 2:
      ^
------------------------------------------------------------

/home/jdemeyer/sage/local/lib/python2.7/site-packages/sage/cpython/string.pxd:106:7: Compile-time name 'PY_MAJOR_VERSION' not defined


Error compiling Cython file:
------------------------------------------------------------
...
from sage.cpython.string cimport str_to_bytes^
------------------------------------------------------------

_home_jdemeyer__sage_temp_sage4_21832_tmp_mx5xCy_pyx_0.pyx:1:0: 'sage/cpython/string/str_to_bytes.pxd' not found

Since the Cython code is almost C anyway, the easiest solution is probably to implement it actually in C (replacing the IF by #if).

CC: @simon-king-jena @embray

Component: cython

Author: Jeroen Demeyer

Branch: 85451ee

Reviewer: Simon King

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

[jdemeyer]

Branch: u/jdemeyer/_pxd_files_should_not_use_py_major_version_compile_time_variable

[jdemeyer]

New commits:

85451ee

Implement string conversion in C

[jdemeyer]

Commit: 85451ee

[jdemeyer]

Author: Jeroen Demeyer

[simon-king-jena]

comment:4

What is the rationale for this:

 cpdef inline str bytes_to_str(b, encoding=None, errors=None):
@@ -64,14 +51,13 @@ cpdef inline str bytes_to_str(b, encoding=None, errors=None):
         ...
         TypeError: expected bytes, list found
     """
-    if not isinstance(b, bytes):
+    if type(b) is not bytes:
         raise TypeError(f"expected bytes, {type(b).__name__} found")

That's to say, why don't you allow sub-types of bytes?

[jdemeyer]

comment:5

Replying to @simon-king-jena:

That's to say, why don't you allow sub-types of bytes?

Mainly to be on the safe side. I have no idea what the C API does with subclasses of bytes.

Whenever a real use case for bytes subclasses comes up, we can always revisit this.

[simon-king-jena]

Reviewer: Simon King

[simon-king-jena]

comment:6

Replying to @jdemeyer:

Replying to @simon-king-jena:

That's to say, why don't you allow sub-types of bytes?

Mainly to be on the safe side. I have no idea what the C API does with subclasses of bytes.

Whenever a real use case for bytes subclasses comes up, we can always revisit this.

OK.

Apart from that, the code looks fine to me, all tests pass, and you added a test that shows that the bug is fixed.

[vbraun]

Changed branch from u/jdemeyer/_pxd_files_should_not_use_py_major_version_compile_time_variable to 85451ee

[embray]

Changed commit from 85451ee to none

[embray]

comment:8

I guess I don't mind, but what even is the use case for this:

cython('''from sage.cpython.string cimport str_to_bytes''')

?

IMO str_to_bytes() is just a convenience utility for use within Sage, and not otherwise. It's very easy to implement similar wrappers for one's own purposes.

[embray]

comment:9

Or for that matter, what's wrong with just documenting "so and so requires the following compile-time variables to be defined"?

[embray]

comment:10

On the third hand, I think it's common enough, especially writing code that is Python 2/3-compatible, to want to have the Python version as a compile-time variable, so maybe Cython really ought to provide this by default...

[jdemeyer]

comment:11

Replying to @embray:

I guess I don't mind, but what even is the use case for this:

cython('''from sage.cpython.string cimport str_to_bytes''')

Third-party code already using Sage that wants to be Python 2/3 compatible.

[simon-king-jena]

comment:12

Replying to @embray:

On the third hand, I think it's common enough, especially writing code that is Python 2/3-compatible, to want to have the Python version as a compile-time variable, so maybe Cython really ought to provide this by default...

This may be. However, Cython currently doesn't provide it.

In fact I did try to use from sage.cpython.string cimport str_to_bytes in a third-party package for Sage. So, the reason for this ticket was an actual use case.

[embray]

comment:13

That's fine. It just sucks that in order to write such code in a pxd file it's...basically impossible unless you move it into a pure C file.

Wild-haired idea: allow some directives in Cython files to provide compile-time variables, with short eval-able expressions providing their values, perhaps with a few default imports provided. So one can write # cython: <something something> PY_MAJOR_VERSION=sys.version_info[0] and have that variable provided to the Cython compiler automatically.

Not sure how that would work when compiling modules with functions imported from .pxd files, if it should even work at all.

[jdemeyer]

comment:14

Replying to @embray:

On the third hand, I think it's common enough, especially writing code that is Python 2/3-compatible, to want to have the Python version as a compile-time variable

I'm not entirely convinced here. In most cases, you can write Python 2/3 compatible Cython code without needing the PY_MAJOR_VERSION compile-time variable.

[embray]

comment:15

True, but I think this demonstrates that there's a case for it. You can also write such code with normal if statements as well, but I think it goes without saying that it's better to compile away such branches if possible.