Fixes the include and link paths for python2 #793
Conversation
|
It looks like this should be abstracted to CompiledComponentsPythonRecipe, not PythonRecipe itself, but the change seems good (it's definitely an omission right now). Would you be able to make that small fix? |
|
It looks like m2crypto should be a CompiledComponentsPythonRecipe but isn't, as well. If you don't want to fix/test that, feel free to just leave it out for now. |
|
No @inclement, there is no problem, now I'm trying to build with both changes you proposed...I will keep you reported |
|
Ok... builded without errors...so I will commit and push the changes |
pythonforandroid/recipe.py
Outdated
| env['PYTHON_ROOT'] = self.ctx.get_python_install_dir() | ||
| env['CFLAGS'] += ' -I' + env['PYTHON_ROOT'] + '/include/python2.7' | ||
| env['LDFLAGS'] += ' -L' + env['PYTHON_ROOT'] + '/lib' + \ | ||
| ' -lpython2.7' |
There was a problem hiding this comment.
This probably shouldn't hardcode python2.7 due to the python3 build, but I have to think a little about what's supposed to happen.
|
Why not? If this is about keeping the code clean, I think about creating a function that sets the right paths depending on the used python's recipe, something like: What do you think @inclement, It's all about keeping the code clean or maybe I'm missing something? PD: If you like this solution, I will rewrite it this way but it would be great that you could give me the right paths for python 3 and python from crystax because I don't have any written python 3 code...so I never tested the python 3 recipe or python 3 from crystax and I don't know the right paths. |
|
Okay, I misread the diff, the change doesn't have the problem I thought. I do think the linking needs centralising somehow, the -lpython2.7 is passed in different ways in different places. Part of the reason for the current state is that I can't reproduce the problems with it that people seem to regularly encounter on certain setups. |
|
Yes man ...I see your point of view...maybe we should remove the duplicated headers and linkages from the CppCompiledComponentsPythonRecipe (get_recipe_env) this way all would be centralized into CompiledComponentsPythonRecipe. What do you think? PD: Sorry for the delay...I've been upgrading my pc and reinstalling all the stuff...plus some vacational days (this month are my holidays so my contributions will be intermitents) PD2: Also I notice that CythonRecipe sets some python headers...so maybe, instead of put the headers into CompiledComponentsPythonRecipe we should move into Python recipe, this way all the headers related to python will be inherited by CythonRecipe, CompiledComponentsPythonRecipe and CppCompiledComponentsPythonRecipe...I think that this is the way to go. |
|
About the last comment (part PD2)... I think that something like this should do the work (for PythonRecipe): This will force to set the variable "call_hostpython_via_targetpython = False" for "CythonRecipe" in order to get the python headers for cython compilations...and then... we can remove the python header parts from CompiledComponentsPythonRecipe, CppCompiledComponentsPythonRecipe and CythonRecype. This way we ensure that we have the right headers for all PythonRecipes and subclasses, plus, all the python headers are set into the same place. @inclement...Do you like this proposal? |
|
This was a very interesting pull request, @opacam ! It's a shame we could make it to the tree. |
|
Ooh yes, I will take a look to solve the conflicts, no problem (this week I'm a little busy but i will try to resume the work as soon as possible) |
| @@ -13,14 +13,9 @@ def get_recipe_env(self, arch): | |||
| env = super(CryptographyRecipe, self).get_recipe_env(arch) | |||
| r = self.get_recipe('openssl', self.ctx) | |||
| openssl_dir = r.get_build_dir(arch.arch) | |||
| env['PYTHON_ROOT'] = self.ctx.get_python_install_dir() | |||
| env['CFLAGS'] += ' -I' + env['PYTHON_ROOT'] + '/include/python2.7' + \ | |||
| ' -I' + join(openssl_dir, 'include') | |||
There was a problem hiding this comment.
Thanks for updating that pull request.
Please remove the join import since it's not used anymore Travis is complaining.
https://travis-ci.org/kivy/python-for-android/jobs/398189288
Also think about running tox so you don't have to wait for Travis to fail to fix the linter.
| import sh | ||
|
|
||
|
|
There was a problem hiding this comment.
Thanks for updating that pull request.
Linter is complaining because you removed that line 😛
https://travis-ci.org/kivy/python-for-android/jobs/398189288
You can run tox on your dev env so you don't have to wait for Travis to fail to fix the linter.
pythonforandroid/recipe.py
Outdated
| elif self.ctx.python_recipe.from_crystax: | ||
| ndk_dir_python = join(self.ctx.ndk_dir, 'sources', | ||
| 'python', python_version) | ||
| env['CFLAGS'] += '-I{} '.format( |
There was a problem hiding this comment.
This missing space before the -I breaks the build on crystax.
See Travis build https://api.travis-ci.org/v3/job/398192030/log.txt error is:
[DEBUG]: arm-linux-androideabi-gcc: error: unrecognized command line option '-mthumb-I/opt/android/crystax-ndk-10.3.2/sources/python/3.5/include/python'
|
Thanks for picking this up again! docker build --tag=p4a .Then play with it on an interactive shell docker run -it --rm p4aOnce in the shell you can just run stuff that Travis is running e.g. to play with crystax: . venv/bin/activate
cd testapps/
python setup_testapp_python3.py apk --sdk-dir /opt/android/android-sdk --ndk-dir /opt/android/crystax-ndk --requirements python3crystax,setuptools,android'I can connect to IRC if you need support on this. |
|
Also it looks like I'm not too sure yet where in the pull request this was broken. If you can't find it I can take a look later if you give me rights to edit the PR. |
|
First of all, many thanks about all the info about docker. I recently installed it but I didn't had time to play whit it...and I suspect that the container for p4a it will be a little heavier for my system right now...probably I will have to make a little space into my pc to run the docker image for p4a...do you know how much of space take the docker image once builded? (My OS is debian) About the pr, I just fixed the typo error you mention about the includes for crystax and pushed the changes...but probably it will fail in numpy...I will take a look in order to find the error. I have just given you write permission for this pr, so, feel free to make the changes you consider, no problem. |
|
It takes 12.5 gigs. Thanks for the permissions, I don't know when I'll have time to look into it, but I believe we're almost there, you made the big part, thanks for resuming that work. |
|
@AndreMiras...it seems that i found a quick solution, i just upgraded the numpy recipe based on the reference pointed on the numpy recipe itself, and it seems to build ok, without errors. I did not commit/push the changes because I prefer your opinion before making this commit... I just push this changes or maybe we should create another pr for the numpy recipe...what do you think? Note 1: the upgrade points to version 1.13.3, it's not the last version but is more updated that the current one (the last one is 1.15) |
|
Oh great if it compiles with it. I've never played with numpy myself, but I will try to see if I can also compile with the fix you suggested, but then we should also verify we can run a simple APK on Android with it just to be sure. But I have to say I'm curious why it broke, because your change should be more or less silent, I mean it's a refactoring and it should not have a too big impact on the recipes. And since we don't have a large recipe coverage with the continuous integration, so my concern is if it breaks other recipes or not. So I'll also take a look at this and come back to you. |
|
So I tried to build one of my projects and it seems to break at least one other recipe: Edit: so I found what was the issue with gevent, I'll fix it in another pull request. Basically gevent doesn't like |
|
I've tested the numpy upgraded recipe within a simple apk (as you pointed) and leads to runtime errors...so I took the initial aproach of trying to find where it fails and i found it: one of the last commits (09d50b4) causes the build error on numpy so reverting the conflicting commit fixes the build issue with numpy and probably should fix the gevent recipe that you comment (cause the mentioned commit affects all recipes that inherits from CompiledComponentsPythonRecipe). |
|
Great, thanks for testing it. One day we will also have the APK actual running as part of the continuous integration. Edit: here's the gevent fix #1307 |
Tries to match and replace patterns to be moved from one environment variable to another rather than hard coding it. That way unexpected flags added to parent recipes hierarchy also get handled. This issue popped when testing kivy#793
Tries to match and replace patterns to be moved from one environment variable to another rather than hard coding it. That way unexpected flags added to parent recipes hierarchy also get handled. This issue popped when testing kivy#793
fb13a4b to
996dfc7
Compare
|
After squashing commits (and removing the conflictive commit) it seems that all python2 test pass successfully but not for crystax (fails when building extensions for recipes: android and pyjnius). It fails on finding the python includes ...so setting the variable "call_hostpython_via_targetpython = False" for CythonRecipe fixes the issue (Note: be careful cause I commit the change via "amend commit" after squashing) Now passes all tests... but we should make sure that the other inherited recipes from CythonRecipe will not have issues like the numpy recipe. Also we should do some clean up with all the CythonRecipes cause the variable will be redundant on some of them and others maybe will need setting the mentioned variable to True (cause now the default behavior is the opposite) and also it's possible that some of them needs some rework...anyway...I think the default behavior should be to get the python flags for any recipe who compiles with python as a dependency. So my question is...we should test affected recipes and fix it in this pr or create a new pr to fix those after this pr is merged? About pr #1307...probably the tests should be done again cause the circumstances has been changed: the behavior of inherited recipes from CompiledComponentsPythonRecipe has the original behavior (call_hostpython_via_targetpython = True) and the tests where made with the opposite value. |
|
Thanks for the last changes you made, squashing the commits and final CI build fixes for Edit: almost posted at the same time. Regarding the other thing to fix, yes this is what I just stated, I think this PR is good enough as it is and we should go incrementally to update the other recipe. Having test passing is one thing, and having all the other (not even covered) recipes perfect is another. |
|
I really want to test it ASAP, but I'm not sure about today. Edit: I could run my Python3 app (EtherollApp) after putting |
Tries to match and replace patterns to be moved from one environment variable to another rather than hard coding it. That way unexpected flags added to parent recipes hierarchy also get handled. This issue popped when testing kivy#793
This changes affect almost all recipes inherited from PythonRecipe. Some recipes where affected by the changes and were modified in order to succesfully build. One of the changes forces to get python headers and linkages for all the CythonRecipe,so, all inherited recipes from CythonRecipe should be reviewed and cleaned up if necessary and all the recipes who inherited from PythonRecipe probably should be reviewed, taking into account the fact pointed by user @AndreMiras, some flags of some recipes are wrong: - CFLAGS may only be used to specify C compiler flags, for macro definitions use CPPFLAGS - LDFLAGS may only be used to specify linker flags, for libraries use LIBS Resolves: kivy#668 See also: kivy#1307
|
Many thanks about the article, very interesting and useful info, specially "The seven rules of a great Git commit message". I almost never put a description in my commit messages and I will change my habit, I already did by applying into this pr as you suggested 😉 About docker..I tried to build the image but unfortunately it fails...because i'm running out of disk space...:joy::joy::joy:...I already had bad feelings about that...so...I will modify the docker configuration in order to establish the docker container into another disk/partition with enough disk space to support it and trying again. |
|
Glad you liked the article. And thanks for the Docker pull request #1308 |
|
I could test it with the other Python2 app (PyWallet) eventually. The app is using a decent set of recipes and everything is working. I've also previously tested on another Python3 app of mine (Etheroll) which was also OK. Plus the CI compilation went OK on Travis. So it looks pretty safe. I think it's an improvement we've been waiting for long enough. Now the best way to find out if it broke something is to "confront" it to the community via master branch 😄 |
Tries to match and replace patterns to be moved from one environment variable to another rather than hard coding it. That way unexpected flags added to parent recipes hierarchy also get handled. This issue popped when testing kivy#793
Tries to match and replace patterns to be moved from one environment variable to another rather than hard coding it. That way unexpected flags added to parent recipes hierarchy also get handled. This issue popped when testing kivy#793
Tries to match and replace patterns to be moved from one environment variable to another rather than hard coding it. That way unexpected flags added to parent recipes hierarchy also get handled. This issue popped when testing kivy#793
Tries to match and replace patterns to be moved from one environment variable to another rather than hard coding it. That way unexpected flags added to parent recipes hierarchy also get handled. This issue popped when testing kivy#793
Tries to match and replace patterns to be moved from one environment variable to another rather than hard coding it. That way unexpected flags added to parent recipes hierarchy also get handled. This issue popped when testing kivy#793
Tries to match and replace patterns to be moved from one environment variable to another rather than hard coding it. That way unexpected flags added to parent recipes hierarchy also get handled. This issue popped when testing kivy#793
Tries to match and replace patterns to be moved from one environment variable to another rather than hard coding it. That way unexpected flags added to parent recipes hierarchy also get handled. This issue popped when testing kivy#793
Fixes the right include and link paths for python2's recipes when variable "call_hostpython_via_targetpython" set to False. This will fix issues #668 and #669 with the new python2's (Python 2.7.11) but should work with the old python2's recipe. Thanks to @brussee who pointed this in PR #778.
PD: Buuuuf, The last PR #792 was wrong... I cloned from the wrong branch...sorry, those days I'm a little tired cause of my job and I make silly mistakes...