Skip to content

ci: add android test #5714

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

Merged
merged 13 commits into from
Aug 7, 2025
Merged

ci: add android test #5714

merged 13 commits into from
Aug 7, 2025

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Jun 4, 2025

Description

Testing pypa/cibuildwheel#2349.

Suggested changelog entry:

  • Add CI testing Android

📚 Documentation preview 📚: https://pybind11--5714.org.readthedocs.build/

@henryiii henryiii force-pushed the henryiii/ci/android branch from 736ef07 to 898186c Compare June 4, 2025 03:49
@henryiii henryiii force-pushed the henryiii/ci/android branch from 75a86b8 to 2ff65e5 Compare June 12, 2025 04:22
@henryiii
Copy link
Collaborator Author

@mhsmith Had to install wheel and patchelf, and now it's failing at the test step on all three systems.

@mhsmith
Copy link
Contributor

mhsmith commented Jul 11, 2025

The current failures are because KVM hasn't been enabled on Linux, and GitHub Actions isn't able to run the Android emulator on macOS. Both of these are discussed in pypa/cibuildwheel#2349.

It also looks like somehow the merge of henryiii#23 has missed a couple of the files. Correction: they've already been merged in #5733.

But this PR-against-PR workflow is confusing, so would it be simpler if I created my own PR to replace this one? Since you're a member of the pybind11 project, we would both be able to push to it.

@henryiii
Copy link
Collaborator Author

I'm fine if you make a PR, but I'm fine to give you access to my fork.

@mhsmith
Copy link
Contributor

mhsmith commented Jul 12, 2025

Thanks, that works too.

@mhsmith
Copy link
Contributor

mhsmith commented Jul 16, 2025

OK, I've updated the GitHub Actions configuration as discussed in my previous comment, and the Android jobs are passing now.

To enable local testing, I also moved the ANDROID_API_LEVEL environment variable from tests-cibw.yml to pyproject.toml. But this breaks the iOS and Pyodide jobs, because the stable cibuildwheel version doesn't recognize the android namespace. I guess you aren't planning to merge this PR until after the next cibuildwheel release anyway, so I've put a TODO comment in tests-cibw.yml to indicate that.

@henryiii
Copy link
Collaborator Author

Android: can't find wheel in path?

The iOS 3.14 failure is expected (pypa/cibuildwheel#2494).

@mhsmith
Copy link
Contributor

mhsmith commented Jul 24, 2025

I've added wheel back to this PR so it can work with cibuildwheel 3.1, and fixed it properly for the next cibuildwheel version in pypa/cibuildwheel#2515.

I think this PR is now ready to merge.

Comment on lines 80 to 82
# TODO: remove "wheel" once we're using a cibuildwheel version that includes
# https://github.com/pypa/cibuildwheel/pull/2515.
- run: pipx install wheel patchelf
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
# TODO: remove "wheel" once we're using a cibuildwheel version that includes
# https://github.com/pypa/cibuildwheel/pull/2515.
- run: pipx install wheel patchelf
- run: pipx install patchelf

I guess we still need patchelf? Is that something cibuildwheel should install?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I see it's on PyPI for both Linux and macOS, so maybe we could make it a dependency of cibuildwheel.

Copy link
Collaborator Author

@henryiii henryiii Aug 6, 2025

Choose a reason for hiding this comment

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

Does it have to be co-installed with cibuildwheel? cibuildwheel shouldn't depend on compiled dependencies. But in an environment is fine. I haven't looked at where it's used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually on Linux cibuildwheel already depended on patchelf via auditwheel, but neither of them needed to be declared as a Python-level dependency because they're installed in the manylinux Docker image.

On Android, cibuildwheel uses patchelf in a more limited way to deal with linking against the C++ library. I'm planning to eventually move that functionality into auditwheel, and then cibuildwheel will be able to use the same auditwheel "repair" command on both Linux and Android.

Does it have to be co-installed with cibuildwheel? cibuildwheel shouldn't depend on compiled dependencies.

We could use environment markers to limit the dependency to the Linux or Mac platforms that support the Android build tools. patchelf appears to have all the necessary wheels.

@henryiii henryiii force-pushed the henryiii/ci/android branch from a825163 to 8fd25d6 Compare August 6, 2025 21:12
@henryiii henryiii marked this pull request as ready for review August 6, 2025 21:12
@henryiii henryiii merged commit 23c59b6 into pybind:master Aug 7, 2025
98 of 106 checks passed
@henryiii henryiii deleted the henryiii/ci/android branch August 7, 2025 02:32
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Possibly needs a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants