Feature/enable ctypes #343
Conversation
…android into feature/enable-ctypes
c0d6eb6 to
f0c7499
Compare
|
tried it and all works fine in my environment. Ctypes compiled, packaged and runs on Android as expected. Cheers |
|
cheers for testing that @GeoRW hopefully someone will merge it soon |
|
Bump for review / merge |
|
just a heads up. I've tried this pull request, and noticed that PIL's _imaging.so file is not building correctly. I'm investigating it further since I need ctypes for a project of mine, so I'll let you guys known if I find anything. But from an initial investigation, it could be related to the python/recipe.sh change, which explicit specifies the BUILDARCH as x86 linux, where before it didn't. btw, to re-create the error, you have to clean clone a HEAD of p4a, and then apply this pull before running distribute.sh, so to avoid building PIL _imaging.so without it which builds fine!! |
|
@hradec thanks for testing and reporting back I will have a look into this when I get a chance as well see if I can figure out anything. |
|
Ok... after lots of tests, it seems _imaging.so is always 0Kb, even without this patch and everything seems to work ok. Also, there are no errors when running my app with ctypes and this patch, if I don't call my custom built .so shared library function using ctypes. Just importing ctypes in my python script also doesn't throw any error, so I'm assuming the problem is inside my shared library. the error message is a bit misleading though, as you can see: AttributeError: Symbol not found: load_library(linker.cpp:745): library "/data/data/org.test.touchtracer/files/lib/python2.7/lib-dynload/_imaging.so" not found but that error only happens when I call my function from my custom shared library with ctypes. so, I think it's probably my problem, not the patch! sorry about this. |
|
@hradec @olymk2 This is expected. Android has a limit on the number of libraries you can access via dlopen() in versions prior to 4.3, so instead of generating individual .so files for each module we combine them together in libpymodules.so. A zero byte file is put in place of the original .so as a placeholder. |
|
haaaa... that's it then!!! good to known! Thanks!! |
|
@hradec also worth noting that ctypes does by default does not detect where the libraries are because it trys to detect your architecture and fails on android so i had to patch it to look in a folder called /lib with in your app dir which i think is where they normally get put. if your dealing with your own code worth bearing in mind if you look in the PR you can see the logic at least it looks some where instead of no where now :) |
| continue | ||
| return None | ||
| - | ||
| +elif os.name == "posix" and sys.platform == "linux3": |
There was a problem hiding this comment.
This is not a good way to do this. It works right now, while we use Python 2.7.2. Python 2.7.3+ will always report linux2 regardless of the Linux version, and Python 3 dropped the number entirely. You should just replace the original function rather than adding extra checks, since we want them to always evaluate to True anyway. :)
There was a problem hiding this comment.
okay i was not that keen on dropping that much code, as there are a lot of conditions but perhaps its okay just will make the patch a lot bigger.
Will see if i can get this done at the weekend
There was a problem hiding this comment.
@kived I have created a new patch removing the system checks and just leaving the find_library function as suggested.
Can some people test and let us know if it works and perhaps help get it merged :)
|
humm... what I did was to add my own private folder, and put my .so in I was leaving it with the python code, but since pyo files go to sdcard, putting it in private seems to have done the job for me, since the files On Thursday, May 7, 2015, Oliver Marks notifications@github.com wrote:
|
… assumes android and runs our custom library lookup function
|
Looks almost good to me. I wonder if we can make it optionnal, or at least don't fail if we delete the _ctypes.so in the final apk (it is an additionnal module not required for most of the app). Could you change to add a try/catch around it? I don't really like the patch if the find_platform(), all the deletion stuff will break when upgrading a new python version (VS moving the os.name test for linux with "and False" or rewrite it, and add a new test at the end = patch easier to read / maintain) Double-check your PR, there is few part where the indentation is not working ^^ |
|
happy to make the changes but you may need to be more specific. @tito and @kived currently if _ctypes.so is missing it just errors if you try and import ctypes, not sure where i would add the try except do you mean around find library ? thats just for loading other .so files and not related to ctypes.so I could put the other code back in for the platform tests and perhaps just make the first test if posix use custom ctypes find_library else try all the other rules ? with the indentation do you mean outside the python files ? i did test the python code so would have hit an indentation error, i will have a look but can we clarify the other points before i do the work. its a pain each time i do a change as its about an hour of building to test a new package as you have to completely clean the build folder to test properly :/ |
|
I was talking in the java part, dunno why, but you added a _ctypes.so to load. Because he doesn' thave its own try/catch, it might break for later load? (nothing to worry about the python side). Maybe i don't understand and in that case, you're all good ^^ Just look the PR at the end, there is indentation changes on your own line. Maybe tab/space mix up |
|
@tito can you check the changes i have made if it looks good i will run a test and squash the commits. |
|
@olymk2 YES! one last question, did you try your new version, aka on the java part, is both load are necessary to make it work? The previous one was having only the System.load() not the loadLibrary() |
|
@tito I have not tried it yet wanted to make sure it looked good from code point of view in case more changes are needed, i will run it now and report back. I did have both load and loadlibrary previously olymk2@e8c0a4e#diff-3f3340c82c7f6df0d6c292b442086e1bL88 |
|
ok, my bad :) Well done @olymk2 ! |
|
no problem, um i think you missed the part in my above two comments about me not having tested the latest changes. the build has finished anyway and my app still works doing a fresh build so no issues :) cheers for taking the time and having a look i have been keen to get this merged |
Seems to be working can someone else check and merge if happy ?