[WIP] Refactor python recipes and update python2's to version 2.7.15 #1460
Conversation
| '{}', ';', _env=env) | ||
| info('Stripping object files') | ||
| shprint(sh.find, '.', '-iname', '*.so', '-exec', | ||
| '/usr/bin/echo', '{}', ';', _env=env) |
There was a problem hiding this comment.
Looks like it was already like that, but now I'm getting curious why this is working, since:
ls /usr/bin/echo
ls: cannot access '/usr/bin/echo': No such file or directory
There was a problem hiding this comment.
I'm not sure how it would work, but i think the -ls option of find is what one would want to use.
There was a problem hiding this comment.
I've been testing this from terminal, and it fails on the echo stage, because, As @AndreMiras pointed, the echo command is not located at /usr/bin/echo, actually should be /bin/echo or echo...but even changing this the result is exactly the same...so I suspect that the combination of this two commands cause the issue...so the @tshirtman's proposal maybe a solution...can you give me any clue about of how to apply the find command with -ls, filtering by extension ".so" and printing line by line?
There was a problem hiding this comment.
I suppose this line just wasn't doing anything on systems where the path to echo is wrong. sh doesn't actually raise an error if this happens, it would just do nothing.
There was a problem hiding this comment.
Also, you can just take out the -exec stuff, you can still extract a list of files from the result (good enough for printing). Or just remove the print entirely, it isn't really important.
There was a problem hiding this comment.
In the next commit, I was planning to restore this lines at his previous state (to introduce python2legacy recipe) and remove entirely this print statement as you suggested, but now, I'm thinking to go a little further...perhaps we can create a new method like strip_cython_files into TargetPythonRecipe (as you do with create_python_bundle)?
|
I appreciate all the effort, specially since I would not have the energy to maintain python2 related things 😄 |
There was a problem hiding this comment.
Wow, nice work. I didn't really think python2 could be compiled this way, so that's great if it works.
To be honest I had been mentally deprecating python2, but mostly because I didn't intend to spend any effort on it. Revitalising it like this would be great, I have no major issues with it, but here are a few minor points:
- Fix python2's initialization on android device
Definitely worth making sure this works first :<
updates the python2 version to 2.7.15 using the same build process than the refactored python3's recipe
This build process is much neater, but despite its idiosyncrasies the current python2 recipe does have at least one or two unique points, such as the biglinking of library files. I'm not sure about breaking this old code (that's partly why I intended to leave the python2 recipe basically unchanged). Perhaps we could leave that in a legacypython2 recipe or something.
Even if doing that, it would be good to move anything specific to this legacy build into that recipe, like I did with the create_python_bundle method that was previously a mess elsewhere in the code. I was going to start doing this anyway.
Is this pr a good idea or it will be better to not refactoring the python recipes and only update the python2's recipe
I have mixed feelings about this, I guess the real question is how similar are python2 and python3 and will it always be possible to make changes for python3 without worrying about affecting the python2 build. It would also require some thought about recipes, since the simplest way to not break the recipe graph is to retain separate python2 and python3 recipes.
Perhaps it would be neatest to make a PythonRecipe base class that Python2Recipe and Python3Recipe both inherit from. To start with they could both be trivial subclasses (plus the extra patches etc. for python2), and maybe they would stay that way forever.
|
Ohh guys both of you are right with all the comments and reviews (I will apply it in some specific commit) but @inclement touch the point, the python initialization should be solved otherwise all of this is useless....so...I've been working on this point and I fixed the issue. About the "python legacy" recipe, already thought about that, in matter fact my first test was in that direction, but after a coding a while I realised that we will complicate things a lot and we will end up with a duplicated python2 and moreover, if the pygame bootstrap can be run in an updated python recipe (I'm not sure about that but I suspect that this can be achieved with some minor changes and building with an api minor than 21), what is the purpose of maintain the python legacy recipe? We will end up with a lot more of code and harder to maintain...anyway...I will adapt to whatever @inclement decide, no problem. |
|
Update: I already have some of the points mentioned in the first post done in my local machine:
I did not pushed them because I prefer to review it and create the apropiate commits, I will try to do it as soon as possible (I plan to do it today night or tomorrow). The openssl update should not be a problem, the job has been already done by @tito while helping with @inclement's python3 recipe (the only thing to do here should be some kind of mechanism that allow us to build the old openssl libs for python2legacy). It must be mentioned that the new python recipe has the same problem than the python3 recipe, ctypes is not working (as the python3 recipe, see pr #1455), but I thing that I got the solution for that...I suspect that we must add our libffi's recipe into the optional dependencies in order to build the ctypes module...¿what do you think about this ctypes solution @inclement? |
I'm aware of this, but hadn't got around to checking what the python2 and crystax python recipes do instead. It looks like both of these include mechanisms to build libffi as part of the Python build. In the case of python2, it's partially related to the patching we do, perhaps some small change would enable that in your current build. Overall though, making the libffi recipe a dependency would be fine, it looks like this would be an appropriate way for p4a to manage things. Thanks for looking into it. If you're thinking to try implementing this, how about even making it a separate PR for the python3 recipe that we can merge quickly, since it's independent of this PR? Beyond this, my main concern with this PR is that it will break things for people, probably in unexpected ways such as finding that other recipes are no longer compatible, or that people depended on biglinking in some weird way (this may also effectively raise the lowest supported API number, although we can probably live with just no longer 'supporting' the very old ones). It's fine for the python3 recipe because it's new, but the existing python2 build has been unchanged for years. I don't think this should ultimately prevent this being merged or the python2 recipe being replaced, but I think it will require some planning to smooth the transition (e.g. documenting clearly that there is a legacypython recipe). |
|
Of course we could also take the opposite tack and just officially drop the old python2 method much sooner than expected. Maybe we should! But it requires some thought. @AndreMiras @tito what do you think? |
|
Thanks @inclement for considering all these uses cases that I may have overlooked. |
(This branch and commit has been created as a sample to help decide if it is worth it to maintain the old python2 recipe as a new recipe named python2legacy. here we will see the minimal changes that we should make to make it work.) In order to maintain compatibility to pygame bootstrap and old sdl libraries, we perform several operations at once, this way can be reverted easily: - add python2legacy compatibility to basic recipes needed for pygame and bootstraps - solve the conflicts between python recipes - cause the pygame bootstrap has troubles to successfully build with the new python2's build, we enforce to build with python2legacy - update the core files to the new python2legacy - add compatibility python2legacy to all bootstraps - add some python2legacy tests Note: this new addition should be properly documented and add more recipes compatibility for python2legacy. References: Pull Request kivy#1460
|
I see that there is some doubts (I also have some of the concerns that @inclement mention) I am delaying the push of the python2 legacy recipe...yesterday I pushed some branch with the minimal required changes to introduce the original python recipe by @tito as a python2legacy to help us to decide if it's worth it to reintroduce it. This is only to get a global idea of what we have to touch and how it will looks like, and also if anyone want to test it . Some notes about his possible python2legacy recipe:
Regarding deprecating all python2 versions as suggested by @inclement, if it was my call, i will not do it, at least until the python team oficially deprecates it (2020), but guys, it's your decision, and whatever is your decision I'm sure that will be the right one and I will adapt to it, no problem, even if you want that I close this pr. I think than the updated python2recipe make easier to maintain both python versions, thanks to the great work of @inclement, but probably, things will stop work for python2 over the time (then I will start deprecate things for python2). If the team decide that this pr it's better without the python2legacy, then we probably must start deprecating (or directly removing) the pygame related stuff alongside with the old sdl libs. Ok, saying all that, I will continue to fix all stuff without the python2legacy (for now), the code is made and we could apply whenever we want...or not...just tell me...but if we introduce that, I'm afraid that then, what mentioned @AndreMiras about slowing down the development it will happen (In matter fact it does now 😂 😂) My plan is to work with this pr during this weekend and I will try to be connected to discord from afternon on. |
Sorry @inclement 🙏, I forgot about this, I'll try to create a new pr for this, as you propose, today. Edit: pr created at #1465 |
opacam
force-pushed
the
python-2.7.15
branch
from
7fd12fc
to
23bf987
Compare
|
My humble opinion, but I thought I'd wade in: @inclement re: dropping the python2 method, I would definitely hate that. We've been holding off moving our kivy code to python3 until kivy-ios supported it, as we need our apps to support both android and ios targets, but migrating our code isn't going to be a trivial process. A modern 2.7 (such as 2.7.15) would be an excellent place to stabilize things before making the move to python 3. That said, I believe python 2 has to be deprecated by 2020. It just shouldn't do so until python3 is a first-class citizen across the supported kivy targets. |
opacam
force-pushed
the
python-2.7.15
branch
from
6749b9c
to
169a4da
Compare
|
Those last commits grant libraries support for the python recipes (sqlite3, openssl and libffi). So far works well, at least with the test apps, the modules which depends on the optional libraries are getting build for both version of python, so I think that the core is good, and we are ready to move to the other remaining points mentioned at the first post: recipes/bootstraps compatibility and python2 harcoded entries. Would like to mention that two of this last commits, the clang feature (c7edf85) and the update of the openssl libs (71e855a) are not entirely my work, those are based on some patch created by @tito while he was helping with the @inclement's python3 recipe...so...all the credit goes to @tito to solve both problems 😆 PD: If @tito wants to introduce those features himself, no problem, I will solve the merge conflicts when merged to the master branch 😉 |
|
@opacam just wanna say thanks for that. I still have to maintain a python 2 app, and was going to install an old ubuntu just to be able to compile with the current Python 2.7.1. Didn't got a change to test yet. Good work so far! |
|
And i just made a PR for that openssl recipe, that introduce clang compilation. So it will impact your PR as well |
opacam
force-pushed
the
python-2.7.15
branch
from
fcc0096
to
4ea47dd
Compare
|
Ei guys, I Rewroted the history to be more readable and to fix some nasty conflicts appeared due to last changes in master branch and I will push it today. I squashed some commits of the start of the history (includes the fixes/enhancements courtesy of the reviewers). Also I split some of the last commits pushed (the ones related with non sdl2 bootstraps) because those were the source of the conflict, difficult to review...too many changes at once...now I think that it is more readable and easy to solve possible future conflicts with the master branch. Things to be mentioned:
|
|
The C++ additions in this pull request worry me a little. Is the C++ stdlib and boost now unconditionally included, or just on demand for the rare case where that is necessary? Maybe it would be better to put all the C++-relevant changes into a separate pull request so we can take a closer look and figure out how to integrate them in a way that doesn't overly complicate the process, if that is at all possible Edit: after further review the library loading changes look promising, since less stuff is hardcoded. so I like that 👍 |
|
😊 Thanks for the feedback, but don't lose your time reviewing those commits, at least until I push the latest changes with the fixed conflicts and some errors i found (precisely I forgot to add a patch for the libtorrent recipe so... it won't build successfully until push those changes). I will try to do it today night or tomorrow |
|
But just to add: if in doubt, this could also be done later. This looks like we want to merge it really fast, because it changes so many things. E.g. my SDL2 pull request looks like it might already cause new conflicts (which is fine, I'll resolve them, but it makes sense to get this in quickly before other stuff like my large amount of requests breaks it again) |
|
@opacam Could you please checkout into the I'm not sure which commits are weirdly merged and which are not, however if we merge it in this state, it'll do bad things to p4a git repo history. Also, amazing hard work! |
|
One additional thing I noticed: you added many more tests to |
|
|
||
| this.mActivity = this; | ||
| Log.v(TAG, "About to do super onCreate"); |
There was a problem hiding this comment.
this looks familiar, isn't this change on master already? https://github.com/kivy/python-for-android/blob/master/pythonforandroid/bootstraps/sdl2/build/src/main/java/org/kivy/android/PythonActivity.java#L66
is this pull request rebased, or is master merged into it somehow?
it would be quite good if we could see what is actually new, so if the history could be cleaned up that would be very useful
@Jonast , yes, it should. |
@KeyWeeUsr, all the last commits I pushed are the whole history of this pr rebased on the top of the latest changes on p4a master branch, applying the commands that you mentioned...so this is already done...no need to do it again...but...keep in mind that at some point I rewrote the original history (I mentioned that in some comment before) |
|
@opacam you should be able to (I assume rebasing given the convoluted merges might no longer easily be possible, but if it is, that would of course be even better) Edit: basically, all the changes I saw in a quick scan looked really good, but it's hard to see what actually changed since the merges you did kinda mess up the history |
|
@opacam What you could also try is squash all your commits after latest master to one, and then rebase that on latest master and see what that gives us. (make sure to make a safety copy of your repo before you do that!!!) but whatever we have right now, it's not a clean rebase |
|
I made an attempt to squash it here, just to show what I mean: https://github.com/JonasT/python-for-android/pull/1 as you can see, the particular change I highlighted above no longer shows up in the diff, because I made it a single changeset without merge commits left over based on current master All I did is squash all commits past the latest on master with |
|
Ok guys you are right, I squashed all the changes made in this pr until now into the new pr. No changes about the tests as @Jonast suggested, for now...I was planning to reduce the number of tests but I wanted that all of you see that all the builds success...we have to make sure that this works before merge or it will break the whole project... If you are in hurry to merge this, do it, still things to do, but most of the work it's done, plus the remaining points could be done apart and maybe it would be even better, so anyone can contribute. Moreover, this touches a lot of files and each time is harder to mantain, to review..so maybe it's time to merge...this is your decision guys 😉 |
|
@opacam it would also help if you simply made multiple, smaller pull requests for future changes. that makes it a little less of a problem to keep everything free of conflicts. This also makes it easier for us to see what you did and why. Just a hopefully helpful note for the future! Edit: I don't want to complain though, revamping |
|
Moving this to a new pr #1534...because of the mess in the history. |
This pr is intended to complement the amazing work of @inclement in his python3's recipe #1412. It refactors the python3's recipe into the recipe module and updates the python2 version to 2.7.15 using the same build process than the refactored python3's recipe. So, python2 and python3 will share the same build system and allow us to remove some specific source code for python2. Right now the build process completed successfully for both python versions and the python3 recipe also works as expected when installed on android devices...so...
Things to do: