Migrate to use cmd, cmd_expect and file_extract from Buildops.

[Julian-O]

[Attempt No. 2]

This is part of a bigger refactor to reduce the size and complexity of the Buildozer class (and bring it closer to cross-platform).

• Remove buildozer's cmd, cmd_expect and file_extract functions, and tests.

• Migrate clients to use buildops.cmd()

  • Note that the implementation is completely different.
    • Particularly, it doesn't depend on fcntl, so it runs cross-platform.
    • It is much more explicit about the parameters it accepts.
    • It no longer accepts sensible param; quiet has the same effect.
    • It returns a named tuple, rather than a tuple; took advantage of replacing "magic number" indices with meaningful names.
    • The treatment of environment variables has changed.
      • Old system:
        • buildozer.environ contained a list of deltas - overrides to the os.environ variables.
        • buildozer.cmd()'s env parameter was bigger override.
        • buildozer.cmd() would use the env parameter (and only the env parameter), unless it were None, in which case it would grab os.environ and apply buildozer.environ on top.
      • New system:
        • buildozer.environ isn't available to the library function buildops.cmd().
        • So, now the environment must be passed. It becomes the client's task to pass it.
        • Rather than having every client have to merge the os.environ and buildozer.environ together, the definition of buildozer.environ has been changed. It now is not the delta, but the complete set of env vars. (It now defaults to os.environ, rather than [].)
        • It may be the case that many commands do not care about env vars, and the clients would rather omit the parameter. buildops.cmd() could be made to default to os.environ. I did not do this, because I can't tell which need special env vars. I wanted to ensure that every call was updated to include the env. Once that migration is done, it could be modified to accept a default. Leaving that for future refactors.
    • Most commands being run, at first inspection at least, look like they should work on darwin, linux and win32. Added assert statements where the command is clearly not going to run cross-platform.
    • ios.py contained a complicated pipe command that required a shell. Rather than supporting shells in cmd, I ported the client code to Python - which is a better solution for maintenance.

• Migrate clients to use buildops.cmd_expect()

  • This remains platform-dependent, but is only used to automate accepting Android SDK licenses.
  • Making this platform-independent is left for future refactors.
  • Also now requires env param.

• Migrate clients to use buildops.file_extract()

  • The implementation has been substantially rewritten (platform-independent and should be faster because it doesn't normally spawn shells.)
  • It still, rather unexpectedly, will run a .bin file if passed. I do not know if that feature is ever used.
    • Running a .bin depends on cmd() and also now needs an environ parameter. [This is the reason I had to bundle it with this larger PR.]
    • Running a .bin is not platform independent, but this is likely to never come up in practice - Windows versions of SDKs/NDKs etc don't require .bin files to be executed.
  • Fixed one call from osx.py to use extract_file instead of cmd('unzip')

• Last minute: Rolled back changes to extracting zipfiles.

  • Buildops.extract_file now uses the unzip shell command (which is not platform dependent).
  • Python's zipfile doesn't support extracting file permissions. I modified extract_file to chmod as appropriate.
  • That wasn't enough. pythonforandroid.toolchain create was failing with the config script complaining about an error in /home/runner/work/buildozer/buildozer/.buildozer/android/platform/build-arm64-v8a_armeabi-v7a/build/other_builds/libffi/armeabi-v7a__ndk_target_21/libffi and hence C compiler cannot create executables, but only on the CI machine; it works fine on my VirtualBox test machine. Still figuring out what might be different. Hints welcome!
  • Changed tests to match. (Needed more from the logger mock, and behaviour when the file is missing is less specific. In practice, this doesn't occur.)

• Trivial changes

  • Corrected the grammar of a few comments.
  • Improved the indenting of some of Buildops code.

[misl6]

Rather than having every client have to merge the os.environ and buildozer.environ together, the definition of buildozer.environ has been changed. It now is not the delta, but the complete set of env vars. (It now defaults to os.environ, rather than [].)

I guess we can then remove the following:

buildozer/buildozer/targets/android.py

Lines 304 to 307 in 4033c90

	if 'PATH' in self.buildozer.environ:
	path.append(self.buildozer.environ['PATH'])
	else:
	path.append(os.environ['PATH'])

And use self.buildozer.environ instead of os.environ or environ where appropriate?

[misl6]

Why del ?

I guess we now only need to check that into buildozer.environ now PATH is available and contains what we expect.

[Julian-O]

I agree that:

• As suggested by the other comment, the check_requirements code can now be simplified to assume the presence of PATH.
• If we do that, this test code should not call del.

I will work on an update.

---

But to answer your question, my thought process was:

The old test confirmed (with an assert) that the path wasn't present (in the delta) before, and then it was present afterwards, so it had been set by check_requirements().

Because it was no longer a delta, if I wanted to show that it was added by check_requirements(), I needed to remove it beforehand. Hence, the del.

Looking at the existing check_requirements() code, that means both versions of the test only hit one of the two code-paths, but extending the test's code-coverage of android.py wasn't my goal. E

(Extending the test's code-coverage of buildozer\__init__.py is, and in my prototypes it is up to well over 90%.)

[misl6]

LGTM. Thank you!