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

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

Closed
dlech opened this issue Jun 8, 2024 · 8 comments · Fixed by #1876

Comments

@dlech
Copy link
Contributor

dlech commented Jun 8, 2024

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 #1144 (comment)

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
Copy link
Contributor

henryiii commented Jun 10, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

dlech commented Jun 11, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

@dlech
Copy link
Contributor Author

dlech commented Jun 11, 2024

Sure, I can do that.

@MusicalNinjaDad
Copy link
Contributor

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:

to

uses: musicalninjadad/cibuildwheel@MusicalNinjaDad/issue1861

dlech added a commit to dlech/micropython-uncrustify that referenced this issue Jun 30, 2024
@dlech
Copy link
Contributor Author

dlech commented Jun 30, 2024

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

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 a pull request may close this issue.

3 participants