cmake-ing pcl_ros delete CMAKE_PREFIX_PATH and introduce error

[AchmadFathoni]

At melodic branch 8969ea63, using cmake to build pcl_ros will delete environment variable CMAKE_PREFIX_PATH. Here is how I reproduce the problem

$ git clone https://github.com/ros-perception/perception_pcl.git
$ git checkout origin/melodic-devel
$ mkdir build
$ source /opt/ros/noetic/setup.zsh
$ cmake -S perception_pcl/pcl_ros -B build

And the summary of error in the log

-- Using CMAKE_PREFIX_PATH: 
-- Could NOT find dynamic_reconfigure (missing: dynamic_reconfigure_DIR)
-- Could not find the required component 'dynamic_reconfigure'. The following CMake error indicates that you either need to install the package with the same name or change your environment so that it can be found.
CMake Error at /opt/ros/noetic/share/catkin/cmake/catkinConfig.cmake:83 (find_package):
  Could not find a package configuration file provided by
  "dynamic_reconfigure" with any of the following names:

    dynamic_reconfigureConfig.cmake
    dynamic_reconfigure-config.cmake

  Add the installation prefix of "dynamic_reconfigure" to CMAKE_PREFIX_PATH
  or set "dynamic_reconfigure_DIR" to a directory containing one of the above
  files.  If "dynamic_reconfigure" provides a separate development package or
  SDK, be sure it has been installed.
Call Stack (most recent call first):
  CMakeLists.txt:45 (find_package)

As you can see, the CMAKE_PREFIX_PATH becomes blank and then cmake can't find dynamic_reconfigure configuration file. Same problem doesn't happen with pcl_conversion and percpetion_pcl packages

[Itamare4]

I reproduced the same issue on v1.7.3(noetic). I'll do a deeper analysis on that, it's somehow related to PCL cmake, https://github.com/ros-perception/perception_pcl/blob/1.7.3/pcl_ros/CMakeLists.txt#L7

[kunaltyagi]

PCL doesn't know about CMAKE_PREFIX_PATH and shouldn't be able to reset it. I'm not sure if something like that but generic (with all CMake variables/env variables) is happening as an unintentional result.

@Itamare4 Which version of PCL are you using? If it's a recent PCL, could you help us in finding the cause? We'd like to fix it before 1.12.1 is shipped.

cc: @mvieth @larshg

[mvieth]

Found the problem, it is caused by VTK through PCL: VTK uses CMAKE_PREFIX_PATH during configuration in some versions, but does not properly reset it (see here, search for CMAKE_PREFIX_PATH). More specifically, it does reset it to its old value, but if CMAKE_PREFIX_PATH was not set/not defined previously, it is set/defined afterwards, but an empty string. I am not totally sure which VTK versions have this behaviour, probably only VTK 9.x.y versions. catkin checks if CMAKE_PREFIX_PATH is undefined before setting it. If it is defined (even if it is empty), it is left unchanged. pcl_conversions does not have the same problem as pcl_ros because find_package(catkin) is called before find_package(PCL). perception_pcl is just a meta package, so it does not have this problem.

Possible solutions:

1. perception_pcl solution: In pcl_ros, call find_package(catkin ...) before find_package(PCL ...), like in pcl_conversions (melodic-devel would need this change, but probably also ros2)
2. PCL solution: In PCLConfig.cmake, surround find_package(VTK ...) with some additional logic that calls unset(CMAKE_PREFIX_PATH) afterwards if appropriate
3. VTK solution: change the logic in vtk-config.cmake so that the state is exactly as before, meaning calling unset if CMAKE_PREFIX_PATH was not defined before. At the very least, they should be notified of this behaviour Update: I created an issue in the vtk project
4. catkin solution: change the logic so that it is tested whether CMAKE_PREFIX_PATH is empty instead of testing whether it is undefined? Not sure about possible side effects

I would say solution 1 is a must (if there are no reasons against reordering), so that pcl_ros also works with already released PCL and VTK 9.x.y versions. The rest is optional

[Itamare4]

Great work! Thanks @mvieth. I'm generally against specific ordering of CMakes, but it seems there is no other way supporting the existing versions of PCL, VTK.

[kunaltyagi]

Thanks @mvieth for the deep dive and the proposals.

VTK solution (num 3) seems the best approach, but until the issue is closed, I think the first option would be required :(

[mathstuf]

VTK developer here.

Why is catkin being so picky (sorry, I'm not familiar with the ROS corner of the world, so this may be obvious to those familiar)? I think I'd find it surprising if -DCMAKE_PREFIX_PATH=$empty_var ended up different than not passing it. I guess understanding what problem catkin is solving by using DEFINED rather than "doesn't have what I need" would be the thing to investigate (from my POV) because I don't see why projects wanting to mess with CMAKE_PREFIX_PATH don't just do it unconditionally (with restoration if it makes sense) rather than overthinking it.

[mathstuf]

Looking at catkin, it seems to be pulling the envvar down into a CMake variable. Looking at the history of that code, it goes back far, but it's not clear what its original purpose is.

[mvieth]

@mathstuf Thank you for your comment. I created an issue in the catkin repo, maybe the maintainers there can shed some more light on this