Skip to content

COMP: Gate VXL svd and matrix_inverse tests under ITK_FUTURE_LEGACY_REMOVE#6662

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:comp-future-legacy-remove-fixes
Jul 20, 2026
Merged

COMP: Gate VXL svd and matrix_inverse tests under ITK_FUTURE_LEGACY_REMOVE#6662
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:comp-future-legacy-remove-fixes

Conversation

@hjmjohnson

Copy link
Copy Markdown
Member

vnl_algo_test_all fails to link under ITK_FUTURE_LEGACY_REMOVE because commit 232a2ad excluded the LINPACK-svdc-backed vnl_svd family and vnl_solve_qp from the library but left their tests. This gates those tests, which is the only build failure in that configuration.

This is the same defect 434a420 fixed for the eispack vnl_*_eigensystem solvers, and it is what breaks all five RogueResearch Mac Debug nightlies.

Root cause — two mechanisms, not one

1. Tests left behind for removed APIs. Modules/ThirdParty/VNL/src/vxl/core/vnl/algo/CMakeLists.txt drops vnl_svd{,_economy,_fixed}, vnl_solve_qp and their Templates/vnl_svd*+* instantiations under ITK_FUTURE_LEGACY_REMOVE, but tests/CMakeLists.txt gated only the eigensystem tests. Six test objects still referenced the removed symbols:

Object Undefined symbols
test_svd_fixed.cxx.o 31
test_svd.cxx.o 30
test_solve_qp.cxx.o 4
test_complex_algo.cxx.o 4
test_algo.cxx.o 4
test_rank.cxx.o 1

2. Guards that never fired. test_algo.cxx and test_complex_algo.cxx already guarded their removed-API use with #ifndef ITK_FUTURE_LEGACY_REMOVE. That macro is never defined in those translation units: itkConfigure.h is generated at <build>/Modules/Core/Common/itkConfigure.h, but it is not on the include path of the vendored VXL subtree. Verified from compile_commands.json:

Translation unit include dir containing itkConfigure.h
VXL library (Templates/vnl_svd+double-.cxx) none
VXL test (test_algo.cxx) none
ITK module (ITKCommonHeaderTest1.cxx) <build>/Modules/Core/Common

That asymmetry is correct for the headers — the __has_include(<itkConfigure.h>) guard is meant to stay inert for VXL's own compiles and for standalone VXL builds, while firing for ITK modules and downstream consumers. But it means any #ifndef ITK_FUTURE_LEGACY_REMOVE written inside a VXL test source is dead code. The removed-API state is therefore conveyed as VNL_SVD_REMOVED / VNL_MATRIX_INVERSE_REMOVED compile definitions, mirroring the existing VNL_EISPACK_REMOVED.

Verification

Debug, AppleClang, arm64, macOS 26.5 SDK.

Configuration Result
ITK_LEGACY_REMOVE=ON + ITK_FUTURE_LEGACY_REMOVE=ON links; 17/17 vnl_algo tests pass; 0 svd/solve_qp tests registered
default builds; 23/23 vnl_algo tests pass; 3 svd/solve_qp tests still registered — no coverage lost

Full-tree build under ITK_FUTURE_LEGACY_REMOVE: exactly one of 5550 targets failed before this change (vnl_algo_test_all); 0 failures after. No other failure class exists in that configuration.

Coverage is not silently dropped — itkMathSVD.h and itkMathSVDGTest.cxx already exercise the Eigen-backed replacement, so the deprecation policy's scavenge step was already complete; only the test gating was missing.

test_rank.cxx routes its SVD checks through a TEST_SVD_RANK macro that compiles away when removed; its vnl_rank() checks are untouched.

VXL fork carry-through

The identical change is already on the extraction branch so the next UpdateFromUpstream.sh does not revert it:

  • InsightSoftwareConsortium/vxl for/itk-vxl-master: c8c2e0555c..8d91974ab0 (fast-forward)
  • tag for/itk-vxl-master-8d91974

All five files were byte-identical between ITK main and the fork before the change, and are byte-identical after it, so the two trees cannot drift. Standalone VXL is unaffected: with no ITK_FUTURE_LEGACY_REMOVE CMake variable, every test is built and the new macros are never defined, leaving all source guards inert.

Modules/ThirdParty/VNL/UpdateFromUpstream.sh still pins for/itk-vxl-master-ae2f579 (2026-07-06), which predates both c8c2e05 and this commit; bumping that pin is left to a separate change.

…EMOVE

Commit 232a2ad excludes the LINPACK-svdc-backed vnl_svd family and
vnl_solve_qp from the build under ITK_FUTURE_LEGACY_REMOVE but left their
tests, so the vnl_algo_test_all driver fails to link in that configuration.

Exclude test_{svd,svd_fixed,solve_qp} from the driver sources and the
add_test registrations, and skip their testlib DECLARE/REGISTER via a
CMake-defined VNL_SVD_REMOVED macro.

test_algo and test_complex_algo guarded their removed-API use with
#ifndef ITK_FUTURE_LEGACY_REMOVE, but itkConfigure.h is not on the include
path of the vendored VXL compile, so that macro is never defined there and
the guard never fired. Convey the state as VNL_SVD_REMOVED and
VNL_MATRIX_INVERSE_REMOVED compile definitions instead, and gate the
vnl_svd-backed rank checks in test_rank the same way.
@github-actions github-actions Bot added type:Compiler Compiler support or related warnings type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:ThirdParty Issues affecting the ThirdParty module labels Jul 18, 2026
@hjmjohnson
hjmjohnson marked this pull request as ready for review July 18, 2026 21:25
@greptile-apps

greptile-apps Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates VNL algo tests for the future legacy-removal build. The main changes are:

  • Gates SVD and solve_qp test sources and CTest entries behind ITK_FUTURE_LEGACY_REMOVE.
  • Adds VNL-specific removed-API compile definitions for the shared test driver.
  • Compiles out SVD and matrix-inverse references from remaining tests when those APIs are removed.
  • Keeps vnl_rank coverage active while skipping only SVD rank cross-checks.

Confidence Score: 5/5

Safe to merge with minimal risk.

The changes are narrowly scoped to test source selection and preprocessor guards. The guards match the library-side removal conditions. Default test coverage is preserved while the future legacy-removal link path is fixed.

No files require special attention.

T-Rex T-Rex Logs

What T-Rex did

  • I verified the future vnl build and tests completed with EXIT_CODE: 0 using the command cmake --build trex-artifacts/build-vnl-future --target vnl_algo_test_all -- -v and confirmed all 17 tests passed.
  • I checked the future test listing and confirmed Total Tests: 17 with no registered vnl_algo_test_svd, vnl_algo_test_svd_fixed, or vnl_algo_test_solve_qp.
  • I verified the default vnl build and tests completed with EXIT_CODE: 0 using the command cmake --build trex-artifacts/build-vnl-default --target vnl_algo_test_all -- -v and confirmed all 23 tests passed.
  • I checked the default test listing and confirmed Total Tests: 23, including vnl_algo_test_svd, vnl_algo_test_svd_fixed, and vnl_algo_test_solve_qp.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/ThirdParty/VNL/src/vxl/core/vnl/algo/tests/CMakeLists.txt Gates SVD and solve_qp test sources and CTest registration under ITK_FUTURE_LEGACY_REMOVE.
Modules/ThirdParty/VNL/src/vxl/core/vnl/algo/tests/test_algo.cxx Uses VNL-specific compile definitions around SVD and matrix-inverse test coverage.
Modules/ThirdParty/VNL/src/vxl/core/vnl/algo/tests/test_complex_algo.cxx Applies the same VNL-specific guards to complex SVD and matrix-inverse test code.
Modules/ThirdParty/VNL/src/vxl/core/vnl/algo/tests/test_driver.cxx Omits declarations and registrations for SVD-backed tests when the SVD family is removed.
Modules/ThirdParty/VNL/src/vxl/core/vnl/algo/tests/test_rank.cxx Skips SVD rank cross-checks when SVD is removed while keeping vnl_rank checks active.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CMake as CMake configure
participant Target as vnl_algo_test_all
participant Driver as test_driver.cxx
participant Tests as VNL algo tests

CMake->>CMake: Check ITK_FUTURE_LEGACY_REMOVE
alt future legacy removal OFF
    CMake->>Target: Add SVD/eigensystem/solve_qp test sources
    CMake->>Driver: No removed-API compile definitions
    Driver->>Tests: Register all vnl_algo tests
else future legacy removal ON
    CMake->>Target: Omit removed SVD/eigensystem/solve_qp test sources
    CMake->>Driver: Define VNL_EISPACK_REMOVED, VNL_SVD_REMOVED, VNL_MATRIX_INVERSE_REMOVED
    Driver->>Tests: Register only linkable tests
    Tests->>Tests: Compile out removed SVD/matrix_inverse references
end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CMake as CMake configure
participant Target as vnl_algo_test_all
participant Driver as test_driver.cxx
participant Tests as VNL algo tests

CMake->>CMake: Check ITK_FUTURE_LEGACY_REMOVE
alt future legacy removal OFF
    CMake->>Target: Add SVD/eigensystem/solve_qp test sources
    CMake->>Driver: No removed-API compile definitions
    Driver->>Tests: Register all vnl_algo tests
else future legacy removal ON
    CMake->>Target: Omit removed SVD/eigensystem/solve_qp test sources
    CMake->>Driver: Define VNL_EISPACK_REMOVED, VNL_SVD_REMOVED, VNL_MATRIX_INVERSE_REMOVED
    Driver->>Tests: Register only linkable tests
    Tests->>Tests: Compile out removed SVD/matrix_inverse references
end
Loading

Reviews (1): Last reviewed commit: "COMP: Gate VXL svd and matrix_inverse te..." | Re-trigger Greptile

@hjmjohnson
hjmjohnson merged commit 689f058 into InsightSoftwareConsortium:main Jul 20, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ThirdParty Issues affecting the ThirdParty module type:Compiler Compiler support or related warnings type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants