Add support for ctypes to python3's recipe#1463
Conversation
Should be mentioned that the current test app for python3 has been modified by adding libffi to the requirements because the ui for the app has some button to test the ctypes module. References: This commit partially fixes some of the points mentioned in issue number kivy#1455
KeyWeeUsr
left a comment
KeyWeeUsr
left a comment
There was a problem hiding this comment.
Seems quite clean, looks good. I presume you've tested the final APK on your device? :)
Kind of nitpicking here, but if I were a linter, I'd brag about https://github.com/kivy/python-for-android/pull/1463/files#diff-62af0574da035ee296803d8d5f97ef79R76 :D
Yes 😉 |
|
Ah, so it does brag after all 😄 https://travis-ci.org/kivy/python-for-android/jobs/456426763#L526 |
AndreMiras
left a comment
AndreMiras
left a comment
There was a problem hiding this comment.
Also looks good to me. Still I'd like @inclement to take a look since he worked on python3 recently.
In the meantime do you mind pleasing the linter (from Travis). The error was:
pythonforandroid/recipes/python3/__init__.py:76:36: E127 continuation line over-indented for visual indent
For info you can run it locally with tox.
|
One of the tests is failing, I suspect that affected by issue #1454 |
inclement
left a comment
inclement
left a comment
There was a problem hiding this comment.
Seems great, thanks, just some minor comments.
Would you mind also adding a brief note about this to the documentation, in the build-options page under python3? Just something like "ctypes is not included automatically, if you would like to use it then add libffi to your requirements, e.g. --requirements=kivy,libffi,python3.".
| ArrayList<String> libsList = new ArrayList<String>(); | ||
| addLibraryIfExists(libsList, "crystax", libsDir); | ||
| addLibraryIfExists(libsList, "sqlite3", libsDir); | ||
| addLibraryIfExists(libsList, "ffi", libsDir); |
There was a problem hiding this comment.
I suppose we need a more automatic way to add library loads here, unless - is it essential to load this library in this way, or does dynamic access work?
That doesn't need to hold up this PR though.
There was a problem hiding this comment.
I tested without these line and dynamic access seems to work, thanks! :)
I correct myself ( I suppose that I did not update the test app on my device when removed those lines... 😅), another test with a clean branch and without modifying these line lead me to ctypes import error, so...I think these line is required because the ctypes module gets build in python modules...plus seems logical that we need these line, as we do with sqlite3 and openssl (cause are external dependencies for python)...anyway...I'm doing another test re-adding these lines again...which should make it work...
Update:
The test application successfully import ctypes with these line...crash without it.
4 participants
Should be mentioned that the current test app for python3 has been modified by adding libffi to the requirements because the ui for the app has some button to test the ctypes module.
References: This commit partially fixes some of the points mentioned in issue number #1455