From aa8409673c6945cd1ec70fbe3e00e619a0d97d5a Mon Sep 17 00:00:00 2001 From: Andrey Fedorov Date: Tue, 11 Aug 2026 10:53:10 -0400 Subject: [PATCH 1/2] Select the dcmqi archive by target architecture, not by host The archive picked for a wheel was decided by falling through to a default: `archive` started as "linux" and was only reassigned for APPLE or for 64-bit non-ARM WIN32. Two builds came out wrong. The win32 wheel published on every release contained the x86-64 binaries from dcmqi-*-win64.zip. cibuildwheel's CIBW_ARCHS "auto" means AMD64 *and* x86 on Windows, and the 32-bit build ran on a 64-bit runner, so the host-based `cmake_host_system_information(... IS_64BIT)` check said 64-bit and selected win64. A user installing that wheel got executables their interpreter cannot run. Windows ARM64 was worse: it failed the WIN32 branch and kept the initial value, so it would have shipped the *Linux* tarball inside a Windows wheel. Selection now keys off the architecture being built *for*. Visual Studio generators carry that in CMAKE_GENERATOR_PLATFORM, which scikit-build-core sets from the target interpreter, and macOS cross builds carry it in CMAKE_OSX_ARCHITECTURES (derived from the ARCHFLAGS cibuildwheel sets). CMAKE_SYSTEM_PROCESSOR reports the host unless cross-compiling, so it is only the fallback. There is no default archive any more: a platform/architecture with no matching asset is a hard error naming what it looked for, because a mismatched archive extracts and installs happily and only fails much later, on a user's machine. The logic moves to a new hand-written dcmqiArchive.cmake, leaving dcmqiUrls.cmake to the data it is generated from. update-dcmqi.yml used to emit both, so fixing the selection in place would have been reverted by the next weekly run. cd.yml pins Windows to AMD64 rather than probing, since dcmqi publishes no 32-bit binaries for the win32 wheel to have contained. Verified against 16 simulated platform cases, including the two bugs above, and by building a macOS arm64 wheel end to end and confirming its binaries are arm64 Mach-O. Co-Authored-By: Claude Opus 5 --- .github/workflows/cd.yml | 9 +++ .github/workflows/update-dcmqi.yml | 47 +++------------ CMakeLists.txt | 4 +- dcmqiArchive.cmake | 95 ++++++++++++++++++++++++++++++ dcmqiUrls.cmake | 35 +---------- 5 files changed, 118 insertions(+), 72 deletions(-) create mode 100644 dcmqiArchive.cmake diff --git a/.github/workflows/cd.yml b/.github/workflows/cd.yml index e81d4b4..9cfba59 100644 --- a/.github/workflows/cd.yml +++ b/.github/workflows/cd.yml @@ -92,6 +92,12 @@ jobs: skip: "*musllinux*" arch: "x86_64" + # cibuildwheel's "auto" on Windows means AMD64 *and* x86, but dcmqi publishes no + # 32-bit binaries. The win32 wheel it produced could therefore only ever contain + # the 64-bit ones, so the architecture is pinned rather than probed. + - os: windows-latest + arch: "AMD64" + - os: macos-14 arch: "arm64" @@ -102,6 +108,9 @@ jobs: - os: ubuntu-latest arch: "auto" + - os: windows-latest + arch: "auto" + steps: - uses: actions/checkout@v7 with: diff --git a/.github/workflows/update-dcmqi.yml b/.github/workflows/update-dcmqi.yml index 55bbc7f..4d1058b 100644 --- a/.github/workflows/update-dcmqi.yml +++ b/.github/workflows/update-dcmqi.yml @@ -84,9 +84,16 @@ jobs: esac done - # Generate dcmqiUrls.cmake + # Generate dcmqiUrls.cmake. This file carries data only -- the version, and one + # filename/checksum pair per published asset. Choosing between those assets is done + # by dcmqiArchive.cmake, which is hand-written and must not be emitted here: it was + # previously generated below, so any fix to the selection logic was silently + # reverted by the next run of this workflow. { echo '# Checksums computed from assets associated with the dcmqi GitHub release' + echo '#' + echo '# Generated by .github/workflows/update-dcmqi.yml -- do not edit by hand.' + echo '# Which of these assets a build uses is decided in dcmqiArchive.cmake.' echo '' echo "set(version \"$latest_version\")" echo '' @@ -117,44 +124,6 @@ jobs: win_file="dcmqi-${latest_version}-win64.zip" echo "set(win64_filename \"$win_file\")" echo "set(win64_sha256 \"${checksums[$win_file]}\")" - echo '' - echo '' - - # Platform detection - echo 'cmake_host_system_information(RESULT is_64bit QUERY IS_64BIT)' - echo '' - echo 'set(archive "linux")' - echo '' - echo 'if(APPLE)' - if [ "$has_mac_split" = true ]; then - echo ' if(CMAKE_SYSTEM_PROCESSOR STREQUAL "arm64")' - echo ' set(archive "macos_arm64")' - echo ' else()' - echo ' set(archive "macos_x86_64")' - echo ' endif()' - else - echo ' set(archive "macos")' - fi - echo 'endif()' - echo '' - echo 'if(WIN32)' - echo ' if(is_64bit AND NOT (${CMAKE_SYSTEM_PROCESSOR} STREQUAL "ARM64"))' - echo ' set(archive "win64")' - echo ' endif()' - echo 'endif()' - echo '' - echo 'if(NOT DEFINED "${archive}_filename")' - echo ' message(FATAL_ERROR "Failed to determine which archive to download: '"'"'${archive}_filename'"'"' variable is not defined")' - echo 'endif()' - echo '' - echo 'if(NOT DEFINED "${archive}_sha256")' - echo ' message(FATAL_ERROR "Could you make sure variable '"'"'${archive}_sha256'"'"' is defined ?")' - echo 'endif()' - echo '' - echo 'set(dcmqi_archive_filename "${${archive}_filename}")' - echo 'set(dcmqi_archive_sha256 "${${archive}_sha256}")' - echo '' - echo 'set(dcmqi_archive_url "https://github.com/QIICR/dcmqi/releases/download/v${version}/${dcmqi_archive_filename}")' } > dcmqiUrls.cmake # Update binaries.txt with discovered binary list diff --git a/CMakeLists.txt b/CMakeLists.txt index 533a6e2..f273a84 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,10 +4,12 @@ cmake_minimum_required(VERSION 3.15...3.26) project(${SKBUILD_PROJECT_NAME} LANGUAGES NONE) -# Set in the current scope the following variables: +# dcmqiUrls.cmake is generated: it lists every published asset and its checksum. +# dcmqiArchive.cmake then picks the one matching this build and sets in the current scope: # - dcmqi_archive_url # - dcmqi_archive_sha256 include(${CMAKE_CURRENT_SOURCE_DIR}/dcmqiUrls.cmake) +include(${CMAKE_CURRENT_SOURCE_DIR}/dcmqiArchive.cmake) # # Download & extract archive diff --git a/dcmqiArchive.cmake b/dcmqiArchive.cmake new file mode 100644 index 0000000..63bfdf1 --- /dev/null +++ b/dcmqiArchive.cmake @@ -0,0 +1,95 @@ +# Selects the dcmqi release asset matching the platform and architecture being built for. +# +# The available assets and their checksums come from dcmqiUrls.cmake, which the update-dcmqi +# workflow regenerates; this file holds the logic and is written by hand. +# +# Picking the wrong asset is silent: a mismatched archive still extracts and installs, and the +# wheel only fails later, when a user runs a binary that cannot execute on their machine. So +# anything short of an exact platform/architecture match is a hard error here. + +if(NOT DEFINED version) + message(FATAL_ERROR "dcmqiUrls.cmake must be included before dcmqiArchive.cmake") +endif() + +# The architecture this build *targets*, which is not always the host's: +# +# * Visual Studio generators carry the target in CMAKE_GENERATOR_PLATFORM, and +# scikit-build-core sets it from the interpreter the wheel is being built for. A 32-bit +# Python on a 64-bit runner therefore reports "Win32" here while the host still looks +# 64-bit -- precisely the case that used to be mistaken for win64. +# * macOS cross builds set CMAKE_OSX_ARCHITECTURES (scikit-build-core derives it from +# ARCHFLAGS, which cibuildwheel sets). +# +# CMAKE_SYSTEM_PROCESSOR reports the host whenever we are not cross-compiling, so it only +# serves as the fallback. +set(_dcmqi_arch "${CMAKE_SYSTEM_PROCESSOR}") +if(APPLE AND CMAKE_OSX_ARCHITECTURES) + list(LENGTH CMAKE_OSX_ARCHITECTURES _dcmqi_arch_count) + if(_dcmqi_arch_count GREATER 1) + message(FATAL_ERROR + "CMAKE_OSX_ARCHITECTURES asks for a universal build (${CMAKE_OSX_ARCHITECTURES}), but " + "dcmqi publishes one archive per architecture. Build a separate wheel for each.") + endif() + set(_dcmqi_arch "${CMAKE_OSX_ARCHITECTURES}") +elseif(CMAKE_GENERATOR_PLATFORM) + set(_dcmqi_arch "${CMAKE_GENERATOR_PLATFORM}") +endif() + +string(TOLOWER "${_dcmqi_arch}" _dcmqi_arch) +if(_dcmqi_arch MATCHES "^(x86_64|x64|amd64)$") + set(_dcmqi_arch "x86_64") +elseif(_dcmqi_arch MATCHES "^(arm64|aarch64)$") + set(_dcmqi_arch "arm64") +elseif(_dcmqi_arch MATCHES "^(win32|x86|i[3-6]86)$") + set(_dcmqi_arch "x86") +endif() + +if(APPLE) + set(_dcmqi_platform "macos") +elseif(WIN32) + set(_dcmqi_platform "win") +elseif(UNIX) + set(_dcmqi_platform "linux") +else() + message(FATAL_ERROR + "dcmqi publishes no binary package for this operating system " + "(CMAKE_SYSTEM_NAME=${CMAKE_SYSTEM_NAME})") +endif() + +# Asset variables are named _. The two assets whose upstream names predate +# architecture qualification get an alias: "linux" and "win64" both mean x86_64. See +# CPACK_SYSTEM_NAME in QIICR/dcmqi. +set(_dcmqi_candidates "${_dcmqi_platform}_${_dcmqi_arch}") +if(_dcmqi_platform STREQUAL "linux" AND _dcmqi_arch STREQUAL "x86_64") + list(APPEND _dcmqi_candidates "linux") +elseif(_dcmqi_platform STREQUAL "win" AND _dcmqi_arch STREQUAL "x86_64") + list(APPEND _dcmqi_candidates "win64") +elseif(_dcmqi_platform STREQUAL "macos") + # Older releases shipped a single, unqualified macOS archive; the generator still emits + # that name if upstream ever stops splitting by architecture. + list(APPEND _dcmqi_candidates "macos") +endif() + +set(archive "") +foreach(_dcmqi_candidate IN LISTS _dcmqi_candidates) + if(DEFINED ${_dcmqi_candidate}_filename AND DEFINED ${_dcmqi_candidate}_sha256) + set(archive "${_dcmqi_candidate}") + break() + endif() +endforeach() + +if(NOT archive) + string(REPLACE ";" ", " _dcmqi_tried "${_dcmqi_candidates}") + message(FATAL_ERROR + "dcmqi v${version} publishes no binary package for ${_dcmqi_platform}/${_dcmqi_arch}, so no " + "wheel can be built for it. Looked for the asset variables: ${_dcmqi_tried}. " + "Published assets: https://github.com/QIICR/dcmqi/releases/tag/v${version}") +endif() + +set(dcmqi_archive_filename "${${archive}_filename}") +set(dcmqi_archive_sha256 "${${archive}_sha256}") +set(dcmqi_archive_url + "https://github.com/QIICR/dcmqi/releases/download/v${version}/${dcmqi_archive_filename}") + +message(STATUS + "dcmqi archive for ${_dcmqi_platform}/${_dcmqi_arch}: ${dcmqi_archive_filename}") diff --git a/dcmqiUrls.cmake b/dcmqiUrls.cmake index 239be5e..506df90 100644 --- a/dcmqiUrls.cmake +++ b/dcmqiUrls.cmake @@ -1,4 +1,7 @@ # Checksums computed from assets associated with the dcmqi GitHub release +# +# Generated by .github/workflows/update-dcmqi.yml -- do not edit by hand. +# Which of these assets a build uses is decided in dcmqiArchive.cmake. set(version "1.5.6") @@ -13,35 +16,3 @@ set(macos_x86_64_sha256 "6678f2a60547c3a14a9f0376932217c3faee28158b6805c3b89d3 set(win64_filename "dcmqi-1.5.6-win64.zip") set(win64_sha256 "72f4bf6bc6b265e5843c42436267fb59a8079ca765095f218300d019f9e15e3d") - - -cmake_host_system_information(RESULT is_64bit QUERY IS_64BIT) - -set(archive "linux") - -if(APPLE) - if(CMAKE_SYSTEM_PROCESSOR STREQUAL "arm64") - set(archive "macos_arm64") - else() - set(archive "macos_x86_64") - endif() -endif() - -if(WIN32) - if(is_64bit AND NOT (${CMAKE_SYSTEM_PROCESSOR} STREQUAL "ARM64")) - set(archive "win64") - endif() -endif() - -if(NOT DEFINED "${archive}_filename") - message(FATAL_ERROR "Failed to determine which archive to download: '${archive}_filename' variable is not defined") -endif() - -if(NOT DEFINED "${archive}_sha256") - message(FATAL_ERROR "Could you make sure variable '${archive}_sha256' is defined ?") -endif() - -set(dcmqi_archive_filename "${${archive}_filename}") -set(dcmqi_archive_sha256 "${${archive}_sha256}") - -set(dcmqi_archive_url "https://github.com/QIICR/dcmqi/releases/download/v${version}/${dcmqi_archive_filename}") From ce27e8c493309f51971f59b5627b02bcdc4ff6bf Mon Sep 17 00:00:00 2001 From: Andrey Fedorov Date: Tue, 11 Aug 2026 14:21:31 -0400 Subject: [PATCH 2/2] Name the actual variables in the unsupported-platform error The message promised "the asset variables" but listed the candidate prefixes (win64, macos_arm64), which are not names anyone can grep for in dcmqiUrls.cmake. It now prints the _filename variables it looked up and says a matching _sha256 is required, so the message points straight at what is missing. Addresses Copilot's review comment on this PR. Co-Authored-By: Claude Opus 5 --- dcmqiArchive.cmake | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/dcmqiArchive.cmake b/dcmqiArchive.cmake index 63bfdf1..cbada1a 100644 --- a/dcmqiArchive.cmake +++ b/dcmqiArchive.cmake @@ -79,11 +79,15 @@ foreach(_dcmqi_candidate IN LISTS _dcmqi_candidates) endforeach() if(NOT archive) - string(REPLACE ";" ", " _dcmqi_tried "${_dcmqi_candidates}") + set(_dcmqi_tried "") + foreach(_dcmqi_candidate IN LISTS _dcmqi_candidates) + list(APPEND _dcmqi_tried "${_dcmqi_candidate}_filename") + endforeach() + string(REPLACE ";" ", " _dcmqi_tried "${_dcmqi_tried}") message(FATAL_ERROR "dcmqi v${version} publishes no binary package for ${_dcmqi_platform}/${_dcmqi_arch}, so no " - "wheel can be built for it. Looked for the asset variables: ${_dcmqi_tried}. " - "Published assets: https://github.com/QIICR/dcmqi/releases/tag/v${version}") + "wheel can be built for it. Looked in dcmqiUrls.cmake for ${_dcmqi_tried} (and a matching " + "_sha256). Published assets: https://github.com/QIICR/dcmqi/releases/tag/v${version}") endif() set(dcmqi_archive_filename "${${archive}_filename}")