Repository navigation
Make the installed libminc package usable outside the superbuild - #150
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Modernizes the GitHub Actions CI workflow to run on all branches/PRs and expands coverage with a Fedora container matrix, while also addressing two CI-discovered issues: NIFTI staging on lib64 distros and macOS leak-test max RSS unit differences.
Changes:
- Update
.github/workflows/cmake.ymlto run on all branch pushes, add concurrency + least-privilege defaults, and add a Fedora 44 container job mirroring the existing option matrix. - Fix NIFTI external build staging on RPM/lib64 platforms by forcing nifti_clib’s install library directory to match
CMAKE_INSTALL_LIBDIR. - Normalize
ru_maxrssunits on macOS inminc2-leak-testso leak thresholds behave consistently.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
.github/workflows/cmake.yml |
CI modernization (triggers/concurrency/permissions), matrix cleanup, and Fedora container job added. |
cmake-modules/BuildNIFTI.cmake |
Align nifti_clib install libdir with CMAKE_INSTALL_LIBDIR for lib64 staging correctness. |
testdir/minc2-leak-test.c |
Normalize macOS RSS units to KB to prevent leak-test false positives. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+26
to
+29
| * checks for maximum memory usage. | ||
| * ru_maxrss is in kilobytes on Linux but bytes on macOS/BSD. | ||
| * Normalize to kilobytes so the leak-detection threshold works | ||
| * on both platforms. |
Comment on lines
+73
to
+75
| - name: Build | ||
| run: cmake --build build -j4 | ||
|
|
Comment on lines
+126
to
+128
| - name: Build | ||
| run: cmake --build build -j4 | ||
|
|
Comment on lines
+83
to
+87
| fedora: | ||
| name: fedora:44 ${{ matrix.build-params.WITH_MINC1 == 'ON' && 'MINC_1_2' || 'MINC_1' }} ${{ matrix.build-params.USE_NIFTI == 'ON' && 'NIFTI' || '' }} ${{ matrix.build-params.static == 'ON' && 'static' || 'shared' }} | ||
| runs-on: ubuntu-latest | ||
| container: 'fedora:44' | ||
| strategy: |
Comment on lines
+103
to
+105
| - name: Allow git to read the workspace owned by the runner user | ||
| run: git config --global --add safe.directory '*' | ||
|
|
gdevenyi
force-pushed
the
ci-modernize
branch
4 times, most recently
from
July 16, 2026 21:24
77fdf2c to
b26b35b
Compare
This was referenced Jul 16, 2026
nifti_clib defaults NIFTI_INSTALL_LIBRARY_DIR to plain "lib", but
BuildNIFTI.cmake expects the staged static libraries under
${CMAKE_INSTALL_LIBDIR}, which is lib64 on RPM-based distros. On Fedora
the build then fails with:
No rule to make target 'external//usr/local/lib64/libniftiio.a'
Pass NIFTI_INSTALL_LIBRARY_DIR=${CMAKE_INSTALL_LIBDIR} to the external
project so both sides agree.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011nEnygr1q87PepYiJobEGW
…lib/ The installed-tree LIBMINCConfig.cmake pointed LIBMINC_USE_FILE and LIBMINC_LIBRARY_DIRS at <prefix>/lib/..., but with GNUInstallDirs the files are installed under <prefix>/lib64 on RPM-based distros, so consumers failed with: INCLUDE could not find requested file: <prefix>/lib/cmake/UseLIBMINC.cmake Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011nEnygr1q87PepYiJobEGW
Building any consumer (bicpl, minctools, mni_autoreg, ...) against an installed libminc failed outside the minc-toolkit superbuild, because the installed package config only described dependencies as they look from inside that superbuild: - The HDF5/NetCDF entries in LIBMINC_LIBRARIES were reduced to plain file names, and LIBMINC_LIBRARY_DIRS only covers libminc's own prefix. That is right for the superbuild, which stages its own HDF5 into that prefix and keeps the install relocatable, but a system HDF5 lives elsewhere, so consumers failed with "ld: library 'hdf5' not found" (macOS/Homebrew) or "undefined reference to H5Pset_chunk ... DSO missing from command line" (Debian, where HDF5 is libhdf5_serial under /usr/lib/*/hdf5/serial). Keep the absolute path for dependencies outside the install prefix and plain names for those staged with libminc. - minc.h includes <hdf5.h> and volume_io needs netcdf.h with MINC1 support, but LIBMINC_INCLUDE_DIRS held only <prefix>/include, so consumers failed with 'hdf5.h: No such file or directory'. Export the dependency include dirs when they are outside the prefix. - internal_volume_io.h was never installed although consumers include it directly; inside the superbuild it resolves from the source tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011nEnygr1q87PepYiJobEGW
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.
Summary
libminc's installed CMake package describes its dependencies the way they look inside the minc-toolkit-v2 superbuild. As a result,
make install-ing libminc and building a consumer (bicpl, minc-tools, mni_autoreg, …) against it does not work today. The superbuild hides every defect below — it builds its own HDF5, stages it into libminc's own install prefix, and compiles consumers from the source tree.Each fix corresponds to a build that fails on current
develop-1.9.18and passes with this branch.No CI changes here — the workflow that found all of this is split out into a separate PR that stacks on this one (see Context).
1.
BuildNIFTI: install bundled nifti_clib intoCMAKE_INSTALL_LIBDIRnifti_clib defaults its install library dir to plain
lib, butBuildNIFTI.cmakeexpects the staged libraries under${CMAKE_INSTALL_LIBDIR}—lib64on RPM distros. SoLIBMINC_USE_NIFTI=ONon Fedora fails:One line: pass
NIFTI_INSTALL_LIBRARY_DIR=${CMAKE_INSTALL_LIBDIR}to the external project.2. Installed config: use
LIBMINC_INSTALL_LIB_DIR, not hard-codedlib/The generated config pointed
LIBMINC_USE_FILEandLIBMINC_LIBRARY_DIRSat<prefix>/lib/..., while GNUInstallDirs had installed those files to<prefix>/lib64. Consumers on Fedora:3. Installed config: make the exported package usable standalone
Four related defects, all invisible inside the superbuild:
a. Dependency libraries were exported as bare file names. Consumers link those names with
link_directories(${LIBMINC_LIBRARY_DIRS}), which only covers libminc's own prefix. Correct for a superbuild-staged HDF5 — and what keeps such an install relocatable — but a system HDF5 lives elsewhere:(Debian ships HDF5 as
libhdf5_serialunder/usr/lib/*/hdf5/serial.)The rule now applied: a dependency outside the install prefix is exported as an absolute path; one staged inside the prefix keeps its plain name. This leaves the superbuild's relocatable install untouched while making system dependencies resolvable. The mapping handles multi-entry lists (module-mode
FindHDF5reportslibhdf5.so;libsz.so;libz.so;libdl.a;libm.so) and entries that are already bare-lnames (h5cc/Homebrew), whose directories are added to the exportedLIBMINC_LIBRARY_DIRS.b. Dependency include dirs were not exported.
minc.hincludes<hdf5.h>and volume_io needsnetcdf.hwith MINC1 support, butLIBMINC_INCLUDE_DIRSheld only<prefix>/include:Same in/out-of-prefix rule as (a).
c.
internal_volume_io.hwas never installed. It is absent from every header list, yet consumers (bicpl, conglomerate)#include <internal_volume_io.h>directly — inside the superbuild it resolves from the source tree:d.
UseLIBMINC.cmakeinjectedNOTFOUNDinto the include path. It didinclude_directories(${HDF5_INCLUDE_DIR})unconditionally; when the consumer has not resolved HDF5 itself (it no longer needs to, given (b)), the literalNOTFOUNDfails the configure:Now guarded on both
HDF5_INCLUDE_DIRandNETCDF_INCLUDE_DIR.Review notes
HDF5_FOUND=0there — it dropped hdf5 entirely and was worse than the original bug. Discarded in favour of fixing this at the origin; noting it since it looks like the obvious approach.LIBMINC::minc2); this only repairs the legacy variables existing consumers use.Validation
These commits are the base of the CI PR (#151), where libminc's own matrix runs green on ubuntu 22.04/24.04, macOS 14/15 and fedora:44 across shared/static × MINC1 × NIFTI: https://github.com/gdevenyi/libminc/actions/runs/29535929762
Every tier-1 consumer also builds and tests against these fixes on ubuntu:24.04 + fedora:44 + macOS, all green:
EBTKS ·
bicpl ·
minc_gco ·
glim_image ·
mrisim ·
postf ·
xdisp ·
minc-tools ·
BEaST ·
mni_autoreg
(Those consumer runs temporarily build libminc from this branch; their own PRs point back at BIC-MNI once this merges.)
Context
Found while adding standalone build+test CI to the BIC-MNI submodule repos, mirroring the superbuild CI in minc-toolkit-v2. This PR is the keystone — 9 of the 10 tier-1 repos cannot build against an installed libminc without it.
🤖 Generated with Claude Code
https://claude.ai/code/session_011nEnygr1q87PepYiJobEGW