Skip to content

ENH: Remove eigen_internal; export ITKInternalEigen3_DIR#6304

Open
blowekamp wants to merge 4 commits into
InsightSoftwareConsortium:mainfrom
blowekamp:eigen3-remove-eigen_internal
Open

ENH: Remove eigen_internal; export ITKInternalEigen3_DIR#6304
blowekamp wants to merge 4 commits into
InsightSoftwareConsortium:mainfrom
blowekamp:eigen3-remove-eigen_internal

Conversation

@blowekamp
Copy link
Copy Markdown
Member

Remove the eigen_internal CMake target and replace it with exported
ITKInternalEigen3_DIR pointing to the vendored Eigen3Config.cmake.

Design rationale

All ITK-internal Eigen includes go through the ITK_EIGEN(X) macro in
itk_eigen.h, which resolves to itkeigen/Eigen/X. The eigen_internal
INTERFACE target existed to provide a second include root (src/itkeigen)
so that <Eigen/X> worked alongside <itkeigen/Eigen/X>. With the macro
enforcing the itkeigen/ prefix for internal code, that second root is
unnecessary.

Downstream projects that need <Eigen/X> can use the exported
Eigen3Config.cmake via ITKInternalEigen3_DIR, which is now set in
the ITKEigen3 module's export code for both build and install trees.
Eigen3Config.cmake is installed alongside other ITK module files
(${ITK_INSTALL_PACKAGE_DIR}/Modules) to match the path that
ITK_MODULES_DIR exposes at find-time.

EIGEN_MPL2_ONLY is not propagated to external consumers via
Eigen3::Eigen; it remains an ITK-internal enforcement only, applied
through itk_eigen.h when ITK_USE_EIGEN_MPL2_ONLY is ON.

The WIP: second commit removes stale standalone-project boilerplate
from itkeigen/CMakeLists.txt that predates the add_subdirectory
integration and needs verification before promoting.

AI assistance

GitHub Copilot assisted with design, implementation, and commit authoring.
Changes were reviewed, tested locally with ninja install, and verified
by inspecting installed paths in the build tree.

@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:ThirdParty Issues affecting the ThirdParty module labels May 18, 2026

# Install Eigen3Config.cmake alongside other ITK module files,
# matching the path expected by ITKInternalEigen3_DIR.
set(CMAKE_INSTALL_DATADIR "${ITK_INSTALL_PACKAGE_DIR}/Modules")
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

WE should probably follow the ITK3P_ prefix convnetion here.

@hjmjohnson
Copy link
Copy Markdown
Member

