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

gh-116622: Add Android testbed #117878

Merged
merged 14 commits into from
May 1, 2024
Merged

gh-116622: Add Android testbed #117878

merged 14 commits into from
May 1, 2024

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Apr 14, 2024

This PR adds a testbed app to run the Python test suite on Android. Instructions for using it are in Android/README.md.

Most of the files in this PR are boilerplate generated by the Android Studio new project wizard. To make it easier to review, I've split it into two commits:

  • An "empty project" commit with the output of the new project wizard.
  • A "testbed implementation" commit with the custom code.

Android app projects require a binary gradle-wrapper.jar, and two gradlew scripts to launch it. The integrity of these files can be verified by creating a new project in Android Studio Hedgehog – all three files in the new project should be identical to the versions in this PR.

Since myself and @freakboy3742 are now on the triage team, I've also added us to the CODEOWNERS file for Android and iOS respectively.

@mhsmith mhsmith requested a review from encukou April 14, 2024 18:40
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the nature of the recent xz exploit - is there any way that this artefact can be obtained programatically, rather than being embedded as a a binary?

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would make sense to have this in a separate repo, so it's only downloaded when testing Android support?

Copy link
Member Author

Choose a reason for hiding this comment

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

Size isn't a concern, as it's only 59 KB, and it doesn't ever need to be updated. In fact, the version of this file in the Chaquopy demo hasn't been updated in 7 years.

So I think all that's necessary is for a core developer to verify that the file is identical to Android Studio's own copy. You can do that by checking the Android Studio repository here – use the "tgz" link to download just that directory.

Copy link
Member

Choose a reason for hiding this comment

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

peanut gallery comment: If anyone does have a license problem with this being in the repo, the other option is to have this be downloaded at build/test time from the canonical Android Studio repository and verify that its sha256 matches. (it means there'd be a required network connection for this unless someone pre-fetches it themselves, but that usually isn't a problem)

From a CPython perspective I believe the only place we'd be redistributing this beyond the git repo is in our source releases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, based on the discussion below, I'll make it a scripted download rather than including it directly in the repository.

@encukou
Copy link
Member

encukou commented Apr 18, 2024

I can confirm that the gradle-wrapper.jar matches the one generated by Android Studio Hedgehog from https://developer.android.com/studio/archive -- specifically, “Android Studio Hedgehog | 2023.1.1 Patch 2 January 23, 2024”.

But, one thing that's not clear to me: what is the license for these files?

In the Android Studio repository, I couldn't find any license at all.

The Android Studio download & installer made me agree to a “non-assignable, non-sublicensable” license, which limits what the SDK can be used for, and doesn't cover everything that Python's license allows.

@mhsmith
Copy link
Member Author

mhsmith commented Apr 18, 2024

The three files gradlew, gradlew.bat and gradle-wrapper.jar (collectively the "Gradle wrapper"), originate from Gradle, which is used by Android Studio but is an independent project. Like the rest of Gradle, the wrapper is covered by the Apache license, and there's a notice indicating that at the top of the scripts.

@mhsmith
Copy link
Member Author

mhsmith commented Apr 21, 2024

Concerns were raised on Discord that the source of the Gradle files wasn't clear enough, so I've updated them from the current version of Gradle (8.7). Here's how to verify them:

  • The gradlew and gradlew.bat scripts are from the root of the Gradle repository at https://github.com/gradle/gradle/tree/v8.7.0.

  • The gradle-wrapper.jar is from the Gradle release:

    • Go to https://gradle.org/releases/
    • Download the v8.7 "binary-only" package
    • Inside the ZIP, find lib/plugins/gradle-wrapper-8.7.jar
    • Inside the JAR (which is also a ZIP file), find gradle-wrapper.jar, which is the file I've just added to this PR.
    • If you look inside the inner JAR, you'll see that this version also contains a license notice.

@encukou
Copy link
Member

encukou commented Apr 22, 2024

Thanks.
I am still worried about distributing compiled artifacts without corresponding source code, and I would prefer to put this in a separate repository, downloaded on demand for Android tests, than to include it in the CPython repo.

The testing instructions added in Android/README.md are rather manual; I don't think an extra git clone would make things much worse for users.

Including the jar means we're trusting a third party -- Gradle -- and their build process. I don't have a reason to trust them, and I don't think that CPython should implicitly vouch for them.
The text files are at least auditable -- though it's tedious. I don't see why we need to set RIGHT_MARGIN to 80, or include a fully commented-out config file example, to run the tests. If we treat these like binary blobs -- output of Android Studio -- they seem fine (and perhaps non-copyrightable); but they should go in e.g. cpython-bin-deps. If we treat them as source code, then I'll review them, and I'll ask you to prune things that are not needed -- but I think that'd just add work for everyone.

If there are any license issues found later, it's much easier to remove things if they're in an isolated repo/branch. (And it's also easier for over-cautious redistributors to not include the testbed.)


Note that I am a Linux distro packager by training, conditioned to include sources and avoid bundling. I might not be representing the CPython project well. I am definitely unfamiliar with mobile development practices. If you think I'm being unreasonable, please do seek other opinions, e.g. on Discourse.

@mhsmith
Copy link
Member Author

mhsmith commented Apr 22, 2024

Thanks, that all makes sense.

I am still worried about distributing compiled artifacts without corresponding source code, and I would prefer to put this in a separate repository, downloaded on demand for Android tests, than to include it in the CPython repo.

Even simpler than a separate repository would be to add an option to the android.py script to obtain the Gradle wrapper directly from Gradle – essentially automating the steps listed in my previous comment. This would be no different from downloading compilers from a third party, as we do on every platform.

The text files are at least auditable -- though it's tedious. I don't see why we need to set RIGHT_MARGIN to 80, or include a fully commented-out config file example, to run the tests. [...] If we treat them as source code, then I'll review them, and I'll ask you to prune things that are not needed -- but I think that'd just add work for everyone.

Actually I've got a pretty good idea of what can be pruned. My initial plan was to keep the app as close as possible to Android Studio's new project wizard output, but I guess that does include a lot of noise. Let me see what I can do.

@mhsmith
Copy link
Member Author

mhsmith commented Apr 25, 2024

OK, I've removed the Gradle wrapper, added an android.py subcommand to download it, and removed all the unused boilerplate from the app, which should make the PR a lot more manageable.

@encukou
Copy link
Member

encukou commented Apr 26, 2024

Thank you! This looks good; I haven't tested it.
I'm not sure if the setup instructions will tleave the user with wget and unzip on all platforms. If not, could you add a note that they're required?
(Using Python instead of shelling out is probably not worth the work.)

@mhsmith
Copy link
Member Author

mhsmith commented Apr 27, 2024

I've added a list of the required tools to the README. And I've also switched from wget to curl, because it's included with macOS.

Android/README.md Outdated Show resolved Hide resolved
Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!
I was able to start the test suite! (There were some errors, and I didn't wait for it to finish on my non-accelerated VM, but that shouldn't block this PR.)

I have some more suggestions for the guide, to help newbies like me.
Additionally, consider introducing the cross-build therm "host" here:

Additionally

  Building for Android requires doing a cross-build where you have a "build"
- Python to help produce an Android build of CPython. This procedure has been
+ Python to help produce an Android build of CPython (the "host" build). This procedure has been
  tested on Linux and macOS.

Android/README.md Show resolved Hide resolved
Android/README.md Outdated Show resolved Hide resolved
Android/README.md Outdated Show resolved Hide resolved
Android/README.md Outdated Show resolved Hide resolved
@mhsmith
Copy link
Member Author

mhsmith commented Apr 30, 2024

I was able to start the test suite! (There were some errors, and I didn't wait for it to finish on my non-accelerated VM, but that shouldn't block this PR.)

Everything was passing on Android a couple of weeks ago, but unrelated development has introduced a few failures since then. The next priority will be to set up a buildbot so we can find out about such failures more quickly.

Additionally, consider introducing the cross-build term "host" here:

Done.

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@encukou encukou merged commit 2520eed into python:main May 1, 2024
37 checks passed
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
Add code and config for a minimal Android app, and instructions to build and run it.
Improve Android build instructions in general.
Add a tool subcommand to download the Gradle wrapper (with its binary blob). Android
studio must be downloaded manually (due to the license).
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.

5 participants