8000 SQLAlchemy 1.4: Fix building the C extension on Python 3.13 by musicinmybrain · Pull Request #11500 · sqlalchemy/sqlalchemy · GitHub
[go: up one dir, main page]
Skip to content

SQLAlchemy 1.4: Fix building the C extension on Python 3.13 #11500

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

SQLAlchemy 1.4: Fix building the C extension on Python 3.13 #11500

wants to merge 1 commit into from

Conversation

musicinmybrain
Copy link

Description

The C extension for SQLAlchemy 1.4 uses _PyArg_NoKeywords(), an undocumented private API that was moved to the pycore_modsupport.h internal C API in Python 3.13.

As a result, the C extension fails to build on Python 3.13:

  lib/sqlalchemy/cextension/resultproxy.c: In function ‘tuplegetter_new’:
  lib/sqlalchemy/cextension/resultproxy.c:831:10: error: implicit declaration of function ‘_PyArg_NoKeywords’ [-Wimplicit-function-declaration]
    831 |     if (!_PyArg_NoKeywords("tuplegetter", kwds))
        |          ^~~~~~~~~~~~~~~~~

This was reported as #11499.

This PR proposes to fix that by adding a static function wrapper that calls _PyArg_NoKeywords on Python 3.12 and older, and imitates its implementation on Python 3.13 and later.

Checklist

This pull request is:

  • A documentation / typographical / small typing error fix
    • Good to go, no issue or tests are needed
  • A short code fix
    • please include the issue number, and create an issue if none exists, which
      must include a complete example of the issue. one line code fixes without an
      issue and demonstration will not be accepted.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests. one line code fixes without tests will not be accepted.
  • A new feature implementation
    • please include the issue number, and create an issue if none exists, which must
      include a complete example of how the feature would look.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests.
8000

Have a nice day!

Compilation can be easily tested in a virtualenv:

$ python3.13 -m venv _e
(_e) $ pip install build
(_e) $ REQUIRE_SQLALCHEMY_CEXT=1 python -m build

Actually running the test suite is a bit harder, since the version of greenlet on PyPI still doesn’t build on Python 3.13.

I tested this as a patch for the python-sqlalchemy1.4 compat package in Fedora Rawhide and found that the C extension compiled and almost all of the tests passed. There were a few failures I haven’t investigated closely yet,

FAILED test/orm/test_mapper.py::MapperTest::test_synonym_nonexistent_attr - A...
FAILED test/ext/test_serializer.py::ColumnPropertyWParamTest::test_deserailize_colprop
FAILED test/ext/test_serializer.py::SerializeTest::test_aliases - AttributeEr...
FAILED test/ext/test_serializer.py::SerializeTest::test_annotated_one - Attri...
FAILED test/ext/test_serializer.py::SerializeTest::test_any - AttributeError:...
FAILED test/ext/test_serializer.py::SerializeTest::test_attribute - Attribute...
FAILED test/ext/test_serializer.py::SerializeTest::test_columns - AttributeEr...
FAILED test/ext/test_serializer.py::SerializeTest::test_expression - Attribut...
FAILED test/ext/test_serializer.py::SerializeTest::test_mapper - AttributeErr...
FAILED test/ext/test_serializer.py::SerializeTest::test_orm_join - AttributeE...
FAILED test/ext/test_serializer.py::SerializeTest::test_query_one - Attribute...
FAILED test/ext/test_serializer.py::SerializeTest::test_query_three - Attribu...
FAILED test/ext/test_serializer.py::SerializeTest::test_query_two - Attribute...
FAILED test/ext/test_serializer.py::SerializeTest::test_tables - AttributeErr...
FAILED test/ext/test_serializer.py::SerializeTest::test_unicode - AttributeEr...
========= 15 failed, 15198 passed, 2407 skipped in 1336.48s (0:22:16) ==========

with tracebacks like

  File "/builddir/build/BUILD/python-sqlalchemy1.4-1.4.52-build/SQLAlchemy-1.4.52/lib/sqlalchemy/ext/serializer.py", line 174, in dumps
    pickler = Serializer(buf, protocol)
  File "/builddir/build/BUILD/python-sqlalchemy1.4-1.4.52-build/SQLAlchemy-1.4.52/lib/sqlalchemy/ext/serializer.py", line 113, in Serializer
    pickler.persistent_id = persistent_id
    ^^^^^^^^^^^^^^^^^^^^^
AttributeError: '_pickle.Pickler' object attribute 'persistent_id' is read-only

but these also failed when the C extension was not built, so they must represent a separate issue unrelated to this PR.

@musicinmybrain
Copy link
Author 
  File "/builddir/build/BUILD/python-sqlalchemy1.4-1.4.52-build/SQLAlchemy-1.4.52/lib/sqlalchemy/ext/serializer.py", line 174, in dumps
    pickler = Serializer(buf, protocol)
  File "/builddir/build/BUILD/python-sqlalchemy1.4-1.4.52-build/SQLAlchemy-1.4.52/lib/sqlalchemy/ext/serializer.py", line 113, in Serializer
    pickler.persistent_id = persistent_id
    ^^^^^^^^^^^^^^^^^^^^^
AttributeError: '_pickle.Pickler' object attribute 'persistent_id' is read-only

It looks like this requires a backport of (at least part of) 7548046.

@zzzeek
Copy link
Member
zzzeek commented Jun 16, 2024

oh, well yah it looks like i wasnt targeting 1.4 for 3.13 at all, so yes based on what i said above we have to backport all of that to 1.4 as well

@musicinmybrain
Copy link
Author

oh, well yah it looks like i wasnt targeting 1.4 for 3.13 at all, so yes based on what i said above we have to backport all of that to 1.4 as well

I’m not prepared to backport all the tox fixes, but in case it helps, I backported just the code fixes in musicinmybrain@5e7446a, and I can confirm that this sufficed to get all the tests passing on Python 3.13 in the Fedora package.

@zzzeek
Copy link
Member
zzzeek commented Jun 16, 2024

OK for the backport of that other stuff, me or @CaselIT should make separate gerrits for that on our end, move the changelog, and do it like that

@zzzeek
Copy link
Member
zzzeek commented Jun 16, 2024

this is ongoing at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5342 , also adding greenlet test for py313 in https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5343

@zzzeek zzzeek requested a review from sqla-tester June 20, 2024 15:02
sqla-tester
sqla-tester reviewed Jun 20, 2024
View reviewed changes
Copy link
Collaborator
@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 8a5888b of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change 8a5888b: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5339

@sqla-tester
Copy link
Collaborator

Michael Bayer (zzzeek) wrote:

recheck

limited py313 builds to sqlite only

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5339

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5339 has been merged. Congratulations! :)

sqlalchemy-bot pushed a commit that referenced this pull request Jun 22, 2024
@musicinmybrain @zzzeek
SQLAlchemy 1.4: Fix building the C extension on Python 3.13
fc66fda 
Adjustments to the C extensions, which are specific to the SQLAlchemy 1.x
series, to work under Python 3.13.  Pull request courtesy Ben Beasley.

Fixes: #11499
Closes: #11500
Pull-request: #11500
Pull-request-sha: 8a5888b

Change-Id: I1943eb387f9b075bf07e179f7a24762236e234bf
@CaselIT
Copy link
Member
CaselIT commented Aug 1, 2024

this can be closed, since it was merged

@CaselIT CaselIT closed this Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sqla-tester sqla-tester sqla-tester left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@musicinmybrain @zzzeek @sqla-tester @CaselIT
0