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

Introduce buildops #1647

Merged
merged 1 commit into from
Aug 22, 2023
Merged

Introduce buildops #1647

merged 1 commit into from
Aug 22, 2023

Conversation

Julian-O
Copy link
Contributor

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:

  • They consistently all log to debug, only when they are making a change.
  • When an operation is a no-op (e.g. deleting a non-existent file) they all try to handle it quietly.
  • They consistently accept string paths or Pathlib path-like instances. (None of them now accept *arg parameters with multiple path parts.)
  • Wherever possible, they are now platform independent (so Windows can one day be supported). (e.g. Zipfiles are now extracted with the zipfile library rather than a platform dependent call to shell.)
  • They all have unit-tests.
  • A call to (deprecated) pexpect.spawnu was updated.
  • They never raise a base 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() uses pexpect, 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.

At this point, it is never called.
@Julian-O
Copy link
Contributor Author

I am keen to get this PR merged, because it will unblock a number of other PRs.

If the size of the review is proving a difficulty, I propose the following:

  • I comment out all of the code! This will be a comment-only PR.
  • As I migrate the clients to use this code, I uncomment the relevant parts, and just the parts that are uncommented need to be reviewed.

That way, you can see the end-goal and why things are being changed, but don't need to review the details all at once.

Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

I reviewed the code, as-is (unused), and the tests.
The code looks good.

As you might imagine, I'm quite concerned about adding unused code to our repository, as in the past I've seen adding code for a future refactor that never happened (not on a Kivy project). This unused code has been around in the repo for ~3 years, before getting completely removed.

But, you earned a quite amount of trust during your previous PRs, so I want to trust you again.

@misl6 misl6 merged commit c529b10 into kivy:master Aug 22, 2023
12 of 14 checks passed
@Julian-O
Copy link
Contributor Author

@misl6: I share your concern. Life is going to interfere with the plans; it always does. So, this is a race to beat that!

I am keen to maintain as much momentum as possible, to get these changes into the base as quickly as practical. But I also want to maintain a high level of code review, because I am playing with the core code of Buildozer. I think that is going to be the greatest bottleneck, and whatever I can do to make things easier, let me know.

Next PR is coming within hours.

@Julian-O Julian-O deleted the Step1FactorOutBuildOps branch August 22, 2023 22:15
aorizondo pushed a commit to aorizondo/buildozer that referenced this pull request Aug 28, 2023
At this point, it is never called.
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.

2 participants