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 |
mesonpy/__init__.py
Outdated
| # 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.
There was a problem hiding this comment.
I've updated the comment based on the changed approach to use the CIBUILDWHEEL variable.
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.
mesonpy/__init__.py
Outdated
| # 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' |
?
There was a problem hiding this comment.
I was following the pattern of the existing branches of the if. I agree all three of them would be simpler as sys.platform, but that would add noise to this PR, so would better be done in a separate one.
mesonpy/__init__.py
Outdated
| # 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.
mesonpy/__init__.py
Outdated
| 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.
mesonpy/__init__.py
Outdated
| 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.
mesonpy/__init__.py
Outdated
| # 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.
There was a problem hiding this comment.
I've updated the comment to explain this in more detail. I think it's worth keeping the comment in the generated file to explain the setting to anyone who examines that file without being familiar with meson-python internals.
tests/test_project.py
Outdated
| 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?
There was a problem hiding this comment.
Without it, the tests fail with this error:
WARNING: Cross file does not specify strip binary, result will not be stripped.
ERROR: Fatal warnings enabled, aborting
However, I guess Meson could just as easily do this with any other build tool, so I've updated the tests to include them all. This more closely simulates what cibuildwheel does.
|
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. |
|
@dnicolodi I think your main review comment above covers the same ground that my comments did? What this implements is the same as for iOS. And yes I already pointed out that regular cross-compiling is a thing and this is
Initial reply from @mhsmith in this comment above. I'd also like a cleaner way to do this. I looked through the |
|
I haven't read the conversation in detail, and my questions are maybe due to my complete ignorance about how you go about compiling something for Android. I saw that you planned to merge this unless someone spoke up, thus I assumed you are happy with the status of the PR. I noticed the disconnect between the commit message (that is the base for our changelog entries, especially when reporting about features not implemented by one of the maintainers) and I thought that some clarification is necessary. It is fine to merge this to support the |
My intention was to avoid privileging cibuildwheel over any similar tools, but I guess we are still making some cibuildwheel-specific assumptions, so this makes sense. I'm not aware of any other active Android Python cross-compilation tools, but if they appear in the future, we can revisit this. |
I've updated the commit message. |
dnicolodi
left a comment
There was a problem hiding this comment.
Thanks for working on this. Just a couple of minor things, mostly to make the whole easier for me to understand when I'll go back to it in the future.
| # that work out of the box. | ||
| elif sysconfig.get_platform().startswith('android-') and 'CIBUILDWHEEL' in os.environ: | ||
| cpu = platform.machine() | ||
| cpu_family = 'arm' if cpu.startswith('arm') else 'x86' if cpu.endswith('86') else cpu |
There was a problem hiding this comment.
Can we have a comment that explains what this normalization does? I find the 'x86' if cpu.endswith('86') part particularly puzzling. What does platform.machine() return other than x86? Judging from the test, this simply maps i686 to x86. If this is the case, the more explicit 'x86' if cpu == 'i686' would be better.
| # Android may be native-compiled (e.g. with Termux), cross-compiled with manual | ||
| # Meson arguments, or cross-compiled with cibuildwheel, which uses a similar | ||
| # approach to crossenv. In the latter case, we synthesize a cross file to make | ||
| # that work out of the box. |
There was a problem hiding this comment.
I think
| # Android may be native-compiled (e.g. with Termux), cross-compiled with manual | |
| # Meson arguments, or cross-compiled with cibuildwheel, which uses a similar | |
| # approach to crossenv. In the latter case, we synthesize a cross file to make | |
| # that work out of the box. | |
| # Simplify cross-compilation for Android with cibuildwheel: detect the | |
| # cross-compilation environment set up by cibuildwheel and synthesize an | |
| # appropriate cross file. |
is more to the point and describes what the following code block is really about.
| # cibuildwheel monkey-patches platform.system and platform.machine to | ||
| # simulate running on Android, when it's actually running on Linux or | ||
| # macOS. So the host and build machines will appear to match, but host | ||
| # binaries cannot actually be run on the build machine. As described at | ||
| # https://mesonbuild.com/Cross-compilation.html, we indicate this by | ||
| # setting needs_exe_wrapper = true, but NOT setting exe_wrapper. |
There was a problem hiding this comment.
Mostly to make things easier for me to understand when I will go back to read this one year from now:
| # cibuildwheel monkey-patches platform.system and platform.machine to | |
| # simulate running on Android, when it's actually running on Linux or | |
| # macOS. So the host and build machines will appear to match, but host | |
| # binaries cannot actually be run on the build machine. As described at | |
| # https://mesonbuild.com/Cross-compilation.html, we indicate this by | |
| # setting needs_exe_wrapper = true, but NOT setting exe_wrapper. | |
| # Due to Python lacking proper cross-compilation support, for the build | |
| # to produce the correct wheel tags when cross-compiling for Android, | |
| # cibuildwheel monkey-patches platform.system() and platform.machine() | |
| # to simulate running on Android. This makes Meson believe that host and | |
| # build machines match and thus that host binaries can be run on the build | |
| # machine, when this is not actually the case. Override the auto-detection. |
Using the same approach as on iOS.
This PR branch is temporarily used by several other PRs: see the cross-references below.