Properly search native lib dir in ctypes, fixes #1770 · Pull Request #1772 · kivy/python-for-android

ghost · 2019-03-25T14:22:01Z

This will change ctypes.util.find_library() to properly query & search native lib dir, which should fix #1770

Ran fine for me but I tested with an ARMv7 app. I asked the original reporter for an ARM64 test

ghost · 2019-03-28T19:58:20Z

This was now tested by me for ARMv7/python3 with a larger SDL2 app, and by the original bug reporter for ARM64 as confirmed here: #1770 (comment)

Based on this I am confident to recommend an inclusion at the very least based on functionality, of course code quality remains to be judged 😄 happy for any sort of feedback!

pythonforandroid/recipes/python3/patches/fix-ctypes-util-find-library.patch

ghost · 2019-03-30T21:49:50Z

For what it's worth I tested the regex version and it appears to work fine! I also tested the regex itself on various test patterns on command line, I just wasn't sure how to ship a test for it in a non-awkward way but if you have an idea I'd be happy to do so. (Otherwise, at least take this as some sort of indication that the regex pattern isn't complete untested bollocks 😂 )

AndreMiras · 2019-03-30T22:05:21Z

Yes I was wondering the same for the unit tests actually. It's pretty annoying because the function is probably simple enough to test, but it's in a patch file which makes it not easy testable.
So let's maybe just add some code comments explaining what we're expecting to match and trying to do in general in this function

ghost · 2019-03-30T23:32:50Z

Okay, added a comment & tested & deployed it again to check that the patch still works!

pythonforandroid/recipes/python3/patches/fix-ctypes-util-find-library.patch

AndreMiras · 2019-03-31T00:58:05Z

Thanks for you efforts! I'll try to test it on Sunday and merge if it doesn't introduce regression on armv7

AndreMiras · 2019-03-31T17:09:23Z

Ouch, I've just tested it and I found a regression with services.

03-31 19:00:34.400 29223 29239 I service :  Traceback (most recent call last):
03-31 19:00:34.400 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/app/service/main.py", line 25, in <module>
03-31 19:00:34.401 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/pyetheroll/etheroll.py", line 9, in <module>
03-31 19:00:34.402 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/eth_account/__init__.py", line 1, in <module>
03-31 19:00:34.402 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/eth_account/account.py", line 10, in <module>
03-31 19:00:34.403 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/eth_keyfile/__init__.py", line 7, in <module>
03-31 19:00:34.403 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/eth_keyfile/keyfile.py", line 6, in <module>
03-31 19:00:34.404 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/Crypto/Cipher/__init__.py", line 78, in <module>
03-31 19:00:34.405 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/Crypto/Cipher/_mode_ecb.py", line 46, in <module>
03-31 19:00:34.406 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/Crypto/Util/_raw_api.py", line 168, in load_pycryptodome_raw_lib
03-31 19:00:34.406 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/Crypto/Util/_raw_api.py", line 51, in load_lib
03-31 19:00:34.407 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/cffi/api.py", line 141, in dlopen
03-31 19:00:34.408 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/cffi/api.py", line 802, in _make_ffi_library
03-31 19:00:34.408 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/cffi/api.py", line 788, in _load_backend_lib
03-31 19:00:34.409 29223 29239 I service :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/other_builds/python3-libffi-openssl-sqlite3/armeabi-v7a__ndk_target_21/python3/Lib/ctypes/ut
il.py", line 78, in find_library
03-31 19:00:34.409 29223 29239 I service :  AttributeError: 'NoneType' object has no attribute 'getApplicationContext'

ghost · 2019-03-31T17:37:49Z

Oh yeah, that's bad @ services!

As I said in chat I'll follow your suggestion of moving it into the android module and add tests, and figure out how to make it work with the other bootstraps. (which should probably work fine with a simple check if the autoclass worked and trying all the posibilities or something) Stay tuned for an update 😄

ghost · 2019-04-01T14:52:20Z

@AndreMiras done!! works for me so far, can you check out whether it works for you?

tests/test_androidmodule_ctypes_finder.py

AndreMiras · 2019-04-03T21:07:19Z

tests/test_androidmodule_ctypes_finder.py

+        activity = MagicMock()
+        activity.getApplicationContext().getPackageName = (
+             lambda: "com.example.testpackage"
+        )


it's fine to use lambda, but mock also has a built'in feature to do what you're doing here:

activity.getApplicationContext().getPackageName.return_value = "com.example.testpackage"

oh neat! don't think it'd be actually shorter here though, so should I change it? not sure if lambda is considered ugly here 😬 I really haven't seen much mock'ing code

Lambda is completely fine 👍

Anyway with the magic of MagicMock you don't have to define this. So you could skip these lines

it's skipped now in the revamped test except in the one where I actually check for it

AndreMiras · 2019-04-03T22:01:01Z

tests/test_androidmodule_ctypes_finder.py

+            return activity_class
+        elif name == "android.content.pm.PackageManager":
+            manager_class = MagicMock()
+            manager_class.GET_SHARED_LIBRARY_FILES = 1024


mocking explicitly is like documenting. Would the test fail if you remove that line? If not then it means it's not mandatory to prove the point of the interface you're unit testing

I added a check for this now in the updated test (the only part still involving a small if 😬 lol)

tests/test_androidmodule_ctypes_finder.py

AndreMiras · 2019-04-06T19:14:37Z

tests/test_androidmodule_ctypes_finder.py

+import sys
+import tempfile
+
+sys.modules["jnius"] = MagicMock()  # fake jnius used later


I promise this is my last wish, but could you remove this line and patch it explicitly when we need it. At at method level with the decorator, e.g. @mock.patch.dict('sys.modules', jnius=MagicMock()) 🙏
I really prefer when things are as local as possible

tests/test_androidmodule_ctypes_finder.py

AndreMiras

Thanks for another great contribution!
Tested on armv7 and it still works good, merging

ghost mentioned this pull request Mar 25, 2019

Exception calling PythonActivity.getWindow().setSoftInputMode(...) kivy/pyjnius#413

Closed

ghost changed the title ~~[WIP] Properly search native lib dir in ctypes, fixes #1770~~ Properly search native lib dir in ctypes, fixes #1770 Mar 28, 2019

ghost requested a review from inclement March 28, 2019 19:58

AndreMiras reviewed Mar 30, 2019

View reviewed changes