@blowekamp Thank you for continuing to work to make the Eigen3 integration better. This is complicated, but your changes look like improvements. If you have performed the tests that are described in the comment (i.e. building downstream products using ITK's internal Eigen3) then this is looking better. Thank you for adding the usage comments and cleaning up.

@blowekamp
Copy link
Copy Markdown
Member Author

@hjmjohnson I believe that I saw in some of your Eigen work that BRAINS and/or ANTs, are using ITK's provided Eigen3 but doing an "Eigen/X" include. Is that correct? That is unfortunately the 1 ( of 3 ) usages of Eigen that was removed to have clear separation of the path safe ITK eigen, and a system like eigen.

The solution to such usage would be to be to use the ITK_EIGEN macro for the include OR to add the documented cmake code to do a find_package for Eigen.

blowekamp and others added 4 commits May 19, 2026 14:51
Code extracted from:

    https://github.com/InsightSoftwareConsortium/eigen

at commit e8527abce63dee5ed7090a81f06e4d6e51519cf7 (for/itk-eigen-5.0.1-bc3b39870).
# By Eigen Upstream
* upstream-Eigen3:
  Eigen3 2026-05-19 (e8527abc)
ITK_EIGEN(X) covers all internal Eigen includes via the header macro.
Downstream projects locate Eigen3 by setting Eigen3_DIR from the
exported ITKInternalEigen3_DIR. The eigen_internal target and its
second include path are no longer needed.

Eigen3Config.cmake is installed alongside ITK module files so
ITKInternalEigen3_DIR resolves correctly in both build and install trees.
@blowekamp blowekamp force-pushed the eigen3-remove-eigen_internal branch from c1b9017 to fa286ba Compare May 19, 2026 18:53
@blowekamp blowekamp changed the title [WIP] ENH: Remove eigen_internal; export ITKInternalEigen3_DIR ENH: Remove eigen_internal; export ITKInternalEigen3_DIR May 19, 2026
@hjmjohnson
Copy link
Copy Markdown
Member

hjmjohnson commented May 20, 2026

ANTs + BRAINSTools build cleanly with no Eigen-related regression.

Methodology
  • Built ITK twice from the same shared source layout under ~/src/common_support_versions/ITK: installed_mainfresh (baseline) and installed_pr6304 (candidate).
  • Enabled the module set required by ANTs: ITKTBB, ITKMINC, ITKIOMINC, ITKIOTransformMINC, AdaptiveDenoising, GenericLabelInterpolator, ITKReview on both. TBB from a shared Pixi env.
  • Built ANTs (SuperBuild, USE_SYSTEM_ITK=ON, USE_TractographyTRX=OFF) and BRAINSTools (SuperBuild, USE_SYSTEM_ITK=ON) against each ITK install.
  • A failure counts as a new regression only if it occurs on the candidate AND not on the baseline (path-normalized comparison).
  • Verified the PR's structural change was active on the candidate: lib/cmake/ITK-6.0/Modules/Eigen3Config.cmake present, eigen_internal no longer referenced in ITKTargets.cmake.
Results
Probe Baseline (main) Candidate (pr-6304) Same outcome? Verdict
ITK core build ✅ green ✅ green yes no regression
ANTs SuperBuild (SEM + ANTS inner, every translation unit) green [17/17], 0 errors green [17/17], 0 errors yes no regression
BRAINSTools SuperBuild ❌ SEM ModuleDescriptionParser link error (missing Expat symbols) ❌ identical, path-normalized diff = empty yes pre-existing SEM/Expat config gap, not a #6304 regression

Coverage on the candidate: full clean ANTs SuperBuild (hundreds of ANTS source files, all SEM and ANTS Eigen-using paths). ~3974/4237 BRAINSTools inner targets compiled cleanly before hitting the SEM/Expat link step that is configured separately in this test harness. Zero Eigen-related compile or link errors in any ANTs or BRAINSTools translation unit on the candidate.

@blowekamp blowekamp marked this pull request as ready for review May 20, 2026 09:58
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 20, 2026

Greptile Summary

Replaces the eigen_internal CMake INTERFACE target with exported ITKInternalEigen3_DIR, pointing downstream consumers to the vendored Eigen3Config.cmake in both build and install trees.

  • ITKEigen3_LIBRARIES is correctly set to empty (Eigen is header-only); ITKEigen3_INCLUDE_DIRS retains src/ for the ITK_EIGEN() macro path. Build-tree and install-tree ITKInternalEigen3_DIR paths align with the actual location of the generated Eigen3Config.cmake.
  • CMAKE_INSTALL_DATADIR is overridden in src/CMakeLists.txt before calling add_subdirectory(itkeigen) so Eigen3Config.cmake installs alongside other ITK module files at ${ITK_INSTALL_PACKAGE_DIR}/Modules, matching what ITK_MODULES_DIR resolves to at find-time.
  • The 15-line downstream usage block added to CMakeLists.txt and two inaccurate/stale comments in itkeigen/CMakeLists.txt should be trimmed to comply with the ITK prose budget.

Confidence Score: 4/5

The functional CMake changes are correct and safe to merge; the install and build tree paths for ITKInternalEigen3_DIR both point to directories that contain the generated Eigen3Config.cmake.

The core path logic is sound and the eigen_internal removal is clean. The only findings are comment-quality issues: a verbose 15-line usage block that violates the ITK prose budget, a stale eigen_internal reference, and a misleading claim about GNUInstallDirs being called by the parent. None of these affect runtime behaviour.

Modules/ThirdParty/Eigen3/CMakeLists.txt (verbose comment block) and Modules/ThirdParty/Eigen3/src/itkeigen/CMakeLists.txt (stale + inaccurate comments)

Important Files Changed

Filename Overview
Modules/ThirdParty/Eigen3/CMakeLists.txt Removes eigen_internal target, sets ITKEigen3_LIBRARIES to empty (correct for header-only), exports ITKInternalEigen3_DIR for both build and install trees. Path logic is sound. Contains a verbose 15-line downstream-usage comment that violates the ITK prose budget.
Modules/ThirdParty/Eigen3/src/CMakeLists.txt Overrides CMAKE_INSTALL_DATADIR to route Eigen3Config.cmake to ${ITK_INSTALL_PACKAGE_DIR}/Modules before calling add_subdirectory(itkeigen); removes itk_module_target(eigen_internal) call. Functional change is correct.
Modules/ThirdParty/Eigen3/src/itkeigen/CMakeLists.txt Removes eigen_internal INTERFACE target and its standalone-project boilerplate (option, GNUInstallDirs); retains eigen_external. Two comments need correction: stale eigen_internal reference and inaccurate claim that the parent calls include(GNUInstallDirs).
Modules/ThirdParty/Eigen3/src/itk_eigen.h.in Removes the downstream-usage Doxygen comment block that documented ITKInternalEigen3_DIR. Content migrated to CMakeLists.txt comments (where it should be further trimmed). No functional change.
Modules/ThirdParty/Eigen3/UpdateFromUpstream.sh Updates the upstream Eigen3 tag reference from for/itk-5.0.1-v3 to for/itk-eigen-5.0.1-bc3b39870; no logic change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[downstream project\nfind_package ITK] --> B{ITK_USE_SYSTEM_EIGEN?}
    B -- yes --> C[ITKEigen3_EXPORT_CODE\nfind_package Eigen3 system]
    B -- no --> D[ITKEigen3_EXPORT_CODE\nset ITKInternalEigen3_DIR]
    D --> E{build tree?}
    E -- yes --> F[ITKInternalEigen3_DIR =\nITKEigen3_BINARY_DIR/src/itkeigen\ncontains Eigen3Config.cmake]
    E -- no install tree --> G[ITKInternalEigen3_DIR =\nITK_MODULES_DIR\n= prefix/ITK_INSTALL_PACKAGE_DIR/Modules]
    F --> H[downstream:\nset Eigen3_DIR = ITKInternalEigen3_DIR\nfind_package Eigen3 CONFIG\ntarget_link_libraries ... Eigen3::Eigen]
    G --> H
Loading

Comments Outside Diff (1)

  1. Modules/ThirdParty/Eigen3/src/itkeigen/CMakeLists.txt, line 880 (link)

    P2 Stale comment: eigen_internal was removed by this PR. The install rule now only serves eigen_external.

Reviews (1): Last reviewed commit: "ENH: Remove eigen_internal; set ITKInter..." | Re-trigger Greptile

Comment on lines +60 to +74
# Re-using internal Eigen3 in another project:
# A downstream project can re-use ITK's vendored Eigen3 via the exported
# Eigen3Config.cmake without depending on the ITK_EIGEN() macro.
#
# find_package(ITK REQUIRED COMPONENTS ITKEigen3)
# if(DEFINED ITKInternalEigen3_DIR)
# set(Eigen3_DIR ${ITKInternalEigen3_DIR})
# endif()
# find_package(Eigen3 REQUIRED CONFIG)
# add_executable(main main.cpp)
# target_link_libraries(main PUBLIC ${ITK_LIBRARIES})
# target_link_libraries(main PUBLIC Eigen3::Eigen)
#
# Then the project can include Eigen headers directly:
# #include <Eigen/Eigenvalues>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Verbose in-source comment exceeds prose budget

The 15-line downstream usage example violates the ITK prose budget rule for in-source comments (1 short line, ≤ 100 chars; default = none). Usage examples belong in documentation (e.g., Documentation/, Doxygen, or the ITK Software Guide), not in CMakeLists.txt. Moving this documentation comment from itk_eigen.h.in to a CMakeLists.txt source file doesn't change its status as committed text subject to the budget.

Context Used: AGENTS.md (source)

Comment on lines +818 to +819
# CMAKE_INSTALL_INCLUDEDIR and CMAKE_INSTALL_DATADIR are inherited from the
# parent ITK project (which calls include(GNUInstallDirs)) and src/CMakeLists.txt.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Inaccurate comment about include(GNUInstallDirs)

A search of the ITK top-level CMakeLists.txt and CMake/ directory confirms that ITK does NOT call include(GNUInstallDirs) at any parent scope. The previous code called it here directly; the claim that it is "inherited from the parent ITK project" is incorrect. Since CMAKE_INSTALL_INCLUDEDIR is only used to set INCLUDE_INSTALL_DIR, which is never referenced in Eigen3Config.cmake.in, this is currently harmless — but the comment will mislead future readers.

Suggested change
# CMAKE_INSTALL_INCLUDEDIR and CMAKE_INSTALL_DATADIR are inherited from the
# parent ITK project (which calls include(GNUInstallDirs)) and src/CMakeLists.txt.
# CMAKE_INSTALL_DATADIR is set by src/CMakeLists.txt; CMAKE_INSTALL_INCLUDEDIR
# is not provided by the parent and is unused in Eigen3Config.cmake.in.

Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson left a comment

Choose a reason for hiding this comment

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

This represents a good step forward. There are a few nit with your comment about naming, and greptile items.

I think the long comment is good in this case, and greptile should be ignored in this case.

I'll leave it to @blowekamp to decide if another round of nit fixes should be included in this PR.

@thewtex thewtex requested a review from phcerdan May 20, 2026 20:18
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:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants