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

Refactor dependencies and their tests #1686

Merged
merged 4 commits into from
Nov 12, 2023
Merged

Conversation

Julian-O
Copy link
Contributor

@Julian-O Julian-O commented Sep 10, 2023

Addresses #1685 .
Supersedes #1515.

  • setup.py

    • Remove false dependencies: virtualenv or sh
    • Add Cython, on behalf of p4a. Could be removed when p4a is updated.
    • Add sphinx for docs, pytest for tests, and kivy-ios for ios builds.
    • Flake8 fixes
  • android.yml

    • Stop installing Cython so setup.py can be tested.
    • Stop making installs editable. Unnecessary and deprecated by PEP.
    • Remove false dependencies: automake, and ssl hacks
    • Give job's names at the right level of abstraction.
  • ios.yml

    • Stop installing Cython so setup.py can be tested.
    • Stop making installs editable. Unnecessary and deprecated by PEP.
    • Remove false dependencies: cookiecutter and pbxproj
    • Specify [ios] so dependencies are correctly installed.
    • Give job's names at the right level of abstraction.
  • Dockerfile

    • Make Cython version match setup.py.
    • I don't understand Docker enough to know whether this runs setup.py, so I don't know if cython is required here or not.
  • test_python.py

    • Considered modifying to install buildozer[docs]. Decided the cost in development time for waiting for the install outweighed the need to show Sphinx could be installed via buildozer[docs], so just tried it on the command line myself, and left the code as is.
  • tox.ini

    • Simplify by assuming py3
  • Installation.rst

    • Move the pip install buildozer instruction into individual sections, so we can specify [ios]
    • Stop claiming Buildozer will work on earlier versions. setup.py explicitly rejects them.
    • Stop recommending to upgrade pip. If Buildozer needs a particular minimum version of pip, it should say so in setup.py.
    • Stop recommending to install Cython or virtualenv manually.
    • Minor copy-edits.
  • pypi-release.yml

    • Use build rather than calling setup.py, and thus avoiding a deprecation warning.
    • Clarify the task name

@misl6
Copy link
Member

misl6 commented Sep 10, 2023

Remove false dependencies: automake, and ssl hacks

For macOS, now dependencies are handled via python-for-android https://github.com/kivy/python-for-android/blob/develop/pythonforandroid/prerequisites.py (which needs some love, since we need to cache results), but an implementation for Linux (and Windows in future) is missing.

automake is required to build the libffi recipe.
https://github.com/kivy/python-for-android/blob/develop/pythonforandroid/recipes/libffi/__init__.py

@Julian-O
Copy link
Contributor Author

This has left me with lots of questions!

  • lib-ffi requires automake, autoconf and libltdl-dev. Why aren't we installing all three?
  • Why is the integration test passing? It doesn't build libffi? Will it ever?
    • If not, no need to install automake!
    • If recipes might be built, should we be installing everything that the documentation recommends?
  • Automake should be mentioned in installation.rst, right?
  • We still okay with losing the SSL hack?

Tackles #.
Supersede #.

* setup.py
	* Remove false dependencies: virtualenv or sh
	* Add Cython, on behalf of p4a. Could be removed when p4a is
	  updated.
	* Add sphinx for docs, pytest for tests, and kivy-ios for ios builds.
	* Flake8 fixes
	
* android.yml
	* Stop installing Cython so setup.py can be tested.
	* Stop making installs editable. Unnecessary and deprecated by PEP.
	* Remove false dependencies: automake, and ssl hacks
	* Give job's names at the right level of abstraction.
	

* ios.yml
	* Stop installing Cython so setup.py can be tested.
	* Stop making installs editable. Unnecessary and deprecated by PEP.
	* Remove false dependencies: cookiecutter and pbxproj
	* Specify [ios] so dependencies are correctly installed.
	* Give job's names at the right level of abstraction.

* Dockerfile
	* Make Cython version match setup.py.
	* I don't understand Docker enough to know whether this runs setup.py, so I don't know if cython is required here or not.
	
* tox.ini
	* Simplify by assuming py3
	
* Installation.rst
	* Move the pip install buildozer instruction into individual sections, so we can specify [ios]
	* Stop claiming Buildozer will work on earlier versions. setup.py explicitly rejects them.
	* Stop recommending to upgrade pip. If Buildozer needs a particular minimum version of pip, it should say so in setup.py.
	* Stop recommending to install Cython or virtualenv manually.
	* Minor copy-edits.
