ENH: implement support for build-details.json (PEP 739) (continued)#829
ENH: implement support for build-details.json (PEP 739) (continued)#829mgorny wants to merge 2 commits into
Conversation
|
I closed gh-779. Copying over the main comment from that PR discussion: Good point. Yes, there isn't any reason that So for now in this PR we can distinguish three cases, with the user passing:
I focused on (3) first and have a TODO in here already to add (1); I should add (2) as well. |
dnicolodi
left a comment
There was a problem hiding this comment.
I had a quick look at this. Found some possible improvements.
|
I'm going to rebase it now, and make changes later when I find more time. |
f22d77f to
7758d1c
Compare
|
Updated and rebased now. |
3e48d47 to
bba067e
Compare
|
I tied up some loose ends. I think this is ready to be merged now. @rgommers, do you want to take a look? It would be nice to have an integration test for this, but I don't know how hard it is to get a suitable environment setup on GitHub Actions. |
|
Actually, I think it was in Draft because we wanted to make an integration test, but we were blocked on |
|
Sounds great! Indeed, that integration test would be nice. IIRC conda-forge/python-feedstock#858 should have solved the one issue in conda-forge that was blocking for a cross-compile test. |
d683af1 to
3fe11bd
Compare
There was a problem hiding this comment.
This still uses _32_BIT_INTERPRETER as determined from the running interpreter and not from build-details.toml.
There was a problem hiding this comment.
Ok, disabled it when using build details. I suppose it's fine to assume that if you're configuring cross via build-details.json, you'd be targeting the bitness from the file.
I've also adjusted the logic for macosx, though I have to say that the logic there still relies on the running interpreter; I really don't know much about macOS, and I don't understand the idea behind versions there, but on the test box I have access to mac_ver() returns 26.x, while platform is macosx-11.0-arm64; and I don't really see a way of getting that 26 out of build-details.json.
There was a problem hiding this comment.
I've also adjusted the logic for macosx, though I have to say that the logic there still relies on the running interpreter
In this case, it is better to emit an error stating that build-details.json is not supported on macOS rathe than silently emitting wrong wheel tags. The latter would be difficult to debug.
There was a problem hiding this comment.
I really don't know much about macOS, and I don't understand the idea behind versions there, but on the test box I have access to
mac_ver()returns 26.x, whileplatformismacosx-11.0-arm64; and I don't really see a way of getting that26out ofbuild-details.json.
macOS and iOS may introduce ABI incompatible changes at each release. However, a specific ABI can be requested setting some environment variables during compilation. Therefore, the platform tag contains a system ABI version part that is derived from the current OS version, unless the relevant environment variable is set. I am not sure what is the best way to map this to the use of build-details.json. When compiling a redistributable wheel on macOS it is customary to set the env var to have the toolchain compile for the macOS ABI supported by the relevant Python interpreter, however, it is not a requirement. I think that returning an error if the detected macOS ABI version and the one in build-details.json differ may be a good starting point. For the unit test, you can just set the env variable to what the given Python version expects.
There was a problem hiding this comment.
There is nothing in build-details.json that has a relationship with the minimum macOS version, it's orthogonal.
For the unit test, you can just set the env variable to what the given Python version expects.
This should always be the right answer for tests. And at runtime, nothing needs to change to MACOSX_DEPLOYMENT_TARGET logic. Only the platform part of the tag (e.g., x86_64 or arm64) can come from build-details.json.
There was a problem hiding this comment.
There is nothing in
build-details.jsonthat has a relationship with the minimum macOS version, it's orthogonal.
I have experience only with cross architecture compilation on macOS, not cross system. However, IIUC, compiling for iOS on macOS is a thing (and we added support for it to meson-python a while back). IIUC, nowadays this requires some compiler wrappers distributed with CPython. However, I can see a future where the same can be accomplished with a Meson cross file and pointing to a correct build-details.json. In this case, using the build system OS version as the minimum required ABI is clearly wrong. Where should the ABI version come from in this case?
Only the
platformpart of the tag (e.g.,x86_64orarm64) can come frombuild-details.json.
Assuming cross system compilation macOS to iOS is a thing, where should the ABI version come from?
There was a problem hiding this comment.
For iOS: from the IPHONEOS_DEPLOYMENT_TARGET env var, as we already do:
Line 137 in 47ea86f
Linux is no different: the cross file will say you're targeting linux and x86-64, but it will not say what the minimum libc version will be (that's where auditwheel still comes in, or possibly some other override like an env var, or running the target interpreter under QEMU).
It's just a somewhat arbitrary historical choice I believe, what ended up in cross files and what didn't. There's no real reason that minimum glibc version or iOS version couldn't be in a cross, they just aren't. It's largely stemming from cross compilers accepting target triples but not glibc version.
There was a problem hiding this comment.
IPHONEOS_DEPLOYMENT_TARGET does not need to be defined. It is an override for platform.ios_ver(). If setting IPHONEOS_DEPLOYMENT_TARGET is a requirement when cross compiling with build-details.json, it should be documented.
There was a problem hiding this comment.
Maybe it should, but it looks like that's unrelated to this PR. It's already required today, because this try-except doesn't look like it will work:
Lines 136 to 140 in 47ea86f
Gives:
>>> import platform
>>> platform.ios_ver()
IOSVersionInfo(system='', release='', model='', is_simulator=False)
>>> version = tuple(map(int, platform.ios_ver().release.split('.')))[:2]
Traceback (most recent call last):
Cell In[10], line 1
version = tuple(map(int, platform.ios_ver().release.split('.')))[:2]
ValueError: invalid literal for int() with base 10: ''Signed-off-by: Michał Górny <mgorny@quansight.com>
This a continuation of #779. For a start, I just did a dumb rebase to rerun the tests.