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.

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

[CORE UPDATE - PART I] Refactor python recipes + openssl + sqlite3 #1537

Merged
merged 66 commits into from
Jan 9, 2019

Conversation

opacam
Copy link
Member

@opacam opacam commented Dec 18, 2018

This pr unifies the build system for the python recipes using the native build method used by @inclement in his python3 recipe. This is a big change because using the new build method allows us to bump the version number for the python2 recipe to version 2.7.15 and the old python2 recipe has been moved to a new recipe python2legacy, in order that we have a failsafe python recipe, in case that we must target a minimum api lower than 21 (because the new python recipe has the same limitation than the python3 recipe, minimum api >= 21).

Actions taken care here:

  • Refactor python recipes into a new core module ~pythonforandroid.python
  • Add ability to compile recipes with clang This is @tito's works, ¡¡¡thanks for that @tito!!!
  • Update openssl version to 1.1 This is @tito's works, ¡¡¡thanks for that @tito!!!
  • Add libs support for both versions of python: sqlite3 and openssl
  • Make sure that we can build with all architectures (Fix some of the hardcoded entries we used in our archs module)
  • Make it work the old python2 recipe as a new python recipe python2legacy (this one is the one originally created by @tito and reworked by @inclement)

The build has been tested for all archs, but I only could test in a device with armv7a...so it will be great if someone could test this with another archs.

Note for python2legacy:

  • This python2legacy recipe may be removed soon and, if anyone needs to use it, it may require to change the attribute depends of some recipe...because only has been granted a minimal compatibility, to create a simple application using sdl2 bootstrap with kivy, openssl and sqlite. Also should be mentioned that if you make use of the python2legacy recipe, the openssl libs installed will be the old ones (version 1.0) because the new ones are incompatible with the old python. This add a complexity in our build system that we may want to remove very soon...but for now...it may allow us to make a softer transition between python2legacy to python3, first migrating all the recipes to be compatible with the new python2 build system so we can safely remove python2legacy, and once we get in there...then we probably will slowly face out python2 support towards 2020 (the last planned release for python2)

Note for non sdl2 bootstraps:

  • pygame bootstrap: Contains a lot of hardcoded entries for the old python2 build, so it will not work until properly fixed in another pr. My intention here was to safe the pygame bootstrap using the python2legacy recipe...but we will see...because the python2legacy recipe may be removed soon and I'm not sure if we must maintain this pygame bootstrap because without the python2legacy recipe it may be difficult to make it work.
  • service_only bootstrap: Again, hardcoded entries for python 2, but this is fixed in pr [CORE UPDATE - PART II] Fix bootstraps for webview and service_only #1541 plus in that pr we will make it work for python3 too
  • webview bootstrap: Again, hardcoded entries for python 2, but this is fixed in pr [CORE UPDATE - PART II] Fix bootstraps for webview and service_only #1541 plus in that pr we will make it work for python3 too

Note for openssl affected recipes:

Well, the openssl update and the new python2 build system breaks most of the recipes that relies on openssl because most of them contains hardcoded entries of the old python2. All those affected recipes has been modified to work with the new python build system, plus we granted python3's compatibility to all of them in the corresponding prs:

Note for graphical affected recipes:

  • Pillow and pil needs to be updated to work with new python build system. I already have done that (I mean fix the hardcoded entries and grant compatibility for both versions of python) but still not created the proper prs...because I touch some libraries needed by this recipes (libjpeg, png, freetype, harfbuzz) and still needs to review it and properly test.

Other recipes that may need touches:

  • I think that lxml was failing with all this changes (now I'm not sure about that)...anyway...also I had granted compatibility for both versions of python for lxml (Also I touch libxml2 and lxslt). For this still not created prs... I need to review before push those changes...

Important note

This pr needs one last fix, it may be some users that face the issues described in #1501, if that is the case, you can use a patch to solve this. Look at the reverted commit 99ee28b, extract a patch and apply it to your fork. We plan to fix it in #1568.

This has been tested with ndk r17c and also I made some builds using ndkr18b, but this last one needs more testing

Development note: This pr was started in #1460 and also discussed in #1534. Has been split in several prs to make easier to discuss and merge or discard all the initial work made in those prs

opacam and others added 21 commits December 18, 2018 01:49
To make it use as a base class for hostpython2 and hostpython3 recipes in a near future.
To make it use as a base class for python2 and python3 recipes in a near future.
…and python2legacy.

This recipes will be useless until properly fixed or maybe it will be removed...
Those new recipes will use the same build method than python3. Also added a set of patches, some of them are enabled by default because they are necessary to perform a successful build. The others are added because maybe we will need them.
Cause we share the same build system for python2 and python3 recipes, we can safely remove some specific python2's code in favour of the python3's code, otherwise we will surely find troubles in our builds.
The python2's test with openssl and sqlite it will probably success, but without the libs until those are enabled. The pygame's test app will fail until properly fixed.
The android ndk has been migrated his default compiler from gcc to clang and soon will be removed from the ndk. Here we prepare p4a to compile recipes with clang, which will allow us to successfully compile an updated openssl libs. Without this openssl upgrade we will not be able to grant support to python3's _ssl.so module.

Note: The default p4a compiler still is gcc...so...no changes on the compile behaviour unless explicitly requested

References: kivy#1478
This openssl version is an LTS, supported until 11th September 2023.

Important: There is now a distinction between the version we use for downloading, and the version used for creating the library: OpenSSL 1.1 now have prefix with "1.1.so". This explains why others recipes that depends on it require the "library" version. Those recipes, still not are modified to support the new version.

References: kivy#1478
Several actions done:
  - The openssl recipe has been enhanced by introducing a couple of methods (include_flags and link_flags) which should help us to link other recipes with the openssl libs.
  - Remove lib_version introduced in the last openssl version update, so we don't have to modify all dependant recipes
  - Add a new variable `url_version`, used to download our recipe in the subclassed method versioned_url (this replaces the removed lib_version)
  - Add D__ANDROID_API__ to the configure command (needed to successfully build the libraries and link them with python)
  - Add documentation
This commit grants openssl support for both python versions (2 and 3). The python2 recipe needs some extra work to make it link with python, we need to patch the setup.py file in order to that the python's build system recognises the libraries. This is not the case for the python3 recipe which works like a charm, except for the fact that we have to modify our static class variable ` config_args ` for the python's recipe, i found this to be necessary in my tests, otherwise the openssl libs aren't detected.
Also:
  - add numpy to CI tests for python3: this module is some kind of special and probably a dependency for a lot of python packages so better make sure that we not break anything...as we already do with python2
  - Add numpy to python2 and python3 test apps, cause already have some buttons that allows to test it from the running app.
…ty to dependant recipes

To be able to run successfully the new test we must add python3 compatibility to some recipes that is why we touch them here.
Or the ci tests where libffi is involved will fail.
@opacam opacam changed the title Refactor python recipes + openssl + sqlite3 [CORE UPDATE - PART I] Refactor python recipes + openssl + sqlite3 Dec 18, 2018
When building an apk for service_only's bootstrap the build will crash if we don't add some pure python packages, because the directory "python-install/<distribution directory>" will not be ever created, so...this will make p4a crash when we try to enter into this directory.
@ghost
Copy link

ghost commented Dec 18, 2018

Which openssl-dependant things exactly are broken by this? And in what way are the other bootstraps broken, is it a paths related issue? Maybe it would be good to at least add in the bootstrap parts as well?

@opacam
Copy link
Member Author

opacam commented Dec 18, 2018

Which openssl-dependant things exactly are broken by this?

All the recipes which uses openssl, mostly the ones I fixed in original pr (plus some others like ffmpeg or libsec..)...except for the python recipes.

@opacam
Copy link
Member Author

opacam commented Dec 18, 2018

And in what way are the other bootstraps broken, is it a paths related issue?

Yes, and the way we pack and unpack the new python's build system is different.

@ghost
Copy link

ghost commented Dec 18, 2018

The libraries/stl unpacking? I think these changes are splendid, so I hope since they're now split off that you'll make a pull request for that right after 👍 I really liked those.

Nevertheless, I think we should get this one in ASAP (after a review of course) and not waste more time about what exactly should be in or not, so my vote is on keeping this pull request here as it is (minus minor change requested by the others) and just getting it merged.

Thanks again for your work & patience!

@opacam
Copy link
Member Author

opacam commented Dec 18, 2018

Maybe it would be good to at least add in the bootstrap parts as well?

I'm creating a new branch for that right now, with the original content of the pr with the changes we discussed earlier..the .java files unified...

But if you want we could put all in here, but will increase considerably the diff ...because we move all the files to be gradle compatible...so I thought it will be better to put in a new pr as a part 2.

@ghost
Copy link

ghost commented Dec 18, 2018

Sure make it as a part 2, because I think the others prefer it as smaller bits. I think I'm the one here who minds large pull requests the least but I don't have a direct vote in merges so I'm kind of irrelevant in that regard 😄 🤷‍♀️

I liked the path unification & gradle compatibility as well, so really looking forward to that pull request!

@opacam
Copy link
Member Author

opacam commented Dec 18, 2018

The libraries/stl unpacking? I think these changes are splendid, so...

Thanks man 😄, I though that should be the part3, but for that we depend on part 2...and that is only the initial work...all of the libs should be adapted because I suspect that if we build something against any shared lib, we will must load that lib when we initialize the app, otherwise we will get errors about missing symbols/functions...(at least this is true for all the libraries which python depends on, plus libtorrent, boost, stl...)

This should be implemented properly as described in kivy#1558, but for now we will avoid issues
To make it work the python2legacy recipe with the sdl2's bootstrap
…k the python2legacy recipe

Note: With this commit, we will be able to successfully build using the python2legacy recipe, but be aware that you may need to add python2legacy compatibility to some recipes to make it work with your app, because only some minimal set of recipes has been touched (the ones needed to make work the python2legacy with the sdl2 bootstrap and kivy)
…ython3crystax

Also this should fix the the openssl build issues reported when using crystax. Notice that this changes will leave us with two versions of openssl libs. We need the most updated one for our python2 and python3 recipes, and the legacy version for python2legacy and python3crystax. The python2legacy version is too old to support the openssl version 1.1+ and python3crystax is not building fine with the new openssl libs so to fix the crystax issues we force to use the legacy version, which previously has been proved to work.
…ite3)

