Skip to content
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

support Windows cross compilation (x64 -> arm64) #1108

Closed
wants to merge 1 commit into from

Conversation

pbo-linaro
Copy link

@pbo-linaro pbo-linaro commented May 13, 2022

Native support for windows arm64 was added a few months ago in cibuildwheel.

To enable more projects to benefit from it, without the need to get an arm64 machine, this patch
enables cross compilation from x64 to arm64 on Windows.

This PR is a first try, and I'd like to start a discussion around it.
In particular, since cpython does not fully support this, we need to do some "hacky" things (copy libs, rename pyd files in wheel).
If you know better alternatives, I'm open to try them.

With this patch, it's possible to cross compile numpy and matplotlib (with minor tweaks on those projects setup):

# from x64 machine
set CIBW_BUILD=cp310-win_arm64
set CIBW_ARCHS=ARM64
cibuildwheel --platform windows

Your thoughts and ideas are welcome!

@pbo-linaro pbo-linaro force-pushed the main branch 3 times, most recently from c200dea to 6735f11 Compare May 13, 2022 10:54
@pbo-linaro
Copy link
Author

Polite ping :)

@joerick
Copy link
Contributor

joerick commented May 25, 2022

Apologies for the delay here @pbo-linaro, work's been busy lately. I'll try to get to this soon!

Copy link

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @pbo-linaro. Given that this PR seems at least in part motivated by getting NumPy to work (and it is a project that indeed does not support SETUPTOOLS_USE_DISTUTILS=local), I'd like to point out that NumPy will move away from distutils/setuptools completely. And SciPy has already done that.

So if it'd make the approach here simpler, maybe requiring local is not much of a problem. The start of Python 3.12 development is not that far off after all, and then local is the only option.

@pbo-linaro
Copy link
Author

Thanks for your feedback.

Indeed, numpy is one of the first targets, and we saw they planned to move to a more standard setup workflow.

