Repository navigation
Install dicom3tools from PyPI instead of GitHub release assets - #561
Merged
Merged
Conversation
dciodvfy was obtained two different ways, and one of them had never worked. The composite action downloaded per-platform tarballs from a GitHub release, verifying three SHA-256 digests because a release asset can be replaced in place under an unchanged tag. Installing from PyPI removes that concern along with the download, unpacking and per-OS archive handling: PyPI filenames are immutable, so the exact version pin is itself a pin on the bytes CI executes. The packaged programs are located through the distribution's own bin_dir(), which gives CMake the native executables rather than the console-script shims. docker/Makefile's prereq.build_dicom3tools fetched the *Windows* .zip, unpacked it with `tar xjf` and cd'd into a directory the flat archive does not contain, with the whole && chain swallowed by `|| echo "Reusing "`. It reported success while find_program() found nothing, so the conformance tests were silently dropped from the Linux/x86_64 job. It now installs the same PyPI package inside the dockcross container, where the tests actually run; manylinux_2_17_x86_64 matches that image's glibc 2.17. The wheels also close the gap that kept dciodvfy off Linux/arm64: the published manylinux_2_28_aarch64 build matches that job's container exactly, so it uses the action too. Both Linux jobs now assert the tests were registered before running the suite, as macOS and Windows already did. Verified locally that the pip-installed dciodvfy produces byte-identical output and exit codes to the release binary it replaces, across all 19 DICOM files in data/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Python terminates lines with CRLF on Windows and `read -r` keeps the CR, so the captured path carried a trailing carriage return: the file test missed dciodvfy and the step failed with ls: cannot access '.../dicom3tools/bin'$'\r': No such file or directory The install itself was fine -- the win_amd64 wheel downloaded and unpacked -- so this only ever affected the path capture. No-op on macOS and Linux, where the same output is LF-terminated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Matches docker/Makefile's dicom3tools_version and the DICOM3TOOLS_VERSION environment variable, and follows the convention of the setup-* actions (python-version, node-version) rather than a bare `version`. All three callers use the default, so nothing else changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
fedorov
added a commit
that referenced
this pull request
Sep 11, 2026
The dciodvfy tests for the three multiframe source variants derived both the name of the test they depend on and the file they verify from one loop variable, but the producing tests and their output files used different suffixes: the tests are named after the full variant (_2frames_per_plane, _shared_plane_position) while the files were abbreviated (_2frames, _shared). Two of the three TEST_DEPENDS therefore named tests that do not exist. CTest ignores an unknown DEPENDS silently, so nothing failed - but under ctest -j the dciodvfy test could run before the conversion that writes its input. This stayed invisible because CI had no dciodvfy until #559/#561, which install dicom3tools from PyPI on all platforms; the tests are created and run there now. Name the output files after the variants as well, so a single identifier drives the test name, the file and the dependency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dciodvfywas obtained two different ways, and one of them had never worked.The composite action
.github/actions/install-dicom3toolsdownloaded per-platform tarballs from a GitHub release, verifying three SHA-256 digests because a release asset can be replaced in place under an unchanged tag. It now installsdicom3toolsfrom PyPI, pinned to20260901(the current latest) and overridable via aversion:input.This drops the download, the per-OS archive handling (tar / 7z / unzip) and the digests: PyPI filenames are immutable, so the exact version pin is itself a pin on the bytes CI executes. The programs are located through the distribution's own
bin_dir(), which puts the native executables onPATHrather than the Python console-script shims.The Linux/x86_64 job
docker/Makefile'sprereq.build_dicom3toolsfetched the Windows.zip, unpacked it withtar xjf, andcd'd into a directory the flat archive does not contain — with the whole&&chain swallowed by|| echo "Reusing ". It reported success whilefind_program()found nothing, so the conformance tests were silently dropped from this job.It is replaced by
prereq.install_dicom3tools, which installs the same PyPI package inside the dockcross container, where the tests actually run (the composite action installs on the runner and would never reach them).manylinux_2_17_x86_64matches that image's glibc 2.17.The Linux/arm64 job
The wheels also close the gap that kept
dciodvfyoff arm64: the publishedmanylinux_2_28_aarch64build matches that job's container exactly, so it now uses the action too. This is what the existing TODO in the workflow asked for once manylinux assets existed.Both Linux jobs now assert the tests were registered before running the suite, as macOS and Windows already did.
Verification
Done locally before pushing:
data/.RUNNER_TEMP,GITHUB_PATH);dciodvfyresolves onPATHexactly asfind_program()would.PATH: nonexistent version, and--only-binaryrefusing the sdist stub on a platform with no wheel.dockcross, and the registration guard was tested across the registered / zero / no-match cases.Two bugs surfaced during that checking and are fixed in this branch: the
bin_dir()command substitution was being evaluated by the host shell (where the venv does not exist) instead of the container, and twodockcrossinvocations would have mounteddocker/instead of the repo root.What CI needs to confirm
Docker was not available locally, so the dockcross path was verified only by expansion and stub execution, not against the real container. Specifically:
/opt/python/(the wheels require it; the Makefile still pins cp38 for the build itself). The recipe picks the newestcp31[0-9]-cp31[0-9]and fails with a clear message if there is none.dciodvfytests have never run before, so they may surface genuine conformance failures on first run.ð 8000 Ÿ¤– Generated with Claude Code