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

ctypes_patch breaks with Python 3.13.0a6 #444

Closed
freakboy3742 opened this issue Apr 15, 2024 · 2 comments · Fixed by #446
Closed

ctypes_patch breaks with Python 3.13.0a6 #444

freakboy3742 opened this issue Apr 15, 2024 · 2 comments · Fixed by #446
Labels
bug A crash or error in behavior.

Comments

@freakboy3742
Copy link
Member

Describe the bug

As of Python3.13.0a6, the ctypes patch used to allow struct return values no longer works.

Steps to reproduce

Run the test suite on Python 3.13.0a6.

There will be 7 test failures, all of the form:

Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/unittest/case.py", line 58, in testPartExecutor
    yield
  File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/unittest/case.py", line 651, in run
    self._callTestMethod(testMethod)
    ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/unittest/case.py", line 606, in _callTestMethod
    if method() is not None:
       ~~~~~~^^
  File "/Users/runner/work/rubicon-objc/rubicon-objc/tests/test_ctypes_patch.py", line 96, in test_patch_idempotent
    ctypes_patch.make_callback_returnable(TestStruct)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
  File "/Users/runner/work/rubicon-objc/rubicon-objc/.tox/py/lib/python3.13/site-packages/rubicon/objc/ctypes_patch.py", line 197, in make_callback_returnable
    stgdict = get_stgdict_of_type(ctype)
              ~~~~~~~~~~~~~~~~~~~^^^^^^^
  File "/Users/runner/work/rubicon-objc/rubicon-objc/.tox/py/lib/python3.13/site-packages/rubicon/objc/ctypes_patch.py", line 175, in get_stgdict_of_type
    raise TypeError(
        f"The given type's dict must be a StgDict, not {type(stgdict).__module__}.{type(stgdict).__qualname__}"
    )
TypeError: The given type's dict must be a StgDict, not builtins.dict

Expected behavior

Test suite should pass.

Screenshots

No response

Environment

  • Operating System: macOS
  • Python version: 3.13.0a6

Logs

No response

Additional context

Test suite passes with 3.13.0a5.

@freakboy3742 freakboy3742 added the bug A crash or error in behavior. label Apr 15, 2024
@dgelessus
Copy link
Collaborator

Ah, my awful hack finally broke! I'm surprised it didn't happen sooner...

The relevant CPython commit appears to be python/cpython@dcaf33a, which removes the StgDict class and replaces all uses of it with regular dict objects. The extra C data previously stored in StgDicts is now stored as part of the regular dicts, using a new standard mechanism added in CPython 3.12.

Thankfully, the fields we care about are still structured almost identically as before - only the location where they are stored has changed. So as long as we can figure out the new location of the data, it should be easy to adjust the patch to work with both the old and new ways.

The way to access the new attached data is using the PyStgInfo_FromType function from _ctypes/ctypes.h. Unfortunately, that function is static inline, so we can't call it directly. Instead, ctypes_patch would have to reimplement the relevant code. Thankfully, it doesn't look too complex.

That said, this would also be a good opportunity to upstream our patch into CPython itself, so it would be properly supported and maintained. Then we could stop doing our risky runtime patching... There is already an open CPython issue for this since 2009 (python/cpython#49960, formerly known as bpo-5710), so clearly others are interested in this too. I don't remember why we didn't try upstreaming this patch earlier - though most likely there is no particular reason and just nobody had the time for it.

@freakboy3742
Copy link
Member Author

I took a quick poke after I logged this bug, and came to about the same conclusion as you have.

The upstream bug has been on my "I really should do something about that" list for a while. I agree that this would be an ideal opportunity to fix this bug upstream - finally removing this compatibility shim would be awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A crash or error in behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants