Conversation
|
@rgommers: This PR is ready for review. Compared to the iOS PR, this one is a bit simpler:
|
rgommers
left a comment
There was a problem hiding this comment.
Thanks @mhsmith. The main question I have now is: it looks like you're assuming that only cibuildwheel will be supported, and not regular cross compilation to Android. Is that correct? If so, I'm not sure that's right - and the iOS support doesn't do that as far as I can tell (or it's more hidden and missing code comments?).
Other questions that came up for me, like what platforms Android supports (e.g., armv7) and whether that's supported when using cpu_family = {platform.machine()!r} kinda depend on that first question.
That's a good point: Android packages could be native-compiled in an environment like Termux. I've changed the code to only use cross-compilation mode when it detects it's running within cibuildwheel. As far as I know, there's no native compilation option on iOS, because the platform is too restrictive, e.g. apps cannot create subprocesses.
Officially Python only supports Android on aarch64 and x86_64. However, it's used unofficially on other architectures, so I've added coverage for all the known values of |
Anecdotally, Termux for me shows python 3.12 with a platform value of "Linux". :) |
|
Yes, that was changed in Python 3.13 when we added official Android support. cibuildwheel will only support Android on Python 3.13 or later, so the cross file should use the new value. |
|
We get the occasional Termux-related bug report in NumPy, so that does appear to be used indeed. The approach you took now looks better to me. |
|
I'm not aware of any public way to detect that a build is running within cibuildwheel. The environment variables in their documentation are all used as configuration inputs to cibuildwheel, rather than outputs that you can rely on being set within the build. So I've changed it to use |
|
Thanks for the update.
It does depend on tools, right? I guess all we care about here is that |
|
Python doesn't really provide any "proper cross-compiling" mechanism, so the crossenv-like approach is the only feasible option for cross-compiling Android and iOS wheels at the moment.
I've mentioned that in the comment on line 749. |
|
Thanks @mhsmith. For context: Python itself doesn't, and probably won't for at least a couple more years, but both Meson and CMake have solid cross-compilation support, and with Meson cross files + PEP 739 ( Support in Meson for PEP 739 has landed, and support in tl;dr we should not build in implicit assumptions around |
| # with cibuildwheel), in which case we should generate a cross file. | ||
| # sys.cross_compiling isn't an official Python API, but it originated from | ||
| # crossenv and has been followed by other cross-compiling tools. |
There was a problem hiding this comment.
Hence I suggest to change the comment along these lines:
| # with cibuildwheel), in which case we should generate a cross file. | |
| # sys.cross_compiling isn't an official Python API, but it originated from | |
| # crossenv and has been followed by other cross-compiling tools. | |
| Cross compilation can be done the regular Meson way with separate build and host envs, | |
| or with `cibuildwheel` which is `crossenv`-based. Here we try to detect whether | |
| cibuildwheel/crossenv are used, through the `sys.cross_compiling` variable which they | |
| define, and we synthesize a cross file in that case to make that work out of the box. |
I think the idea and code are fine.
dnicolodi
left a comment
There was a problem hiding this comment.
I am not sure I understand what this PR implements. The commit message states that this implements support for Android. However, it seems to implement support for cross-compiling for Android. I think native compilation for Android works already as I don't have evidence of the contrary and this PR does not affect native compilation.
Furthermore, this PR seems to only address the case of a cross compilation environment setup by cibuildwheel. However, the code and the comments therein seem to indicate that this is somehow more generic.
I think that supporting cibuildwheel covers an important use case, thus this PR is good to have. On the other hand, it seems that we are designing support for new systems with the same ugly hacks that were used in the past to work around limitations it the build tools. Concretely, i don't understand why we need to rely on sys.cross_compiling. If this is designed to support only cibuildwheel I am sure we can use some other indicator to detect the cross compilation environment, especially if we already rely on environment variables to setup the compilation environment.
| # sys.cross_compiling isn't an official Python API, but it originated from | ||
| # crossenv and has been followed by other cross-compiling tools. | ||
| elif ( | ||
| sysconfig.get_platform().startswith('android-') |
There was a problem hiding this comment.
Why not simply
| sysconfig.get_platform().startswith('android-') | |
| sys.platform == 'android' |
?
| # Binaries are controlled by environment variables, so they don't need | ||
| # to be repeated here. |
There was a problem hiding this comment.
I don't think there is any value in adding this comment to the generated file.
| cpu_family = ( | ||
| 'arm' if cpu.startswith('arm') else 'x86' if cpu.endswith('86') else cpu | ||
| ) |
There was a problem hiding this comment.
| cpu_family = ( | |
| 'arm' if cpu.startswith('arm') else 'x86' if cpu.endswith('86') else cpu | |
| ) | |
| cpu_family = 'arm' if cpu.startswith('arm') else 'x86' if cpu.endswith('86') else cpu |
This is ugly, but please don't use the black code uglifier to make it uglier.
| elif ( | ||
| sysconfig.get_platform().startswith('android-') | ||
| and getattr(sys, 'cross_compiling', False) | ||
| ): |
There was a problem hiding this comment.
| elif ( | |
| sysconfig.get_platform().startswith('android-') | |
| and getattr(sys, 'cross_compiling', False) | |
| ): | |
| elif sys.platform == 'android' and getattr(sys, 'cross_compiling', False): |
Please don't use the black code uglifier.
| # cibuildwheel's cross virtual environment will make Meson believe it's | ||
| # running on Android when it's actually running on Linux or macOS. | ||
| needs_exe_wrapper = true |
There was a problem hiding this comment.
I don't get how the comment (which I don't think should be in the generated file) goes with the implemented setting. How does cibuildwheel makes Meson believe it is running on Android? And why does it do so? It is clearly not the case. Also, what it the exe wrapper that is needed? This seems incomplete at best.
| monkeypatch.setattr(sysconfig, 'get_platform', Mock(return_value='android-24')) | ||
| if cross: | ||
| monkeypatch.setattr(sys, 'cross_compiling', True, raising=False) | ||
| monkeypatch.setenv('STRIP', '/path/to/strip') |
There was a problem hiding this comment.
Why is setting this environment variable needed?
|
Thanks for the comments. I have some business travel and vacation coming up, so it'll probably take me a couple of weeks to reply. |
Using the same approach as on iOS.
This PR branch is temporarily used by several other PRs: see the cross-references below.