Skip to content

ENH: support setuptools-style cross compilation on macOS #226

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 4 commits into from
Dec 13, 2022

Conversation

dnicolodi
Copy link
Member

Use sysconfig.get_platform() instead of platform.mac_ver() to base the platform tag computation. The ability to overwrite the value returned by former via the _PYTHON_HOST_PLATFORM environment variable is used in macOS extension modules cross-compilation.

See #222 for background.

@henryiii
Copy link
Contributor

From what I understand, this won't make cross-compiled binaries yet, it will just be tagged cross compiled?

@dnicolodi
Copy link
Member Author

dnicolodi commented Nov 22, 2022

It does if you put the right arguments in $CFLAGS and $LDFLAGS. I'm testing it right now.

@dnicolodi
Copy link
Member Author

Great, sysconfig.get_platform() and platform.mac_ver() return two different things when no environmental variables tricks are involved.

@dnicolodi
Copy link
Member Author

Also, simply setting CFLAGS="-arch arm64" is not enough. Meson tries to run an executable compiled with the build compiler to verify that everything works as expected and it fails, of course. Having a cross compiler definition to pass with --cross-file is the right way to do this. However, parsing ARCHFLAGS in meson-python and using that to synthesize a cross file and pass extra arguments to meson is as ugly as it gets.

@rgommers
Copy link
Contributor

However, parsing ARCHFLAGS in meson-python and using that to synthesize a cross file and pass extra arguments to meson is as ugly as it gets.

This can be done, and should make things work for some cases. But is it really desirable to add this code complexity internally? Teaching the end user how to do it the right way with a cross file themselves seem cleaner, and much more educational because it scales to cross-compilation on any platform.

I am also wondering how many packages would even use this at all. If you use numpy for example, this auto-generated cross file may not work - because Meson needs to pick up a numpy for the host system rather than the build system (it contains static libraries like libnpymath.a that may be pulled in).

The tweak made in this PR right now is fairly minimal, so if that works and nothing else breaks then it should be fine to merge. But it seems like it doesn't, and auto-generating a cross file is quite a bit of complexity and tech debt for probably very limited number of users and the wrong design.

@dnicolodi
Copy link
Member Author

dnicolodi commented Nov 23, 2022

AFAIK, cross compilation is supported by the Python ecosystem only in one case: on macOS when building binaries "for the other architecture" (for arm64 when building on x86_64 or for x86_64 when building on arm64) and when not linking to any dependency that does not come in also with the object files for the other architecture. This is enabled by the fact that the macOS toolchain allows to cross-build

This is currently supported by a number of tools in an ad-hoc way involving the ARCHFLAGS and _PYTHON_HOST_PLATFORM environment variables. Interestingly I don't see this documented anywhere.

The Meson way to setup cross-compilation is via a cross-file. However, this alone does not result in usable extension modules and package wheels: Meson uses the Python sysconfig module to compute the extension modules filename suffix and this reports the extension for the current architecture, not for the architecture we are cross-compiling for, and meson-python similarly introspects the python interpreter to determine the wheel tags. Therefore, if meson-python wants to support cross-compilation it needs to find a way to affect these two things. The _PYTHON_HOST_PLATFORM (and patching meson-python as in this PR) allows to do this.

I think it is possible to get meson-python to generate correct cross-built wheels using the just introduced capability of specifying options for Meson on the command line to specify a proper cross-file and setting the _PYTHON_HOST_PLATFORM environment variable (once something like this PR lands).

This would not however integrate well with cibuildwheel and similar tools that expect the magic environment variables to work. As a downstream python extension author I would very much like to be able to delegate the nitty gritty details of preparing redistributable wheels to cibuildwheel and not having to find the proper incantations myself. Therefore, I see value in supporting the cibuildwheel way of doing things, even if it looks ugly.

For doing so we need to check the ARCHFLAGS environment variable and pass to Meson a generated cross-file. Given that we need to do this only on a relatively well controlled platform, I think it can be done reliably. On the other hand, I'm also happy with setting up a CI job to build the macOS arm64 wheels natively when the need arises, thus I will not be working on this.

What concerns me, however, is that this PR highlights the fact that there may be something wrong in how we determine the wheel tags. I modeled the code in meson-python after what the packaging module does, but apparently, on macOS packaging a wheel disagree on what is the correct platform tag: the first derives it from platform.mac_ver() and the latter from sysconfig.get_platform() and the two do not necessarily report the same macIOS version. I'm afraid that on macOS we are producing wheels where the wheel filename tags and the extension modules filename suffix do not agree.

@henryiii
Copy link
Contributor

henryiii commented Nov 23, 2022

Just to point out, hundreds of packages today use cibuildwheel cross-compiling, and are very happy with it. The only downside is it's tricker to handle dependencies, but if you are handling them right (usually downloading a compatible build or building them yourself with the proper flags), then that works too. It's also important to check a produced wheel on Apple Silicon, but that's up to the author, we can't automate that.

I believe it's important to make the simple cases easy, and work at least as well as setuptools. Setuptools does support cross compiling on macOS, also on Windows, even if it is not completely official.

Having a crossfile for tricker cases is fine. But it will be more instructive to a user if they first understand why they need it by trying the simple way first. And if simple cases work "out of the box", many users won't have to learn something they don't want to learn (very few people actually want to learn about packaging & building, they just want it to work). If you avoid all compiled dependencies, for example, you shouldn't have to do anything.


MacOS version is something I think you should compute yourself (along with the arch, which I also think should be computed on macOS). If MACOSX_DEPLOYMENT_TARGET is set, it should report that. If not, it should be the same as the system (and in all cases, the minor version is zeroed out if major is 11+). If there's some way to set the deployment target in Meson, that needs to be respected too if the dep target variable is not set. Setuptools requires that the minimum version also be equal to or higher than whatever Python was built with or it prints a warning, but I think this is not needed, it was only due to using sysconfig's flags and there was a change on what flags it reported for 10.3. Maybe again for 10.9. But I don't think there's been a change since. You don't link to the binary so it shouldn't otherwise matter.

@dnicolodi
Copy link
Member Author

dnicolodi commented Nov 23, 2022

Having a crossfile for tricker cases is fine.

It is not "fine", it is a requirement to cross-compile with Meson. Either it needs to be provided by the user or generated. I feel I'm start to repeat myself a bit too much.

MacOS version is something I think you should compute yourself.

Please define "compute yourself". Do you mean go and poke around the system to get the information or rely on the Python standard library?

Everything else you write is already how things work AFAIK so, unless you have evidence of the contrary, keeping repeating some abstract requirement that we already agreed upon and implemented only adds to the confusion.

@henryiii
Copy link
Contributor

henryiii commented Nov 23, 2022

Sorry, I just meant having a user provide a crossfile is fine for tricker cases, but should not be required for simple to moderate cases if the flags setuptools, scikit-build-core, and Maturin all use are set (ie, generated if it doesn't exist and the flags are set). I was responding to @rgommers's suggestion that this rarely or maybe never works - for hundreds of small to moderate projects, it works very well.

By compute yourself, I was responding to "we are producing wheels where the wheel filename tags and the extension modules filename suffix do not agree" - I think both need to be set following the procedure rather than relying on anything else. The value of macOS in _PYTHON_HOST_PLATFORM, especially, is meaningless - I wanted to fix it in cibuildwheel, but it was turned down because it complicated the code very slightly, but was not used by downstream projects (setuptools) if MACOSX_DEPLOYMENT_TARGET was set (which it always is in cibuildwheel) so it was fine if it was wrong.

@rgommers rgommers added the enhancement New feature or request label Nov 23, 2022
@rgommers
Copy link
Contributor

As a downstream python extension author I would very much like to be able to delegate the nitty gritty details of preparing redistributable wheels to cibuildwheel and not having to find the proper incantations myself. Therefore, I see value in supporting the cibuildwheel way of doing things, even if it looks ugly.

Okay, fair enough. I think longer term we'll all use native M1 CI and this will become a bit of technical debt, but you say it has value for you and you're putting in the work, so I'll pause my concerns here.

Also, let me volunteer to help write docs for cross-compiling. I'll need docs for that at some point anyway for SciPy.

@henryiii
Copy link
Contributor

and you're putting in the work

Not quite yet, my priority currently is shipping the fist non-beta scikit-build-core, which is already on day 5 of of my 0-4 day estimate. And it sounds like I'll have to figure out how to generate a cross-compile file for Meson to do this, so I'd like to wait till after or during the docs being written, so I know what to do. :) But I can work on it if no one else gets to it first.

@rgommers
Copy link
Contributor

and you're putting in the work

Not quite yet

This is a PR already, I meant @dnicolodi. But thanks - looks like we're all volunteering to do something here!

@henryiii
Copy link
Contributor

Quick thought, and something I did for the scripts dir in scikit-build-core: what about throwing an error if _PYTHON_HOST_PLATFORM doesn't match the current platform and no cross-compile file is found? And/or ARCHFLAGS contains a non-native arch. That way, you'd be able to provide a nice, descriptive error until this feature is implemented, rather than producing broken wheels.

@henryiii
Copy link
Contributor

henryiii commented Nov 23, 2022

There are two things here: producing the target wheel tag, and producing binaries for the target arch. The PR as I understand it does the first without doing the second. My suggestion above would handle the second one by producing an error for now, and eventually I think it would be nice to generate a crossfile if the envvar(s) are set.

On the flip side, if a user does supply a crossfile, I'm assuming they still also need to set _PYTHON_HOST_PLATFORM too?

@dnicolodi
Copy link
Member Author

@henryiii

I think both need to be set following the procedure

Which procedure? Where is "the right thing" documented? For now I've seen packaging doing one thing and wheel doing another and the two do not agree.

@dnicolodi
Copy link
Member Author

Not quite yet, my priority currently is shipping the fist non-beta scikit-build-core, which is already on day 5 of of my 0-4 day estimate. And it sounds like I'll have to figure out how to generate a cross-compile file for Meson to do this, so I'd like to wait till after or during the docs being written, so I know what to do. :) But I can work on it if no one else gets to it first.

I hear you. This journey for me started more than a month ago trying to publish an uber simple package and so far it involved patching pip, meson, meson-python, and we are not there yet. Generating the cross file seems straightforward. I'll give it a try later today.

@dnicolodi dnicolodi force-pushed the macos-platform-tag branch 8 times, most recently from 526e8cb to 06840fb Compare November 24, 2022 11:15
@dnicolodi dnicolodi changed the title BUG: allow to overwrite macOS platform tag via _PYTHON_HOST_PLATFORM ENH: support setuptools-style cross compilation on macOS Nov 24, 2022
@dnicolodi
Copy link
Member Author

@henryiii I added a first draft of support for ARCHFLAGS. The tests are failing left and right, but the stuff actually works. Either the tests are testing the wrong thing or I'm misunderstanding something. Anyhow, I've build wheels for macOS arm64 via cibuildwheel on macSO x86_64 with this. It would be cool if someone could test if the produced wheels actually work.

@dnicolodi dnicolodi force-pushed the macos-platform-tag branch 3 times, most recently from 0f306c6 to 03149fd Compare November 24, 2022 14:17
@dnicolodi
Copy link
Member Author

Tests are passing. Turns out that blanket exceptions suppression does not really aid debugging... To keep the tradition with setuptools this is carefully undocumented, but it should work. @FFY00 This builds on the support for passing options to meson that you recently added. I would appreciate if you could have a close look. Thank you!

@dnicolodi dnicolodi force-pushed the macos-platform-tag branch 2 times, most recently from d2c3f5b to 96ec63c Compare November 24, 2022 15:57
@dnicolodi
Copy link
Member Author

I tested the cross-compiled wheels on macOS arm64 via Cirrus CI and they work. I think this can be merged.

Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

Does this overwrite a user provided cross file? It shouldn’t ideally.

@dnicolodi
Copy link
Member Author

Options are applied in this order: meson-python defaults, pyproject.toml options, environment variables, command line options. Options are actually only appended at the end of the command line. Most options simply override previously specified options. If more --cross-file options are present, I don't know whether Meson uses the latest specified or somehow merges the configurations. Anyhow, I think that setting $ARCHFLAGS while specifying a --cross-file option can be considered undefined behavior and if it breaks you keep both pieces.

@dnicolodi
Copy link
Member Author

If there are no comments I'm going to merge this soon. I've tested this with one project of mines and it works.

@dnicolodi
Copy link
Member Author

@FFY00 can you please have a look? I would like your opinion on the meson setup arguments handling.

Use sysconfig.get_platform() instead of platform.mac_ver() to base the
platform tag computation.  The ability to overwrite the value returned
by former via the _PYTHON_HOST_PLATFORM environment variable is used
in macOS extension modules cross-compilation.

See mesonbuild#222 for background.
Comment on lines +579 to +602
if sysconfig.get_platform().startswith('macosx-'):
archflags = self._env.get('ARCHFLAGS')
if archflags is not None:
arch, *other = filter(None, (x.strip() for x in archflags.split('-arch')))
if other:
raise ConfigError(f'multi-architecture builds are not supported but $ARCHFLAGS={archflags!r}')
macver, _, nativearch = platform.mac_ver()
if arch != nativearch:
x = self._env.setdefault('_PYTHON_HOST_PLATFORM', f'macosx-{macver}-{arch}')
if not x.endswith(arch):
raise ConfigError(f'$ARCHFLAGS={archflags!r} and $_PYTHON_HOST_PLATFORM={x!r} do not agree')
family = 'aarch64' if arch == 'arm64' else arch
cross_file_data = textwrap.dedent(f'''
[binaries]
c = ['cc', '-arch', {arch!r}]
cpp = ['cpp', '-arch', {arch!r}]
[host_machine]
system = 'Darwin'
cpu = {arch!r}
cpu_family = {family!r}
endian = 'little'
''')
self._meson_cross_file.write_text(cross_file_data)
self._meson_args['setup'].extend(('--cross-file', os.fspath(self._meson_cross_file)))
Copy link
Member

Choose a reason for hiding this comment

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

Humm, I'd prefer the cross file not to be added to meson_args, but rather added in _configure directly. I think it would be alright to always have a cross file, and it just being empty when it is not needed. I feel like that is cleaner and better sets things up for the future if we need to use it for something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding --cross-file for a non cross build sounds more confusing than helpful to me. I'd much prefer to not see that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with @rgommers Also, at the moment I cannot imagine any other feature that would automagically generate a cross-file. If we will find one, we will revisit this. One main advantage of not having the --cross-file argument when not needed, is that its presence when scanning the build output is already a good indication that the environment variables are set correctly.

Copy link
Member

Choose a reason for hiding this comment

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

This could still be done in optionally _configure, without hijacking the user args.

@dnicolodi dnicolodi merged commit 13c6748 into mesonbuild:main Dec 13, 2022
@rgommers rgommers modified the milestones: v0.13.0, v0.12.0 Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants