pypa / cibuildwheel

🎡 Build Python wheels for all the platforms with minimal configuration.
https://cibuildwheel.pypa.io
Other
1.84k stars 235 forks source link

cibuildwheel GitHub action fails due to duplicate skbuild build directory on Windows ARM64 build #1861

Closed dlech closed 2 months ago

dlech commented 3 months ago

Description

I hit a build failure when using skbuild in conjunction with cibuildwheel.

Here is the relevant part of the build log:

     Configuring Project
      Working directory:
        D:\a\micropython-uncrustify\micropython-uncrustify\_skbuild\win-amd64-3.11\cmake-build
      Command:
        'C:\Users\runneradmin\AppData\Local\Temp\pip-build-env-x7dj25c1\overlay\Lib\site-packages\cmake\data\bin/cmake.exe' 'D:\a\micropython-uncrustify\micropython-uncrustify' -G 'Visual Studio 17 2022' --no-warn-unused-cli '-DCMAKE_INSTALL_PREFIX:PATH=D:\a\micropython-uncrustify\micropython-uncrustify\_skbuild\win-amd64-3.11\cmake-install' -DPYTHON_VERSION_STRING:STRING=3.11.9 -DSKBUILD:INTERNAL=TRUE '-DCMAKE_MODULE_PATH:PATH=C:\Users\runneradmin\AppData\Local\Temp\pip-build-env-x7dj25c1\overlay\Lib\site-packages\skbuild\resources\cmake' '-DPYTHON_EXECUTABLE:PATH=C:\Users\runneradmin\AppData\Local\Temp\cibw-run-dk_dqzxv\cp311-win_arm64\build\venv\Scripts\python.exe' '-DPYTHON_INCLUDE_DIR:PATH=C:\Users\runneradmin\AppData\Local\pypa\cibuildwheel\Cache\nuget-cpython\python.3.11.9\tools\Include' '-DPYTHON_LIBRARY:PATH=C:\Users\runneradmin\AppData\Local\pypa\cibuildwheel\Cache\nuget-cpython\pythonarm64.3.11.9\tools\libs\python311.lib' '-DPython_EXECUTABLE:PATH=C:\Users\runneradmin\AppData\Local\Temp\cibw-run-dk_dqzxv\cp311-win_arm64\build\venv\Scripts\python.exe' '-DPython_ROOT_DIR:PATH=C:\Users\runneradmin\AppData\Local\Temp\cibw-run-dk_dqzxv\cp311-win_arm64\build\venv' -DPython_FIND_REGISTRY:STRING=NEVER '-DPython_INCLUDE_DIR:PATH=C:\Users\runneradmin\AppData\Local\pypa\cibuildwheel\Cache\nuget-cpython\python.3.11.9\tools\Include' '-DPython_LIBRARY:PATH=C:\Users\runneradmin\AppData\Local\pypa\cibuildwheel\Cache\nuget-cpython\pythonarm64.3.11.9\tools\libs\python311.lib' '-DPython3_EXECUTABLE:PATH=C:\Users\runneradmin\AppData\Local\Temp\cibw-run-dk_dqzxv\cp311-win_arm64\build\venv\Scripts\python.exe' '-DPython3_ROOT_DIR:PATH=C:\Users\runneradmin\AppData\Local\Temp\cibw-run-dk_dqzxv\cp311-win_arm64\build\venv' -DPython3_FIND_REGISTRY:STRING=NEVER '-DPython3_INCLUDE_DIR:PATH=C:\Users\runneradmin\AppData\Local\pypa\cibuildwheel\Cache\nuget-cpython\python.3.11.9\tools\Include' '-DPython3_LIBRARY:PATH=C:\Users\runneradmin\AppData\Local\pypa\cibuildwheel\Cache\nuget-cpython\pythonarm64.3.11.9\tools\libs\python311.lib' -T v143 -A ARM64 -DCMAKE_BUILD_TYPE:STRING=Release

    Not searching for unused variables given on the command line.
    CMake Error: Error: generator platform: ARM64
    Does not match the platform used previously: x64
    Either remove the CMakeCache.txt file and CMakeFiles directory or choose a different binary directory.
    Traceback (most recent call last):
      File "C:\Users\runneradmin\AppData\Local\Temp\pip-build-env-x7dj25c1\overlay\Lib\site-packages\skbuild\setuptools_wrap.py", line 666, in setup
        env = cmkr.configure(
              ^^^^^^^^^^^^^^^
      File "C:\Users\runneradmin\AppData\Local\Temp\pip-build-env-x7dj25c1\overlay\Lib\site-packages\skbuild\cmaker.py", line 357, in configure
        raise SKBuildError(msg)

We can see that this is targeting ARM64 Windows, but the build directory is _skbuild\win-amd64-3.11\cmake-build. This is the same build directory that was already used by the amd64 build in the same CI job. So cmake is giving the expected error.

I did some digging into the code and this is happening because skbuild is distutils.util.get_platform() to get the platform, so it is getting the host platform instead of the target platform.

This can be overridden by setting the VSCMD_ARG_TGT_ARCH environment variable.

So I added the following to my pyproject.toml:

# HACK: causes distutils to return the proper platform name so that scikit-build
# will not use the same build directory as the host platform build.
[[tool.cibuildwheel.overrides]]
select = "*-win_arm64"
environment = "VSCMD_ARG_TGT_ARCH=arm64"

It would be nice if this "just worked" or was at least the workaround was documented in the various examples.

FWIW, using VSCMD_ARG_TGT_ARCH was also mentioned in https://github.com/pypa/cibuildwheel/pull/1144#issuecomment-1347189520

Build log

https://github.com/dlech/micropython-uncrustify/actions/runs/9429791938/job/25976561751

CI config

https://github.com/dlech/micropython-uncrustify/actions/runs/9429791938/workflow

henryiii commented 3 months ago

I think this works correctly out of the box with scikit-build-core. Well, actually it uses a unique tmp dir every time out of the box, but if you set the recommended build-dir = "build/{wheel_tag}", it uses the computed wheel tag, which is correct.

I think we could probably fix this in scikit-build (classic)?

Personally, I'd like to set VSCMD_ARG_TGT_ARCH, but there was some worry about some systems misbehaving if only that was set.

MusicalNinjaDad commented 3 months ago

Would setting VSCMD_ARG_TGT_ARCH be a (potential) problem? I'd imagine that it would help any systems that use distutils.get_platform() at the possible cost of making other issues more likely, which currently fail in an obvious way. E.g. when the build directory isn't set as recommended.

I'd be happy to take on the task of raising a PR, but don't yet know the codebase principles well enough to make the call on whether this is a good idea.

dlech commented 3 months ago

If it is only a problem with skbuild (classic), then maybe better to fix there? It looks like there is already some special casing for macOS at https://github.com/scikit-build/scikit-build/blob/4dab4576d7a480da7484cfc5c249c86f7d3ecde3/skbuild/constants.py#L39

Maybe we could add something there that looks at SETUPTOOLS_EXT_SUFFIX similar to https://github.com/scikit-build/scikit-build/blob/4dab4576d7a480da7484cfc5c249c86f7d3ecde3/skbuild/platform_specifics/windows.py#L123 ?

henryiii commented 3 months ago

then maybe better to fix there?

That would be good (and why I said we probably could fix it there).

IMO VSCMD_ARG_TGT_ARCH is likely a good thing to set anyway, though; the only problem is if some tooling sees it and expects all the other MSVC variables to be set. I am not aware of anything that does that, and fixing distutils.get_platform() is nice.

henryiii commented 3 months ago

Would you mind copy-pasting the issue to scikit-build?

dlech commented 3 months ago

Sure, I can do that.

MusicalNinjaDad commented 3 months ago

Hi @dlech, I think I have a fix in place for this. Would you be able to point your build(*) to the relevant fork https://github.com/MusicalNinjaDad/cibuildwheel/tree/MusicalNinjaDad/issue1861 and confirm if it works for you?

Based on my tests it should be fine with one exception - building for windows ARM on azure pipelines may well not work; github actions should be OK.

(*) For example if you are building in a gha you can temporarily change:

uses: pypa/cibuildwheel@v2.19.0

to

uses: musicalninjadad/cibuildwheel@MusicalNinjaDad/issue1861
dlech commented 3 months ago

I tried it and the CI passed. https://github.com/dlech/micropython-uncrustify/actions/runs/9733674497/job/26860867054