Skip to content

Rewrite README: correct install instructions, document what ships - #227

Merged
vfonov merged 4 commits into
BIC-MNI:develop-1.9.18from
gdevenyi:docs/readme-rewrite
Aug 6, 2026
Merged

Rewrite README: correct install instructions, document what ships#227
vfonov merged 4 commits into
BIC-MNI:develop-1.9.18from
gdevenyi:docs/readme-rewrite

Conversation

@gdevenyi

Copy link
Copy Markdown
Contributor

The README's install section had drifted from the build system. Several documented options no longer exist, so following the README as written produces a configure line with dead flags and a wrong picture of what is bundled.

Everything below was checked against CMakeLists.txt, cmake-modules/, .github/workflows/, and an install tree.

Correctness fixes

Options that do not exist

  • -DMT_BUILD_IM — no such option anywhere in the project.
  • -DUSE_SYSTEM_PCRE — PCRE was removed in 886ec1a (Drop vendored PCRE; oobicpl now uses C++ std::regex). There is no option, no BuildPCRE.cmake, and no binary links it. PCRE was also still listed under "built internally".
  • -DUSE_SYSTEM_GLUT=OFF — misleading. MT_USE_GLFW defaults to ON, so GLUT is not used at all unless GLFW is turned off.

Wrong versions

  • CMake minimum is 3.10, not 3.1.
  • ITK is 4.14 (pinned commit cae3eb9). The README said 4.11 in one list and 4.13 in another.
  • The prose said the example installs to /opt/minc/1.9.17; the command said 1.9.18.

Environment setup was backwards

The old text said to source minc-toolkit-config.sh only if you used a non-default prefix. It is required in every shell, at any prefix, and the README never gave its path.

Markup — the </pre></code> closing tag was malformed and prose was mixed inside the code blocks.

New material

  • Download section — the .deb/.rpm/.pkg artifacts, the distro matrix, and minimal vs full, taken from release.yml. The README previously had no mention that binary packages exist.
  • Homebrew — the vfonov/minc tap as a macOS alternative.
  • Cloninggit submodule update --init --recursive for an existing non-recursive clone, plus a branch/tag table. Submodules being mandatory was never stated.
  • Option tables with real defaults, including the previously undocumented MT_USE_BLAS, MT_BUILD_OPENBLAS, BLAS_PREFERENCE, MT_USE_GLFW, MT_BUILD_ABC, MT_BUILD_DCM2MNC_TESTS, and the JPEG/OpenJPEG/libarchive/PNG/expat system options.
  • Dependencies matched to CI. The old lists omitted GSL, FFTW, NetCDF, HDF5, OpenJPEG, libarchive, expat, OpenBLAS, gfortran, ImageMagick and GLFW. CentOS 7 is EOL, so it is replaced by Fedora/RHEL and macOS.
  • What is installed — the install tree layout, programs grouped by job, the pipeline scripts, and the data in share/ and etc/.
  • Data files that are not included — the two things that make a tool fail out of the box: the stereotaxic models mritotal looks for in share/mni-models, and the BEaST prior library in share/beast-library-1.0.

Verification

Program names come from an install tree or from the source. Option names and defaults come from CMakeLists.txt and cmake-modules/. Artifact names and the distro matrix come from release.yml. Dependency lists come from ci.yml.

Documentation only. No build files touched.

🤖 Generated with Claude Code

https://claude.ai/code/session_019katBUnczdvUS7WQ4RpVoa

Copilot AI review requested due to automatic review settings July 31, 2026 19:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gdevenyi

gdevenyi commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Added a commit here because #230 invalidates one line of this rewrite.

The note under the USE_SYSTEM_* table currently reads:

USE_SYSTEM_NIFTI has no option() entry, so it does not show up in ccmake. Passing it on the command line still works.

#230 makes it a real option(), so that stops being true. Replaced it with the thing actually worth documenting — the trade-off:

USE_SYSTEM_NIFTI carries one caveat. The bundled NIfTI is built from a pinned nifti_clib with every exported symbol renamed to minc_*, so that it cannot collide with the unmangled niftiio that ITK bundles (ITK 4.x has no ITK_USE_SYSTEM_NIFTI switch). A system NIfTI is unmangled. That is fine against the shared libniftiio.so distributions package — which is what the release .deb and .rpm link — but do not combine USE_SYSTEM_NIFTI with a static system NIfTI and MT_BUILD_ITK_TOOLS, or the two copies will collide on nifti_image_read and friends at link time.

The table row itself is unchanged. If #230 lands first this is already accurate; if this lands first the note describes behaviour that arrives one PR later, so #230 is the one to merge first of the two.

gdevenyi and others added 2 commits August 2, 2026 00:04
The install section had drifted from the build system. Fixes:

- Drop `-DMT_BUILD_IM` and `-DUSE_SYSTEM_PCRE` from the example configure
  line. Neither option exists. PCRE was removed in 886ec1a ("Drop vendored
  PCRE; oobicpl now uses C++ std::regex") and was also still listed as a
  bundled library.
- Drop `-DUSE_SYSTEM_GLUT=OFF`. MT_USE_GLFW defaults to ON, so GLUT is
  unused unless GLFW is explicitly turned off.
- CMake minimum is 3.10, not 3.1.
- ITK is 4.14 (pinned commit cae3eb9). The README claimed 4.11 in one list
  and 4.13 in another.
- The prose said the example installs to /opt/minc/1.9.17 while the command
  said 1.9.18.
- minc-toolkit-config.sh must be sourced in every shell at any prefix. The
  old text said it was only needed for a non-default prefix, and never gave
  its path.
- Fix the malformed `</pre></code>` block and move prose out of the code
  blocks.

New material:

- Download section covering the .deb/.rpm/.pkg artifacts, the distro matrix,
  and the minimal vs full variants, taken from release.yml.
- The vfonov/minc Homebrew tap as a macOS alternative.
- `git submodule update --init --recursive` for existing non-recursive clones,
  and a branch/tag table.
- Full build-option tables with real defaults, including the previously
  undocumented MT_USE_BLAS, MT_BUILD_OPENBLAS, BLAS_PREFERENCE, MT_USE_GLFW,
  MT_BUILD_ABC and MT_BUILD_DCM2MNC_TESTS.
- Dependency lists matched to CI. The old lists omitted GSL, FFTW, NetCDF,
  HDF5, OpenJPEG, libarchive, expat, OpenBLAS, gfortran, ImageMagick and
  GLFW. CentOS 7 is EOL, so it is replaced by Fedora/RHEL and macOS.
- "What is installed": the install tree layout, programs grouped by job, the
  pipeline scripts, and the data in share/ and etc/.
- "Data files that are not included": the stereotaxic models mritotal needs
  in share/mni-models, and the BEaST prior library in
  share/beast-library-1.0. Both make a tool fail out of the box.

Program names were checked against an install tree or the source. Option
names and defaults were checked against CMakeLists.txt and cmake-modules/.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019katBUnczdvUS7WQ4RpVoa
The note said USE_SYSTEM_NIFTI has no option() entry and must be passed on
the command line. BIC-MNI#230 makes it a real option(), so
that stops being true, and the table row above it stops needing the
explanation.

What is worth documenting is the trade-off. The bundled NIfTI has every
exported symbol renamed to minc_* so it cannot collide with the unmangled
niftiio ITK bundles; a system NIfTI is whatever the distribution shipped.
That is fine against the shared libniftiio.so distributions package, which
is what the release .deb and .rpm link, but pairing USE_SYSTEM_NIFTI with
a static system NIfTI and MT_BUILD_ITK_TOOLS collides at link time.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019katBUnczdvUS7WQ4RpVoa
@gdevenyi
gdevenyi force-pushed the docs/readme-rewrite branch from 22ff03a to 3198173 Compare August 2, 2026 04:04
@gdevenyi

gdevenyi commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto develop-1.9.18 at 342c896. One conflict, in the section this PR replaces: #222 removed arguments from the old bundled-packages list while this rewrite replaces that list wholesale, so the rewrite wins.

That merge does leave one stale row in the new package table, fixed as part of the rebase — the rewrite still listed arguments, which #222 replaced with cxxopts:

| [cxxopts](https://github.com/jarro2783/cxxopts) | Header-only command-line argument parsing, used by oobicpl. |

Now MERGEABLE again.

The note I added claimed a system NIfTI was fine against the shared
libniftiio.so distributions ship, and that the hazard was a link-time clash
on nifti_image_read with a static one. Both halves were wrong.

ITK renames its bundled niftiio but not its znzlib, so the clash is on
znzopen and seven siblings, not nifti_image_read. And shared is precisely
the case that breaks: ITK's copies interpose over libznz.so at runtime, so
nothing fails at link time and reads simply start returning nothing.

BIC-MNI#230 now refuses the combination outright, so
describe that rather than a caveat the reader has to police.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019katBUnczdvUS7WQ4RpVoa
@gdevenyi
gdevenyi force-pushed the docs/readme-rewrite branch from 68415e8 to 6d72196 Compare August 2, 2026 04:27
InsightSoftwareConsortium/ITK#6756 gave ITK's bundled znzlib the itk_*
prefixing its niftiio has had since 2017, and the pinned ITK now carries it.
The two NIfTI copies no longer meet, so the combination the note warned
against is fine.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019katBUnczdvUS7WQ4RpVoa
@vfonov
vfonov merged commit f89d67d into BIC-MNI:develop-1.9.18 Aug 6, 2026
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants