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

Standardise ensure_dir and rmdir #2871

Merged
merged 6 commits into from
Aug 25, 2023
Merged

Standardise ensure_dir and rmdir #2871

merged 6 commits into from
Aug 25, 2023

Conversation

Julian-O
Copy link
Contributor

  1. A new version of Flake8 dropped while I was submitting this PR. There are several inconsequential edits to unrelated files, because Flake8 held my tests at gunpoint until I did it.

  2. When p4a wanted to create a directory, it used any of several different techniques:

    • it called os.mkdirs()
    • it called shutil.mkdir()
    • it called util.ensure_dir()
    • it called bootstraps.common.build.build.ensure_dir()
    • it called sh.mkdir()

The latter has terrible performance and isn't cross-platform.

  • I modified util.ensure_dir() to log if and only if it made a change.
  • I modified all directory creations to use util.ensure_dir().
  • I removed any now unnecessary logs and existence checks from the code, so the clients are now simpler.
  1. When p4a wants to remove a directory, it used any of several different techniques:
    • it called shutil.rmtree
    • it called sh.rm("-r")
    • it called subprocess.check_output(["mkdir"..])

The latter two have terrible performance and aren't cross-platform.

  • I created util.rmdir(). It logs if and only if it makes a change.
  • I modified all directory deletions (outside of test code) to use util.rmdir().
  • I removed any now unnecessary logs and existence checks from the code, so the clients are now simpler.
  • recipe.py had some weird code to delete then create then delete a folder. I simplified it to delete.
  1. Whenever I had to touch the imports, I sorted them to make them PEP8 compliant.

  2. A couple of the receipes (e.g. sqlite3) weirdly imported shutil indirectly, via toolchain, which caused a problem when toolchain stopped importing it (no longer required). Changed the recipe code to import shutil directly.

Note I only tackled two system calls; I didn't even look at removing single files. There are more to come, but I want to keep each review simple, and this one is already too complex.

@misl6
Copy link
Member

misl6 commented Aug 6, 2023

libmysqlclient is known to be broken (see: #2808). Can you please add it to BROKEN_RECIPES_PYTHON3 (ci/constants.py) so we can see ✅ on "Updated recipes" checks?

@Julian-O
Copy link
Contributor Author

Julian-O commented Aug 6, 2023

I don't pretend to understand what the implications of that request, but I went ahead and did it.

@misl6
Copy link
Member

misl6 commented Aug 6, 2023

I don't pretend to understand what the implications of that request, but I went ahead and did it.

Build tests are performed on pre-defined demo apps (always) and on updated recipes (when a recipe gets updated).

Since you edited libmysqlclient recipe file, the CI was trying to build it as part of the "Build Updated Recipes" test.
But, unfortunately, this recipe is broken (since a while), and the whole "Build Updated Recipes" CI pipeline failed.

Adding libmysqlclient to BROKEN_RECIPES_PYTHON3 allows us skip libmysqlclient build during the "Build Updated Recipes" test phase.

@misl6
Copy link
Member

misl6 commented Aug 22, 2023

Hi @Julian-O !

Sorry for the late reply, but I was on vacation 😀📷

Seems we still have some issues to perform the build as boost recipe is not working anymore. (boost is required by libtorrent, which has been edited by this PR)

Similarly to libmysqlclient, can you please add boost and libtorrent to BROKEN_RECIPES_PYTHON3 ?

shprint(sh.rm, '-rf', build_dir)
shprint(sh.mkdir, '-p', build_dir)
shprint(sh.rmdir, build_dir)
rmdir(build_dir)
ensure_dir(build_dir)
Copy link
Member

Choose a reason for hiding this comment

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

Uh 😅

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.

Excellent.
LGTM. Thank you!

@misl6 misl6 merged commit c6f76b9 into kivy:develop Aug 25, 2023
31 checks passed
@NomadDemon
Copy link

how about to migrate everything to "pathlib" from python ? :) it would be also nice improvement

@Julian-O
Copy link
Contributor Author

@NomadDemon: I agree. Given the amount of path-processing involved, pathlib would be a boon.

I am trying to keep my changes small and incremental to make reviewing easier and to reduce the chance that I break anything. I already have a lot on my plate. So I haven't included it here.

But what I have done is make the new Buildops library compatible with path-like parameters, so it is easier to migrate the clients that use it in the future.

@Julian-O
Copy link
Contributor Author

@NomadDemon: Sorry. Ignore that last sentence. That refers to Buildozer, not Python-For-Android.

@Julian-O Julian-O deleted the rmtreemkdirs branch August 31, 2023 10:30
misl6 added a commit that referenced this pull request Sep 17, 2023
* Update `cffi` recipe for Python 3.10 (#2800)

* Update __init__.py

version bump to 1.15.1

* Update disable-pkg-config.patch

adjust patch for 1.15.1

* Use build rather than pep517 for building (#2784)

pep517 has been renamed to pyproject-hooks, and as a consequence all of
the deprecated functionality has been removed. build now provides the
functionality required, and since we are only interested in the
metadata, we can leverage a helper function for that. I've also removed
all of the subprocess machinery for calling the wrapping function, since
it appears to not be as noisy as pep517.

* Bump actions/setup-python and actions/checkout versions, as old ones are deprecated (#2827)

* Removes `mysqldb` recipe as does not support Python 3 (#2828)

* Removes `Babel` recipe as it's not needed anymore. (#2826)

* Remove dateutil recipe, as it's not needed anymore (#2829)

* Optimize CI runs, by avoiding unnecessary rebuilds (#2833)

* Remove `pytz` recipe, as it's not needed anymore (#2830)

* `freetype` recipe: Changed the url to use https as http doesn't work (#2846)

* Fix `vlc` recipe build (#2841)

* Correct sys_platform (#2852)

On Window, sys.platform = "win32".

I think "nt" is a reference to os.name.

* Fix code string - quickstart.rst

* Bump `kivy` version to `2.2.1` (#2855)

* Use a pinned version of `Cython` for now, as most of the recipes are incompatible with `Cython==3.x.x` (#2862)

* Automatically generate required pre-requisites (#2858)

`get_required_prerequisites()` maintains a list of Prerequisites required by each platform.

But that same information is already stored in each Prerequisite class.

Rather than rather than maintaining two lists which might become inconsistent, auto-generate one.

* Use `platform.uname` instead of `os.uname` (#2857)

Advantages:

- Works cross platform, not just Unix.
- Is a namedtuple, so can use meaningful  fieldnames.

Also snuck in correction to typo in Readme which doesn't warrant its own review.

* Fix simple typos in comments (#2863)

One typo I introduced while trying to fix the other.

* `build_platform` should be all-lowercase (#2864)

* Docs: Fix typos and improved README quickstart (#2860)

* Made p4a apk build command more general - readme

* Fix twice spelled - readme

* updated p4a build command

* Cleanup `patching.py` (#2868)

Major changes to comments, param names, function ordering.
Removed deprecated LooseVersion
Move os.uname to platform.uname
Added win32 check
Windows fix

* Update Python versions in CI tests

Python 3.11 was released October 2022.
Python 3.7 went end-of-life June 2023

* Update documentation to reflect Python 3.7 being end-of-life.

* Correct check for `--sdk` option (#2870)

* Factor out dependency checking. Use modern version handling (#2866)

LooseVersion used again


Handle bad SDK versions

* Linter fixes (#2874)

The errors were:
```
pythonforandroid/bootstrap.py:136:5: F811 redefinition of unused 'name' from line 73
pythonforandroid/build.py:111:5: F811 redefinition of unused 'libs_dir' from line 82
pythonforandroid/build.py:127:5: F811 redefinition of unused 'aars_dir' from line 83
pythonforandroid/graph.py:48:12: E721 do not compare types, for exact checks use `is`
pythonforandroid/graph.py:163:20: E721 do not compare types, for exact checks use `is`
tests/test_build.py:39:41: E231 missing whitespace after ','
tests/test_build.py:40:58: E231 missing whitespace after ','
tests/test_build.py:41:61: E231 missing whitespace after ','
tests/test_build.py:42:71: E231 missing whitespace after ','
```

* Remove deprecated FlatDir in Gradle template (#2876)

Based on lessons from https://stackoverflow.com/questions/68215302/using-flatdirs-should-be-avoided-because-it-doesnt-support-any-meta-data-format

* Standardise `ensure_dir` and `rmdir` (#2871)

* Standardise ensure_dir and rmdir

* Standardise ensure_dir and rmdir

* Add libmysqlclient to broken list

* Libtorrent failing to be rebuilt

* Add boost to broken recipes list

* Standardise on move (files, directories) (#2884)

* Add new util function (and tests) called `move`.
* Change references to sh.mv to use move (as it is faster and cross-platform).
* Change conditional "mv -t" to a for loop.

* Standardise on touch files (#2886)

Existing code shells out to run the `touch` Unix command.
This can be done faster (and cross-platform) with `pathlib`'s touch method.

* Created a utility to accept string paths or pathlib paths and applies touch.
* Created test for it.
* Update all instances to use it, and updated their tests.

* Update CHANGELOG.md and bump version to 2023.09.16

---------

Co-authored-by: HyTurtle <[email protected]>
Co-authored-by: Steve Kowalik <[email protected]>
Co-authored-by: Mathias Lindström <[email protected]>
Co-authored-by: Ansh Dadwal <[email protected]>
Co-authored-by: Julian <[email protected]>
Co-authored-by: Kulothungan U.G <[email protected]>
Co-authored-by: Andre Miras <[email protected]>
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.

3 participants