use ctypes.pythonapi in flush_mro_cache - #69
Open
ejd wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are you trying to accomplish?
I would like to be able to use this library on Android via Chaquopy. When I tried using a "stock" build of this library on Android, my program crashed in
flush_mro_cachebecause the value returned byc.PyDLL(None)didn't have aPyType_Modifiedsymbol.Here's a traceback to show where the error was raised within
backports.datetime_fromisoformat:I'm using backports.datetime_fromisoformat 2.0.3 under Python 3.8.20.
I'm not entirely sure why this doesn't work on Android. I'm assuming it's related to how Python is configured/built for Android. The copy of Python that the Chaquopy Gradle plugin bundles with my library is built and distributed by the Chaquopy project. If you feel this PR is misdirected and reject it, my next stop will be to ask Chaquopy why this project's
flush_mro_cachedoesn't work on Android. That said, I think there's value in eliminating platform-specific code paths frombackports.datetime_fromisoformat.What approach did you choose and why?
In the current implementation,
flush_mro_cacheobtains a C Python API handle by callingctypes.PyDLL()with the appropriate arguments for a given platform. The object returned should be a handle to a the C Python API loaded from elsewhere. However, at least on Android, this handle seems to be missing a symbol forPyType_Modified.I found that
ctypeshas apythonapi. The docs introduce it as, "For accessing the C Python api directly, a ready-to-use Python shared library object is available: ctypes.pythonapi." In this PR, I replaced the use of a runtime-loadedPyDLLwithctypes.pythonapi.What should reviewers focus on?
I believe the comment in
flush_mro_cachelinking to the SO post is still relevant so I left it in place.The impact of these changes
This change eliminates the need to have platform-specific execution paths. It should work anywhere
ctypesis available.Testing
The project's test suite passed when I ran the tests locally.