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.

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

Properly search native lib dir in ctypes, fixes #1770 #1772

Merged
merged 1 commit into from Apr 6, 2019
Merged

Properly search native lib dir in ctypes, fixes #1770 #1772

merged 1 commit into from Apr 6, 2019

Conversation

ghost
Copy link

@ghost ghost commented Mar 25, 2019 •
edited by ghost
Loading

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 ghost mentioned this pull request Mar 25, 2019
Closed
@ghost 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
Copy link
Author

ghost commented Mar 28, 2019 •
edited by ghost
Loading

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!

@ghost ghost requested a review from inclement March 28, 2019 19:58
@ghost ghost changed the title Properly search native lib dir in ctypes, fixes #1770 [wip] Properly search native lib dir in ctypes, fixes #1770 Mar 30, 2019
@ghost ghost changed the title [wip] Properly search native lib dir in ctypes, fixes #1770 Properly search native lib dir in ctypes, fixes #1770 Mar 30, 2019
@ghost
Copy link
Author

ghost commented Mar 30, 2019

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
Copy link
Member

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 ghost changed the title Properly search native lib dir in ctypes, fixes #1770 [WIP] Properly search native lib dir in ctypes, fixes #1770 Mar 30, 2019
@ghost
Copy link
Author

ghost commented Mar 30, 2019

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

@ghost ghost changed the title [WIP] Properly search native lib dir in ctypes, fixes #1770 Properly search native lib dir in ctypes, fixes #1770 Mar 30, 2019
@AndreMiras
Copy link
Member

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

@AndreMiras
Copy link
Member

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 ghost changed the title Properly search native lib dir in ctypes, fixes #1770 [WIP] Properly search native lib dir in ctypes, fixes #1770 Mar 31, 2019
@ghost
Copy link
Author

ghost commented Mar 31, 2019

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
Copy link
Author

ghost commented Apr 1, 2019

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

@ghost ghost changed the title [WIP] Properly search native lib dir in ctypes, fixes #1770 Properly search native lib dir in ctypes, fixes #1770 Apr 1, 2019
AndreMiras
AndreMiras reviewed Apr 3, 2019
View reviewed changes
tests/test_androidmodule_ctypes_finder.py Outdated
activity = MagicMock()
activity.getApplicationContext().getPackageName = (
lambda: "com.example.testpackage"
)
Copy link
Member

Choose a reason for hiding this comment

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

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"

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Lambda is completely fine 👍

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

AndreMiras
AndreMiras reviewed Apr 3, 2019
View reviewed changes
tests/test_androidmodule_ctypes_finder.py Outdated
return activity_class
elif name == "android.content.pm.PackageManager":
manager_class = MagicMock()
manager_class.GET_SHARED_LIBRARY_FILES = 1024
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

AndreMiras
AndreMiras reviewed Apr 6, 2019
View reviewed changes
tests/test_androidmodule_ctypes_finder.py Outdated
import sys
import tempfile

sys.modules["jnius"] = MagicMock() # fake jnius used later
Copy link
Member

Choose a reason for hiding this comment

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

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

AndreMiras
AndreMiras approved these changes Apr 6, 2019
View reviewed changes
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

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

@AndreMiras AndreMiras merged commit 6b12281 into kivy:master Apr 6, 2019
@ghost ghost deleted the ctypes_patch_fix branch April 7, 2019 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inclement inclement Awaiting requested review from inclement

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

Successfully merging this pull request may close these issues.

ctypes.util.find_library() doesn't work on arch arm64-v8a
1 participant
@AndreMiras