NIfTI: track the maintained nifti_clib fork, drop the dead vendored copy

[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 exactly the condition it exists to prevent: an unmangled symbol can collide at link time with the niftiio ITK bundles. The mangle list is regenerated against the new pin.

What

• cmake-modules/BuildNIFTI.cmake — pin InsightSoftwareConsortium/nifti_clib@f24a607.
• cmake-modules/nifti_mangle.h — regenerated. All 112 exported symbols are now minc_*; nifti_extension_size is dropped since it is static upstream now. The regeneration recipe in the header comment is also fixed: it previously contained a sed 's/.*/…/' whose */ closed the C comment early.
• NIFTI::niftiio / NIFTI::znz imported targets — build_nifti() now defines the same targets nifti_clib's own NIFTIConfig.cmake provides, so consumers link identically whether NIFTI came from the ExternalProject or from find_package(NIFTI CONFIG). The real config package cannot be used there: ExternalProject builds at build time, long after consumers configure. Build ordering still rides on the existing add_dependencies(minc2 NIFTI).
• LIBMINC_USE_SYSTEM_NIFTI now uses find_package(NIFTI CONFIG), and the hand-rolled cmake-modules/FindNIFTI.cmake is deleted. It hardcoded /usr/local/bic and reported everything it found as "NetCDF".
• Deleted nifti/ — a tracked copy of nifti1_io.c / znzlib.c / nifti1.h (~380 KB) that no build system referenced.

Verification

• libminc builds end-to-end against the new pin; all 38 ctests pass, including nifti_readback and the nifti_mangle_symbols guard.
• nm -g --defined-only libniftiio.a libznz.a on the mangled build: 112 symbols, all minc_-prefixed, zero overlap with the unmangled build of the same source.

Ordering

BIC-MNI/minc-toolkit-v2 carries an identical pin and reads nifti_mangle.h out of this submodule, so its companion PR and this one should land together — a new pin with the old mangle list leaves those three symbols exposed.

🤖 Generated with Claude Code

https://claude.ai/code/session_019katBUnczdvUS7WQ4RpVoa

[Copilot]

🟡 Not ready to approve

The new CMake target plumbing has portability/configure-time issues (notably reading deprecated LOCATION and unconditionally linking m) that can break configuration or non-UNIX builds.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR updates libminc’s NIfTI integration to track the maintained ITK fork of nifti_clib, removes the unused vendored NIfTI source tree, and standardizes CMake usage around NIFTI::niftiio / NIFTI::znz targets to avoid symbol collisions and improve consumer linking behavior.

Changes:

• Pin ExternalProject NIfTI to InsightSoftwareConsortium/nifti_clib@f24a607… and regenerate the symbol-mangling header to cover newly exported symbols.
• Replace legacy variable-based linking/include plumbing with NIFTI::niftiio target-based linking (including in tests) and switch system NIfTI discovery to find_package(NIFTI CONFIG).
• Delete the dead vendored nifti/ sources and remove the obsolete cmake-modules/FindNIFTI.cmake.

File summaries

File	Description
CMakeLists.txt	Switch system NIfTI discovery to config mode and link libminc via NIFTI::niftiio; derive legacy exported variables from targets.
cmake-modules/BuildNIFTI.cmake	Pin to ITK fork commit and add imported NIFTI::niftiio / NIFTI::znz targets for ExternalProject builds.
cmake-modules/FindNIFTI.cmake	Deleted legacy module-mode finder.
cmake-modules/nifti_mangle.h	Regenerated mangling list to cover the fork’s exported symbols and fix header regeneration notes.
testdir/CMakeLists.txt	Update NIfTI tests to link with NIFTI::niftiio target.
nifti/nifti1.h	Deleted vendored header (part of removing unused vendored NIfTI copy).
nifti/nifti1_io.h	Deleted vendored header (part of removing unused vendored NIfTI copy).
nifti/znzlib.c	Deleted vendored source (part of removing unused vendored NIfTI copy).
nifti/znzlib.h	Deleted vendored header (part of removing unused vendored NIfTI copy).

Review details

• Files reviewed: 9/10 changed files
• Comments generated: 3
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[gdevenyi]

Keeping LOCATION here — CMP0026 doesn't apply, and the suggested replacement would break this.

CMP0026 forbids reading LOCATION from build targets, not imported ones; imported targets are explicitly exempt. (The superbuild even sets CMP0026 NEW explicitly and this configures fine.)

More concretely, IMPORTED_LOCATION is the wrong property to read here. nifti_clib's installed NIFTITargets-release.cmake records only the per-configuration location:

set_target_properties(NIFTI::niftiio PROPERTIES
  IMPORTED_LOCATION_RELEASE "${_IMPORT_PREFIX}/lib/libniftiio.so.2.1.0"

so plain IMPORTED_LOCATION is never set. Measured against an installed system nifti_clib:

CMAKE_BUILD_TYPE	LOCATION	IMPORTED_LOCATION
(unset)	/usr/lib/libniftiio.so.2.1.0	NOTFOUND
Release	/usr/lib/libniftiio.so.2.1.0	NOTFOUND
Debug	/usr/lib/libniftiio.so.2.1.0	NOTFOUND
RelWithDebInfo	/usr/lib/libniftiio.so.2.1.0	NOTFOUND

LOCATION resolves through IMPORTED_CONFIGURATIONS regardless of build type, which is exactly the fallback the comment asks for — CMake already does it. Switching to IMPORTED_LOCATION would silently empty NIFTI_LIBRARY, and with it NIFTI_LIBRARY_NAME in the exported LIBMINCConfig.cmake.

Added a comment in place recording this so it doesn't get re-raised.

The other two comments on this PR were both right and are fixed in ad8b403.

[gdevenyi]

Right, fixed in ad8b403.

nifti_clib itself only links a math library where one exists — its NIFTI_SYSTEM_MATH_LIB is m on Emscripten/WASI, find_library(m) on other non-Windows platforms, and empty on WIN32 — and the rest of libminc only links m under if(UNIX). The interface now matches:

set(NIFTI_INTERFACE_LIBS NIFTI::znz)
if(UNIX)
  list(APPEND NIFTI_INTERFACE_LIBS m)
endif()

[gdevenyi]

Agreed, fixed in ad8b403 — NIFTI::znz now links ZLIB::ZLIB rather than the raw ${ZLIB_LIBRARY} path.

Confirmed the target exists on both paths before build_nifti() runs: standalone libminc calls find_package(ZLIB REQUIRED) first (and already links ZLIB::ZLIB elsewhere), and in the superbuild BuildZLIB.cmake creates ZLIB::ZLIB as a GLOBAL imported target — so this holds with bundled zlib too, not just system zlib.

[gdevenyi]

Added a second commit: nifti_mangle_symbols now only runs when the NIfTI came from the bundled build.

That test asserts every exported symbol is minc_*, which is only true for the copy patched on the way in. Under LIBMINC_USE_SYSTEM_NIFTI the library is whatever the distribution shipped — unmangled — so the invariant would fail on a perfectly correct build. nifti_readback still runs in both modes; it is the functional check and is agnostic about naming.

Verified against an installed system nifti_clib (shared, unmangled): standalone libminc builds in both modes and passes 38 tests bundled / 37 system, the difference being exactly this test.

BIC-MNI/minc-toolkit-v2#230 forwards LIBMINC_USE_SYSTEM_NIFTI so the superbuild gets the same behaviour when its restored USE_SYSTEM_NIFTI option is on.

[vfonov]

using system NIFTI will collide with ITK's nifti

[gdevenyi]

Addressed the review in ad8b403. Two of the three were right and are fixed; the third I pushed back on with measurements, in the thread.

• NIFTI::znz links ZLIB::ZLIB instead of the raw ${ZLIB_LIBRARY} path.
• NIFTI::niftiio no longer hardcodes m; it is added under if(UNIX), matching nifti_clib's own NIFTI_SYSTEM_MATH_LIB (empty on WIN32) and the rest of this file.
• file(MAKE_DIRECTORY) is quoted and names both include dirs explicitly.
• Also added add_dependencies() on the imported targets, mirroring ZLIB::ZLIB in the superbuild's BuildZLIB.cmake, so ordering rides on the target itself.

Not changed: the LOCATION read. CMP0026 applies to build targets, not imported ones, and nifti_clib's installed targets file sets only IMPORTED_LOCATION_RELEASE — so the suggested IMPORTED_LOCATION reads NOTFOUND under every CMAKE_BUILD_TYPE while LOCATION resolves correctly. Measurements in the thread; a comment now records it in the source.

Re-verified against an installed system nifti_clib: 38 tests pass with LIBMINC_USE_SYSTEM_NIFTI=OFF, 37 with ON (the difference being the correctly-skipped mangling invariant).

[gdevenyi]

@vfonov you're right, and the failure is quieter than I'd assumed. Investigated properly — details below, and BIC-MNI/minc-toolkit-v2#230 now refuses the combination outright.

Where the collision actually is. ITK 4.x does mangle its bundled niftiio: Modules/ThirdParty/NIFTI/src/nifti/niftilib/itk_nifti_mangle.h, pulled in from its nifti1.h, renames 103 names to itk_*. But it ships no equivalent for znzlib — ITK's znzlib.h includes no mangle header — so ITKznz exports these unprefixed:

Xznzclose  znzopen  znzputs  znzread  znzrewind  znzseek  znztell  znzwrite

Diffing the installed system nifti_clib's 110 exported symbols against ITK's mangle list: 102 are covered, and those 8 are not. So nifti_image_read itself is fine; the znz layer underneath it is not.

It does not fail at link time, which is what makes it worse than I expected. Reproduced with the installed system nifti_clib plus a stand-in for ITKznz (unmangled znzopen, pulled in by an ITK-side caller, exactly as ITKniftiio pulls it in a real link):

$ gcc main.c libitkznz.a -lniftiio -lznz -o repro     # links clean, no duplicate symbols
$ nm -D --defined-only repro | grep znz
0000000000001192 T znzopen                            # in the executable's *dynamic* symbol table
$ ./repro
>>> ITK's znzopen was called (path=t.nii)             # system libniftiio.so reaching ITK's znz
>>> ITK's znzopen was called (path=t.nii.gz)
** ERROR (nifti_image_read): failed to find header file for 't.nii'
-- system nifti_image_read returned NULL --

ITK's copies land in the executable's dynamic symbol table and interpose over libznz.so, so the system libniftiio.so reaches ITK's znz layer for every read and returns NULL on a file that is perfectly valid.

Control, same fake ITKznz, same file, only the nifti side mangled:

$ ./repro_mangled
>>> ITK's znzopen was called (path=itk-internal)      # only ITK's own code
-- mangled nifti_image_read returned an image --

Which is precisely the collision nifti_mangle.h exists to prevent.

Worth adding: the two znzseek/znztell signatures have also drifted — ITK's take long, nifti_clib 3.x takes znz_off_t (off_t). They coincide on 64-bit Linux and macOS, so the ABI happens to line up there, but not on a 32-bit build with LFS. struct znzptr itself is byte-identical, so this is interposition rather than memory corruption.

Fix. A prebuilt system library can't be renamed, and ITK 4.x has no ITK_USE_SYSTEM_NIFTI to point it at a single copy, so #230 makes the combination a hard configure error rather than something documented and policed by hand:

CMake Error at CMakeLists.txt:320 (MESSAGE):
  USE_SYSTEM_NIFTI=ON is incompatible with MT_BUILD_ITK_TOOLS=ON.
  ITK bundles an unmangled znzlib whose znzopen/znzread/znzseek/... interpose
  over a system libznz.so at runtime, silently breaking NIfTI reading in every
  tool that links both ITK and libminc.
  Use USE_SYSTEM_NIFTI=ON with MT_BUILD_ITK_TOOLS=OFF (or MT_BUILD_LITE=ON), or
  keep the bundled, symbol-mangled NIfTI with USE_SYSTEM_NIFTI=OFF.

All four combinations verified: only that one is rejected. USE_SYSTEM_NIFTI stays available for builds without the ITK tools, which is where a distribution package would want it — and since CI and the release workflow both build MT_BUILD_ITK_TOOLS=ON, neither can trip over this now.

Nothing in this PR changes. libminc has no ITK in the picture; the guard belongs in the superbuild, where ITK is built. The only libminc-side interaction is LIBMINC_USE_SYSTEM_NIFTI, which the superbuild forwards, and it can no longer be reached in an ITK build.

[gdevenyi]

Correction to my previous comment, @vfonov — I overstated the consequence, and the difference matters.

I demonstrated nifti_image_read returning NULL using a stand-in for ITKznz whose znzopen returned NULL. That was my stub failing, not ITK's code. I've since rebuilt against ITK release-4.14's actual znzlib.c/znzlib.h and re-run the A/B:

ITKznz built from	unmangled exports	znz names in exe .dynsym	system nifti_image_read("t.nii")
real ITK release-4.14 sources	15	8	an image
same + an itk_znzlib_mangle.h	0	0	an image

So your core point stands — the collision is real and measurable, 8 znz names interpose over libznz.so — but on LP64 ITK's znzlib and nifti_clib's agree closely enough that reads still succeed. Both are compiled with HAVE_ZLIB, so sizeof(struct znzptr) matches as well.

What's genuinely left is narrower than I implied, and still worth refusing:

• It is one definition of each function silently replacing another — UB, and invisible.
• It does break on ILP32 with large-file support: ITK's znzseek/znztell take and return long, nifti_clib 3.x uses znz_off_t. Measured here: long=4, off_t=8 under -m32 -D_FILE_OFFSET_BITS=64.
• It breaks wherever the two builds disagree on HAVE_ZLIB, since that changes sizeof(struct znzptr).

BIC-MNI/minc-toolkit-v2#230 keeps the guard, with the rationale rewritten to say that rather than the alarming version.

Still nothing to change in this PR.

[gdevenyi]

@vfonov ITK has merged my patch to fix their symbol mangling on the 4.14 branch, so we can now handle SYSTEM_NIFTI properly:
InsightSoftwareConsortium/ITK#6756

minc-toolkit-v2 PRs updated

[gdevenyi]

Added REQUIRED to the system-nifti lookup, which closes #102 as well as #117.

LIBMINC_USE_SYSTEM_NIFTI asks for the system copy. Without REQUIRED, find_package quietly reported it as not found, LIBMINC_NIFTI_SUPPORT stayed off, and the build carried on with no NIfTI support at all — the user got neither the library they asked for nor any hint of it. That silent fallback is exactly what #102 reported:

I have used -DLIBMINC_USE_SYSTEM_NIFTI=ON [...] but the system's nifti did not get used. I also did not see any message to that effect [...] If I request system nifti, and system nifti is not found, I expect the configuration to exit with an error message.

Verified both directions against an installed nifti_clib:

• present — configures, builds, 37/37 tests pass (unchanged; the mangling invariant is correctly skipped in system mode);
• absent — configure now stops:

CMake Error at CMakeLists.txt:96 (find_package):
  Could not find a package configuration file provided by "NIFTI"

The other half of #102 — that NIFTI_INCLUDE_DIR / ZNZ_INCLUDE_DIR were never set and had to be filled in by hand — is handled by the switch to config mode: nifti_clib ships its own CMake package, so the include directories arrive on the NIFTI::niftiio target instead of being guessed. Worth noting the reporter had nifticlib 2.0.0, which predates that package; a current nifti_clib is needed for this path.

#117 is covered by the earlier commits in this PR: libminc/nifti/ (the unused copy it asks about) is deleted, and the ExternalProject now tracks the maintained fork rather than the 2010 sourceforge tarball it mentions.