NIfTI: track the maintained nifti_clib fork, modernise the CMake plumbing

[gdevenyi]

Why

nifti_clib upstream (NIFTI-Imaging) has not moved since v3.0.0 in July 2020. The ITK fork is the maintained line, so this pins that instead. It carries no tags, so the pin is a commit SHA (f24a607, 2026-07-22).

The bump matters beyond freshness: the fork exports three symbols v3.0.0 did not — nifti_image_write_status, nifti_image_write_bricks_status, nifti_set_fix_floats. The existing nifti_mangle.h left those unprefixed, which is the exact condition it exists to prevent: an unmangled symbol can collide at link time with the niftiio ITK bundles (ITK 4.x here has no ITK_USE_SYSTEM_NIFTI switch).

What

cmake-modules/BuildNIFTI.cmake

• Pin InsightSoftwareConsortium/nifti_clib@f24a607, in step with the identical pin in the libminc submodule.
• Define the NIFTI::niftiio / NIFTI::znz imported targets that nifti_clib's own NIFTIConfig.cmake provides, so the ADD_SUBDIRECTORY()'d consumers (libminc, minctools, Display) link the same way whether NIFTI came from here or from find_package(NIFTI CONFIG). The real config package cannot be used here — ExternalProject builds at build time, long after those consumers configure — so this mirrors it. Build ordering still rides on the existing add_dependencies(... NIFTI) calls, not on the imported targets.

USE_SYSTEM_NIFTI is now a real option(). It had been commented out for years, leaving FIND_PACKAGE(NIFTI REQUIRED) unreachable; it is restored as a proper option so it shows up in ccmake alongside the other USE_SYSTEM_* switches. It selects find_package(NIFTI CONFIG REQUIRED) over build_nifti(), and needs no consumer changes — both paths supply the same two imported targets. LIBMINC_USE_SYSTEM_NIFTI is forwarded so libminc can skip its mangling invariant, and NIFTI drops out of MINC2_deps in system mode since there is no ExternalProject to wait for.

The mangling caveat is documented at the call site rather than treated as a reason to drop the option: the bundled build is renamed to minc_* so it cannot collide with ITK's bundled ITKniftiio, whereas a system nifti is unmangled. That is fine against the shared libniftiio.so distributions package — which is what the release .deb/.rpm link — but not against a static system nifti with MT_BUILD_ITK_TOOLS on.

cmake-modules/FindNIFTI.cmake — deleted. nifti_clib ships a real config package, and the hand-rolled module hardcoded /usr/local/bic and reported everything it found as "NetCDF headers"/"NetCDF library".

Verification

Against a genuine installed system nifti_clib (shared, unmangled, exporting NIFTI::niftiio):

• USE_SYSTEM_NIFTI=ON — configures and generates; mnc2nii/nii2mnc link /usr/lib/libniftiio.so.2.1.0 and /usr/lib/libznz.so.3.0.0; no NIFTI ExternalProject is generated; the exported LIBMINCConfig records the system library.
• USE_SYSTEM_NIFTI=OFF — unchanged; consumers link the staged libniftiio.a/libznz.a and pick up the nifti include directory as -isystem.
• Display, with GIFTI off, references nifti in neither mode.
• Standalone libminc builds and tests in both modes: 38 tests bundled, 37 system, the difference being the correctly-skipped mangling invariant.
• The mangled bundled build exports 112 symbols, all minc_-prefixed, with zero overlap against an unmangled build of the same source.

Ordering

Companion PRs, all against develop-1.9.18:

• NIfTI: track the maintained nifti_clib fork, drop the dead vendored copy libminc#152 — same pin, regenerated nifti_mangle.h, deletes a dead vendored nifti/ copy, skips the mangling test in system mode. Must land with this one: PATCH_COMMAND here reads nifti_mangle.h out of that submodule, so a new pin with the old mangle list leaves those three symbols exposed.
• NIfTI: consume the NIFTI::niftiio imported target minc-tools#127 — consumes NIFTI::niftiio. Its pin should not move before this merges, or the target is undefined and mnc2nii/nii2mnc are silently skipped.
• NIfTI: treat nifti as the GIFTI dependency it actually is Display#90 — folds nifti into its GIFTI_FOUND guards. Independent, since GIFTI is disabled here.
• NIfTI: find nifti_clib properly instead of guessing /usr/local/bic BEaST#15 — replaces NIFTI_ROOT guessing with find_package(NIFTI CONFIG). Independent; BuildBEAST.cmake passes USE_NIFTI=OFF.

Submodule pin bumps are deliberately not included here.

Not in scope

• README.md's options table documents USE_SYSTEM_NIFTI; that table lives in the in-flight Rewrite README: correct install instructions, document what ships #227, where the entry needs its caveat updated.
• INSTALL_COMMAND $(MAKE) in BuildNIFTI.cmake blocks the Ninja generator, but all 24 Build*.cmake modules do the same, so fixing one changes nothing.

🤖 Generated with Claude Code

https://claude.ai/code/session_019katBUnczdvUS7WQ4RpVoa

[Copilot]

Pull request overview

This PR updates the superbuild to always build NIfTI from a pinned, maintained nifti_clib fork (ITK/InsightSoftwareConsortium) and removes the dead “system NIFTI” path and legacy FindNIFTI.cmake, ensuring symbol-mangling avoids collisions with ITK’s bundled niftiio.

Changes:

• Remove the unreachable USE_SYSTEM_NIFTI branch and always build NIfTI from BuildNIFTI.cmake.
• Pin InsightSoftwareConsortium/nifti_clib to a specific commit SHA and add NIFTI::niftiio / NIFTI::znz imported targets mirroring NIFTIConfig.cmake.
• Delete the legacy cmake-modules/FindNIFTI.cmake.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
CMakeLists.txt	Removes the dead system-NIFTI branch and makes NIFTI a consistent superbuild dependency.
cmake-modules/BuildNIFTI.cmake	Pins the maintained fork and adds imported targets for consistent consumer linking.
cmake-modules/FindNIFTI.cmake	Removes obsolete find-module for the dropped system-NIFTI path.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gdevenyi]

Fixed in the review commit.

The quoting is a real issue — unquoted, a prefix containing spaces splits into several arguments and creates the wrong directories. Now:

FILE(MAKE_DIRECTORY "${NIFTI_INCLUDE_DIR}" "${ZNZ_INCLUDE_DIR}")

On the second half: ZNZ_INCLUDE_DIR is set to the same string as NIFTI_INCLUDE_DIR a few lines up (${staging_prefix}/${install_prefix}/include/nifti), so it was already being created — but relying on that is fragile, so both are now named explicitly.

[gdevenyi]

Agreed, fixed in the review commit — and the pointer to BuildZLIB.cmake was the useful part.

NIFTI::znz now links ZLIB::ZLIB. Checked it exists on both paths before build_nifti() runs: USE_SYSTEM_ZLIB=ON goes through FIND_PACKAGE(ZLIB REQUIRED), and OFF goes through build_zlib(), which creates it as a GLOBAL imported target at BuildZLIB.cmake:113. build_zlib() is called well before build_nifti(), so ordering is fine.

Following that file the rest of the way, I also added ADD_DEPENDENCIES(NIFTI::niftiio NIFTI) / ADD_DEPENDENCIES(NIFTI::znz NIFTI), mirroring its ADD_DEPENDENCIES(ZLIB::ZLIB ZLIB). Build ordering now rides on the imported target itself rather than only on the add_dependencies calls at each call site.

[gdevenyi]

