Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Fix building on 3.13 #411

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

WIP: Fix building on 3.13 #411

wants to merge 7 commits into from

Conversation

dkropachev
Copy link
Collaborator

@dkropachev dkropachev commented Jan 23, 2025

Fixes two problems:

  1. PyEval_InitThreads got depricated and does not exists in python 3.13:
building 'cassandra.io.libevwrapper' extension
gcc -fno-strict-overflow -Wsign-compare -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DNDEBUG -fcf-protection -fexceptions -fcf-protection -fexceptions -fcf-protection -fexceptions -O3 -fPIC -I/usr/include/libev -I/usr/local/include -I/opt/local/include -I/usr/include/python3.13 -c cassandra/io/libevwrapper.c -o build/temp.linux-x86_64-cpython-313/cassandra/io/libevwrapper.o
cassandra/io/libevwrapper.c: In function ‘PyInit_libevwrapper’:
cassandra/io/libevwrapper.c:668:10: error: implicit declaration of function ‘PyEval_ThreadsInitialized’ [-Wimplicit-function-declaration]
  668 |     if (!PyEval_ThreadsInitialized()) {
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~
cassandra/io/libevwrapper.c:669:9: warning: ‘PyEval_InitThreads’ is deprecated [-Wdeprecated-declarations]
  669 |         PyEval_InitThreads();
      |         ^~~~~~~~~~~~~~~~~~
In file included from /usr/include/python3.13/Python.h:124,
                 from cassandra/io/libevwrapper.c:1:
/usr/include/python3.13/ceval.h:114:37: note: declared here
  114 | Py_DEPRECATED(3.9) PyAPI_FUNC(void) PyEval_InitThreads(void);
      |                                     ^~~~~~~~~~~~~~~~~~
command '/usr/lib64/ccache/gcc' failed with exit code 1
/home/avi/python-driver/setup.py:291: UserWarning: 
  1. Cython fails to convert unicode to 'str':
Error compiling Cython file:
------------------------------------------------------------
...
if metadata_request_timeout is None:
    return stmt
ms = int(metadata_request_timeout / datetime.timedelta(milliseconds=1))
if ms == 0:
    return stmt
return f"{stmt} USING TIMEOUT {ms}ms"
               ^
------------------------------------------------------------
    
cassandra/util.py:1813:11: Cannot convert Unicode string to 'str' implicitly. This is not portable and requires explicit encoding.

Fixes: #409

Tested

Localy on fedora:41 image

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@dkropachev dkropachev requested review from fruch and Lorak-mmk January 23, 2025 00:59
@dkropachev dkropachev force-pushed the dk/fix-3.13-building branch from 0754abc to 4003ded Compare January 23, 2025 01:00
@dkropachev dkropachev changed the title Dk/fix 3.13 building Fix building on 3.13 Jan 23, 2025
@dkropachev dkropachev marked this pull request as ready for review January 23, 2025 04:21
cassandra/io/libevwrapper.c Outdated Show resolved Hide resolved
cassandra/util.py Outdated Show resolved Hide resolved
@fruch
Copy link

fruch commented Jan 23, 2025

I think something like: #412
would help catching those, and not getting to release with those working as expected

ontop of it some testing should be added to make sure.

@dkropachev dkropachev force-pushed the dk/fix-3.13-building branch from 4003ded to 9a8b4dd Compare January 23, 2025 11:16
dkropachev and others added 3 commits January 23, 2025 07:46
PyEval_InitThreads is depricated, since 3.7 it is not necessary to call
it.
Ref: https://docs.python.org/3/c-api/init.html#c.PyEval_InitThreads
Otherwise it fails to convert unicode to 'str':
```
Error compiling Cython file:
------------------------------------------------------------
...
    if metadata_request_timeout is None:
        return stmt
    ms = int(metadata_request_timeout / datetime.timedelta(milliseconds=1))
    if ms == 0:
        return stmt
    return f"{stmt} USING TIMEOUT {ms}ms"
           ^
------------------------------------------------------------

cassandra/util.py:1813:11: Cannot convert Unicode string to 'str' implicitly. This is not portable and requires explicit encoding.
```
we run running again and again into situation our wheels are missing
some of the part we expect cause of extensions silently failing

with this change we can stop the builds cause of that, and attend to it
@dkropachev dkropachev force-pushed the dk/fix-3.13-building branch from 9a8b4dd to 4ac9bf0 Compare January 23, 2025 11:46
@dkropachev
Copy link
Collaborator Author

I think something like: #412 would help catching those, and not getting to release with those working as expected

ontop of it some testing should be added to make sure.

Thanks, I have cherry-picked it.

@dkropachev dkropachev force-pushed the dk/fix-3.13-building branch from f578fb0 to 0c39303 Compare January 23, 2025 13:42
- name: Install cython
run: |
python -m pip install 'Cython>=3.0.11,<4'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we install cython on scylladb frozen toolchain? Or will install-dependencies.sh pick up a pre-built package and so not require a compilation during installation?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If pip would pick a correctly built wheel

You don't need todo anything on your end.

Also installing other version of cython won't help, it would pick the version mentioned in this line to build.

Its exactly what we were missing for quite some time, hence building with very old version

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take one thing back, I'm not sure why this specific one was added, I was referring to the referring cython in setup.py

Copy link
Collaborator Author

@dkropachev dkropachev Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wait, it is still in progress.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this would help:

9a008b0

switching to build with build tool

let's see how CI in my PR works with it

@dkropachev dkropachev changed the title Fix building on 3.13 WIP: Fix building on 3.13 Jan 23, 2025
Comment on lines 70 to 76
python -m pip install Cython

- name: Install setuptools
run: |
python -m pip install setuptools --upgrade

- name: Install wheel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you adding those ? they have zero affect inside cibuildwheel that works inside docker (and it's own venv per python)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still in progress, I am working on to find out why it does not recognize cython module for python >=3.12

There is a bug in distutils that does not allow it to pick up cython for
python 3.12 and later.
@dkropachev dkropachev force-pushed the dk/fix-3.13-building branch from 0c39303 to 5791dc8 Compare January 23, 2025 15:35
@dkropachev dkropachev force-pushed the dk/fix-3.13-building branch from fec5d0d to cf9abe8 Compare January 23, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libev integration doesn't compile on Python 3.13 / Fedora 41
4 participants