Repository navigation
Run dciodvfy conformance tests on macOS and Windows CI - #559
Conversation
The optional dciodvfy tests in apps/{seg,sr,paramaps}/Testing/CMakeLists.txt are
registered only when find_program(DCIODVFY_EXECUTABLE dciodvfy) succeeds, and no
CI job has ever provided dciodvfy, so all seven have been silently skipped
everywhere. They are a real gate: dciodvfy exits non-zero whenever it reports an
Error, so registering them turns DICOM conformance regressions into build
failures.
Add a composite action that installs a prebuilt dicom3tools distribution and
prepends it to PATH, and use it from the macOS and Windows workflows. The action
verifies the archive's SHA-256 rather than trusting the release tag alone, since
a release asset can be replaced in place under an unchanged tag, and smoke-tests
that the binary can actually be loaded so an incompatible download fails loudly
instead of degrading back into a silent skip. Each workflow also asserts that
ctest sees the dciodvfy tests before running the suite.
The install step is placed early because PATH must be set before the inner dcmqi
project is configured, which happens during the build step rather than the
configure step of the superbuild.
Both Linux jobs remain uncovered and are now commented with the reason: the
prebuilt Linux binaries are built on ubuntu-22.04 and reference GLIBC_2.34 /
GLIBC_2.29, while the Linux tests run in dockcross/manylinux2014-x64 (glibc 2.17)
and manylinux_2_28_aarch64 (glibc 2.28), and no aarch64 asset is published.
Tracked upstream in ImagingDataCommons/dicom3tools#19.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
||
| url="https://github.com/$RELEASE_REPO/releases/download/$RELEASE_TAG/$asset" | ||
| echo "Downloading $url" | ||
| curl -fsSL --retry 3 --retry-delay 5 -o "$archive" "$url" |
The Multi-frame Dimension module declared DimensionIndexPointer =
ImagePositionPatient (0020,0032) with FunctionalGroupPointer =
RealWorldValueMappingSequence (0040,9096). Per PS3.3 C.7.6.17.1 the functional
group pointer names the sequence that contains the attribute the dimension index
points at, and ImagePositionPatient lives in PlanePositionSequence (0020,9113),
which ParaMapConverter already writes per frame. RealWorldValueMappingSequence
does not contain it, so the reference resolved to nothing.
The SEG converter already gets this right (Itk2DicomConverter.cpp), which is why
only the parametric map outputs were affected.
Caught by the dciodvfy tests enabled in the preceding commit, which had never
actually run in CI:
Warning - DimensionIndexPointer Attribute not present in any Shared or
Per-Frame Functional Group - (0x0020,0x0032)
Despite the "Warning" wording this fails validation: the emitting check in
dciodvfy sets success = false, and dciodvfy exits 1 when success is false. Most
warnings do not affect its exit status, so the wording is misleading here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
First CI run: 5 of 7 tests pass, 2 caught a real bugmacOS arm64 result: "Warning" here really does fail validationThe output contains no It is not the case that dciodvfy treats warnings as errors in general.
The first warning is the exception. Its emitting site in The underlying defect
mfdim.addDimensionIndex(DCM_ImagePositionPatient, dimUID,
DCM_RealWorldValueMappingSequence, "Frame position");
No baseline or schema references the tag, so metadata round-trip comparisons are unaffected. Note for reviewersThis is the payoff from these tests never having run: the defect has presumably been shipping |
|
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>
Problem
The
dciodvfyconformance tests inapps/{seg,sr,paramaps}/Testing/CMakeLists.txtareregistered only if
find_program(DCIODVFY_EXECUTABLE dciodvfy)succeeds. No CI job has everprovided
dciodvfy, so all seven have been silently skipped everywhere, on every platform,since they were added.
The one place dicom3tools was nominally provisioned —
prereq.build_dicom3toolsindocker/Makefile— does not work:dicom3tools_winexe_*.zip, i.e. prebuilt Cygwin Windows binaries, not source;.zipwithtar xjf, which GNU tar rejects (macOS bsdtar happens to acceptit, which is why this went unnoticed);
cds into a versioned directory the flat archive does not contain.The whole
&&chain falls through to|| echo "Reusing ", so the step reports success whileproducing nothing.
These tests are a genuine gate, not a formality:
dciodvfyexits non-zero whenever it prints anError. Checked against 19 DICOM files underdata/, exit status tracked error count exactly.Change
New composite action
.github/actions/install-dicom3toolsinstalls a prebuilt dicom3toolsdistribution and prepends it to
PATH. It is used from the macOS and Windows workflows, whichenables the seven tests on three jobs:
macos-15-intel,macos-15, andwindows-2022.Design notes:
release asset can be replaced in place under an unchanged tag, so pinning the tag alone does
not pin the bytes CI executes.
means it could not be executed at all). Without this, an incompatible download would degrade
straight back into the silent-skip behaviour this PR exists to fix.
PATHmust be set before the inner dcmqiproject is configured, which happens during the build step, not the configure step, of the
superbuild.
ctest -N -R dciodvfyfinds a non-zero countbefore running the suite, so this cannot rot back into silence unnoticed.
7zon Windows, asunzipis not present on the hosted Windows images.Linux is not covered, deliberately
The published Linux binaries are built on
ubuntu-22.04and referenceGLIBC_2.34/GLIBC_2.29:dcmqi's Linux tests run inside deliberately old-glibc containers, to keep the glibc floor of the
shipped binaries low:
build-linuxdockcross/manylinux2014-x64build-linux-arm64quay.io/pypa/manylinux_2_28_aarch64Both are below the floor, so the binary cannot be loaded. Requested upstream in
QIICR/dicom3tools equivalent repo: ImagingDataCommons/dicom3tools#19. Once
manylinux-built Linux assets exist, enabling Linux is one added step per job plus a digest bump.
Both Linux jobs are commented with this reasoning, replacing a comment that understated the
situation, and the broken
docker/Makefiletarget is documented in place.Review notes
all seven tests pass on current
master— reproducing needs a DCMTK rebuild, since dcmqi nowuses
setBackgroundPixelValue/getBackgroundSegmentNumber. Givendciodvfyhas nevergated anything here, and known conformance gaps already exist in dcmqi output (see the
labelmap exclusion note in
apps/seg/Testing/CMakeLists.txt), a latent error in anon-labelmap path is plausible. Worth watching this PR's own CI run.
Windows-style paths from ctest. Cygwin normally handles these, but it is not verified here.
docker/Makefileis left alone. Its broken dicom3tools target is an independent bug;repairing or removing it is worth a separate change, since even a fixed version cannot use the
current prebuilt Linux release.
fedorov/dicom3tools, a personal repository. Digestpinning limits the blast radius, but if these binaries become a load-bearing CI dependency,
moving the release to
QIICR/dicom3tools— where the 2020 snapshot already lives — would bethe more durable home.
🤖 Generated with Claude Code