-
Notifications
You must be signed in to change notification settings - Fork 262
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
base: main
Are you sure you want to change the base?
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.
|
||
# platform ################################################################ | ||
# | ||
# We can't determine the user-visible Android version number from the API level, so return a |
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.
Any need to patch platform.uname()
and platform.system()
?
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.
It's enough to patch sys.platform
and platform.android_ver
, and the other platform
functions will then do the right thing.
|
||
# sysconfig ############################################################### | ||
# | ||
sysconfig._init_config_vars() # type: ignore[attr-defined] |
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.
Does this generate the right values in sysconfig.get_paths()
? On iOS, additional handling is needed to ensure that include
doesn't point at the build install.
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.
Also - does subprocess._can_fork_exec
need patching?
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.
The sysconfig paths are handled in localize_sysconfigdata
in android.py. We alter the sysconfigdata file before the pth file even executes.
subprocess._can_fork_exec
doesn't need patching, because unlike on iOS, subprocesses are possible on Android, they're just not recommended. So _can_fork_exec
is already true.
|
||
### Build frontend support | ||
|
||
Android builds only support the `build` frontend. In principle, support for the |
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.
Any reason there's no support for the pip
frontend?
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.
The build frontend does two things:
- Create the build environment.
- Call the PEP 517 "build wheel" API within that environment.
In this case cibuildwheel is creating the build environment itself, so the only thing we're using the build frontend for is to convert a command line into a PEP 517 API call. pip
and build
should produce identical calls here, so there's no point in doubling the amount of code and testing.
There's some precedent for this, as pyodide also requires the build
frontend. build
has also now become the default frontend in cibuildwheel 3.0.
|
||
system_machine = (platform.system(), platform.machine()) | ||
if system_machine not in [("Linux", "x86_64"), ("Darwin", "arm64"), ("Darwin", "x86_64")]: | ||
pytest.skip( |
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.
Should this skip if ANDROID_HOME
isn't defined (or some other marker that Android tools are available?) Not sure if the preference should be to skip or fail if you're on a platform that could run Android tests, but you don't have an Android environment configured - or if that's something where it's an xfail locally, but a hard fail in an actual CI environment.
At the very least, there's an option to fail this module fast and early if ANDROID_HOME
doesn't exist, rather than trying to run a handful of tests that we can reliably predict won't work.
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.
Good point, I'll just make it skip.
"--extra-index-url", | ||
"https://chaquo.com/pypi-13.1/", |
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.
Why is this needed? Won't it potentially introduce a dependency on "non-official" binary builds?
|
||
* `python -c command ...` | ||
* `python -m module_name ...` | ||
* `pytest ...` (converted to `python -m pytest ...`) |
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.
Flagging the obvious overlap here with #2363.
shutil.rmtree(self.tmp_dir) | ||
log.build_end() | ||
|
||
def setup_python(self) -> None: |
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.
Flagging that this should probably differentiate setup_target_python()
from setup_build_python()
if a dependency on standalone is added.
# Apply custom environment variables, and check environment is still valid | ||
self.env = self.build_options.environment.as_dictionary(self.env) | ||
self.env["PIP_DISABLE_PIP_VERSION_CHECK"] = "1" | ||
for command in ["python", "pip"]: |
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.
Is this not duplicating the check for python
a few lines above (139-143)?
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 idiomatic in cibuildwheel, the second check is to make sure that the user didn't mess up the PATH variable in CIBW_ENVIRONMENT
by doing something like PATH=/usr/local/bin:$PATH
and thus changing the active python interpreter.
self.pip_install("build", *constraint_flags(dependency_constraint)) | ||
|
||
# Install build-time requirements. These must be installed for the build platform, not for | ||
# Android, which is why we can't allow them to be installed by the `build` subprocess. |
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.
So - I know the context and background because we've spoken about this in person; but the general approach is a bit weird (and different to iOS). It would be worth giving a high-level description of what is going on here - that it's deliberately not invoking python -m build
, and instead reproducing what build
does in steps so that the build dependencies can be explicitly installed in an "non-cross" environment.
I guess the bigger question is whether this is something we should be doing at all - the iOS example shows that some packages can be compiled without this workaround; and there's clearly a bigger discussion that needs to happen around build
and the PEP 517 interface.
def native_platform() -> PlatformName: | ||
if sys.platform.startswith("linux"): | ||
return "linux" | ||
elif sys.platform == "darwin": | ||
return "macos" | ||
elif sys.platform == "win32": | ||
return "windows" | ||
else: | ||
msg = ( | ||
'Unable to detect platform from "sys.platform". cibuildwheel doesn\'t ' | ||
"support building wheels for this platform. You might be able to build for a different " | ||
"platform using the --platform argument. Check --help output for more information." | ||
) | ||
raise errors.ConfigurationError(msg) | ||
|
||
|
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.
could we move this to platform/__init__.py
? or does that hit a circular import problem...
if existing_value is None: | ||
config_settings[setting] = value | ||
elif isinstance(existing_value, str): | ||
config_settings[setting] = [existing_value] |
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.
it looks to me that value
would be discarded in this branch?
config_settings[setting] = [existing_value] | |
config_settings[setting] = [existing_value, value] |
|
||
|
||
@dataclass | ||
class Builder: |
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'd say +1, for the sake of consistency with the other platforms, and passing around lots of variables (while it's a bit of a drag) prevents other classes of bugs in its explicitness.
] | ||
|
||
|
||
def shell_prepared(command: str, build_options: BuildOptions, env: dict[str, str]) -> None: |
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.
Could we add some kwargs, for the sake of readability?
def shell_prepared(command: str, build_options: BuildOptions, env: dict[str, str]) -> None: | |
def shell_prepared(command: str, *, build_options: BuildOptions, env: dict[str, str]) -> None: |
# Apply custom environment variables, and check environment is still valid | ||
self.env = self.build_options.environment.as_dictionary(self.env) | ||
self.env["PIP_DISABLE_PIP_VERSION_CHECK"] = "1" | ||
for command in ["python", "pip"]: |
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 idiomatic in cibuildwheel, the second check is to make sure that the user didn't mess up the PATH variable in CIBW_ENVIRONMENT
by doing something like PATH=/usr/local/bin:$PATH
and thus changing the active python interpreter.
for path in [ | ||
resources.PATH / "_cross_venv.py", | ||
next(self.python_dir.glob("prefix/lib/python*/_sysconfigdata_*.py")), | ||
]: | ||
out_path = Path(shutil.copy(path, self.site_packages)) | ||
if "sysconfigdata" in path.name: | ||
self.localize_sysconfigdata(out_path) |
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.
Can we unroll this loop? I was very confused why these files were in a list together.
def pip_install(self, *args: PathOrStr) -> None: | ||
if args: | ||
call("pip", "install", "--upgrade", *args, env=self.env) |
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 feels like an unnecessary abstraction imo. Could we inline this please?
env_output = call( | ||
self.python_dir / "android.py", "env", env=self.env, capture_stdout=True | ||
) |
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.
Where does android.py
come from? I don't see it get created.
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.
It's part of cpython, and included as part of an "Python on Android binary release".
os.environ.update(self.android_env) | ||
|
||
pth_file = self.site_packages / "_cross_venv.pth" | ||
pth_file.write_text("import _cross_venv; _cross_venv.initialize()") | ||
|
||
try: | ||
yield | ||
finally: | ||
pth_file.unlink() | ||
for key, original_value in original_env.items(): | ||
if original_value is None: | ||
del os.environ[key] | ||
else: | ||
os.environ[key] = original_value |
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.
Rather modifying global state here, it would be better to return (or yield, if necessary) an env
dict here that contained the environment appropriate for cross-compilation.
This PR depends on a number of changes to CPython's android.py script, which are being developed in python/cpython#132870.