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

Migrate to use cmd, cmd_expect and file_extract from Buildops. #1674

Closed
wants to merge 1 commit into from

Conversation

Julian-O
Copy link
Contributor

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.

* 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.
@Julian-O
Copy link
Contributor Author

Nevermind. I will try again soon.

@Julian-O Julian-O closed this Aug 30, 2023
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.

1 participant