Rebased onto develop-1.9.18 at 342c896 (picks up #222 arguments→cxxopts, #223, #228) and addressed both review comments in a third commit.

• NIFTI::znz links ZLIB::ZLIB instead of the raw ${ZLIB_LIBRARY} path.
• NIFTI::niftiio no longer hardcodes m in its interface; it is added under if(UNIX), matching nifti_clib's own NIFTI_SYSTEM_MATH_LIB (empty on WIN32).
• FILE(MAKE_DIRECTORY) is quoted and names both include dirs explicitly.
• Following BuildZLIB.cmake the rest of the way, the imported targets now ADD_DEPENDENCIES(... NIFTI), so build ordering rides on the target rather than only on the call sites.

Re-verified on the new base, both modes, against an installed system nifti_clib:

	configure	nii2mnc links
USE_SYSTEM_NIFTI=OFF	clean	staged libniftiio.a + libznz.a
USE_SYSTEM_NIFTI=ON	clean	/usr/lib/libniftiio.so.2.1.0 + /usr/lib/libznz.so.3.0.0

One thing worth knowing rather than fixing: USE_SYSTEM_NIFTI=ON with USE_SYSTEM_ZLIB=OFF puts both /usr/lib/libz.so and the staged libz.a on the link line, because the system NIFTITargets.cmake bakes in the absolute zlib path it was built against. That is inherent to mixing a system nifti with a bundled zlib and predates this PR; pair the two USE_SYSTEM_* switches to avoid it.

[gdevenyi]

Added a fourth commit (e7cbe31) in response to @vfonov on BIC-MNI/libminc#152, who pointed out that a system NIfTI collides with ITK's. It does, and I had the mechanism wrong in this PR's earlier description.

What I got wrong. I claimed the clash was on nifti_image_read with a static system nifti, and that the shared libniftiio.so distributions ship was safe. Both halves are false:

• ITK 4.x mangles its bundled niftiio (itk_nifti_mangle.h, 103 names), so nifti_image_read is not where they meet.
• ITK ships no mangle for znzlib, so ITKznz exports Xznzclose, znzopen, znzputs, znzread, znzrewind, znzseek, znztell, znzwrite unprefixed — 8 of the system library's 110 exported symbols.
• Shared is exactly the case that breaks. ITK's copies are pulled into the executable, land in its dynamic symbol table, and interpose over libznz.so; the system libniftiio.so then reaches ITK's znz layer for every read.

Nothing fails at link time. Reproduced against the installed system nifti_clib with a stand-in for ITKznz: nifti_image_read returns NULL on a valid file, while the same test with the nifti side mangled returns the image. Full transcript in the libminc thread.

The change. USE_SYSTEM_NIFTI=ON with MT_BUILD_ITK_TOOLS=ON is now a hard configure error explaining why and offering both ways out. A prebuilt library cannot be renamed and ITK 4.x has no ITK_USE_SYSTEM_NIFTI, so there is no third option worth building.

USE_SYSTEM_NIFTI	MT_BUILD_ITK_TOOLS	configure
OFF	ON	ok (default; what CI and release use)
OFF	OFF	ok
ON	OFF	ok
ON	ON	rejected

MT_BUILD_LITE=ON forces the ITK tools off, so it pairs with USE_SYSTEM_NIFTI too — verified.

This keeps the option where a distribution packager would actually want it while making the broken combination unreachable. CI and the release workflow both build MT_BUILD_ITK_TOOLS=ON, so neither can trip over it.

#227 carries the matching README correction — the caveat I wrote there described the wrong symbols and the wrong linkage mode.

[gdevenyi]

Correcting my own evidence above (commit 2572521).

The claim that the system libniftiio.so "returns NULL on valid files" came from a stand-in for ITKznz whose znzopen returned NULL — my stub, not ITK's code. Rebuilt against ITK release-4.14's actual znzlib.c/znzlib.h:

ITKznz from	unmangled exports	znz in exe .dynsym	system nifti_image_read
real ITK release-4.14	15	8	an image
+ itk_znzlib_mangle.h	0	0	an image

The interposition is real and measurable; the breakage is not, on LP64 with both sides built HAVE_ZLIB. What remains, and what the guard is now justified on:

• one definition of each function silently replacing another (UB, invisible);
• a genuine ABI mismatch on ILP32 + large-file support — ITK's znzseek/znztell use long (4 bytes) against nifti_clib 3.x's znz_off_t (8), measured under -m32 -D_FILE_OFFSET_BITS=64;
• sizeof(struct znzptr) differing between builds that disagree on HAVE_ZLIB.

The guard and the behaviour are unchanged — only the comment, the error message and the README note in #227, which now say this instead of the overstated version.

[gdevenyi]

The upstream fix landed, so the restriction is gone. Added in 20dee19.

InsightSoftwareConsortium/ITK#6756 merged to release-4.14 on 2026-08-05 (CI green on Linux, Windows and macOS), giving ITK's bundled znzlib the itk_* prefixing its niftiio has had since 2017. That was the single thing making a system NIfTI unusable alongside the ITK tools.

Pin moves cae3eb95 → 6073b968. Those are the only two commits in between — the fix and its merge — so the bump carries nothing else.

The FATAL_ERROR is removed. With both halves of ITK's copy prefixed, USE_SYSTEM_NIFTI and MT_BUILD_ITK_TOOLS no longer collide.

Verified against the newly pinned tarball:

	old pin cae3eb95	new pin 6073b968
ITKznz unmangled exports	15	0
ITKznz itk_* exports	0	15
znz names in .dynsym of a binary also using system nifti_clib	8	0

and all four permutations configure clean, including the one that was rejected a commit ago:

USE_SYSTEM_NIFTI	MT_BUILD_ITK_TOOLS	configure
ON	ON	ok (was rejected)
OFF	ON	ok
ON	OFF	ok
OFF	OFF	ok

Our own NIfTI stays renamed to minc_* regardless — ITK still has no ITK_USE_SYSTEM_NIFTI, so those remain two separate copies and the mangling is what keeps them apart.

This closes out the packaging goal that started the thread: a release build can now use a distribution NIfTI and ship the ITK tools. #231, the local PATCH_COMMAND fallback, is closed as superseded. #227 carries the matching README correction.

[vfonov]

why do we still need to mangle , if ITK accepts system NIFTI?

[gdevenyi]

why do we still need to mangle , if ITK accepts system NIFTI?

For the same reason ITK mangles, we're vendoring our own copy of NIFTI as part of the superbuild, so we should mangle symbols so we don't mess with the system copies if they exist.

[gdevenyi]

Rebased onto develop-1.9.18 at a87c3c1, picking up #226 (shared libs on by default), #227 (README rewrite) and #229 (packaging-tree removal). Six commits replayed with no conflicts.

The README commits this branch depended on landed via #227, so nothing README-related remains here.

Re-verified on the new base, which matters because MT_BUILD_SHARED_LIBS now defaults to ON:

USE_SYSTEM_NIFTI	MT_BUILD_ITK_TOOLS	configure
ON	ON	ok
OFF	ON	ok
ON	OFF	ok
OFF	OFF	ok

On the macOS CI failures: they are pre-existing and not from this branch. develop-1.9.18 itself fails both macOS jobs, at the old base 342c896 and the current a87c3c1, on the same test with a byte-identical value:

relative RMS difference: 0.000850981867212377 larger then 0.0001
The following tests FAILED:
	146 - nu_reference_1 (Failed)
99% tests passed, 1 tests failed out of 150

That is the N3 nu_estimate numerical reference comparison, unrelated to NIfTI. The NIfTI tests on those same runs pass — mnc2nii 7 tests, nii2mnc 13. Linux is green on this branch.

(Unrelated, but noticed while checking: the ubuntu (full) failure on the base run is a GitHub infrastructure blip — Service Unavailable / Failed to resolve action download info — not a build failure.)

[gdevenyi]

@vfonov ITK does not accept a system NIfTI — there is no ITK_USE_SYSTEM_NIFTI, on release-4.14 or on main. Compare the two third-party modules:

Modules/ThirdParty/ZLIB/CMakeLists.txt — system-capable:

option(ITK_USE_SYSTEM_ZLIB "Use system-installed zlib" ${ITK_USE_SYSTEM_LIBRARIES})
if(ITK_USE_SYSTEM_ZLIB)
  find_package(ZLIB REQUIRED)
  set(ITKZLIB_LIBRARIES "ZLIB::ZLIB")

Modules/ThirdParty/NIFTI/CMakeLists.txt — no option, no find_package, always its own copy:

set(ITKNIFTI_LIBRARIES ITKznz ITKniftiio)
itk_module_impl()

So there are always two NIfTI copies in any binary linking both ITK and libminc.

That said, your instinct is right that something changed. Since InsightSoftwareConsortium/ITK#6756 the pinned ITK prefixes both halves of its copy — niftiio to itk_* since 2017, and now znzlib too. Against that ITK, our minc_* mangling is redundant: itk_nifti_image_read and nifti_image_read cannot collide whatever we do.

Two reasons I would keep it anyway, though I am happy to be overruled:

1. It only holds for the 4.14 line. main and v5.4.x have no nifti mangling at all — itk_nifti_mangle.h was deleted in 0cdb1dd5 (2018) when the vendored nifti was refreshed and NIFTI_PACKAGE_PREFIX was adopted, which renames CMake targets rather than C symbols. Any move off 4.14 reinstates the collision, and it reappears silently.
2. ITK is not the only way a second copy arrives. Anything else in the process bringing its own libniftiio — including a system one pulled in by another library — meets our copy directly.

Dropping it while pinned to 4.14 is a defensible call; it would just want a comment saying it depends on the ITK pin carrying #6756, so an ITK upgrade re-examines it. Say the word and I will strip the mangling, the patch step and the two tests out of BIC-MNI/libminc#152.