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

[WIP] Restores the ability to compile the python files into .pyo/.pyc (for both versions of python) #1601

Merged
merged 8 commits into from
Jan 27, 2019

Conversation

opacam
Copy link
Member

@opacam opacam commented Jan 19, 2019

We already had before this feature for python2 (python2legacy now) and we loosed it with the recently merged core update. Also we will be able to compile our python install files for python 3 (new feature for python3). But be aware that to achieve this we must modify a little our source code because as of python 3.5 the .pyo extension for compiled python files has been removed in favour of .pyc extension, so it requires to modify start.c and PythonActivity.java files, which only contemplated the .pyo extension.

Notes:

  • the default behaviour will be to compile all python files to the corresponding compiled extension, so this way the apk will run faster
  • also we compile the code with the option -00 which will strip the docs, so we will generate lighter apks
  • this change affects bootstraps: sdl2, webview and service_only

See also:

Status: I leave the WIP status because this may conflict with the sdl2 update by @Jonast (#1528) and, probably, the merge conflicts will be aesier to solve here than in there, so...let @Jonast decide if we must remove the WIP status (be aware that this will also be an enhancement)

Thanks to @tito which make me notice that we had loose that feature

@hackalog
Copy link

Just a note (learned in kivy/kivy-ios#340) If you are going to ship only .pyc files, it is mandatory to run python with -OO, or to set PYTHONOPTIMIZE=2.

@opacam
Copy link
Member Author

opacam commented Jan 19, 2019

Ahhh, I didn't know about that, I leaved a todo in there to make it optional this option. I will remove that and make sure that -00 is always aplied...and I will update the post.

¡¡¡Thanks @hackalog!!!

😄

args += ['-OO', '-m', 'compileall', '-b', '-f', dir]
else:
args += ['-OO', '-m', 'compileall', '-f', dir]
subprocess.call(args)
Copy link
Member

Choose a reason for hiding this comment

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

Why not using the sh module?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it fails:

  • in python 3 fails when the compilation is about to finish (after compile zipfile module) without a clear message of why it failed...still seems that the files are compiled well but to bypass the error we would had to put a try/except (mmm...I don't like that without knowing why it fails...)
  • in python 2 the compilation not even starts and complains about elf32 ...

Then I realized that we have another call to compile all files in meth:~pythonforandroid.bootstraps.common.build.build.compile_dir and in there we used the subprocess.call...so I tried with that...and it works fine for both versions of python 😄

🤔...my guess is that some of our environment variables is causing some kind of conflict between hostpython and python when we try to use the compileall command with sh...but I'm not sure about that...maybe @inclement, @tito, @hackalog or @Jonast could give us some more light in here...

Copy link

Choose a reason for hiding this comment

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

@AndreMiras is the sh module really that great anyway? First time I encountered I had to dig a while to even find out how it works, and while it may look more pythonic it just duplicates what subprocess already does with little benefit other than requiring new coders to look up how it works. I wouldn't mind if we stopped using it altogether

@opacam you could just specify the environment as a dictionary by passing dict(os.environ), this should work with both subprocess and sh. Then you could print it out before the call and check. However, as I just wrote above, I'm kind of in favor of not using sh anyway for new code

Copy link

@ghost ghost Jan 20, 2019

Choose a reason for hiding this comment

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

Just to add as extra explanation why you'd have to look up how sh works: because it's unclear how it handles 1. the environment, 2. stdout/stderr buffering, 3. stdout/stderr redirect, and similar things. (for some of which p4a is using a really horrible wrapper in the logging file) So sure, the call itself is simple enough that it's not much of an issue, but if you care for details the sh module IMHO is really kind of a problem just because it isn't obvious for someone only familiar with submodule what it'll do or which parameters make it do certain things

Copy link
Member

Choose a reason for hiding this comment

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

I'm not opposed to using subprocess, but I do think the sh module is pretty nice. I don't really see anything you mention as an issue, it isn't obvious how subprocess works either until you read the documentation, although it's fair to say that more people will have done so in advance. Most people will never have to read the details of either.

That said, sh is overused in p4a partly due to me being lazy when originally duplicating the functionality of the old-p4a shell scripts.

I would most like to use some nice pythonic file management module for a lot of things that in a shell script would be calls to cp, rm, mkdir etc.. It annoys me that shutil and os are both quite poorly structured. https://github.com/inclement/nosh is the kind of thing I had in mind.

Copy link

@ghost ghost Jan 20, 2019

Choose a reason for hiding this comment

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

@inclement not sure I agree with Most people will never have to read the details of either, at a point where that starts to matter people will have, and that's exactly the point where sh then ends up problematic. Therefore I personally prefer to stick to core modules for such basic functionality even if they might arguably be not nearly as pythonic as they should be, but of course that's just my opinion

Edit: this definitely includes shutil btw, because e.g. the error management and other corner cases of things like shutil.rmtree can be pretty important to know. I'm against wrapping that into another module where that again might not be obvious at first glance

Edit 2: just to support this further, the problem is also not only that the detail semantics might change in non-obvious ways, but that it's not always obvious where to find them in the first place. The first time I encountered sh it took me a while to find the right page, whereas if I have a question about subprocess I know exactly where the relevant docs are and I'll have them up in 5 seconds. So this is also something that factors in. In the end all wrappers come with a friction cost, and I'm just not convinced sh is worth that for the small pythonic benefit

Copy link
Member

Choose a reason for hiding this comment

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

@opacam OK failing is a fair enough point, I was just wondering anyway 😛
@Jonast I didn't know the sh module before knowing p4a, I can't tell if it's that great, but I can tell it's not that bad. I'm also more a fan of going with core modules as much as possible. I was asking @opacam why he didn't use, because this is what's being used by the project so far, so not using it could have a reason I was curious about

@AndreMiras
Copy link
Member

Pretty awesome pull request!
I made a quick check from the phone.
I can't wait to review it properly and test it when I get on a laptop

@opacam
Copy link
Member Author

opacam commented Jan 20, 2019

Thanks @AndreMiras 😊

Update: I have done a rebase for the branch of this pr on top of the current master branch, to make sure that the tests are properly passed so we test this pr and the recently merged fix for sdk license error...here we go...

…stribution files

Because this will allows us to restore the ability to compile python code to .pyo/.pyc (python2/python3)
Because the compilation to .pyo extension only exists for python2. Since python >= 3.5 the extension for the compiled files is .pyc.
Because as of Python 3.5, the .pyo filename extension is no longer used, see:
  - `PEP 488 -- Elimination of PYO files` (https://www.python.org/dev/peps/pep-0488/)
  - `PEP 3147 -- PYC Repository Directories` as to why a separate directory is used (https://www.python.org/dev/peps/pep-3147/)

Also moves comment to the right place
… depending on our python version

Because this way the initialization of our apk it will be faster. We must use the .pyc extension for our python 3 because as of Python 3.5, the .pyo filename extension is no longer used and has been removed. Notice that the command to compile the files for python 3 is slightly different (`-b` option added). This is done that way because otherwise our compiled files would be put in `__pycache__` folder and this option makes that the compiled files gets putted in the same directory than the compiled file.

See also:
  - `PEP 488 -- Elimination of PYO files` (https://www.python.org/dev/peps/pep-0488/)
  - `PEP 3147 -- PYC Repository Directories` as to why a separate directory is used (https://www.python.org/dev/peps/pep-3147/)
Because since the introduction of the ability to compile our python installation files, the copy command fails (because of the relative path and the fact that the compile command, recently introduced, probably changes the environment path, so when we try to copy the library the target directory is not found)
Because as of Python 3.5, the .pyo filename extension is no longer used

See also: `PEP 488 -- Elimination of PYO files` (https://www.python.org/dev/peps/pep-0488/)
….java files

Because as of Python 3.5, the .pyo filename extension is no longer used and has been eliminated in favour of .pyc, and we recently restored the capacity to compile our python installation files, so we must update this files in order to make it work the compiled files for both versions of python

Note: this changes affects all bootstraps excepts pygame, because the pygame bootstrap can only be used with python2legacy and the extension of the compiled files are already set to .pyc...which is fine for now...because we cannot use the pygame bootstrap with python3

See also: `PEP 488 -- Elimination of PYO files` (https://www.python.org/dev/peps/pep-0488/)
…rvice_only)

Or we will not be able to use our apk because we will not have the necessary compiled files (.pyc/.pyo)
@inclement
Copy link
Member

@opacam I'm looking to merge this but it has conflicts due to some recent merges, would you be able to rebase it again? I would normally do it myself, but now that we have mandatory PR approval I wouldn't then be able to merge it.

@opacam
Copy link
Member Author

opacam commented Jan 27, 2019

@inclement, no problem, let me do one more test with the conflicts solved and I will push the changes....testing right now

@inclement
Copy link
Member

@opacam Thanks, I'm also testing it, I just don't think I can push my merged version without someone else approving it.

@opacam
Copy link
Member Author

opacam commented Jan 27, 2019

Still don't merge, let me finish the test

@opacam
Copy link
Member Author

opacam commented Jan 27, 2019

python2 build and runs fine (tested with sqlite openssl testapp)
tetsting for python3...

@opacam
Copy link
Member Author

opacam commented Jan 27, 2019

python3 is also ok (tested with sqlite openssl app)

@inclement, now it should be ready

@inclement
Copy link
Member

I've also tested it, seems great. Thanks!

@inclement inclement merged commit d22246d into kivy:master Jan 27, 2019
@opacam opacam deleted the fix-compile-to-pyo branch June 7, 2019 00:49
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.

4 participants