Fixes the include and link paths for python2

[opacam]

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...

[inclement]

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?

[inclement]

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.

[opacam]

No @inclement, there is no problem, now I'm trying to build with both changes you proposed...I will keep you reported

[opacam]

Ok... builded without errors...so I will commit and push the changes PD: I don't have any call to m2crypto in my apps so it would be great that someone could confirm that works at runtime as expected...but it should work.

[opacam]

Why not? This is not affecting the python 3 recipe. For python 3's recipes (python 3 and python from crystax) the right paths will be definitively different...so...I think that somewhere in the code should be some kind of mechanism who detects the python's recipe used. I do it this way (with an "if else") because it will be very easy to add below the right paths for python 3.

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:

def set_python_env(self, env):
    if 'python2' in self.ctx.recipe_build_order:
        # python2 paths
        ...
    elif 'python3' in self.ctx.recipe_build_order:
        # python3 paths
        ...
    else:
        # python from crystax paths
        ...
    return env

def get_recipe_env(self, arch=None, with_flags_in_cc=True):
    ...
    if not self.call_hostpython_via_targetpython:
        env = self.set_python_env(env)
    ...

What do you think @inclement, It's all about keeping the code clean or maybe I'm missing something? And if this is about keeping the code clean...do you like the solution above?

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.

[inclement]

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.

[opacam]

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.

[opacam]

About the last comment (part PD2)... I think that something like this should do the work (for PythonRecipe): `

def get_recipe_env(self, arch=None, with_flags_in_cc=True):
    env = super(PythonRecipe, self).get_recipe_env(arch, with_flags_in_cc)
    if not self.call_hostpython_via_targetpython:
        # SET PYTHON HEADERS...DEPENDING ON PYTHON'S RECIPE
        if 'python2' in self.ctx.recipe_build_order:
            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'
        elif self.ctx.python_recipe.from_crystax:
            env['CFLAGS'] = '-I{} '.format(
                join(self.ctx.ndk_dir, 'sources', 'python',
                     self.ctx.python_recipe.version, 'include',
                     'python')) + env['CFLAGS']
            # TODO: Must set ldflags for from_crystax (where is the python lib?)
        elif 'python3' in self.ctx.recipe_build_order:
            # This headers are temporary cause python3 recipe isn't working
            # and should be changed if python3 recipe is upgraded...
            env['PYTHON_ROOT'] = self.ctx.get_python_install_dir()
            env['CFLAGS'] += ' -I' + env['PYTHON_ROOT'] + '/include/python3.4m'
            env['LDFLAGS'] += ' -L' + env['PYTHON_ROOT'] + '/lib' + \
                              ' -lpython3.4m'
            pass

        hppath = []
        hppath.append(join(dirname(self.hostpython_location), 'Lib'))
        hppath.append(join(hppath[0], 'site-packages'))
        builddir = join(dirname(self.hostpython_location), 'build')
        hppath += [join(builddir, d) for d in listdir(builddir)
                   if isdir(join(builddir, d))]
        if 'PYTHONPATH' in env:
            env['PYTHONPATH'] = ':'.join(hppath + [env['PYTHONPATH']])
        else:
            env['PYTHONPATH'] = ':'.join(hppath)
    return env`

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?

[AndreMiras]

This was a very interesting pull request, @opacam ! It's a shame we could make it to the tree. Would you like to resume your work?

[opacam]

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)

[AndreMiras]

Thanks for picking this up again! Regarding my comment on the broken Travis build here https://github.com/kivy/python-for-android/pull/793/files#r199130280 fixing it is not a big deal, but for your information, you can also run the same builds locally if you pull/build the Docker image. Then you can run the commands like it's being done here: https://github.com/kivy/python-for-android/blob/37dc89f/.travis.yml So for instance. Build the image:

docker build --tag=p4a .

Then play with it on an interactive shell

docker run -it --rm p4a

Once 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.

[AndreMiras]

Also it looks like numpy failed to build https://api.travis-ci.org/v3/job/398192029/log.txt Looks like the cross compiler is not being picked up when linking. The error is:

error: Command "gcc -pthread -shared --sysroot /opt/android/android-ndk-r16b/platforms/android-19/arch-arm -lm -L/root/.local/share/python-for-android/build/libs_collections/bdisttest_python2/armeabi -L/root/.local/share/python-for-android/build/python-installs/bdisttest_python2/lib -lpython2.7 -DANDROID -mandroid -fomit-frame-pointer -D__ANDROID_API__=19 -isystem /opt/android/android-ndk-r16b/sysroot/usr/include/arm-linux-androideabi -isysroot /opt/android/android-ndk-r16b/sysroot -I/root/.local/share/python-for-android/build/python-installs/bdisttest_python2/include/python2.7 -I/root/.local/share/python-for-android/build/python-installs/bdisttest_python2/include/python2.7 build/temp.linux-x86_64-2.7/numpy/core/src/dummymodule.o -Lbuild/temp.linux-x86_64-2.7 -lm -o build/lib.linux-x86_64-2.7/numpy/core/_dummy.so" failed with exit status 1

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.

[opacam]

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.

[AndreMiras]

It takes 12.5 gigs.

docker ps -s
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES               SIZE
d712a52390f5        p4a                 "/bin/bash"         12 seconds ago      Up 11 seconds                           gracious_vaughan    0B (virtual 12.5GB)

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.

[opacam]

@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)

[AndreMiras]

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.

[AndreMiras]

So I tried to build one of my projects and it seems to break at least one other recipe: gevent with python3crystax. The log is pretty big and I'm not yet sure what happens. See log below: https://gist.github.com/AndreMiras/0fc5826449c3355d2fd4fda1d0988bea Feel free to take a look at that one too. In the mean time if you don't mind trying to squash your commits and have a meaning full commit title and body that would be awesome. If you're not sure how to proceed, I can also help.

Edit: so I found what was the issue with gevent, I'll fix it in another pull request. Basically gevent doesn't like -l in LDFLAGS, LIBS should be used instead for this recipe. I was escaping them before, but it's hardcoded so I'll try the same thing with a regex trying to match -l*. Let's see...

[opacam]

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 (https://github.com/kivy/python-for-android/pull/793/commits/09d50b4eefe9edce44f2e042a190c8f811dbb59d) 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).

[AndreMiras]

Great, thanks for testing it. One day we will also have the APK actual running as part of the continuous integration. Yes I was also suspecting call_hostpython_via_targetpython at first, but for gevent it's not the case. I'm almos ready with the side pull request that fixes gevent and it's only in gevent recipe. For numpy I haven't dig yet, so I may take a look with you later.

Edit: here's the gevent fix https://github.com/kivy/python-for-android/pull/1307 You don't have to merge it since it's not part of the CI anyway.

[opacam]

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.

[AndreMiras]

Thanks for the last changes you made, squashing the commits and final CI build fixes for numpy. I don't know why I didn't receive any email notifications for the commit :confused: Anyway it looks good, I'll try another time with two apps I know one Python2 another Python3, plus I'll also double check numpy at run time and I'll merge if it's OK. Unless @inclement has something to add. Also I think we could do something with LDSHARED, but let's keep that for later, I mean that pull request is already a great improvement. We'll have to clean a whole bunch of other recipes since most of the things are now handled in the core.

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. So let me play with it a bit and I'll merge if everything is green.

[AndreMiras]

I really want to test it ASAP, but I'm not sure about today. By the way did you manage to run Docker properly? Pro-tips for next time you can improve your commit message making a short meaningful title and a comprehensive body description. https://chris.beams.io/posts/git-commit/ Well if you don't know what to do meanwhile I'm testing you can still amend the commit message :wink: otherwise don't worry we will still make it to the tree. And man once again, thanks for picking up that pull request two years later and dealing with all the conflicts. We think it really improves the overall quality (maintainability & robustness).

Edit: I could run my Python3 app (EtherollApp) after putting call_hostpython_via_targetpython = False back in gevent pull request. Later tonight or tomorrow I'll try the Python2 one and then numpy. Hopefully it gets merged over the weekend.

[opacam]

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 :wink:

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.

[AndreMiras]

Glad you liked the article. And thanks for the Docker pull request #1308 Sorry for not giving news lately, I was struggling to fix some issues on a project I wanted to use to test the Python2 version of your pull request. I'm almost there, so I'll soon resume the review process and hopefully merge this weak. In the meantime Python 2.7 will retire pretty soon so I think it would be great if we consider dropping its support at some point and focus on Python 3. I'll keep you posted.

[AndreMiras]

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 :smile: Thank you again for all your efforts @opacam !