Added CMake options to manage the Eigen dependency.

[LTLA]

This does a few things to provide more options for managing the Eigen dependency in downstream projects.

• If the find_package works and USE_SYSTEM_EIGEN is set to ON (the default), the system Eigen is directly used. This preserves the behavior of the current approach.
• If either of the two conditions above is false, we FetchContent the latest version of Eigen directly and expose its targets for downstream use. This means that Spectra will work without requiring users to pre-install Eigen; it is also necessary for certain buildchains that don't work with the usual find_package (in my case, Emscripten, for compiling to Webassembly).
• The entire section is only evaluated if Eigen3::Eigen is not already available as a target. This allows projects to easily control how Eigen is made available without having to worry about conflicts with Spectra's definition.

[yixuan]

Thanks Aaron, it looks great to me. I'm not an expert on CMake, so I may ask @guacke @JensWehner @jschueller @shivupa to give some comments.

[codecov-commenter]

Codecov Report

Merging #127 (8411d62) into master (904bbbe) will not change coverage. The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #127   +/-   ##
======================================
  Coverage    92.1%   92.1%           
======================================
  Files          45      45           
  Lines        2140    2140           
======================================
  Hits         1971    1971           
  Misses        169     169           

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 904bbbe...8411d62. Read the comment docs.

[JensWehner]

I am not a big fan of using CMake as a package manager. So I not really see the benefit of it, but maybe there are good reasons.

People who need eigen should install it before in my opinion, but I have not seen each usecase. @LTLA can you elaborate a bit why this is needed?

[shivupa]

I generally do a similar "find library or fetch library" approach for most packages in my downstream application (even for a large gaussian integral library), and it works well for me.

The current behavior is unchanged if eigen is present, which is a pro in my eyes. The added flexibility could be nice as well.

The only "con" is that most of the time (always?) Eigen will be necessary to build a matrix before spectra is used to diagonalize it so presumably it is already installed.

[jschueller]

this is probably overkill, plus you might getting version conflicts if this doesnt match some other version in a top-level package not directly dependent of spectra, eigen doesnt like linking to different versions, even across patch versions versions

[LTLA]

Thanks all for the comments. Responses interspersed below.

can you elaborate a bit why this is needed?

In my case, I am cross-compiling for a different architecture (specifically, Webassembly). It seems that the toolchain for doing so (Emscripten) does not acknowledge system installations of CMake projects. I'm not a CMake expert but I believe this is actually intentional and correct; find_package is designed to bring pre-built binaries into the current project, and if one is cross-compiling, pre-built binaries for the host architecture don't make any sense for the target architecture. (In this case, it doesn't matter because Eigen is header-only, but I guess that isn't relevant to CMake when it comes to find_package's availability in this scenario.)

In contrast, FetchContent brings the Eigen source code into the current project (see comments here about the distinction), which makes it immediately available for cross-compilation.

For the more general (not cross-compiled) use case, FetchContent still provides quite a few benefits. Fetching is done automatically so users don't need to do an extra installation step - Spectra "Just Works" without any more effort. I have already taken advantage of this elsewhere in various GitHub Actions to avoid the need to install the Eigen headers before building my project. In addition, the Eigen version can be pinned to something that is known to work with Spectra, facilitating reproducibility across different build platforms.

And of course, the initial if(NOT TARGETS Eigen3) ensures that project maintainers can always explicitly control the version of Eigen, by making the Eigen3 target available before pulling in Spectra. This is useful when developing in an environment where the version of Eigen discovered by find_package is not satisfactory, e.g., on HPCs that typically have medieval versions of the OS and associated packages.

The only "con" is that most of the time (always?) Eigen will be necessary to build a matrix before spectra is used to diagonalize it so presumably it is already installed.

Right, the Eigen3 target (and thus, its headers) has to be available in the project. However... whether that actually involves installing Eigen is another question.

this is probably overkill, plus you might getting version conflicts if this doesnt match some other version in a top-level package not directly dependent of spectra,

Indeed, which is the reason for the initial if(NOT TARGETS Eigen3). If a project uses Spectra + some other Eigen-dependent package, the maintainer can control the exact version of Eigen to resolve any conflicts, rather than relying on whatever crawls out of find_package(). There's no [version] specification in Spectra's find_package() call, so I don't know what it'll actually pick when some other dependency needs a different version of Eigen. Maybe CMake does something sensible here with respect to version resolution - I don't know - but as a maintainer of a downstream project, I would like to avoid the guesswork and pin the version explicitly.

[yixuan]

Sorry for the long delay -- I just didn't have time to focus on this.

From the conversations above, it seems that overall this change is useful in certain scenarios, and more importantly, it does not break the normal use. I'd like to merge it, but before that I want to ask if the following scheme can be implemented or not.

Instead of providing the USE_SYSTEM_EIGEN option, we use an AUTO_DOWNLOAD_EIGEN option, which is OFF by default. Only when Eigen is not found and AUTO_DOWNLOAD_EIGEN is ON, we let CMake automatically download Eigen. The difference is that we still give an error by default to remind users of installing Eigen, unless they explicitly indicate that they want Eigen to be downloaded. This behavior seems to be more consistent. Any thoughts? @LTLA

[LTLA]

Sure; now that I think about it, I actually don't need the FetchContent part for my projects. I just put it in as a courtesy for other users who didn't have Eigen installed and wanted Spectra to work by default. However, if your preferred default is to remind users to install Eigen, then users would have to fiddle with their CMakeLists.txt anyway. In that case, we could simplify this PR to:

if (NOT TARGET Eigen3::Eigen)
    find_package(Eigen3 NO_MODULE REQUIRED)
    set_package_properties(Eigen3 PROPERTIES TYPE REQUIRED PURPOSE "C++ vector data structures")
    message(STATUS "Found Eigen3 Version: ${Eigen3_VERSION} Path: ${Eigen3_DIR}")
endif()

And then tell users who don't want to/can't install a system Eigen to put the following in their CMakeLists.txt:

include(FetchContent)
FetchContent_Declare(
    eigen3 
    GIT_REPOSITORY https://gitlab.com/libeigen/eigen
    GIT_TAG 3.4.0 # maybe need some recommendations for "known working" versions
)
FetchContent_MakeAvailable(eigen3)

# ... and then pull in Spectra here.

This will create the Eigen3::Eigen target before Spectra's CMakeLists.txt is evaluated, thereby skipping the find_package(). So users who know better can create the target, and this will be respected by Spectra; and everyone else will just be prompted to install Eigen (or use FetchContent, if you want to throw that snippet above into the Spectra installation docs somewhere).

[alecjacobson]

Any harm in merging this? I'm in need of a similar change to use spectra from libigl.

[jdumas]

Hi. I'd like to add my 2c and vote for this simplified version of this PR:

if (NOT TARGET Eigen3::Eigen)
    find_package(Eigen3 NO_MODULE REQUIRED)
    set_package_properties(Eigen3 PROPERTIES TYPE REQUIRED PURPOSE "C++ vector data structures")
    message(STATUS "Found Eigen3 Version: ${Eigen3_VERSION} Path: ${Eigen3_DIR}")
endif()

Having a simple way to override the provided Eigen is all that matters when the default approach is not satisfactory for reason X or Y.

[jschueller]

yes, dont merge this one, go for https://github.com/yixuan/spectra/pull/152

[yixuan]

Thank you all for the helpful discussions. It seems that the current consensus is to use the simplified version that was originally proposed by @LTLA (again, big thanks!) and recently implemented in #152.