Yarp: PCL usage requirements are not properly propagated by YARP_pcl

[PeterBowman]

Background:
I have been recently working on a YARP wrapper around several PCL cloud processing and surface reconstruction algorithms (library, rationale). The YARP_pcl component had come in useful in terms of YARP2PCL and PCL2YARP type conversions, although not devoid of some shortcomings on the CMake side of life.

Minimal not working example:

# CMakeLists.txt
cmake_minimum_required(VERSION 3.12 FATAL_ERROR)
project(yarp-pcl LANGUAGES CXX)
find_package(YARP 3.4 REQUIRED pcl)
add_executable(${PROJECT_NAME} main.cpp)
target_link_libraries(${PROJECT_NAME} YARP::YARP_pcl)

// main.cpp
#include <yarp/pcl/Pcl.h>
auto main() -> int {}

In file included from /home/bartek/Desktop/temp/yarp-pcl/main.cpp:1:
/usr/local/include/yarp/pcl/Pcl.h:12:10: fatal error: pcl/io/pcd_io.h: No such file or directory
   12 | #include <pcl/io/pcd_io.h>
      |          ^~~~~~~~~~~~~~~~~

Diagnosis:
As expected, YARP_pcl is instructed to propagate PCL usage requirements to downstreams:

https://github.com/robotology/yarp/blob/0481f994c6e03897d038c5f1d1078145646a1772/src/libYARP_pcl/src/CMakeLists.txt#L23-L25

However, the ${PCL_INCLUDE_DIR} and ${PCL_LIBRARIES} variables are empty. These have not been populated since YARP does not perform a find_package(PCL) call anywhere. As a result, downstream projects are instructed to search for PCL on the system (thanks to the third instruction), but neither PCL targets/libraries nor PCL include directories are automatically propagated via target_link_libraries(... YARP::YARP_pcl) as they should be, thus resulting in the missing header inclusion error.

Remark regarding YARP_cv:
I believe this component was developed in a similar vein to YARP_cv. Please note YARP_cv does not export OpenCV's usage requirements either. Even though I haven't tested it thoroughly, I believe this same issue prevails after inspecting the generated YARP_cvTargets.cmake file. In that case, however, I tracked down the problem to the fact that there is no such thing as OPENCV_INCLUDE_DIR (ref) on my OpenCV 4.4 installation, it should read OpenCV_INCLUDE_DIRS instead or, ideally, favor modern CMake targets.

Besides, YARP actually looks for OpenCV as there exists some CV-dependent code such as the opencv_grabber device.

Additional suggestions:
There is more room for improvements beyond fixing the find_package thing:

• In yarp/pcl/Pcl.h, I'd add these header inclusions:
  cxx #include <cstring> // std::memcpy, also mind prepending std:: to the existing call #include <pcl/point_cloud.h> // pcl::PointCloud<T>
• The following line in CMakeLists.txt:
  cmake list(APPEND YARP_pcl_PUBLIC_DEPS PCL)
  is better written as:
  cmake list(APPEND YARP_pcl_PUBLIC_DEPS "PCL COMPONENTS common io")
  In this way, downstreams only need to search for specific PCL components as requested by YARP_pcl. Otherwise, all available PCL components and their related variables are needlessly populated and brought to scope.
• Care should be taken to check this against different PCL versions, but on PCL 1.10 I can confirm PCL_INCLUDE_DIR is empty. It should read PCL_INCLUDE_DIRS instead.

Tested configuration:

• YARP version 3.4.1+81-20201214.6+git1b0060380
• PCL 1.10 (via apt install libpcl-dev)
• cmake version 3.16.3
• g++ (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
• Ubuntu 20.04.1 (focal), Linux 5.4.0-58-generic

[PeterBowman]

Current workaround for downstreams depends on the PCL version. PCL 1.9 and higher implements modern CMake targets, therefore it suffices to write target_link_libraries(${PROJECT_NAME} YARP::YARP_pcl ${PCL_LIBRARIES}). In previous releases, though, you should explicitly state you need PCL headers via include_directories(${PCL_INCLUDE_DIRS}) or similar.

[drdanz]

Can you post the content of your YARP_pclConfig.cmake file?
In my case it contains:

# [...]
include(CMakeFindDependencyMacro)
find_dependency(YARP_sig HINTS "${CMAKE_CURRENT_LIST_DIR}/.." NO_DEFAULT_PATH)
find_dependency(PCL)
# [...]

[PeterBowman]

In my case it contains:

Mine is exactly the same, but that's not the point. The find_dependency(PCL) makes my project call find_package(PCL) behind the scenes, but I'd expect a target_link_libraries(myTarget YARP::YARP_pcl) to be enough for propagating PCL headers. This is not happening (although it presumably was the original intention if we analyze YARP_pcl's CMakeLists.txt). It is YARP_pclTargets.cmake what we should be focusing on:

# build tree, i.e. build/YARP_pcl/YARP_pclTargets.cmake
add_library(YARP::YARP_pcl INTERFACE IMPORTED)

set_target_properties(YARP::YARP_pcl PROPERTIES
  INTERFACE_COMPILE_FEATURES "cxx_std_14"
  INTERFACE_INCLUDE_DIRECTORIES "/home/bartek/git/yarp/src/libYARP_pcl/src"
  INTERFACE_LINK_LIBRARIES "YARP::YARP_sig"
)

# install tree, i.e. build/src/libYARP_pcl/CMakeFiles/Export/lib/cmake/YARP_pcl/YARP_pclTargets.cmake
add_library(YARP::YARP_pcl INTERFACE IMPORTED)

set_target_properties(YARP::YARP_pcl PROPERTIES
  INTERFACE_COMPILE_FEATURES "cxx_std_14"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "YARP::YARP_sig"
)

[drdanz]

I see the problem now...
This might come from (at least) 2 different problems:

The CMake file contains:

target_include_directories(YARP_pcl SYSTEM INTERFACE ${PCL_INCLUDE_DIR})
target_link_libraries(YARP_pcl INTERFACE ${PCL_LIBRARIES})

1) PCL no longer defines PCL_LIBRARIES and PCL_INCLUDE_DIR, therefore they are empty when target_include_directories and target_link_libraries are executed.

2) These variables are resolved at CMake time, therefore if PCL is not available when CMake is executed, these are empty. We always considered that being a header only library, there was no need to have the library at compile time, but I see now that it is required to fill the library dependencies.

[drdanz]

Actually I just noticed that PCL is not even searched in YARP, that's why the variables are empty.
Just adding a find_package(PCL) changed

 set_target_properties(YARP::YARP_pcl PROPERTIES
   INTERFACE_COMPILE_FEATURES "cxx_std_14"
   INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}§/include"
-  INTERFACE_LINK_LIBRARIES "YARP::YARP_sig"
+  INTERFACE_LINK_LIBRARIES "YARP::YARP_sig;pcl_common;pcl_kdtree;pcl_octree;pcl_search;pcl_sample_consensus;pcl_filters;pcl_io;pcl_features;pcl_ml;pcl_segmentation;pcl_visualization;pcl_surface;pcl_registration;pcl_keypoints;pcl_tracking;pcl_recognition;pcl_stereo;pcl_apps;pcl_outofcore;pcl_people;/usr/lib/x86_64-linux-gnu/libboost_system.so;/usr/lib/x86_64-linux-gnu/libboost_filesystem.so;/usr/lib/x86_64-linux-gnu/libboost_date_time.so;/usr/lib/x86_64-linux-gnu/libboost_iostreams.so;/usr/lib/x86_64-linux-gnu/libboost_regex.so;\$<\$<NOT:\$<OR:\$<CONFIG:DEBUG>,\$<CONFIG:RELWITHDEBINFO>,\$<CONFIG:DEBUGFULL>,\$<CONFIG:PROFILE>>>:/usr/lib/x86_64-linux-gnu/libqhull.so>;\$<\$<OR:\$<CONFIG:DEBUG>,\$<CONFIG:RELWITHDEBINFO>,\$<CONFIG:DEBUGFULL>,\$<CONFIG:PROFILE>>:/usr/lib/x86_64-linux-gnu/libqhull.so>;vtkChartsCore;vtkCommonColor;vtkCommonCore;vtksys;vtkCommonDataModel;vtkCommonMath;vtkCommonMisc;vtkCommonSystem;vtkCommonTransforms;vtkCommonExecutionModel;vtkFiltersGeneral;vtkCommonComputationalGeometry;vtkFiltersCore;vtkInfovisCore;vtkFiltersExtraction;vtkFiltersStatistics;vtkImagingFourier;vtkImagingCore;vtkalglib;vtkRenderingContext2D;vtkRenderingCore;vtkFiltersGeometry;vtkFiltersSources;vtkRenderingFreeType;/usr/lib/x86_64-linux-gnu/libfreetype.so;/usr/lib/x86_64-linux-gnu/libz.so;vtkFiltersModeling;vtkImagingSources;vtkInteractionStyle;vtkInteractionWidgets;vtkFiltersHybrid;vtkImagingColor;vtkImagingGeneral;vtkImagingHybrid;vtkIOImage;vtkDICOMParser;vtkmetaio;/usr/lib/x86_64-linux-gnu/libjpeg.so;/usr/lib/x86_64-linux-gnu/libpng.so;/usr/lib/x86_64-linux-gnu/libtiff.so;vtkRenderingAnnotation;vtkRenderingVolume;vtkIOXML;vtkIOCore;vtkIOXMLParser;/usr/lib/x86_64-linux-gnu/libexpat.so;vtkIOGeometry;vtkIOLegacy;vtkIOPLY;vtkRenderingLOD;vtkViewsContext2D;vtkViewsCore;vtkRenderingContextOpenGL2;vtkRenderingOpenGL2;FLANN::FLANN"
)

[drdanz]

So, I think this means that

1. PCL library should be searched when building YARP (Eigen and OpenCV are already searched)
2. YARP_pcl, YARP_eigen, and YARP_cv should be built only if PCL/Eigen/OpenCV are available when building YARP, otherwise tge

I'm not sure if it is possible to escape the ${PCL_INCLUDE_DIR} and ${PCL_LIBRARIES} variable expansion, so that they are copied (unescaped) in the Targets file.

CC @Nicogene

[drdanz]

@PeterBowman I believe that #2440 solves the issue, can you please let me know if it works for you?

[drdanz]

Closing this for now, but we should come back to this issue when all the supported platforms have PCL with cmake targets.