Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am planning a moderately large refactor (not game-changing, but bigger than a typical PR) so have divided this change into several steps, purely to make reviewing easier and to increase confidence in the changes.
The Buildozer class includes a number of very basic OS-level methods required for building, but which require no knowledge about what is being built.
In particular:
checkbin()
cmd()
cmd_expect()
mkdir()
rmdir()
file_matches()
file_exists()
file_rename()
file_copy()
file_extract()
file_copytree()
download()
The Buildozer class is too big and has too many responsibilities, so I want to move all of these into a separate file,
buildops.py
, and update the clients.However, this PR is just creating the new file, ready to be adopted, but NOT moving the clients across yet. I plan to do that in a number of stages, so each review is straightforward.
Changes include:
Buildozer's Exceptions were moved into a separate file to prevent cyclic import dependencies. [Again, these aren't used yet, but will be as I update the clients.]
file_remove() has been added to the list - it isn't (yet) used by clients, but is used internally by this library.
file_rename() was changed to rename() because it was being used to rename directories.
The functions were then all updated:
*arg
parameters with multiple path parts.)zipfile
library rather than a platform dependent call to shell.)pexpect.spawnu
was updated.Exception
instance.There were three places where Windows could not be supported:
extract_file()
, surprisingly, will execute .bin files. That is impossible on Windows. I am not sure if this is desirable on other platforms, but I haven't investigated to see under which conditions this happens. An assert statement now protects the command.cmd_expect()
usespexpect
, which is not available on Windows. I have spent very little effort on finding a replacement. For now, it means that the option to automatically accept the Android SDK licence can't be used on Windows. An assert statement now protects the command._ensure_virtualenv()
makes a call to bash shell. I experimented with some portable replacements, but ultimately this command is never run with pythonforandroid, so I could never see it run in Windows. I decided to leave it in Buildozer, as is.By far the biggest change was
cmd()
.The old code used select on stdin and stdout. This is not possible on Windows. A complete reimplementation, using threads and queue.Queue, now works on all platforms.
The available parameters to cmd and their defaults, were obscured by extensive use of
**args
. This also made testing difficult, because it was never clear what options might be passed. Now all the legal parameters are explicitly spelled out, and their defaults are clearer.The "sensible" parameter was unclear and unneeded, so has been removed. Now
cmd()
always takes a tuple of command parts and always logs it as a string.cmd() now returns a namedtuple rather than a regular tuple, which makes using the results clearer.
cmd() now accepts path-like objects as well as strings.
In the old code, cmd() would accept an env as an optional parameter, so the caller could use (for example) a virtualenv's environment. If no parameter was provided, it used the hosts' os.environ, and then overrode it with Buildozer's environ (which only included a few the deltas).
However, buildops code no longer has privy access to Buildozer's environ, so cmd() would need the use to pass both the overriding environment (if any) and the deltas (if not). The simpler solution was to make the env a required parameter, and let the client decide which environment was best.
[Future note: Having every client do a merge is confusing. Instead, I will change the definition of
Buildozer.environ
. It will contain the complete desired environment, not just the deltas, so each of the clients can simply reference it. It will be a few iterations before you see this change get to review.]In future PRs, I plan to migrate clients to adopt each of the buildops function one-by-one (i.e. all the file_extracts as one PR), so the review can focus on just one set of semantics at a time and hopefully agree the new code achieves the same (or better) results as the old code.
Again, I am dividing this PR up to make life easier for the poor reviewers. They are the bottleneck. If they would prefer a different approach, let me know.