To make easier for us to test if python2legacy is working
@opacam
Copy link
Member Author

opacam commented Dec 28, 2018

Ok , I think that now this is in a pretty good shape, you should take a look when you have some time, if any of you feel that I miss something just tell me.

With the latest changes introduced we will be able to use the python2legacy recipe with sqlite3 and openssl almost as before. To achieve this I was forced to reintroduce the old openssl recipe but the new openssl will also work (7a7d9f6). But I must warn you that some applications may require to add python2legacy compatibility to some recipes in order to work, because I only granted python2legacy compatability to some minimal set of recipes (66d6394).

I must say that with this changes we will be able to target sdk minimun api 19 via the python2legacy recipe which is a concern for @KeyWeeUsr, and also solves one of the concerns of @inclement about merging this pr with a broken python2legacy recipe.

The commits needed to make it work the python2legacy recipe has been named with the prefix Python2legacy:, to make it easier to revert the changes applied in case that the team decide that python2legacy must be removed.

Also it would be great that @AndreMiras could try again to build his etheroll app with crystax so we would know if openssl pass the build stage and works at runtime.

@ghost
Copy link

ghost commented Dec 28, 2018

@opacam thanks for all the work!! ❤️

The commits needed to make it work the python2legacy recipe has been named with the prefix Python2legacy:, to make it easier to revert the changes applied in case that the team decide that python2legacy must be removed.

Not sure how useful that is, most likely we'll need to merge it squashed anyway.

