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

Enable building for Windows ARM64 on a non-ARM64 device #1144

Merged
merged 48 commits into from Oct 11, 2022
Merged

Conversation

zooba
Copy link
Contributor

@zooba zooba commented Jun 17, 2022

No description provided.

@zooba
Copy link
Contributor Author

zooba commented Jun 17, 2022

This replaces #1108. Note that it won't actually work until we get a setuptools release with pypa/distutils#153 integrated (which hopefully will be pretty soon).

I'll add at least one test, just wanted to get the core of the change up first.

cibuildwheel/windows.py Outdated Show resolved Hide resolved
@zooba
Copy link
Contributor Author

zooba commented Jun 17, 2022

So the test is xfail right now (I haven't marked it), until we get a setuptools update out. Since it's going to fail for literally everyone all of the time, I'd understand not merging until it's going to work.

Though anyone trying to cross-compile today is going to fail earlier (when it tries to run ARM64 Python on an x64 machine), so from the POV that this is technically better, I'd be happy to get it in. That also means I won't have to redo the PR if the code changes in the meantime ;)

Up to the maintainers. I'm happy to tweak/adjust/document whatever else is needed, just lmk.

cibuildwheel/windows.py Outdated Show resolved Hide resolved
Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this @zooba ! Cross-compilation is always difficult, but this one looks fairly manageable to me.

  • We'll have to note in our documentation of this feature that it's setuptools-only.
  • I suspect this isn't going to be compatible with CIBW_BUILD_FRONTEND=build, at least in its current form. The only workaround I can think would be to preinstall setuptools in the venv and disable build isolation --no-isolation. Or we could say that this cross-compilation method only supports CIBW_BUILD_FRONTEND=pip. But I was hoping to switch the default to build at some point, so maybe the build-specific workarounds would be a good idea.

cibuildwheel/windows.py Outdated Show resolved Hide resolved
Comment on lines 132 to 138
for p in env["PATH"].split(os.pathsep):
distutils_cfg = Path(p) / "Lib/site-packages/setuptools/_distutils/distutils.cfg"
if distutils_cfg.parent.is_dir():
break
else:
log.warning("Did not find setuptools install to configure")
return
Copy link
Contributor

Choose a reason for hiding this comment

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

On this method more broadly, I do worry that it would end up modifying something outside of the build venv if setuptools isn't installed there, but is in a venv higher up.

So I might propose a change to this logic, like

Suggested change
for p in env["PATH"].split(os.pathsep):
distutils_cfg = Path(p) / "Lib/site-packages/setuptools/_distutils/distutils.cfg"
if distutils_cfg.parent.is_dir():
break
else:
log.warning("Did not find setuptools install to configure")
return
venv_python = call(
["python", "-c", "import sys; sys.stdout.write(sys.executable)"],
env=env,
capture_stdout=True,
)
distutils_cfg = Path(venv_python).parent / "Lib/site-packages/setuptools/_distutils/distutils.cfg"
if not distutils_cfg.parent.is_dir():
log.warning("Did not find setuptools install to configure for cross-compilation.")
return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with this new logic is that call(["python" ... doesn't actually search PATH to resolve python. It first looks in the current executable directory (normal Windows behaviour, assuming that you want the files you installed with your own app before searching other installs). So it only works if we're already running in the correct Python, in which case we may as well go straight to sys.prefix.

Also, if we're going to run Python and import it, I'd rather import distutils; print(distutils.__file__) and work from that to save a few assumptions. Or deliberately keep the venv path around from earlier - I tried that briefly and decided it wasn't worth the changes, but it's going to be the most reliable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, and we alse know that env["PATH"] contains the right venv first because we just updated it ourselves.

Copy link
Contributor

@joerick joerick Jul 4, 2022

Choose a reason for hiding this comment

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

The problem with this new logic is that call(["python" ... doesn't actually search PATH to resolve python. It first looks in the current executable directory (normal Windows behaviour, assuming that you want the files you installed with your own app before searching other installs).

🤯 whoa really? when you say the 'current executable directory', do you mean the working directory, which would be the project dir in this case? We do call(["python" ... all over the place and we've never had an issue with the wrong one being invoked. Maybe because people don't tend to have a python.exe in the root of their project.

Also, if we're going to run Python and import it, I'd rather import distutils; print(distutils.__file__) and work from that to save a few assumptions.

I'd prefer that, I think. We probably won't preinstall setuptools in the build venv forever. Though I think it should be import setuptools._distutils; print(setuptools._distutils.__file__), right? (though, depending on the outcome of the isolated build-backend venv conversation, it might be a moot point...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoa really? when you say the 'current executable directory', do you mean the working directory, which would be the project dir in this case?

No, it's more like (don't reuse this code) os.path.split(sys.orig_argv[0]). The current working directory lookup is a "feature" of the old cmd shell, but looking in the application directory is a "feature" of CreateProcess (the underlying OS API call). Best way to disable it is to pass a full path.

Though I think it should be import setuptools._distutils; print(setuptools._distutils.__file__), right?

That isn't forward compatible though. setuptools._distutils is an internal name, and I expect setuptools to deprecate their distutils shim almost immediately and get people using their commands directly.

But yeah, isolated backends are going to break this approach anyway. Setuptools is currently broken anyway because of another change, so if they're amenable to having their build* commands pick up default values from environment variables, we can probably get that in (though I'm about to be AFK for at least a week, and then at EuroPython the week after that, so it might have to wait until the sprints).

cibuildwheel/windows.py Outdated Show resolved Hide resolved
test/test_windows.py Outdated Show resolved Hide resolved
cibuildwheel/windows.py Show resolved Hide resolved
cibuildwheel/windows.py Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor

henryiii commented Jul 2, 2022

It is very important to me that scikit-build can support this (without using it's current dependence on setuptools), as well as mesonpy and Maturin, maybe enscons too. I think it's fine to set setuptools specific variables, maybe write out a config file, but not hack setuptools itself.

I also wouldn't want to mess with anything that might be used in an existing build, and my experience of setup.py files is that could be literally anything.

I don't think we have to (or even can) support every possible setup.py. For a redistributable wheel, retargeting the user profile would hopefully be quite rare. If it's a case of supporting multiple build backends vs. worrying about this, I'd be all for multiple build backends.

I suspect this isn't going to be compatible with CIBW_BUILD_FRONTEND=build, at least in its current form. The only workaround I can think would be to preinstall setuptools in the venv and disable build isolation --no-isolation.

This would be a huge downgrade for build, since build isolation allows the install of arbitrary and pinned packages. I'd say we can't give up build isolation except as an optional and opt-in last resort. Pip uses build isolation too, by the way, is this modifying the isolated setuptools in that case? Edit: yes, looks like the test project is missing build isolation, which I'd hope most cibuildwheel projects use, and basically all cibuildwheel projects should use; that's the correct way to specify build dependencies.

Wouldn't it be possible to add a cross compile setting to setuptools? Needing a newer version of setuptools seems better than hacking it. Or the user setting file isn't that bad - we could include a note that changing the user profile directory will break Windows ARM cross-compiles. If you have such a setup.py, you need to natively compile on ARM. :)

@joerick
Copy link
Contributor

joerick commented Jul 2, 2022

It is very important to me that scikit-build can support this (without using it's current dependence on setuptools), as well as mesonpy and Maturin, maybe enscons too.

So say we all! Maybe it's time to resurrect https://discuss.python.org/t/towards-standardizing-cross-compiling/10357 and @benfogle's draft PEP benfogle/cross-compile-pep-draft#1 ? I'm certainly no expert, but, this PEP makes a lot of sense to me. I do wonder why there are so many options though. I'm still a little fuzzy on why it's more complicated than passing just the arch to the build-backend, but that'll be my ignorance speaking I'm sure!

My other thought is that we do this on macOS using _PYTHON_HOST_PLATFORM, which changes how sysconfig reports the platform. Maybe we could also set that here, since it's become a sort of informal standard?

I think it's fine to set setuptools specific variables, maybe write out a config file, but not hack setuptools itself.

Well, technically, it's hacking distutils :) honestly, I also wish it was possible to do this stuff without such hacks. But it seems that cross-compiling in Python is really this hard. It's certainly no more hacky than the stuff that crossenv does to its virtualenvs!

@henryiii
Copy link
Contributor

henryiii commented Jul 2, 2022

Hacking distutils is even worse, I would expect setuptools wants to start cleaning up the distutils / setuptools distinction after 3.12. :)

I'd love a standardized way to cross compile, or if setuptools could cross compile on windows with _PYTHON_HOST_PLATFORM if that's possible. It would be nice to have something that other backends can easily use, rather than translating a setuptools config file.

@zooba
Copy link
Contributor Author

zooba commented Jul 4, 2022

Yeah, adding the distutils.cfg into the venv and then having the backend create another venv to install setuptools into isn't going to work. Other than that, because all the configuration is implicit, the frontend (pip vs. build) won't matter - we're passing ambient state into any supported backend (currently only setuptools, others will follow, but right now only setuptools has it).

Anyone relying on stdlib distutils is broken as of 3.12, and unsupported as of 3.10. setuptools._distutils is required because it has bugfixes that are not in stdlib.

Adding _PYTHON_HOST_PLATFORM support into setuptools is a much bigger feature. I'm not opposed to it, but it will invariably behave differently from how the macOS one does. Reviving the thread about standardising cross-comp options is a good start, but almost certainly going to become a multi-year process.

I think it's fine to set setuptools specific variables, maybe write out a config file, but not hack setuptools itself.

Well, technically, it's hacking distutils :) honestly, I also wish it was possible to do this stuff without such hacks.

Technically it's using supported configuration files ;) Nothing has been hacked, setuptools just has additional feature development to support this. Adding two more env variables to setuptools for setting plat_name and library_dirs in all the commands that need it would mean we don't need the config file anymore.

@zooba
Copy link
Contributor Author

zooba commented Jul 4, 2022

I guess it's also worth mentioning that I think this is step 1 of the process, and I expect this change to evolve over time as libraries update. If we get the current state of the world working in cibuildwheel, users can use it, and for them any future updates are transparent.

My worry is that if people can't use cibuildwheel for this platform today(ish), they won't use it at all and won't benefit from later updates. I genuinely believe cibuildwheel is the best way forward for Python packaging, and want people to be able to adopt it.

@joerick
Copy link
Contributor

joerick commented Jul 4, 2022

Yeah, adding the distutils.cfg into the venv and then having the backend create another venv to install setuptools into isn't going to work.

Given that this current method isn't going to work pypa/build or with projects that have a pyproject.toml, we should explore some other options. How about adding an env var option to distutils, like

        if "DISTUTILS_CONFIG" in os.environ:
            files.append(os.environ["DISTUTILS_CONFIG"])

in this code:

https://github.com/pypa/distutils/blob/152c13de811b302dcc673f6ed4c595cd29fd671d/distutils/dist.py#L346-L355

@zooba
Copy link
Contributor Author

zooba commented Jul 4, 2022

I think the option should probably be a setuptools one - it's only really bugfixes that go into their distutils copy, and anything new belongs in setuptools.

So I'd suggest it goes around https://github.com/pypa/setuptools/blob/main/setuptools/command/build_ext.py#L136 (and similar places for all the build* commands) to look up plat_name, include_dirs and library_dirs from SETUPTOOLS_PLAT_NAME etc. environment variables if they aren't already set (i.e. by command line options or config file) or append the additional options (since include/library dirs are priority ordered lists).

Adding @jaraco as well, since there's no point designing a feature for him without him here.

@joerick joerick marked this pull request as draft July 15, 2022 17:04
@zooba zooba marked this pull request as ready for review September 26, 2022 19:00
@zooba
Copy link
Contributor Author

zooba commented Sep 26, 2022

I don't know what to make of the test failures - looks like macOS is having its own kind of issue today? And Cirrus CI seems to have blown up or been cancelled or something weird...

@zooba
Copy link
Contributor Author

zooba commented Sep 29, 2022

Called it, Cirrus CI doesn't have the ARM64 compilers installed. So my skip works perfectly :)

SKIPPED [2] test\test_windows.py:38: Requires ARM64 compiler to be installed

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

This looks good to me! Nice work on this @zooba. I feel much happier about this - I think it was worth it to get the environment variable in.

@zooba
Copy link
Contributor Author

zooba commented Oct 3, 2022

I did my docs updates - apologies for not doing it sooner.

I'd love to get this one off my todo list. What's left for me to get it into mergeable state?

cibuildwheel/architecture.py Outdated Show resolved Hide resolved
docs/options.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@joerick joerick merged commit 61999a8 into pypa:main Oct 11, 2022
@joerick
Copy link
Contributor

joerick commented Oct 11, 2022

Thank you @zooba !

@zooba zooba deleted the arm64 branch October 11, 2022 16:38
@zooba
Copy link
Contributor Author

zooba commented Oct 11, 2022

Thanks! Hopefully we won't see any issues appear immediately from this (and that any issues that do appear aren't our fault), but feel free to ping me for anything that comes up. I at least would like to know about it, even if someone else fixes it.

@EwoutH
Copy link

EwoutH commented Nov 5, 2022

When adding win_arm64 to our matrix, I currently get the following error.

cibuildwheel version 2.11.2

Build options:
  platform: 'windows'
  architectures: {<Architecture.AMD64: 'AMD64'>, <Architecture.x86: 'x86'>}
  build_selector: {'build_config': 'cp311-win_arm64', 'skip_config': '',
...

Here we go!

cibuildwheel: No build identifiers selected: BuildSelector(build_config='cp311-win_arm[64](https://github.com/EwoutH/cython/actions/runs/3400179489/jobs/5654426762#step:4:67)', skip_config='', requires_python=None, prerelease_pythons=True)

I think I'm following the docs, but architectures doesn't list Arm64 in the configuration above. Is this a configuration error or an issue with cibuildwheel?

@joerick
Copy link
Contributor

joerick commented Nov 5, 2022

You'll also need to add ARM64 to CIBW_ARCH, since this is a cross-compilation.

@EwoutH
Copy link

EwoutH commented Nov 5, 2022

Thanks for getting back so quickly! So declaring CIBW_BUILD: cp311-win_arm64 for example wouldn't be enough? Would it be nice if CIBW_BUILD implied CIBW_ARCHS? That would make build matrices less complex (and I believe it already does that for macOS arm64).

Edit: I was wrong, macOS doesn't already do that by default, it as explicitly stated in the pyproject.toml.

Thanks!

@joerick
Copy link
Contributor

joerick commented Nov 5, 2022

Rather than using BUILD, you can do --only=cp311-win_arm64 if a single specific build is what you want. The CIBW_BUILD/CIBW_SKIP semantics are such that they can be set cross-platform.

and I believe it already does that for macOS arm64

No, in this case archs is set elsewhere in pyproject.toml.

@henryiii
Copy link
Contributor

henryiii commented Nov 5, 2022

If you want this behavior, you can set arches to all in your config file. Then you will control it entirely with BUILD. But you have to be more careful to not build things your system doesn't support (linux may not have QEMU setup, it will be slow, etc). But you can, it's an easy opt-in.

@henryiii
Copy link
Contributor

What about setting VSCMD_ARG_TGT_ARCH? That's something general that other build backends could pick up and use? Or is there a potential problem in setting it? (Inside a vs environment, it's already set, but then that's what you'd target anyway, I think?)

Currently if I want to use this in scikit-build-core or meson-python I'd have to look in SETUPTOOLS_EXT_SUFFIX for arm.

@zooba
Copy link
Contributor Author

zooba commented Dec 12, 2022

My thinking was that other build backends would want to use their own variables anyway, and cibuildwheel would add additional functions to set those.

If we were to add something more universal, I'd make it, well, more universal :) CIBW_BUILDING_ARCH=<whatever we're building>.

But in general, ISTM that the aim is to pull backend-specific logic into cibuildwheel, rather than pushing cibuildwheel logic down into the backends (in the absence of standardised logic, that is, such as this situation).

@zooba
Copy link
Contributor Author

zooba commented Dec 12, 2022

Or is there a potential problem in setting it? (Inside a vs environment, it's already set, but then that's what you'd target anyway, I think?)

The only potential problem I can think of is if a tool sees the variable and assumes that it's in a VS environment, rather than only having that single variable set. setuptools doesn't make this assumption (on purpose), so hopefully anyone else would follow suit, but it can't really be guaranteed.

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

6 participants