Pcl: Wrong Eigen found from FindEigen.cmake

[alspitz]

Context

This issue can cause an incorrect Eigen version to be compiled in my program resulting in seg faults.

Expected Behavior

I expect that the same eigen version used to compile pcl be included in PCL_INCLUDE_DIRS.

Current Behavior

Currently, an Eigen version different from that used to compile pcl is included in PCL_INCLUDE_DIRS

Code to Reproduce

Pass custom EIGEN_INCLUDE_DIRS to cmake for PCL compilation. When this PCL is find_packaged'ed it will search for the first Eigen version it finds on the system and include that in PCL_INCLUDE_DIRS.

Possible Solution

The EIGEN_ROOT variable should be the first one used to search for Eigen in FindEigen.cmake. This could be solved by not setting CMAKE_INCLUDE_PATH at the top of FindEigen.cmake, which causes cmake's find_path to search in /usr before /usr/local before EIGEN_ROOT.

[taketwo]

But what are the contents of EIGEN_ROOT. Who sets it?

[alspitz]

At least in the case of an installed PCL, EIGEN_ROOT is set by PCLConfig.cmake, which correctly sets EIGEN_ROOT to the Eigen include dir used during compilation of PCL. The problem is that FindEigen.cmake places /usr in a higher priority than EIGEN_ROOT. This causes any programs that depend on PCL to compile with an Eigen version that may be different from that used to compile PCL.

[taketwo]

Oh, I see, you are referring to:

https://github.com/PointCloudLibrary/pcl/blob/a61e6aa26c752b9dbb61c3e91ae249e4ba026087/PCLConfig.cmake.in#L120

in the config file. Right, this will be populated with the Eigen directory used during compilation of PCL.

I think what you propose makes sense. Perhaps we'd also want to promote EIGEN_DIR to be before directories discovered by PkgConfig.

PR is welcome.

[alspitz]

I would propose to revert this commit: https://github.com/PointCloudLibrary/pcl/commit/4d967aa0fad3b7944f65f0d96e63c55d733bc48e#diff-b7120367666f916877df69ebfa4c49f4

and the Eigen part of https://github.com/PointCloudLibrary/pcl/commit/0e0f2c4e4fecef352ac46f136a2f6dac9f7aea17#diff-b7120367666f916877df69ebfa4c49f4

Do you know why those were made? cmake's find_path should already search all of the paths in the PATH env variable. I suppose /usr and /usr/local can be hardcoded by appending them to CMAKE_SYSTEM_INCLUDE_PATH (instead of CMAKE_INCLUDE_PATH), which is searched after the HINTS option to find_path.

[taketwo]

The first one seems to solve some legacy PCL/ROS issues (discussed in #50).

The second one I don't know. @SergioRAgostinho is an occasional Mac user, maybe he can shed some light on why this might be necessary.

I feel it should be okay to just drop CMAKE_INCLUDE_PATH modifications.

[SergioRAgostinho]

if(APPLE)
  list(APPEND CMAKE_INCLUDE_PATH /opt/local)
  set(CMAKE_FIND_FRAMEWORK NEVER)
endif()

Regarding the first line. No idea. All our 3rd party deps are usually installed through homebrew and those get dropped at /usr/local/Cellar and are symlinked into /usr/local, example:

$ pkg-config eigen3 --cflags-only-I    
-I/usr/local/Cellar/eigen/3.3.4/include/eigen3
$ ls -dl /usr/local/include/eigen3
lrwxr-xr-x  1 neglective  staff  36 Feb 23  2018 /usr/local/include/eigen3 -> ../Cellar/eigen/3.3.4/include/eigen3

Edit: I had completely forgot about Mac ports. That’s the one who installs to /opt/local.

The second line is just to prevent linking against frameworks. I don't understand why this one needs to be set. I think all our 3rd party deps are not shipped as frameworks.

[taketwo]

Thanks Sergio, I suppose this clears out the question. Should be safe to delete these path modifications.

[SergioRAgostinho]

I made an edit to my comment. That path is used by the Mac ports project, a package manager. It used to have an extensive user base but I think it fell in popularity vs homebrew.

[taketwo]

Ok, then we probably want to keep it around for the time being.

[svenevs]

Since its 2018, and and we're switching to modern CMake, I propose the vendored FindEigen.cmake be removed entirely.

Eigen has been properly exporting their CMake targets for years now (and they also _FINALLY_ released 3.3.5, which has many fixes related to CUDA 9 :heart:). From their docs

find_package(Eigen3 3.3 REQUIRED NO_MODULE)
# or
find_package(Eigen3 3.3 NO_MODULE)
if (TARGET Eigen3::Eigen)
  # suggestion for future interface
  target_link_libraries(pcl-interface INTERFACE Eigen3::Eigen)
endif()

A note about NO_MODULE, it's specifically to skip things like FindEigen3.cmake (the old-school find modules in general). From the cmake docs

The CONFIG option may be used to skip Module mode explicitly and switch to Config mode. It is synonymous to using NO_MODULE. Config mode is also implied by use of options not specified in the reduced signature.

This places the burden on the user to configure their environment correctly, namely the eigen3Config.cmake file needs to be accessible in their CMAKE_PREFIX_PATH. Personally I have a strong preference for CONFIG over NO_MODULE, as it is a little more literal -- _find the *Config.cmake thing instead of Find*.cmake_ but it doesn't really matter.

[taketwo]

@svenevs I fully support you, including CONFIG vs. NO_MODULE. We'll lift our requirements after 1.9.0 release and can definitely make this transition.

That said, we can still include the fix suggested by @pumaking into 1.9.0. So if anyone cares about that, a PR is welcome in the next few days. After that we'll freeze and start testing release candidates.