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

Introduces pythonforandroid/prerequisites.py (Experimental). This allows a more granular check and install process for dependencies on both CI jobs and users installation. #2591

Merged
merged 2 commits into from
May 10, 2022

Conversation

misl6
Copy link
Member

@misl6 misl6 commented May 1, 2022

✂️ (partially) from #2586

Instead of installing the JDK from brew, we can use actions/setup-java@v3 which correctly handles the JAVA_HOME as macos-latest environment comes with multiple versions of the JDK pre-installed.

On our self-hosted runner, the download via actions/setup-java@v3 seems to be correctly handled (see runs on #2586)

During a discussion with @AndreMiras (See below), I decided to propose a different approach for the prerequisites check and related installations, which will allow more granular control over prerequisites, which may lead to potential (unnoticed) issues.

This PR:

  • Introduces pythonforandroid/prerequisites.py (Experimental as it doesn't support all the python-for-android prerequisites)

  • Allows handling prerequisites in the same way on Github Action and on the developer machine.

  • PYTHONFORANDROID_PREREQUISITES_INSTALL_INTERACTIVE=0 disables the interactivity (which is enabled by default) of the prerequisite install section.

  • Even if check_and_install_default_prerequisites() is called when the toolchain is invoked, ATM we should keep a Install prerequisites via pythonforandroid/prerequisites.py (Experimental) step, as install_android_ndk_sdk needs a valid Java installation. This will be changed in future.

  • This will allow, when completed, to simplify the onboarding process of a new python-for-android / buildozer user, as python-for-android is expected to run all the checks and (eventually) the needed installations. Lowering the "first-customer" entry barriers will mean fewer support tickets and increasing the Kivy ecosystem fan base 😄 .

  • "Check, even multiple times before installing anything" approach. As an example, JDKPrerequisite is checking the availability of a valid JDK over multiple places, installing a new one should be a last resort, and an installation should be done only if the user consents.

As a working example, #2586 has been updated to use this approach.

  • ubuntu-latest, doesn't have any rule (yet) so just ignores it.
  • macos-latest have a valid JDK pre-installed, but is not the default one. JDKPrerequisite notices it and sets JAVA_HOME to the valid JDK path.
  • apple-silicon-m1 doesn't have a valid JDK installed. JDKPrerequisite notices it and downloads+installs OpenJDK17U-jdk_aarch64_mac_hotspot_17.0.2_8, allowing the CI run to complete.

@misl6 misl6 requested a review from AndreMiras May 1, 2022 13:51
AndreMiras
AndreMiras previously approved these changes May 2, 2022
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

That works for me if we prefer it that way.
One thing I liked about installing it manually was that in a way it documented how to reproduce it from the command line in macOS.
Say that I didn't have a macOS laptop, but I was testing things on a VM 😏
https://github.com/AndreMiras/VagrantBoxes/tree/master/osx-sierra
Then this can be useful because you can up your VM and run the same script

@misl6
Copy link
Member Author

misl6 commented May 4, 2022

That works for me if we prefer it that way.
One thing I liked about installing it manually was that in a way it documented how to reproduce it from the command line in macOS.
Say that I didn't have a macOS laptop, but I was testing things on a VM 😏
https://github.com/AndreMiras/VagrantBoxes/tree/master/osx-sierra
Then this can be useful because you can up your VM and run the same script

Considering that improving the setup process and docs should be a priority, I got your point and I agree.

I will try to investigate an approach that works for both the Github Actions environment and the one which could have a user, so let's keep this PR paused.

Currently, during the CI run we're installing the JDK via brew install --cask adoptopenjdk13, but without changing the JAVA_HOME env var, which means that during the CI run the default (In that case OpenJDK 8 IIRC) is being used.
That has been un-noticeable until the newer Gradle (introduced in #2586) was expecting an OpenJDK version > 11.

Just thinking out loud: I imagine that a "prerequisites lookup and install script" that inspects the user / CI runner environment and asks to perform the prerequisites installations may avoid installing an un-needed JDK version for both the users and CI runners?

@AndreMiras
Copy link
Member

Oh right I see thanks for sharing more context.
Yeah maybe an helper script that we ship with p4a and use in the CI could make sense too indeed.
But I don't want it to make it more painful for you to work on the CI then.
I mean having a documented and easy to reproduce process is one thing, but it should be balanced with the rest.
So if that change from your PR end up being much easier than something else, we can stick with it.
Or we can also merge it as it is and make a follow up later to introduce the reusable script

@misl6 misl6 force-pushed the feat/use-setup-java-for-macos branch 13 times, most recently from 54c7a30 to 0847b9c Compare May 7, 2022 13:10
…ows a more granular check and install process for dependencies on both CI jobs and users installation.
@misl6 misl6 force-pushed the feat/use-setup-java-for-macos branch from 0847b9c to 0de9354 Compare May 7, 2022 13:22
@misl6 misl6 changed the title On macOS CI jobs, use actions/setup-java@v3 instead of manually installing an outdated brew cask Introduces pythonforandroid/prerequisites.py (Experimental). This allows a more granular check and install process for dependencies on both CI jobs and users installation. May 7, 2022
@misl6 misl6 force-pushed the feat/use-setup-java-for-macos branch from efd6260 to 72e9720 Compare May 7, 2022 15:39
@misl6
Copy link
Member Author

misl6 commented May 8, 2022

@AndreMiras I may have found a valid solution, which will allow what we just talked about.
I've updated the PR description.

Let me know your thoughts. 😄

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Thanks for giving it a though and providing an implementation.
Looks good to me, that's probably enough for a start.
In future post merge iteration we may want to:

  • set the JDK version from a single place
  • cover the change via tests
  • document the feature for user interested to use it

@misl6
Copy link
Member Author

misl6 commented May 10, 2022

Thanks for giving it a though and providing an implementation. Looks good to me, that's probably enough for a start. In future post merge iteration we may want to:

  • set the JDK version from a single place
  • cover the change via tests
  • document the feature for user interested to use it

Yeah, absolutely.

I've scheduled a major docs review for python-for-android, which will land just after the #2586 splicing is completed, as it seems (at least on macOS) we have some outdated docs.

Instead, tests will inevitably land in the next related PR, as (now that I can have a clearer idea of which are the needs) I will switch to a TDD approach for the next prerequisites.

@misl6 misl6 merged commit 4baec32 into kivy:develop May 10, 2022
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