-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Python 3 recipe #1412
Python 3 recipe #1412
Conversation
Good job ! |
Sounds good to me :)
Glad to hear it! I was worried there could be changes that break things. |
@@ -5,3 +5,4 @@ | |||
|
|||
# APP_ABI := armeabi armeabi-v7a x86 | |||
APP_ABI := $(ARCH) | |||
APP_PLATFORM := android-21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes sdl2 recipe to fail. We can comment this line to bypass the error...as a temporary hack in order to make further tests, NOT AS A FINAL SOLUTION.
# APP_PLATFORM := android-21
Note: This already has been informed by @inclement into the "to do" section : "add proper APP_PLATFORM setting functionality"
Here are the log of the failure:
[DEBUG]: -> running ndk-build V=1
[DEBUG]: Android NDK: APP_PLATFORM set to unknown platform: 21.
[DEBUG]: /media/DEVEL/Android/android-ndk-r17b/build/core/setup-app-platform.mk:135: *** Android NDK: Aborting . Stop.
Exception in thread background thread for pid 31274:
Traceback (most recent call last):
File "/usr/lib/python2.7/threading.py", line 801, in __bootstrap_inner
self.run()
File "/usr/lib/python2.7/threading.py", line 754, in run
self.__target(*self.__args, **self.__kwargs)
File "/home/opacam/.local/lib/python2.7/site-packages/sh.py", line 1540, in wrap
fn(*args, **kwargs)
File "/home/opacam/.local/lib/python2.7/site-packages/sh.py", line 2459, in background_thread
handle_exit_code(exit_code)
File "/home/opacam/.local/lib/python2.7/site-packages/sh.py", line 2157, in fn
return self.command.handle_command_exit_code(exit_code)
File "/home/opacam/.local/lib/python2.7/site-packages/sh.py", line 815, in handle_command_exit_code
raise exc
ErrorReturnCode_2:
RAN: /media/DEVEL/Android/android-ndk-r17b/ndk-build V=1
STDOUT:
Android NDK: APP_PLATFORM set to unknown platform: 21.
/media/DEVEL/Android/android-ndk-r17b/build/core/setup-app-platform.mk:135: *** Android NDK: Aborting . Stop.
STDERR:
Traceback (most recent call last):
File "/home/opacam/.local/bin/p4a", line 11, in <module>
load_entry_point('python-for-android', 'console_scripts', 'p4a')()
File "/home/opacam/Devel/python-for-android/pythonforandroid/toolchain.py", line 981, in main
ToolchainCL()
File "/home/opacam/Devel/python-for-android/pythonforandroid/toolchain.py", line 518, in __init__
getattr(self, args.subparser_name.replace('-', '_'))(args)
File "/home/opacam/Devel/python-for-android/pythonforandroid/toolchain.py", line 148, in wrapper_func
build_dist_from_args(ctx, dist, args)
File "/home/opacam/Devel/python-for-android/pythonforandroid/toolchain.py", line 192, in build_dist_from_args
build_recipes(build_order, python_modules, ctx)
File "/home/opacam/Devel/python-for-android/pythonforandroid/build.py", line 579, in build_recipes
recipe.build_arch(arch)
File "/home/opacam/Devel/python-for-android/pythonforandroid/recipes/sdl2/__init__.py", line 41, in build_arch
shprint(sh.ndk_build, "V=1", _env=env)
File "/home/opacam/Devel/python-for-android/pythonforandroid/logger.py", line 176, in shprint
for line in output:
File "/home/opacam/.local/lib/python2.7/site-packages/sh.py", line 863, in next
self.wait()
File "/home/opacam/.local/lib/python2.7/site-packages/sh.py", line 792, in wait
self.handle_command_exit_code(exit_code)
File "/home/opacam/.local/lib/python2.7/site-packages/sh.py", line 815, in handle_command_exit_code
raise exc
sh.ErrorReturnCode_2:
RAN: /media/DEVEL/Android/android-ndk-r17b/ndk-build V=1
STDOUT:
Android NDK: APP_PLATFORM set to unknown platform: 21.
/media/DEVEL/Android/android-ndk-r17b/build/core/setup-app-platform.mk:135: *** Android NDK: Aborting . Stop.```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what caused this for you, does your NDK not have platform-21?
with current_directory( | ||
join(self.ctx.python_recipe.get_build_dir(arch.arch), | ||
'Lib')): | ||
shprint(sh.zip, '-r', stdlib_zip, *os.listdir()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix missing argument for command os.listdir, we can fix it like this:
stdlib_dir = join(
self.ctx.python_recipe.get_build_dir(arch.arch), 'Lib')
with current_directory(stdlib_dir):
shprint(sh.zip, '-r', stdlib_zip, *os.listdir(stdlib_dir))```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative fix: drop python2 support :<
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you maintain the affected code as it is you will also drop support for python 3 prior to version 3.2 and it would be great that was documented in somewhere: https://docs.python.org/3/library/os.html?highlight=listdir#os.listdir
References: kivy#1412
Good job @inclement, this is going well and fast! I applied some changes to your python3 recipe in order to grant support for some libs: sqlite3, openssl, libffi and libexpat. I pushed this work into a branch of my p4a fork (with your commits until 9b6436b, squashed in one commit) . It would complete the last two points of the "to do". Be aware that the libs aren't fully tested in a real application, but successfully builds and the apk initializes fine. Feel free to use that if you want, no problem 😉 Here is the commit that do the job: opacam/python-for-android@b38c6ae |
Includes commits from @inclement's pr "Python 3 recipe" (squashed into this commit): - Moved from bpo-30386 branch to python 3.7.0 official distribution - Made all version references in python url dynamic Plus, update the libs patches in order to work with python 3.7.0. References: kivy#1412, inclement/python-for-android@588f736 and inclement/python-for-android@a7f9348
In order to keep the recipe more maintainable and readable, all the environment variables has been moved into method "get_recipe_env", overwriting the base class. To achieve this goal some other changes has been made: - Remove unused variables (platform_dir) - Move some "toolchain bin variables" directly into the env dictionary (because they are only used to set the corresponding env variables) - Add new env variable HOSTARCH (we use that in build_arch, cause this commit moves the needed variable that was used before this commit...and moreover...this new variable targets 'android_host' which still is not set as it should, so introducing this variable we avoid future errors when the target variable 'android_host' will get fixed) - Shorted long lines to avoid pep8 errors and to be more readable - Add some todo comments References: kivy#1412
- F401 (imported but unused) - W293 blank line contains whitespace - E305 expected 2 blank lines after class or function definition, found 1 - Also add/remove some line breaks to make code more readable Note: The imports for "pythonforandroid.patching" has been commented because we probably will need those imports when patches applied References: kivy#1412
- F401 XXX imported but unused - E303 too many blank lines (2) - Shortened long lines to improve reading References: kivy#1412
The squashed commits in order to update the branch are: - Made bootstraps check the ndk api against the requested minsdk (inclement/python-for-android@cf1491a) - Fixed APP_PLATFORM setting (inclement/python-for-android@5acace3) So, right now, the branch should be up to date with @inclement's branch until date 2018-10-18, except the recipes (hostpython and python which has some additions and the commit 5db62ac which grants build compatibility with all supported python version supported by p4a) References: kivy#1412
Thanks @opacam, it's good to hear that these libraries will apparently work without issue. |
So you wrote you tested with API 24, but which NDK are you using? |
I spoke with @tito on discord about the question above, but for the benefit of others:
|
For reference, @tito found some more places where we need to be careful to use the right ndk api target, see https://gist.github.com/tito/73a2d35c236bb34f6441b64be44c5683 . These will be fixed in this PR soon. |
OOoo, this is great, I'm testing the @tito's patch over the last published @inclement's work, we are getting closer but unfortunately the openssl libs causes troubles, linkages error when generating python 3 lib, but this can bypassed adding a new argument into the perl's configure (if we don't do that, the openssl build system will use the higher api we have available and if it don't match with the one we use with python 3 we have the mentioned linkage errors), so here are a patch to apply after @tito's patch: And another patch to add testapp for python3 with openssl and sqlite3 This will solve the linkage problem, however no_ssl generated. The good news are the sqlite3 module is generated. Note: Right now I'm testing with ndk-r17b, python 3.6 interpreter and targeting api 24 and with @inclement's method but with the testapp i posted above with openssl and sqlite added. |
The following commit solves the _ssl problem: And here another commit that we grant support fo libffi and libexpat (with testapp included), following the method introduced in the openssl fix and tested with both api 24 and 27 and using ndk-r17b: Fix python3crystax: Fix python2 (no working openssl with newest libraries): By the way, so many thanks for your testing method @inclement!!...I was using some kind of hacky method which I prefer not to mention 😬 |
General update: Currently seems working fine with API 27 and newest SDK, I'm moving on to cleaning up the code and fixing some of the other things I broke along the way (like the other python recipes as noted by @opacam). I appreciate the testing and feedback. I won't let the openssl and sqlite bullet points hold up this main PR, since others have confirmed so well that they work fine, but I expect to check through everything else in my original post before merging. The amount of progress will depend on how much time I have to work on it, but hopefully I can get a lot of the remaining work done by next week. |
I think this is essentially ready now. I'm still working to fix some code style issues and I'll try to make the tests pass, so don't merge just yet, but it should be good to review. I've focused on making the new NDK API arguments work properly, as this is the most important thing for API 26+ support. When this is merged, I'll make a new issue with a list of the things that still need doing for the python3 recipe. I've already fixed all the TODOs I left in the code except within the python3 recipe itself, which can be considered experimental for now. I also haven't tested python3crystax yet because I don't have python3.6 installed. Has anyone checked that this works? I'll do it if necessary. ^^ for @tito @AndreMiras in particular, but comments from others are welcome. |
Awesome work! The overall looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through the code and didn't see any red flag so far.
I'll know more after checking out and building for python3crystax
to look for regressions to begin with.
In the meantime fixing the linter will also help us checking for regression since Travis will go further in the integration testing process.
Some unit tests also broke in tests/recipes/
, but this is probably because of the refactoring. I may pick this up later.
@@ -243,6 +244,8 @@ def make_package(args): | |||
tar_dirs.append('private') | |||
if exists('crystax_python'): | |||
tar_dirs.append('crystax_python') | |||
if exists('_python_bundle'): | |||
tar_dirs.append('_python_bundle') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestion for this and the lines above:
for tar_dir in {'private', 'crystax_python', '_python_bundle'}:
if exists(tar_dir):
tar_dirs.append(tar_dir)
@@ -409,7 +412,17 @@ def make_package(args): | |||
|
|||
def parse_args(args=None): | |||
global BLACKLIST_PATTERNS, WHITELIST_PATTERNS, PYTHON | |||
default_android_api = 12 | |||
|
|||
# Get the default minsdk, equal to the NDK API that this dist it built against |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo:
s/it built/is built/
info = json.load(fileh) | ||
if 'ndk_api' not in info: | ||
print('Failed to read ndk_api from dist info') | ||
default_android_api = 12 # The old default before ndk_api was introduced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add the actual Android API that will be used in the message?
print('Failed to read ndk_api from dist info, defaulting to android API {}'.format(default_android_api))
And of course invert both lines so default_android_api
is well defined
} else { | ||
LOGP("crystax_python does not exist"); | ||
LOGP("crystax_python does not exist"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, should not this be?
LOGP("crystax_python or _python_bundle does not exist");
# Create the Setup file. This copying from Setup.dist | ||
# seems to be the normal and expected procedure. | ||
assert exists(join(build_dir, 'Modules')), ( | ||
'Expected dir {} does not exist'.format(join(build_dir, 'Modules'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: maybe we want to go for an explicit raise
rather than assert
. Who knows perhaps some people run p4a with python -O
😄
info('Skipping hostpython3 build as it has already been completed') | ||
|
||
self.ctx.hostpython = join(build_dir, 'python') | ||
self.ctx.hostpgen = '/usr/bin/false' # is this actually used for anything? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, should we try to remove it and if it leads to issues we can just fix it and add a comprehensive comment?
for suffix in EXCLUDE_EXTS: | ||
if filename.endswith(suffix): | ||
removes.append(filename) | ||
shprint(sh.rm, '-f', *removes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope since it was already in our code base.
But it hurts a little bit knowing this entire block could be a single line command using find
:
find . -name '*.ext`' -o -name '*.ext2' -o -name '*.ext3' -delete
But yes that's not portable. However I'm wondering if there's not a way to do it in Python that would look nicer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking more and more about making some better python abstractions for this. Something for the near future, perhaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is fnmatch in the standard lib that can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I realize endswith
can take tuple 😄
So we could actually go one for
less:
if filename.endswith(EXCLUDE_EXTS):
But I think it's OK to tackle it outside this pull request, since we have enough to check
shprint(sh.rm, '-rf', 'lib2to3') | ||
shprint(sh.rm, '-rf', 'idlelib') | ||
for filename in glob.glob('config/libpython*.a'): | ||
shprint(sh.rm, '-f', filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, maybe this would work directly?
sh.rm('-f', glob.glob('config/libpython*.a'))
configurable */ | ||
/* AND: P4A uses env vars...not sure what's best */ | ||
LOGP("Initialize Python for Android"); | ||
setenv("P4A_BOOTSTRAP", "SDL2", 1); // env var to identify p4a to applications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great addition!
@@ -45,6 +45,8 @@ protected static void addLibraryIfExists(ArrayList<String> libsList, String patt | |||
addLibraryIfExists(libsList, "crypto.*", libsDir); | |||
libsList.add("python2.7"); | |||
libsList.add("python3.5m"); | |||
libsList.add("python3.6m"); | |||
libsList.add("python3.7m"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't theses changes added in all others bootstraps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other bootstraps don't actually support python3 yet, I think it's best to leave that as a separate task rather than delay merging.
options = {'apk': {'debug': None, | ||
'requirements': 'sdl2,pyjnius,kivy,python3crystax', | ||
'android-api': 19, | ||
'ndk-dir': '/home/asandy/android/crystax-ndk-10.3.2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.environ["ANDROIDNDK"] ?
|
||
def link_root(self, arch_name): | ||
return join(self.ctx.ndk_dir, 'sources', 'python', self.major_minor_version_string(), | ||
'libs', arch_name) | ||
|
||
recipe = Python3Recipe() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we rename this lass Python3CrystaxRecipe
to avoid having it clashing with the new Python3Recipe
one?
py2 = self.get_recipe('python2', arch.ctx) | ||
env['PYTHON2_NAME'] = py2.get_dir_name() | ||
py3 = self.get_recipe('python3', arch.ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
py3
is unused here, right?
pythonforandroid/build.py
Outdated
@@ -615,7 +626,9 @@ def run_pymodules_install(ctx, modules): | |||
|
|||
venv = sh.Command(ctx.virtualenv) | |||
with current_directory(join(ctx.build_dir)): | |||
shprint(venv, '--python=python2.7', 'venv') | |||
shprint(venv, | |||
'--python=python{}'.format(ctx.python_recipe.major_minor_version_string()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
major_minor_version_string
is a property, not a method, so we should remove the brackets
d9e14ef
to
e48d82e
Compare
The python3 testapp is failing because something tries to call python3.7. I will investigate tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that PR, amazing work! I'm in for the merge even though two builds out of five are broken.
Find below few of my personal feelings regarding things I wish we could improve:
- I think it's a shame we broke
E129
because it's very easy to fix and the warning made sense - organise the git log history a bit more or squash
- go for a "more mainstream" Python version by default i.e. 3.6 rather than 3.7
As for the broken tests, for records we have:
-
configure: error: python3.7 interpreter not found
Having python3.7 installed on the Docker host would help. Or simply putting python3.6 in the requirements rather than 3.7. -
Didn't find any valid dependency graphs.
That's a known conditional build limitation, I can fix later.
https://github.com/kivy/python-for-android/blob/3e561b0/ci/rebuild_updated_recipes.py#L17
I haven't actually tried, but I wouldn't be surprised if this doesn't work with Python 3.6.
What concerns me about this is that we have a hostpython3 recipe, there should be no need for the user to have their own python3.7 installed, so I'll look into it before merging. It's a nice thing for a test to catch because of course I didn't test locally without python3.7! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
Conditional build is failing because of #1454
But we're good for a merge
Really fine work, thank you. Below my full Python3/Crystax build recipe - builds, installs, and runs great.
These are the changes to my Crystax build environment for r17c
This builds, installs, and runs great. Sorry for earlier mis-infomation
|
@Ham-Merhead does this produce an apk that the Google Play Store accepts? And have you tried running the apk on an Android 8.1 platform? (I've hit a wall with an apk that worked on 8.0, but fails on 8.1, failing to import _sqlite3.so) |
I thought crystax only supported up to 21? |
@ApplySci My .apk works with Android 9 (I dont know about 8.1), but my app does not use sql which perhaps yours does. I have not tried submitting to the store. |
@OptimusGREEN My understanding is the NDK (here Crystax) defines the minimum, and the (Android) SDK the maximum. But perhaps I don't understand?! |
I haven't had much time to work on this since my initial proof of concept, but I've got back to it now. This PR is a work in progress, I wouldn't usually make it anywhere near this early but I know people are following this closely so I thought I might as well be clear what is going on with it.
Right now this is the most minimal possible implementation, I literally just got it working and there are many hacks to be resolved. However, it should work, the whole build process is now included although it's only tested on my machine so far there could be issues in other environments (especially macOS?). The main difference to the proof of concept is that I began moving the build into p4a's system rather than the shell/Makefile system used in Python bpo-30386. There's nothing wrong with that system, but since it isn't maintained it seems safer to make full use of p4a's tools instead.
Non-exhaustive non-ordered list of things to do (note: even if checked off it doesn't mean the implementation is final, just that the bulk of the work is done on that feature):
make the python3 recipe use p4a's arch system as much as possible, including making p4a understand clang toolchains[edit: won't hold up merging for this, will make a new issue for it]currently tested only with API 24 and there are definitely things that need doing to make everything work with 27+check why ctypes isn't building and fix it if it's simple (it should be, I think it works in bpo-30386)[edit: won't hold up merging for this, will make a new issue for it]openssl recipe compatibility[edit: won't hold up merging for this, will make a new issue for it]sqlite recipe compatibility[edit: won't hold up merging for this, will make a new issue for it]**Minor note: as it stands this recipe doesn't support API lower than 21 due to Android limitations. There are some patches that exist to fix that, but I'm probably not going to look into that any time soon, so API 21 may effectively be the lowest supported API for this recipe.