cibuildwheel icon indicating copy to clipboard operation
cibuildwheel copied to clipboard

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

Open dlech opened this issue 1 year ago • 8 comments

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

dlech avatar Jun 08 '24 17:06 dlech

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.

henryiii avatar Jun 10 '24 06:06 henryiii

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.

MusicalNinjaDad avatar Jun 11 '24 04:06 MusicalNinjaDad

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 ?

dlech avatar Jun 11 '24 14:06 dlech

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 avatar Jun 11 '24 20:06 henryiii

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

henryiii avatar Jun 11 '24 20:06 henryiii

Sure, I can do that.

dlech avatar Jun 11 '24 21:06 dlech

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/[email protected]

to

uses: musicalninjadad/cibuildwheel@MusicalNinjaDad/issue1861

MusicalNinjaDad avatar Jun 14 '24 08:06 MusicalNinjaDad

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

dlech avatar Jun 30 '24 17:06 dlech