@rgommers : Would a first version of cross compilation using hacks like we did would be acceptable (until it's not necessary anymore)? Or do you prefer to wait for official 3.12 support in numpy to avoid modifying the wheel? That could take quite some time, but we can understand it's a prerequisite to do things cleanly.

@rgommers
Copy link

rgommers commented Jun 7, 2022

@rgommers : Would a first version of cross compilation using hacks like we did would be acceptable (until it's not necessary anymore)? Or do you prefer to wait for official 3.12 support in numpy to avoid modifying the wheel?

A few thoughts:

  • I wanted to point out a potentially relevant topic here, but I don't see a problem with merging this PR - because it's helpful for a lot of projects that will remain setuptools based.
  • I also don't see a problem testing this with NumPy, and giving something a tweak in numpy.distutils if needed; Windows on ARM support in general is valuable, so we don't need to gate that on future changes
  • The main concern is whether we should actually upload a win32-aarch64 wheel to PyPI. That's a separate topic that can be part of the discussion that's currently happening on the numpy-discussion mailing list. In the end, the most critical part there is still hardware/CI availability, and whether a maintainer wants to commit to supporting this via manual testing in a VM.

@pbo-linaro
Copy link
Author

The final goal is to get pip install numpy working directly on windows on arm, which implies to upload a wheel on PyPi, that is built automatically with cibuildwheel. Since there is no Windows on ARM runner available on github, that's why we use cross compilation as a solution.

So having this merged here would be nice to start a discussion with numpy community.

@pbo-linaro
Copy link
Author

Is there something blocking to merge this?

cibuildwheel/windows.py Outdated Show resolved Hide resolved
# to change name of .pyd files when building wheel.
# Alas, it requires using distutils from setuptools
# (SETUPTOOLS_USE_DISTUTILS=local), which is not supported on all projects.
# Thus, we change name of pyd files here.
Copy link
Contributor

@henryiii henryiii Jun 10, 2022

Choose a reason for hiding this comment

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

The correct fix is to change sysconfig.get_config_var('EXT_SUFFIX') / distutils.sysconfig.get_config_var('EXT_SUFFIX'), I believe - this should work for everyone. (Edit: there's also SOABI - CMake prefers SOABI over EXT_SUFFIX, so I think that's important to set for cross-compiling too)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would https://github.com/benfogle/crossenv be useful for this? It seems like it does the right thing as far as setting sysconfig, etc.

Copy link
Author

Choose a reason for hiding this comment

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

Changing sysconfig stuff implies to change project targeted. I'm not sure it's feasible for numpy since they override those things. In more, only "future" versions would be compatible. Patching the wheel is dirty, but it provides an applicable solution. As said in comment, there is already a builtin env var supporting this in setuptools, so we can drop wheel patching once concerned projects are all "compatible". Do you have another idea for a cleaner and applicable alternative?

For crossenv, it's only supporting Linux for now (it seems there was interest for porting to WIndows, but it was not made).

cibuildwheel/windows.py Show resolved Hide resolved
When python executing cibuildwheel is x64 and target cpython is arm64,
automatically enable cross compilation.

When cross compiling, wheel can't be installed nor tested (deactivated).

Only supported for cpython build variants.

Support is a bit hacky, but python does not offer better way to do it for now:
- libs folder is copied from target to host python
- wheel must be patched to change some file names
@zooba
Copy link
Contributor

zooba commented Jun 17, 2022

Sorry, I'm going to step in here and say that this should not be merged as it's simply not right Okay, I've understood the trick that you're doing in here, but it's still not right ;)

I'm preparing my own PR that gets about as far as this one does, though it's going to be blocked on a fix to setuptools/distutils before it can properly work with that backend.

Unfortunately, there's no standard spec for backends to know when they should cross-compile on Windows. Historically it's never been a thing, so the best we can do right now is have backend-specific options to enable it and also encode those into cibuildwheel so that most users never have to face them. Hopefully, as more backends add support, they can copy the interfaces and we don't have a severe multiplication problem.

@pbo-linaro
Copy link
Author

Hi @zooba, and thanks for reviewing this. I understand your point, and I'm disappointed too that there is no cleaner/standard way to solve this.

If I can try a last argument: the hack done here (patching the resulting wheel) is only done when trying to cross compile from x64 windows to arm64 windows, which is failing without this MR. So, it's not breaking anything working. I thought it could be acceptable to have a temporary hack, until more things are solved on distutils/numpy side. (support for python 3.12 will make things cleaner if I understood well). IMHO, the goal of a project like cibuildwheel is precisely to solve those things to just "make it work", instead of waiting for projects to support what is needed.

It's good to see that you plan to do something on your side. Do you have an estimation of when it could be available (and merged in cibuildwheel)? Will that be usable with numpy before they officially support future python 3.12?

Thanks,
Pierrick

@zooba
Copy link
Contributor

zooba commented Jun 21, 2022

The fix to make setuptools work (via distutils) is at pypa/distutils#153 and once that's merged and released then my PR here should work.

There's currently no standard between backends for requesting cross-compilation, but the support definitely requires more work than just renaming the wheel (the fact that you can build an ARM64 wheel with the wrong name is only because of how backends work today, not because of any spec or documented setup). If other backends choose to detect the same variables as setuptools, they'll just work. If they invent their own, those will need to be added to cibuildwheel as well, but I've tried to set up my change to make that easier to handle.

So short answer: numpy's build system needs to know how to cross-compile anyway, so it should also be taught either what suffix to produce, or how to let cibuildwheel tell it.

@rgommers
Copy link

So short answer: numpy's build system needs to know how to cross-compile anyway, so it should also be taught either what suffix to produce, or how to let cibuildwheel tell it.

The interface will use a cross file (see https://mesonbuild.com/Cross-compilation.html#cross-compilation), which can be passed to Meson through --cross-file cross_file.txt. There's no interface yet to pass options through the pyproject.toml hook, but it'll look something like in https://github.com/FFY00/meson-python/issues/54.

Regarding @zooba's PR (gh-1144), that requires SETUPTOOLS_USE_DISTUTILS=local, which doesn't work with numpy.distutils in its current state, and there's little interest in working on changing that. Either way cross-compiling with distutils is hacky, but from a "will it work with numpy at all" perspective, this PR may work while gh-1144 is unlikely to ever work.

@zooba
Copy link
Contributor

zooba commented Jun 21, 2022

If numpy isn't using setuptools, then SETUPTOOLS_USE_DISTUTILS=local won't matter, right? It's only required because the stdlib distutils has known issues that have been fixed.

The biggest complexity in all this is knowing when to pass the extra args along. Neither pip nor build (to my knowledge) has a standard, shared option that will correctly inform any backend that it should be using a different host machine. Without that, cibuildwheel can't (currently) run a cross-build, even if the configuration exists in a pyproject.toml. There's more work required to standardise on triggering cross builds via our existing frontends before this has a chance at all.

Probably the quickest way to get it going is for Meson to support an environment variable to point to that cross-build file, and we teach cibuildwheel(?) or just the specific build definitions to set it. If cibuildwheel carries that file, it can then set up Meson builds to cross-compile in the same way it does setuptools, and then it continues to not need to care what backend is in use.

@zooba
Copy link
Contributor

zooba commented Jun 21, 2022

from a "will it work with numpy at all" perspective, this PR may work while gh-1144 is unlikely to ever work

I'm 99.9% sure the answer is that this PR won't work either, because numpy will be built for amd64 and then renamed for ARM64. The only way that's changing is if numpy.distutils already knows how to decide to build for ARM64 (since that's not in distutils), has fixed the bugs that are fixes in setuptools, and in that case it can generate the correct .cp310-win_arm64.pyd extension by itself and doesn't need the renaming that's in this PR.

@rgommers
Copy link

Ah sorry, should have given a bit more context: as of today, numpy builds with numpy.distutils, which is compatible with setuptools < 60.0 + plain distutils. We're moving away from that soon, and switching the whole build system to Meson (following what SciPy already did).

We are still accepting targeted fixes/patches for numpy.distutils for the time being, as long as it doesn't break anything, seems reasonable, and is reviewable with modest effort.

If numpy isn't using setuptools, then SETUPTOOLS_USE_DISTUTILS=local won't matter, right? It's only required because the stdlib distutils has known issues that have been fixed.

So it matters today, not in the future. setuptools has new fixes, but also multiple new regressions in combination with numpy.distutils. Given the whole monkey-patching business, that's just not supportable. Plain distutils is stable, setuptools.distutils does not work for numpy and is explicitly not supported.

@pbo-linaro
Copy link
Author

from a "will it work with numpy at all" perspective, this PR may work while gh-1144 is unlikely to ever work

I'm 99.9% sure the answer is that this PR won't work either, because numpy will be built for amd64 and then renamed for ARM64. The only way that's changing is if numpy.distutils already knows how to decide to build for ARM64 (since that's not in distutils), has fixed the bugs that are fixes in setuptools, and in that case it can generate the correct .cp310-win_arm64.pyd extension by itself and doesn't need the renaming that's in this PR.

distutils recognizes env var VSCMD_ARG_TGT_ARCH to find which compiler should be called (it works well with numpy). The only issue staying is the name of compiled dll, which was fixed later in distutils, but not yet available in the version they use. That's what this PR was targetting.

@pbo-linaro
Copy link
Author

Hope to see things going on for numpy in near future! You can close this PR.

@joerick
Copy link
Contributor

joerick commented Jul 1, 2022

I'd like to understand a little better what's going on here. I'll try to summarise, please tell me where I'm wrong. Of the two approaches, we have:

  • support Windows cross compilation (x64 -> arm64) #1108 - this PR. It works by

    1. patching VSCMD_ARG_TGT_ARCH, a compiler flag that sets the target arch of the compiler to arm64
    2. copying .lib files from arm64-python/libs to native-python/libs.

    That works to compile the code correctly, but setuptools/distutils doesn't know it's cross-compiling, so gives us the wrong dylib (aka pyd) file extension inside the wheel. So there's a post-processing step to rename those files.

  • Enable building for Windows ARM64 on a non-ARM64 device #1144 from @zooba. It works by patching a file inside the setuptools library on-disk called 'distutils.cfg', which changes distutils' behaviour, doing a few things:

    1. it sets plat_name, meaning setuptools invokes the compiler to target the correct arch
    2. it sets library_dirs, meaning that the .lib files for the target arch can be found during compilation

    SETUPTOOLS_EXT_SUFFIX is then set to resolve the dylib file extension issue. And then we set SETUPTOOLS_USE_DISTUTILS=local to ensure that the modifications to distutils.cfg are picked up.

Have I got that right so far?

@pbo-linaro
Copy link
Author

pbo-linaro commented Jul 1, 2022

Yes, you got things right. To summarize, the big problem is that VSCMD_ARG_TGT_ARCH was added in distutils (controls compilation), while SETUPTOOLS_EXT_SUFFIX (controls name of dll in wheels) was added after in setuptools.

For #1108, I would add that the original intention was to work with existing setuptools/distutils, and not relying on projects having updated their version.

I'm a bit afraid that with #1144, even if it's more correct on the long term (using existing env var to solve problem), we'll have to wait for several months before projects (...numpy) update to minimal version required.

IMHO opinion, both approach are complementary and could coexist in the tool (and #1108 could be activated only if version of setuptools is not high enough). I still understand it's not ideal and not easy to maintain, but that's the status of python cross compilation at this date alas.

@joerick
Copy link
Contributor

joerick commented Jul 2, 2022

Thanks for the response @pbo-linaro! Then I feel more confident in moving forward with #1144 over this. There, the modifications are only to the build venv & environment, not to the installed python or a postprocessing step.

And since stdlib distutils is deprecated, the advantages that this PR has will be (hopefully!) short-lived. So I'll close this in favour of #1144. Thank you for submitting it though!

@joerick joerick closed this Jul 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants