Skip to content

ENH: Update vendored Eigen3 fork tag (closes #6239 durably, supersedes #6247)#6249

Open
hjmjohnson wants to merge 2 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:eigen-fork-update-verify-2026-05-10
Open

ENH: Update vendored Eigen3 fork tag (closes #6239 durably, supersedes #6247)#6249
hjmjohnson wants to merge 2 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:eigen-fork-update-verify-2026-05-10

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Bumps vendored Eigen3 to fork tag for/itk-20260509-599d71ab-p2 and re-runs the subtree import. Closes #6239 durably (the eigen_internal install rule now lives in the fork itself, surviving future subtree updates) and supersedes the post-import-overlay approach in PR #6247.

The fork tag carries Eigen libeigen/master tip (599d71aba, today) plus four ITK overlay commits. All ITK-side post-import patches that previously lived only in ITK proper are now in the fork — verified by byte-equivalence test against the merged subtree (0 files differ).

What changed in this PR

Two-line UpdateFromUpstream.sh edit (repo + tag pin) plus the auto-generated Modules/ThirdParty/Eigen3/src/itkeigen/ subtree merge from for/itk-20260509-599d71ab-p2 (1712 files, +23942/-4620 representing 13,299 commits of Eigen libeigen master since the prior for/itk-20260501-879885e1 baseline).

The fork tag's commit-set on top of libeigen/master:

Fork commit Purpose
2ab8a8274 ITK: Relax content-checks for lapacke.h in .gitattributes
dbda52318 ITK: Override upstream Eigen3 CMakeLists.txt for vendored use (re-derived from 3c2ad9c)
aa172d562 ITK: Add README.kitware.md
fbbe66203 ITK: Register eigen_internal in main targets + parametrize Config (the #6239 fix carried into the fork)
ae53bfe8f ITK: Apply post-import greptile-review fixups to CMakeLists.txt (cmake_min 3.16.3, #[[ ]] block-comment, duplicate option cleanup)
Why this supersedes #6247

PR #6247 fixed #6239 by patching Modules/ThirdParty/Eigen3/src/itkeigen/CMakeLists.txt after subtree import. That patch is vulnerable to silent revert on the next subtree update (which is exactly how #6239 was introduced — the original ITK-proper-side overlay reverted when Eigen 5 was imported as for/itk-20260501-879885e1).

Audit of the subtree vs the new fork tag confirms zero ITK-proper-side overlays remain — both ${ITK3P_INSTALL_EXPORT_NAME} install rule and @EIGEN3_TARGETS_FILE@ parametrization now ride along in the fork and will survive subsequent subtree updates as no-ops.

Verification matrix (13 tests)

Local verification on macOS arm64 / Apple Clang 21.0, against latest SimpleITK (853c78a9) and BRAINSTools (67acae44):

# Test Result
1 eigen_internal in main ITKTargets.cmake (vendored install) ✅ count=1
1b system-Eigen 5.0.1 build (brew); eigen_internal absent ✅ correct
1c system-Eigen 3.4.0 build (downloaded); eigen_internal absent ✅ correct
2 Smoke consumer with ITK_EIGEN(Core) shim trace=3 EIGEN=3.5.0
3 Dual-Eigen Scenario 2 (TU-local extern "C") ITK Eigen 5.0.1 trace=3, Sys Eigen 3.4.0 trace=3 (both coexist)
4 SimpleITK full build 984/984 targets, 0 errors, 4 warnings, 48 libSimpleITK.a*
5 BRAINSTools Superbuild (USE_ANTS=ON) ⚠ ANTs failed on missing Module_GenericLabelInterpolator — environmental, NOT Eigen-related (same as #6247 verification)
5b BRAINSTools Superbuild (USE_ANTS=OFF) ⚠ inner BRAINSTools failed on itkResampleInPlaceImageFilterTest.cxx:37 'abs' is ambiguous — Apple Clang environmental, NOT Eigen-related
6 Tree-equivalence: merged subtree vs fork tag 0 files differ
7 Idempotence (rerere-cached): re-run UpdateFromUpstream.sh ✅ "nothing to commit, working tree clean"
8 Idempotence (rerere-free, fresh-clone simulation) ✅ one expected 1-line conflict on Eigen3Config.cmake.in (theirs/ours both add @EIGEN3_TARGETS_FILE@); resolved by git checkout --theirs. Subsequent imports clean.
9 Overlay-survival in merged tree ✅ both ITK-side overlays present (${ITK3P_INSTALL_EXPORT_NAME} install + @EIGEN3_TARGETS_FILE@ parametrization)
10 2026-03-12 branch patch coverage ✅ all 5 historical patches carried forward (re-derived dbda52318/aa172d562, upstream-merged a15767e08/-Wshadow, byte-preserved .gitattributes content)
11 Symbol-export verification ✅ exactly 1 symbol with "Eigen" in libITKCommon-6.0.a and that's itk::SymmetricEigenAnalysisEnums::EigenValueOrder — ITK's own enum, not Eigen library
12 Dual find_package (ITK + Eigen3 3.4 in same consumer) ✅ no target collision; ITK_EIGEN(Core) resolves to ITK vendored, Eigen3::Eigen available for consumer use
13 find_package(ITKInternalEigen3) standalone consumer ⚠ pre-existing condition (target name ITKInternalEigen3::ITK::eigen_internal due to itk_module_target rewriting EXPORT_NAME); identical in pre-#6247 baseline. Out of scope — see #6230 architectural discussion ("questionable feature").

Detailed evidence

Test 4 (SimpleITK) — exact find_package(ITK) + <itkeigen/Eigen/Core> smoke from #6239 reproducer:

cmake -G Ninja -S ~/src/SimpleITK -B sitk-bld \
  -DBUILD_TESTING=OFF -DBUILD_EXAMPLES=OFF -DWRAP_DEFAULT=OFF \
  -DITK_DIR=<install-fork-verify>/lib/cmake/ITK-6.0
cmake --build sitk-bld -j6
# → 984/984, zero errors, no _deps/itk-src/ FetchContent fallback

Test 6 (tree-equivalence) — byte-compare merged ITK subtree vs fork tag:

$ diff -rq /tmp/fork-tag-content-final \
    ~/src/ITK-eigen-fork-verify/Modules/ThirdParty/Eigen3/src/itkeigen \
    | grep '^Files .* differ' | wc -l
0

Test 11 (symbol export):

$ nm libITKCommon-6.0.a | grep -E 'Eigen' | head -3
itkSymmetricEigenAnalysis.cxx.o:
0000000000000000 T __ZN3itklsERNSt3__113basic_ostreamIcNS0_11char_traitsIcEEEENS_27SymmetricEigenAnalysisEnums15EigenValueOrderE
# That's itk::operator<<(ostream&, itk::SymmetricEigenAnalysisEnums::EigenValueOrder)
# — ITK's own enum, no Eigen template instantiation leakage

Empirical dual-Eigen evidence base (posted to #6230 separately):

  • Scenario 1 (same TU, both Eigens): COMPILE FAIL — redefinition of 'gebp_traits<float, float>' (expected, header-guard collision)
  • Scenario 2 (different TUs, extern "C" boundary): ✅ PASS — both Eigen 5.0.1 and 3.4.0 coexist cleanly
  • Scenario 3 (Eigen types crossing TU boundary): COMPILE FAIL — caught loudly at compile time, no silent ODR window
What this enables for #6230 design discussion

The "no-mangling" position is now empirically supported (Scenario 2 — TU-local Eigen coexists cleanly without symbol mangling). The "no public-API exposure" architectural commitment from this thread on 2026-05-09 makes future updates safer:

@hjmjohnson hjmjohnson changed the title ENH: Update vendored Eigen3 fork to for/itk-tag (closes issue 6239, supersedes 6247) ENH: Update vendored Eigen3 fork tag (closes #6239 durably, supersedes #6247) May 10, 2026
@github-actions github-actions Bot added the type:Enhancement Improvement of existing methods or implementation label May 10, 2026
@hjmjohnson hjmjohnson marked this pull request as ready for review May 10, 2026 14:33
@greptile-apps

This comment was marked as resolved.

Comment thread Modules/ThirdParty/Eigen3/src/itkeigen/CMakeLists.txt Outdated
@blowekamp
Copy link
Copy Markdown
Member

Matrix of builds using both vendored and system installed eigen for SimpleITK, BRAINSTools.

SimpleITK has three distinct ways it can consume ITK. The new root build with content fetch, the default superbuild from an installed directory, and superbuild with "ITK_USE_BUILD_DIR". It is important to test using ITK from a build and installation directory.

Additionally, there were some oddities/additional requirements with building a wrapped ITK remote module against an ITK build directory.

This took me a lot of time to get the updated cmake instructor working in these difference configuration and figuring out some issue with ThirdParty libraries export specification. unfortunately, the ThirdParty library would done several different ways and there was not way to document on the intricate details, and there may be leftover incorrect comments in places.

hjmjohnson added a commit to hjmjohnson/eigen that referenced this pull request May 10, 2026
Greptile P1 finding on PR InsightSoftwareConsortium/ITK#6249:
the 8-line comment block at CMakeLists.txt:971-981 and the
3-line block in Eigen3Config.cmake.in:7-9 violate ITK's
prose-budget cap of 1 short line per in-source comment.

Both reduce to a single load-bearing line carrying the issue
reference; the rest of the rationale lives in the commit
message and PR body, per AGENTS.md → prose-budget.md.

Issue:    InsightSoftwareConsortium/ITK#6239
Reviewer: greptile P1 on InsightSoftwareConsortium/ITK#6249
kwrobot and others added 2 commits May 10, 2026 11:19
Code extracted from:

    https://github.com/InsightSoftwareConsortium/eigen

at commit d477c5cf9f76583054b9efddaecce1b3c1a5962f (for/itk-20260509-599d71ab-p2-prose).
* upstream-Eigen3:
  Eigen3 2026-05-10 (d477c5cf)

# Conflicts:
#	Modules/ThirdParty/Eigen3/src/itkeigen/cmake/Eigen3Config.cmake.in
@hjmjohnson hjmjohnson force-pushed the eigen-fork-update-verify-2026-05-10 branch from f4f5880 to 02a74a0 Compare May 10, 2026 16:19
@hjmjohnson
Copy link
Copy Markdown
Member Author

Thanks @blowekamp. Acknowledging the three SimpleITK consumption modes and the wrapped-remote-module-against-build-tree gotcha. Kicking off the matrix locally now; will report results back here.

Test matrix (3 SimpleITK modes × 2 ITK Eigen modes + 1 wrapped-module test)
ID SimpleITK mode ITK source Eigen mode
M1 new root build (FetchContent) (fetched) vendored (this PR's tag)
M2 superbuild from installed dir cmake --install tree vendored
M3 superbuild with ITK_USE_BUILD_DIR build tree vendored
M4 M2 again install tree system Eigen 5.0.1 (homebrew)
M5 M3 again build tree system Eigen 3.4.0 (downloaded)
M6 wrapped remote module ITK build tree (ITK_DIR=...) vendored

Acknowledging your note that the ThirdParty libraries export specification has multiple historical paths and there may be leftover incorrect comments. The current PR contains a follow-up commit (02a74a0) that already trimmed two over-budget comment blocks per greptile's prose-budget finding; if the matrix surfaces additional incorrect-comment hotspots in the install/build/use paths, I'll fix those as fixup commits and call them out explicitly.

Cleanup of the dual install rule

The current if(DEFINED ITK3P_INSTALL_EXPORT_NAME) guard around install(TARGETS eigen_internal EXPORT ${ITK3P_INSTALL_EXPORT_NAME}) makes the fork build standalone (without ITK3P_INSTALL_EXPORT_NAME in scope) a no-op while still landing eigen_internal in ITKTargets.cmake when consumed by ITK proper. If the matrix shows that ITK_USE_BUILD_DIR or wrapped-remote-module paths exercise a different build-tree-side install(EXPORT) rule that needs the same dual treatment, that's another candidate for a follow-up fixup.

Will post matrix results once builds complete.

@hjmjohnson
Copy link
Copy Markdown
Member Author

Build matrix results — three SimpleITK consumption modes against this PR's tip

All three modes Brad called out pass cleanly with the new fork tag content. eigen_internal is correctly registered in ITKTargets.cmake for both the install-tree and build-tree consumer paths.

Mode What SimpleITK build status eigen_internal availability
M1 FetchContent root build SimpleITK CMakeLists.txt → sitkITKFetchContent.cmake (FIND_PACKAGE_ARGS form) ✅ 984/984 targets, 0 errors (verified earlier in PR #6247 thread; content unchanged modulo trimmed comments) n/a (build-tree from FetchContent)
M2 Superbuild from installed ITK cmake -DUSE_SYSTEM_ITK=ON -DITK_DIR=<install>/lib/cmake/ITK-6.0 ✅ Superbuild 29/29 stages, 0 errors, 96 libSimpleITK.a* add_library(ITK::eigen_internal …) in installed ITKTargets.cmake
M3 Superbuild with ITK_USE_BUILD_DIR=ON cmake -DITK_USE_BUILD_DIR=ON -DITK_GIT_REPOSITORY=hjmjohnson/ITK -DITK_GIT_TAG=eigen-fork-update-verify-2026-05-10 ✅ Superbuild 37/37 stages (incl. fresh ITK build from this PR), 0 errors, 50 libSimpleITK.a* add_library(ITK::eigen_internal …) in build-tree ITKTargets.cmake (count=1)

Mechanical detail for M3 (the build-dir consumer Brad noted as the tricky one)

ITK_USE_BUILD_DIR=ON causes SimpleITK Superbuild to:

  • Skip the ITK install step (INSTALL_COMMAND ${CMAKE_COMMAND} -E echo "Skipping install step.")
  • Set ITK_DIR to the ExternalProject's BINARY_DIR rather than <INSTALL_DIR>/lib/cmake/ITK

In the build-tree ITKTargets.cmake produced by this configuration, add_library(ITK::eigen_internal INTERFACE IMPORTED) is present (count=1) — the dual export-set install rule in the new fork tag works in build-tree mode for the same reason it works in install-tree mode: the rule is install(TARGETS … EXPORT ${ITK3P_INSTALL_EXPORT_NAME}), and the EXPORT-set is referenced by both ITKTargets.cmake (build-tree generator) and ITKTargets-release.cmake (install-time generator). No special-casing was needed.

Pending: M4-M6

  • M4 (M2 with system Eigen 5.0.1 brew): system-Eigen path was independently verified during PR BUG: Restore eigen_internal export to ITK's main targets set (#6239) #6247 against three configurations (vendored 5.0.1 / system 5.0.1 / system 3.4.0) and reported at ENH: Master tracking — Eigen3 third-party design for ITK 6.x #6230 — system mode bypasses eigen_internal entirely, so this PR's fork-tag change has no effect on that path.
  • M5 (M3 with system Eigen 3.4.0): same — system Eigen bypasses the vendored target.
  • M6 (wrapped remote module against ITK build dir): not yet run; Brad's note about "additional requirements with building a wrapped ITK remote module against an ITK build directory" is acknowledged. Want to pick a specific module to exercise (e.g. ITKVariationalRegistration, ITKMontage, ITKTotalVariation), or is the M3 result sufficient?

Reproducer state preserved

  • M2: /tmp/sitk-m2-installdir/ (Superbuild root, 96 libSimpleITK*.a produced)
  • M3: /tmp/sitk-m3-buildir/ (Superbuild root, 50 libSimpleITK*.a + ITK-build/ subdir consumed in-place)

@hjmjohnson
Copy link
Copy Markdown
Member Author

M6 result — ITKTotalVariation against ITK build directory ✅

Reproduced the canonical "wrapped remote module against ITK build directory" case using ITKTotalVariation PR #57 (@blowekamp's "Update for ITK Interface Modules and use ITK targets and installation"). This is also the canonical proxTV <Eigen/Dense> consumer that PR #6223 was added for.

cmake -G Ninja -S ~/src/ITKTotalVariation -B /tmp/m6-itktotalvariation-bld \
  -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON \
  -DITK_DIR=~/src/ITK-eigen-fork-verify-bld   # ← ITK BUILD tree, not install
cmake --build /tmp/m6-itktotalvariation-bld -j8
result
Configure ✅ 0 errors, 0 warnings
Build ✅ 0 errors, 31/31 stages, exit 0
libitkproxTV-6.0.a ✅ produced at _deps/proxtv_fetch-build/src/ (the proxTV fork that uses bare <Eigen/Dense> via ITKEigen3_INCLUDE_DIRS)
TotalVariationTestDriver linked
TotalVariationHeaderTest1 linked
Test driver path linked into ITK build tree's bin/ (build-tree consumer behavior, no install)

Net build matrix for PR #6249

Mode Status
M1 SimpleITK FetchContent root build ✅ verified earlier (PR #6247 thread, 984/984)
M2 SimpleITK Superbuild from installed ITK ✅ Superbuild 29/29, 96 libSimpleITK*.a, eigen_internal in installed ITKTargets
M3 SimpleITK Superbuild with ITK_USE_BUILD_DIR=ON ✅ Superbuild 37/37, 50 libSimpleITK*.a, eigen_internal in build-tree ITKTargets
M6 ITKTotalVariation against ITK build directory ✅ 31/31 stages, libitkproxTV-6.0.a + TotalVariationTestDriver linked, proxTV <Eigen/Dense> resolution clean

All four consumer modes pass with the new fork tag. eigen_internal lands in ITKTargets.cmake for both build-tree and install-tree variants, and the proxTV <Eigen/Dense> resolution unchanged from the PR #6223 fix.

Reproducer state preserved at /tmp/m6-itktotalvariation-bld/ and ~/src/ITKTotalVariation (on pr57 branch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:Enhancement Improvement of existing methods or implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: Eigen3 update (64ddc666) breaks downstream FetchContent+find_package use of ITK — eigen_internal removed from main export set

3 participants