Rolling back to change to allow other changes to be merged while this one is resolved.
@misl6
Copy link
Member

misl6 commented Sep 10, 2023

lib-ffi requires automake, autoconf and libltdl-dev. Why aren't we installing all three?

Good question. We likely need to investigate if all 3 dependencies are still really required (I think is a YES for automake and autoconf, as automake and autoconf are a requirement for multiple recipes)

Why is the integration test passing? It doesn't build libffi? Will it ever?

libffi is a dependency of python3 recipe, so it's built.
The best guess is that the Github Runner comes with automake and autoconf installed. But, that may change.

If recipes might be built, should we be installing everything that the documentation recommends?
automake should be mentioned in installation.rst, right

IMHO ( and that's why I started a long time ago the prerequisites management via python-for-android ), buildozer docs and buildozer itself should not take care about recipes build dependencies, as the build dependencies may change (python-for-android update, set of recipes that the user need to build, etc ...) . And, more importantly, the build-time dependencies management should be less error-prone as possible for the user. (AKA: without reading the docs)

In my plans, the automated prerequisite management was a strategy to apply on both python-for-android and kivy-ios for all the supported platforms.
The idea was to introduce a check for every recipe, that runs before the pre-build so can check that all the recipe prerequisites are installed and suggest to install it (or perform an auto-install on CI environments).

So, the reply is: Yes for now, but I really would like to get rid of it.

We still okay with losing the SSL hack?

Yeah, since prerequisites are already handled when running on macOS (even if does not cache the result and impacts the build time)

I hope you now have a better view of the long-time plans. (and feel free to propose changes / make suggestions)

@Julian-O
Copy link
Contributor Author

That did help a lot.

In the short-term, to keep the momentum up, I have added auto-make back to the yaml script and also added it to the docs.

I appreciate this isn't the long-term goal.

@Julian-O
Copy link
Contributor Author

I am mulling over the goal.

My observation is that the whole Buildozer eco-system is about bootstrapping up until you get a build.

The YAML scripts are installing enough to get the PIP system going. The PIP system is enough to get Buildozer going. Buildozer tries to installs all the SDKs, NDKs, and p4a/kivy-ios/xcode requirements to get the build happening. p4a's recipes get the 3rd party libraries available for the final build.

So, one thought is: if p4a is going to become responsible for installing all the requirements of its recipes, why doesn't it handle the Android SDK, NDK, etc. itself too? Do we have two parts of the system both responsible for the same issue: prepping the dependencies for a build? How will that task be divided up?

Buildozer can't guess what future recipes will require. Should there be a handshake process where p4a reports the dependencies it will need, so Buildozer can install them? One reason not to like that, is that p4a can be run stand-alone without Buildozer.

I have had concerns about having a program install pip packages and parts of the OS, while still running. The pip team's discussion boards seem dead against this. It seems to be working, to date. I have been wondering if p4a and/or Buildozer will eventually need to have two passes - one to homebrew/apt-get/pip install/venv all the dependencies, and then spawn another process that runs in the next environment to do the actual work. But when I think about that, I think the division is already there - Buildozer sets up for p4a. If that is the architecture, it would be good for Buildozer to be able to determine what is needed for a recipe.

@misl6
Copy link
Member

misl6 commented Sep 10, 2023

If you're OK with that, I'm going to summarize this discussion, on a separate issue (so can continue even after this PR is merged), in order to involve other contributors, core devs and users.

This is a game-changing decision to take, that could help to easily maintain buildozer (and the whole ecosystem) in the future years to come.

@Julian-O
Copy link
Contributor Author

One final thought: It might be sufficient/a stepping stone for the p4a recipe system to check that the dependencies they need are installed, and if not, simply fail with a clear error message telling the user they need to install it.

Not as swish as installing as having the Buildozer/p4a system install the dependencies itself, but should be enough to reduce the support requests as beginners are given clear instructions on what they need to do to fix their own systems.

@Julian-O
Copy link
Contributor Author

Despite conversation about future goals, this should still be merged now.

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.

LGTM. Thank you!

@misl6 misl6 merged commit dda7eaa into kivy:master Nov 12, 2023
15 checks passed
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