Skip to content

COMP: Give install_linking the source directory instead of guessing it - #29

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/ci-install-linking-path
Sep 21, 2026
Merged

hjmjohnson merged 1 commit into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/ci-install-linking-path

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown

The test hard-codes the path to the downstream example project:

cmake ... ../../nifti_clib/real_easy/minimal_example_of_downstream_usage

That only resolves when the build directory happens to be a sibling of a
source tree named exactly "nifti_clib". It fails for an in-tree build,
for a build directory named anything else, and on CI, where the checkout
lives at /nifti_clib and the path resolves to
/nifti_clib/nifti_clib/real_easy/...

CMake now passes CMAKE_SOURCE_DIR to the script, the way the other test
scripts in that directory already receive their arguments.


Interface impact: none. On the union of all these changes, configured with USE_FSL_CODE=ON and USE_CIFTI_CODE=ON: all 448 exported symbols across libniftiio, libnifti2, libznz, libfslio, libnifticdf and libcifti are identical to master under nm -D --defined-only, and all ten installed headers are identical under gcc -E -P. Under gcc -dM -E one macro definition differs, intentionally and only in text: #61 makes FSL_RADIOLOGICAL read (-1) so it is safe inside an expression. Its value is still -1, checked by compiling against each installed fslio.h and printing it.

Verification. This branch: builds with gcc 16.1.1, ctest unchanged from master (2 of 345 fail on master itself in this environment; #31 and #29 each fix one). The union of all the PRs: 0 errors under both gcc 16.1.1 and clang 22.1.8, ctest 345/345 under each, and the whole suite under valgrind memcheck with --trace-children=yes gives 484 traced processes with no invalid access, no uninitialised value and no leak in any nifti binary.

Coordination. Every line of every branch was compared, whitespace-normalised, against the diffs of the open PRs (#11, #21, #22, #23, #24). Where one of those already changes a line, the line was left alone, and the few deliberate overlaps are named in the text above. What survives is 17 compiler warnings, all of them on those lines: 9 -Wsign-conversion (5 in fslio.c for #22, 2 in nifti2_io.c and 2 in nifti_tester001.c for #24) and 8 -Wcalloc-transposed-args in nifti_findhdrname and nifti_findimgname, which #11 rewrites. No formatting changes appear anywhere, to stay clear of #10 and #12.

One of a set of independent, single-purpose PRs. Each bases on master and can be merged on its own, in any order.

The full set of PRs (35)

The union of all of them is on the fork as all-changes, if you want to build and test the lot at once.

CI and build

Configuration and documentation

Defects

Warning and check classes

The test hard-codes the path to the downstream example project:

    cmake ... ../../nifti_clib/real_easy/minimal_example_of_downstream_usage

That only resolves when the build directory happens to be a sibling of a
source tree named exactly "nifti_clib".  It fails for an in-tree build,
for a build directory named anything else, and on CI, where the checkout
lives at <workspace>/nifti_clib and the path resolves to
<workspace>/nifti_clib/nifti_clib/real_easy/...

CMake now passes CMAKE_SOURCE_DIR to the script, the way the other test
scripts in that directory already receive their arguments.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KSPnbwpDjVcAYqDdVqLkMU
This was referenced Aug 15, 2026
@hjmjohnson

Copy link
Copy Markdown
Member

Reviewed as part of a sweep over the open backlog. This is the one to merge first — the diagnosis is exactly right, and install_linking is the only failing test in both #57 and #59, so both go green on this.

Confirmed from the CI logs rather than by inference: run 31865527775 (#59) gives

CMake Error: The source directory "/home/runner/work/nifti_clib/nifti_clib/nifti_clib/real_easy/minimal_example_of_downstream_usage" does not exist.

Note the doubled nifti_clib/nifti_clib — precisely the failure mode described, because the CI checkout lives at <workspace>/nifti_clib.

Two questions, neither a hold.

1. CMAKE_SOURCE_DIR or PROJECT_SOURCE_DIR?

CMAKE_SOURCE_DIR is correct for a standalone build. When nifti_clib is vendored via add_subdirectory (as in real_easy/parent_project_demo/), it resolves to the parent's root and real_easy/... would not exist beneath it.

That configuration is arguably already ill-defined for TEST_INSTALL, and the current behavior there is a hard failure either way, so this changes nothing in practice. But PROJECT_SOURCE_DIR is marginally more robust for no cost. Your call.

2. install_linking does not run in the per-PR workflow at all — should it?

TEST_INSTALL is gated on BUILD_SHARED_LIBS (CMakeLists.txt:120), which defaults OFF (CMakeLists.txt:51). cmake-multi-platform.yml never sets it, so TEST_INSTALL is OFF in all four of those jobs and the test is never registered. It only runs under build.yml, whose dashboard scripts set BUILD_SHARED_LIBS=ON and NIFTI_BUILD_APPLICATIONS=ON (cmake/travis_dashboard.cmake:46,86,88).

That means the install/export path — install_nifti_target, the generated NIFTIConfig.cmake, and the find_package(NIFTI ... CONFIG REQUIRED) in real_easy/stand_alone_app/CMakeLists.txt — is untested on every pull request. Out of scope here, but this PR is what makes fixing it worthwhile.

Separately, and possibly a typo worth a look: NIFTI_PACKAGE_PREFIX defaults to "" (CMakeLists.txt:83) yet appears as a required truthy condition in the same cmake_dependent_option, while the comment above it says the test should be added when "no prefix string is set". Those read as contradictory.

Minor notes (all cosmetic, none blocking)
  • The argument handling is now asymmetric: a missing $1 warns and guesses make, a missing $2 exits 1. The stricter treatment of the new argument is the right choice — there is nothing sane to guess — but making both hard errors would read better, and is safe since nifti2/CMakeLists.txt:187 is the only caller.
  • Quoting is correct as written. SRC_DIR=$2 unquoted is safe in POSIX sh, and the quoting that matters ("${SRC_DIR}/real_easy/...") is there. A source path containing spaces now works, which it did not before.
  • The explanatory comment is ~10 lines and narrates the previous form. The fact it records is genuinely non-obvious, so I would cut it to two lines rather than drop it — but in a test script the cost is near zero.
  • Pre-existing and not this PR's business: the script configures the inner example with -G 'Unix Makefiles' and then runs bare make, ignoring $BUILD_TOOL. A Ninja-configured outer build still needs make for this test to pass.

@hjmjohnson
hjmjohnson merged commit 9f46c96 into InsightSoftwareConsortium:master Sep 21, 2026
4 checks passed
@hjmjohnson

Copy link
Copy Markdown
Member

The commit messages in this range were rewritten to remove trailers that do not belong in permanent history: Co-Authored-By: naming an AI tool, and Claude-Session: URLs that resolve for nobody. Only messages changed — the tree at the tip of master is byte-identical, and author, committer, and dates are preserved.

This PR's commit on the rewritten master:

  • 6458714c7b COMP: Give install_linking the source directory instead of guessing it

The SHA recorded above by GitHub is from the pre-rewrite history and no longer resolves.

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.

2 participants