-
Notifications
You must be signed in to change notification settings - Fork 277
Add support for building Android wheels #2349
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
Conversation
Let us know if you need (the rest of) CI triggered. :) |
@henryiii: This is close to being complete, so please enable the rest of CI. |
To run the integration tests on most of the CI machines, it looks like I'll need to automate installation of the correct Python version. For macOS this can be done the same way as iOS, but for Linux there's no existing code to reuse, because the native Linux build uses Docker. So I'll probably implement something that uses python-build-standalone, unless anyone has another suggestion. Apart from that, I think this PR is complete enough now that it's worth reviewing. @freakboy3742 and anyone else who's interested. |
python-build-standalone is fine, in fact, I'd like to use that for pyodide in the future, too. |
I've already written code to install a version of python-build-standalone in #2002, along with version pinning etc. I think it would work nicely here too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments inline; my two high level concerns are:
- Whether the Builder class actually delivers any benefit here; and
- The general approach around avoiding
python -m build
in order to get platform-specific build requirements.
This is very strange. It's evidently running Python, as we see it loading several standard library modules, but it doesn't show any Python stdout or stderr. Despite that, the test apparently passes, and then the logcat process returns non-zero. This non-zero return is normal when the emulator is shutting down, but since the testbed hasn't seen any output from Python, it treats this as a fatal error. The same thing has happened in CI on this PR, but only once. I've never seen it in the CI of Python itself, which uses the same testbed and runs many times every day. @joerick: Does it still fail with the current version of this PR, and does it fail the same way every time you try? |
Ah, now I think I see. Your test script is just I should probably change the testbed so it ignores a non-zero return from logcat if it's seen anything that looks like a valid logcat message, whether from Python or not. That still doesn't explain why this problem happened once in CI in a test which actually was producing output. Maybe the emulator shut down before the output could be read. The output should have occurred before the "TestRunner: finished" message, but it came from a separate thread, so maybe the message ordering isn't guaranteed. If this happens again, I'll try making the testbed pause for a moment after the test script completes before reporting the result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds comprehensive support for building and testing Android wheels in cibuildwheel.
- Introduces a new Android platform implementation (
platforms/android.py
) with environment setup, build, repair, and test logic. - Extends core option handling, typing, packaging, and default schemas to recognize
android
. - Adds end-to-end tests, updates CI workflows, examples, and documentation to cover Android builds.
Reviewed Changes
Copilot reviewed 35 out of 37 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
cibuildwheel/platforms/android.py | New Android build, repair, and test logic |
cibuildwheel/options.py | Make build_frontend always present, default to build |
cibuildwheel/typing.py | Include android in PlatformName literal |
cibuildwheel/util/packaging.py | Support ignoring version number in Android wheel tags |
docs/platforms.md | Add Android platform guide |
docs/options.md | Add Android-specific environment & option variables |
docs/ci-services.md | Update example description to include Android |
examples/github-deploy.yml | Extend GitHub Actions example for Android |
azure-pipelines.yml | Install JDK for Android SDK tools |
.github/workflows/test.yml | Enable KVM for Android emulator in CI |
unit_test/options_test.py | Adjust default frontend test expectations |
unit_test/main_tests/main_options_test.py | Test parsing of Android config settings |
unit_test/architecture_test.py | Add arch_synonym tests |
test/utils.py | Generate deterministic, Android-aware expected wheels |
test/test_android.py | End-to-end Android wheel build & test scenarios |
Comments suppressed due to low confidence (4)
examples/github-deploy.yml:34
- The example workflow doesn’t include steps to install or configure the Android SDK (
ANDROID_HOME
); consider adding commands to download the command-line tools, install necessary SDK packages, accept licenses, and setANDROID_HOME
before building Android wheels.
- os: android-intel
cibuildwheel/platforms/android.py:1
- [nitpick] The Android platform implementation exceeds 600 lines in one file. Splitting environment setup, toolchain generation, and build/test logic into smaller modules would improve readability and maintainability.
import csv
cibuildwheel/util/packaging.py:161
- There are no unit tests verifying
find_compatible_wheel
behavior for Android wheel tags. Adding tests for Android identifier patterns (e.g.,android_21_arm64_v8a
) would ensure this logic remains correct.
if platform.startswith(("manylinux", "musllinux", "macosx", "android", "ios")):
docs/platforms.md:317
- [nitpick] The note about requiring
test-sources
for iOS was removed; consider reinstating guidance to copy test files into the testbed app viatest-sources
so users don’t miss this required configuration on iOS.
The iOS test environment can't support running shell scripts, so the [`test-command`](options.md#test-command) value must be specified as if it were a command line being passed to `python -m ...`.
@freakboy3742 I rerequeted a review from you since it's still "requesting changes". |
Acknowledged - I'm currently at EuroPython; I'll try to take a look, but I might not get a chance until I return to my office next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't read the full change in detail, but Pyodide-side changes looks trivial and good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming along nicely!
I just confirmed this on my end as well. I added a print statement and it works beautifully. It would be great to get this fixed before release, but if it's a really complicated one, we could document it as a known issue. |
I've submitted a CPython PR to fix this (python/cpython#136845). Once that's merged, I'll update build-platforms.toml to pick it up. |
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me, and worked well in my local experiments with some simple projects.
The only thing that seems worthy of note is that we've now got three slightly different approaches to cross-platform build environment handling:
- pyodide: running Python through node on the local machine to create a "native" venv
- iOS: using an environment conversion script provided by the support package
- Android: using an environment conversion script provided with cibuildwheel
There's enough commonality in the requirements of the three platforms that there's likely opportunity to standardise the approach that is used for cross-platform environments. However, I don't think that needs to block merging this PR - I'm just flagging it as an area for future consolidation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant work on this @mhsmith!
The corresponding CPython PR, from which the Android Python releases in build-platforms.toml are being generated, is python/cpython#132870.