ENH: Update external Eigen from release 3.4 to master

[phcerdan]

ENH: Update Eigen from 3.4 to master

[phcerdan]

Test is passing. The failure in Windows is unrelated, it was failing for other PR's as well.

[N-Dekker]

Thanks @phcerdan but would it still be an option to use a tagged revision of Eigen? Looks like 3.4.1 is quite a recent version: https://gitlab.com/libeigen/eigen/-/commits/3.4.1

---

Oh, now I see, 3.4.1 is just a branch, not (yet?) a tag.

[phcerdan]

Good spotting @N-Dekker, it would be good to discuss what we prefer, this 3.4.1 branch or a commit from master.

We could discuss this in the open issue: #3546

[thewtex]

Awesome!

[thewtex]

@jcfr may have suggestions on how to make the mangling name configurable

[phcerdan]

@jcfr happy to hear suggestions for making the mangling name configurable. If you can provide a link to some existing CMake project with the options, I can copy it here.

This PR introduces a BOOL variable ITK_EIGEN_MANGLE, and if that's defined, we mangle with #define Eigen itkeigen (this is a header-only library). I kept the same approach than VTK (they mangle with vtkeigen).

[jcfr]

re: mangling

See this relevant comment:

• ENH: Update mangled third-party libraries to use MANGLE_PREFIX CMake variable #4102 (comment)

Associated discussion for generalizing the support currently specific to Slicer¹² is discussed in https://discourse.itk.org/t/adding-support-for-customizing-itk-namespace/5170

Footnotes

1. https://github.com/Slicer/ITK/commits/slicer-v5.3rc04-2022-09-19-62eb5ca ↩

2. https://github.com/Slicer/ITK/commit/69e52f05c482012b95228c3fe46f299b1735ca99 (COMP: Add support for customizing ITK namespace (draft)) ↩

[jcfr]

This PR introduces a BOOL variable ITK_EIGEN_MANGLE

I am not sure we want a CMake option to control this as we will always want the function name to be mangled if building against the version bundled in ITK.

[phcerdan]

I am not sure we want a CMake option to control this as we will always want the function name to be mangled if building against the version bundled in ITK.

Not mangling Eigen was proposed to facilitate external ITK modules (see ITKTotalVariation for example) using third-party libraries (libproxTV) that use Eigen to not having to fork the proxTV library and replace all Eigen references with itkeigen, or having to setup a new Eigen for that external module.
Eigen is so common in C++ ecosystem that it seemed a good helper for external modules.

However, I understand mangling is better for the general case, and we should turn it ON by default. But I would appreciate if we keep the option to turn it off, to reuse the internal Eigen on those external modules that use it.

Slicer@69e52f0 (COMP: Add support for customizing ITK namespace (draft)) ↩

Thanks for the links, amazing work!
As a side note and different topic, I think we have to keep all those changes of mangling to third party code in our own fork of the third-party, instead of handling them in ITK history. We should have an ITK/HDF5 fork and a branch with those changes on top.
We share the same tech with VTK to update-from-stream, but we don't seem to comply with their preconditions. Changes to third-party code should be handled in a third-party-fork, not in ITK. We face git history pollution, and merge conflicts because of it (see #2314).

[jcfr]

I also suggest to split this pull request and contribute the support for disabling the mangling of the Eigen include directory into its own pull request.

[jcfr]

Not mangling Eigen was proposed to facilitate external ITK modules (see ITKTotalVariation for example) using third-party libraries (libproxTV) that use Eigen to not having to fork the proxTV library and replace all Eigen references with itkeigen, or having to setup a new Eigen for that external module.

I indeed remember the time we worked on this together. If I recall it is available at https://github.com/phcerdan/proxTV/commits/use_eigen

For reference, see also https://github.com/InsightSoftwareConsortium/ITKTotalVariation/blob/master/CMakeLists.txt

[jcfr]

@phcerdan and I reviewed this pull request and concluded that updating Eigen without adding extra logic for namespacing was a sensible first step.

Approach for supporting including multiple Eigen "header only" library in one project will be evaluated and discussed later.

[phcerdan]

Tested against current ITKTotalVariation external module, which uses the internal ITK eigen, and all green.

[phcerdan]

Not in a hurry, but could we move forward with this? I integrated all the suggestions from @jcfr.
Removed any reference to mangling.

Wrapping the "mangling" conversation. (Note, mangling refers to symbols, there are no symbols in a header-only library).
The simple #define Eigen itkeigen (borrowed from VTK), is not enough to mangle make this header not colliding with third-party installations of Eigen. That define will also modify third-party Eigen.

Our protection is changing the name (the path) of the eigen files.
This protects ITK from picking an external Eigen (for example one installed in the system).

If a third party linking to ITK uses Eigen, the standard procedure, as with many other dependencies is to use their own Eigen and share it with ITK (ITK_USE_SYSTEM_EIGEN), and with their software. Or, which I think is great, reuse the internal Eigen from ITK and use that for their own library without the need of using any ITK macro. See ITKTotalVariation for that: https://github.com/InsightSoftwareConsortium/ITKTotalVariation/blob/b615ce13394fb5fb16bf8abc2390f5dab73ac27e/CMakeLists.txt

  if(NOT ITK_USE_SYSTEM_EIGEN)
    # Set Eigen3_DIR to the internal Eigen3 used in ITK
    set(Eigen3_DIR "${ITKInternalEigen3_DIR}")
    message(STATUS "ITKTotalVariation: Using internal ITK Eigen Config found in: ${Eigen3_DIR}")
  endif()
  find_package(Eigen3 REQUIRED)
  set(${PROJECT_NAME}_EXPORT_CODE_INSTALL
"${${PROJECT_NAME}_EXPORT_CODE_INSTALL}
set(Eigen3_DIR \"${Eigen3_DIR}\")
find_package(Eigen3 REQUIRED CONFIG)
")
  set(${PROJECT_NAME}_EXPORT_CODE_BUILD
"${${PROJECT_NAME}_EXPORT_CODE_BUILD}
if(NOT ITK_BINARY_DIR)
  set(Eigen3_DIR \"${Eigen3_DIR}\")
  find_package(Eigen3 REQUIRED CONFIG)
endif()
")

[dzenanz]

@jcfr Review/merge?

[phcerdan]

This is ready, all @jcfr suggestions have been integrated (thanks!)

[thewtex]

💖