Skip to content

Select the dcmqi archive by target architecture, not by host - #47

Merged
fedorov merged 2 commits into
mainfrom
add-more-arms
Aug 11, 2026
Merged

Select the dcmqi archive by target architecture, not by host#47
fedorov merged 2 commits into
mainfrom
add-more-arms

Conversation

@fedorov

@fedorov fedorov commented Aug 11, 2026

Copy link
Copy Markdown
Member

The bug

Which dcmqi archive a wheel gets was decided by falling through to a default. archive started as "linux" and was only reassigned for APPLE, or for WIN32 when the host looked 64-bit and non-ARM:

set(archive "linux")
if(WIN32)
  if(is_64bit AND NOT (${CMAKE_SYSTEM_PROCESSOR} STREQUAL "ARM64"))
    set(archive "win64")
  endif()
endif()

Two builds come out wrong.

The win32 wheel we publish on every release contains x86-64 binaries. cibuildwheel's CIBW_ARCHS: auto on Windows means AMD64 and x86, so a 32-bit wheel is built — on a 64-bit runner. cmake_host_system_information(... IS_64BIT) asks about that host, says 64-bit, and selects dcmqi-*-win64.zip. Anyone installing dcmqi-*-win32.whl gets executables their interpreter cannot run.

Windows ARM64 would have been worse. It fails the WIN32 branch and keeps the initial value, so a Windows wheel would have shipped the Linux tarball. Nobody has hit this only because no arm64 Windows wheel is built yet.

The fix

Selection now keys off the architecture the build targets rather than the one it runs on:

  • Visual Studio generators carry the target in CMAKE_GENERATOR_PLATFORM, and scikit-build-core sets it from the interpreter the wheel is for — so a 32-bit Python now reports Win32 even on a 64-bit runner.
  • macOS cross builds carry it in CMAKE_OSX_ARCHITECTURES (scikit-build-core derives it 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. Anything short of an exact platform/architecture match is a hard error naming the asset variables it looked for and linking the release page. This is the real point of the change: a mismatched archive extracts and installs perfectly happily, so the failure surfaces much later, on a user's machine, as a binary that will not execute.

cd.yml pins Windows to AMD64 instead of probing, since dcmqi publishes no 32-bit binaries for that wheel to have contained. Without this the build would now fail loudly rather than ship the wrong thing — correct, but there is no reason to attempt it.

Why a new file

The logic moves to a new, hand-written dcmqiArchive.cmake, leaving dcmqiUrls.cmake as the generated data it is meant to be. update-dcmqi.yml emitted both, so fixing the selection in place would have been silently reverted by the next weekly run.

Verification

  • 16 simulated platform cases pass, covering both bugs above, macOS cross-compilation in each direction, universal2 (rejected — dcmqi ships one archive per architecture), and an unsupported OS.
  • update-dcmqi.yml regenerates the committed dcmqiUrls.cmake byte for byte.
  • A real python -m build produced a macOS arm64 wheel whose binaries are confirmed Mach-O 64-bit executable arm64.

Relation to the ARM work

This stands alone — it fixes a wheel we ship today. It also happens to make the arm64 follow-up a data-only change: the four phase-2 cases (linux_arm64, win_arm64, and the two existing architectures unaffected by their arrival) already pass against simulated assets. Once QIICR/dcmqi#556 lands and a release carries the new archives, only update-dcmqi.yml and the cd.yml matrix need touching; dcmqiArchive.cmake needs no changes.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes dcmqi release-asset selection so wheels download the correct upstream archive for the target platform/architecture (instead of accidentally keying off the build host), and makes mismatches fail fast rather than silently shipping wrong binaries.

Changes:

  • Split generated asset metadata (dcmqiUrls.cmake) from hand-written selection logic (dcmqiArchive.cmake) and wire both into CMakeLists.txt.
  • Make archive selection architecture-aware (including macOS cross-build handling) and error out on unsupported platform/arch combinations.
  • Update release workflows to stop regenerating selection logic and to pin Windows wheel builds to AMD64 (avoiding accidental win32 wheel builds).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
dcmqiUrls.cmake Removes embedded selection logic; keeps generated asset/checksum data with clearer provenance comments.
dcmqiArchive.cmake New hand-written CMake logic to select the correct asset based on target platform/arch and fail loudly on mismatches.
CMakeLists.txt Includes the new archive-selection module after loading generated URLs/checksums.
.github/workflows/update-dcmqi.yml Stops emitting selection logic into the generated file; adds header comments documenting the split of responsibilities.
.github/workflows/cd.yml Pins Windows wheel architecture to AMD64 and excludes Windows auto to avoid attempting unsupported 32-bit wheels.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread dcmqiArchive.cmake Outdated
Comment on lines +82 to +86
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}")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch — fixed in ce27e8c. The message now builds the list from the candidates it actually looked up, so it prints e.g. win_x86_filename rather than win64, and states that a matching _sha256 is required:

dcmqi v1.5.6 publishes no binary package for win/x86, so no wheel can be
built for it.  Looked in dcmqiUrls.cmake for win_x86_filename (and a
matching _sha256).  Published assets:
https://github.com/QIICR/dcmqi/releases/tag/v1.5.6

All 16 selection cases still pass.

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 <candidate>_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 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

.github/workflows/cd.yml:113

  • The matrix excludes ubuntu/windows arch: auto but leaves macos-latest with arch: auto from the base matrix, so an extra macOS job will still run in addition to the explicitly pinned macos-14 and macos-15-intel jobs. This can produce duplicate wheel filenames (and overwrite during artifact merge) or unexpected extra architectures, depending on what macos-latest maps to.
          - os: windows-latest
            arch: "auto"

dcmqiArchive.cmake:71

  • The unqualified legacy macos asset is added as a candidate for any macOS architecture. If a release only has macos_filename (no per-arch split), an arm64 build would silently accept it and could ship x86_64 binaries in an arm64 wheel—exactly the kind of mismatch this file aims to prevent. Consider only allowing the macos fallback for x86_64 (or otherwise gating it) so arm64 fails loudly.
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()

@fedorov
fedorov merged commit d838f83 into main Aug 11, 2026
11 checks passed
@fedorov
fedorov deleted the add-more-arms branch August 11, 2026 18:30
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.

2 participants