My suggestion would still be to just drop the old recipe and all changes that would be required to get it to work. (which would give us minimum API of 21, right? That's not ideal, but given the device stats - this seems to affect ~9%ish devices right now - I personally find it somewhat acceptable given this problem will only solve itself)

@KeyWeeUsr what are your thoughts on that?

To inform about the python2legacy recipe
@ghost
Copy link

ghost commented Jan 2, 2019

Happy new year everyone 🎉 🎉

For what it's worth, there is already some user interest in this pull request here #1559 and I'm also interested in moving my app from CrystaX to the regular python 3 after this is merged and help fix the ctypes issue (with a better approach than I attempted in the ill-fated #1517 ).

Is there anything I can do to help move this pull request along? Any input/opinions on whether python2legacy should be just dumped (which is my preferred solution)?

@opacam
Copy link
Member Author

opacam commented Jan 2, 2019

Happy new year!!

I also think that the python2legacy should be removed, but the administrator team must decide, I put in here because I thought that with it we will have more possibilities....I mean...we can slowly fase out python2:

  • First we could remove python2legacy, when the statics for devices that we will leave out are in our favour (lower than 5%) and when most of the recipes works with the new python2 build system.
  • Then we could begin to migrate all the recipes to be python3 compatible, and when we have most of them working in python3, we could remove python2.

Also, by acting this way, we will have some kind of fail safe python recipe, in case someone must target a minimum api lower than 21, or needs some recipe that have troubles to build with the new python formula...then ...the python2legacy recipe could be useful (at least for a while...until we begin to touch recipes...then ...I think that will be more problematic than useful to have this python2legacy recipe).

Anyway, I have no problem in removing the python2legacy recipe and the latest commits which make it work, if the team decide that it must be done, I will do it 😉

Because this is how you are supposed to do it, you must use LDFLAGS for linker flags and LDLIBS (or the equivalent LOADLIBES) for the libraries
The Java method called to load the libraries only supports a .so suffix, so we must remove the version for our python libraries, or we will get linkage errors

Resolves: kivy#1501
for lib in [python_lib_name + '.so', python_lib_name + '.so.1.0']:
shprint(sh.cp, join(python_build_dir, lib),
'libs/{}'.format(arch.arch))
shprint(sh.cp, join(python_build_dir, python_lib_name + '.so'),
Copy link

Choose a reason for hiding this comment

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

I don't understand, why would you remove the loop here? The only thing that seems to achieve is that in case it is ever named .so.1.0 again in the future (which I assume can happen depending on NDK or compiler version) it will break. Or does this actually fix something?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't remove the versioned library we will get an error when running the copy command, because now the versioned library doesn't exist anymore

Copy link

@ghost ghost Jan 6, 2019

Choose a reason for hiding this comment

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

@opacam I'm just thinking, is this make option guaranteed to exist in the future? Is it some standard thing? Maybe it would be better to make the for lib loop based on os.listdir instead? (filtered for starting with libpython and ending with .so)

Copy link

Choose a reason for hiding this comment

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

Also is this fixing something broken by this pull request? If not, better make a separate one. This one is huge enough and the more things are added on top, the longer it will take us to look through it and get it merged 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm just thinking, is this make option guaranteed to exist in the future?

Nothing is guaranteed to exist in the future 😆 😆...but this option exists at least since the python version 2.7.2...we have it in our python2legacy recipe...so...probably will continue there for a while...

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it would be better to make the for lib loop based on os.listdir instead? (filtered for starting with libpython and ending with .so)

mmmm...I'm not pretty sure about that...we know exactly which name will have the library...what you propose will add some lines that will do nothing at this time...I think that will complicate the source code reading, it would make to think that we should have more than one python library which is not the case right now...

Copy link

Choose a reason for hiding this comment

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

That is a non-issue, we have code comments for that 😉 (you could think that with the old loop as well)

I don't think it would be much longer than the previous code, and avoid that Makefile option. I guess using the Makefile option is kind of neater, but I'm not sure if we should trust it to stay around...? Might be worth checking in the Makefile or python install docs if this is something official or rather some internal variable we hacked into that isn't meant to be used from the outside

Anyway, in conclusion I would suggest you remove this commit, and make a separate pull request (since this is only bringing up new discussion and isn't strictly part of this, we should stop delaying things)

Copy link

@ghost ghost Jan 6, 2019

Choose a reason for hiding this comment

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

but you are also right...better to not be touched anymore unless requested.

true, and I would actually suggest you undo that latest commit and make a separate pull request. it's no good to stuff more things into this, even if they are necessary for the project as a whole

will be better get fixed as soon as possible

Even more of a reason to make a separate pull request! piling up more things into a single one we won't get it in any sooner due to the complexity of all the changes

Copy link
Member Author

@opacam opacam Jan 6, 2019

Choose a reason for hiding this comment

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

INSTNOME INSTSONAME It's official, and it has been there for a long, long, long time...checking the cpython code I see that was introduced at year 2002 (python/cpython@1142de3)...seems to me that has been pretty well tested, even for us because @tito make use of it in his original recipe (bdefea1) and @inclement did the same with his rework of the recipe (when the recipe.sh was removed, 4c8b5bc) so...we are currently having this discussion thanks to this variable jajajaja...at the point that we remove it in python3 recipe... we begin to have this linkage problems...so I have no doubt that we can rely on this INSTNOMEINSTSONAME variable...don't you think?

About removing the commit...mmmm...I already shot the bullet...even if I remove the commit...the damage has been done...lets the administrator team decide ok 😉?

Note: If we create the pr you mention and gets merged before this, we may have troubles resolving the merge conflicts in here and we could delay this even more...that is the reason to not create the pr in first place...and the reason to create a proper patch and post it into the proper issue and to notify it in discord.

Copy link

@ghost ghost Jan 6, 2019

Choose a reason for hiding this comment

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

About removing the commit...mmmm...I already shot the bullet...even if I remove the commit...the damage has been done...lets the administrator team decide ok wink?

not really, the commits will all be squashed anyway, so if you do a revert it will be truly removed with no issues. there is really no problem if you just revert it.

you could already squash the commit history as a preparation if you want, it would need to be done on the final merge with github's squash button anyway

@ INSTSONAME: yeah it looks like this was there from the start as part of their shared library building, that's good! looks like it's made exactly for our use case then

@ghost
Copy link

ghost commented Jan 6, 2019

@opacam just as a general note regarding pull requests, you're making it also increasingly unlikely that we get this merged if you pile on fixes that weren't caused by this pull request. I recommend not changing anything anymore unless it is a requested change or something broken as part of this request, so we can finally settle on some final version

I think the main question is still whether python2legacy should go since it introduce some complexity, @AndreMiras @inclement and @KeyWeeUsr should probably weigh in on that

@KeyWeeUsr KeyWeeUsr merged commit 10351c2 into kivy:master Jan 9, 2019
@KeyWeeUsr
Copy link
Contributor

@opacam Thanks again for your hard work and patience 👍

@ghost
Copy link

ghost commented Jan 9, 2019

@KeyWeeUsr thanks so much for getting us to cross the finish line with this one ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants