Move SSE compiler options to PCL_COMPILE_OPTIONS. Expose PCL as a CMake imported target.

[SergioRAgostinho]

This PR ensures the SSE compiler options are passed properly downstream. After reading the descriptions for

https://cmake.org/cmake/help/v3.0/command/target_compile_definitions.html#command:target_compile_definitions
https://cmake.org/cmake/help/v3.0/command/target_compile_options.html?highlight=target_compile_options

I opted to create the new var PCL_COMPILE_OPTIONS that should be used by downstream projects like this

cmake_minimum_required(VERSION 3.0.0 FATAL_ERROR)
project(my_awesome_project VERSION 0.1.0 LANGUAGES CXX)

find_package(PCL 1.8.1.99 REQUIRED)

add_executable (${PROJECT_NAME} "${PROJECT_NAME}.cpp")
target_include_directories(${PROJECT_NAME} PUBLIC ${PCL_INCLUDE_DIRS})
target_link_libraries (${PROJECT_NAME} ${PCL_LIBRARIES})
target_compile_definitions(${PROJECT_NAME} PUBLIC ${PCL_DEFINITIONS})
target_compile_options(${PROJECT_NAME} PUBLIC ${PCL_COMPILE_OPTIONS})

Fixes #2013, fixes #1725

Edit (IMPORTANT)

This PR also upgrades "PCLConfig.cmake". From now on, only the following two lines are necessary to use PCL in downstream projects:

find_package(PCL REQUIRED)
target_link_libraries (${PROJECT_NAME} ${PCL_LIBRARIES})

The target_include_directories, target_compile_definitions, and target_compile_options mentioned above are superfluous (but they don't hurt and may be kept for compatibility with older PCL versions).

[SergioRAgostinho]

PCL_COMPILE_OPTIONS or simply PCL_OPTIONS?

[jasjuang]

@SergioRAgostinho An even more modern cmake approach is target_link_libraries to "library targets" instead of the "plain library". More explanation here https://cmake.org/cmake/help/v3.9/command/target_link_libraries.html. The targets will contain libraries, includes, and definitions. So in the downstream cmake it will look something like

cmake_minimum_required(VERSION 3.9 FATAL_ERROR)
project(my_awesome_project LANGUAGES CXX)

find_package(PCL 1.8.1.99 REQUIRED)

add_executable (${PROJECT_NAME} "${PROJECT_NAME}.cpp")

target_link_libraries (${PROJECT_NAME} pcl::pcl)

Thanks a lot for attempting to fix #2013

[SergioRAgostinho]

Agreed but that's a massive rework into the whole CMake system. One step at the time.

Also the SSE flags are compiler options, not exactly preprocessor definitions. I'm not sure those are automatically propagated.

[jasjuang]

@SergioRAgostinho My understanding is if you are building a library in the upstream and exporting it as a target for the downstream to target_link, no matter you are doing target_compile_definitions or
target_compile_options in the upstream, the information will be embedded in the target and get passed to the downstream.

[SergioRAgostinho]

So I have the impression that works pretty great if all your targets are from within the same project, or being built in the same system, but when you want to deploy your library on some external system things are a little different no?

In theory you're shipping only your headers, libs, a cmake config with info about which components were built and which external dependencies used/enabled.

Do you have an example I can check of a situation where you linked against a third party which implemented this system properly?

[taketwo]

Agreed but that's a massive rework into the whole CMake system. One step at the time.

Actually a very basic implementation of this is possible without any rework, just changing a bunch of lines in PCLConfig. A can give it a try.

[jasjuang]

@SergioRAgostinho The smallest 3rd party library I know that does this properly is glog. Only target_link_libraries (${PROJECT_NAME} glog::glog) is needed for the downstream. Another even smaller 3rd party library that also does this almost right is tinyxml2, target_link_libraries (${PROJECT_NAME} tinyxml2) will also just work, but they didn't add the namespace (tinyxml2::tinyxml2) which is considered as good practice.

CMake actually allows you export targets differently for internal and external stuff. One classical example is the DLL macros for windows. You can do something like this

if(MSVC)
  set(DLLIMPORT "__declspec(dllimport)")
  set(DLLEXPORT "__declspec(dllexport)")
endif()

target_compile_definitions(${PROJECT_NAME} INTERFACE
                          WINDOWS_DLL=${DLLIMPORT})

target_compile_definitions(${PROJECT_NAME} PRIVATE
                          WINDOWS_DLL=${DLLEXPORT})

So externally the macro is dllimport to them, internally it's dllexport.

[SergioRAgostinho]

These exports then...

install (TARGETS glog
  EXPORT glog-targets
  RUNTIME DESTINATION ${_glog_CMake_BINDIR}
  PUBLIC_HEADER DESTINATION ${_glog_CMake_INCLUDE_DIR}/glog
  LIBRARY DESTINATION ${_glog_CMake_LIBDIR}
  ARCHIVE DESTINATION ${_glog_CMake_LIBDIR})


export (TARGETS glog NAMESPACE glog:: FILE glog-targets.cmake)
export (PACKAGE glog)

install (FILES
  ${CMAKE_CURRENT_BINARY_DIR}/glog-config.cmake
  ${CMAKE_CURRENT_BINARY_DIR}/glog-config-version.cmake
  DESTINATION ${_glog_CMake_INSTALLDIR})

install (EXPORT glog-targets NAMESPACE glog:: DESTINATION
${_glog_CMake_INSTALLDIR})

[taketwo]

Yeah, exporting the targets is "the right way", and this will require serious rework.
Temporary hacky solution is to manually create imported targets inside PCLConfig. Here is my try on it:

diff --git a/PCLConfig.cmake.in b/PCLConfig.cmake.in
index 7aed0a7..c2d1ed1 100644
--- a/PCLConfig.cmake.in
+++ b/PCLConfig.cmake.in
@@ -531,24 +531,50 @@ foreach(component ${PCL_TO_FIND_COMPONENTS})
       list(APPEND PCL_DEFINITIONS ${PCL_${COMPONENT}_DEFINITIONS})
       list(APPEND PCL_LIBRARY_DIRS ${component_library_path})
       if(PCL_${COMPONENT}_LIBRARY_DEBUG)
-        list(APPEND PCL_${COMPONENT}_LIBRARIES optimized ${PCL_${COMPONENT}_LIBRARY} debug ${PCL_${COMPONENT}_LIBRARY_DEBUG})
         list(APPEND PCL_LIBRARY_DIRS ${component_library_path_debug})
-      else(PCL_${COMPONENT}_LIBRARY_DEBUG)
-        list(APPEND PCL_${COMPONENT}_LIBRARIES ${PCL_${COMPONENT}_LIBRARY})
-      endif(PCL_${COMPONENT}_LIBRARY_DEBUG)
-      list(APPEND PCL_LIBRARIES ${PCL_${COMPONENT}_LIBRARIES})
+      endif()
+      list(APPEND PCL_LIBRARIES ${pcl_component})
       mark_as_advanced(PCL_${COMPONENT}_LIBRARY PCL_${COMPONENT}_LIBRARY_DEBUG)
-    endif(_is_header_only EQUAL -1)    
+    endif()
     # Append internal dependencies
     foreach(int_dep ${pcl_${component}_int_dep})
       string(TOUPPER "${int_dep}" INT_DEP)
       if(PCL_${INT_DEP}_FOUND)
         list(APPEND PCL_${COMPONENT}_INCLUDE_DIRS ${PCL_${INT_DEP}_INCLUDE_DIRS})
         if(PCL_${INT_DEP}_LIBRARIES)
-          list(APPEND PCL_${COMPONENT}_LIBRARIES "${PCL_${INT_DEP}_LIBRARIES}")
+          list(APPEND PCL_${COMPONENT}_LINK_LIBRARIES "${PCL_${INT_DEP}_LIBRARIES}")
         endif(PCL_${INT_DEP}_LIBRARIES) 
       endif(PCL_${INT_DEP}_FOUND)
     endforeach(int_dep)
+    if(_is_header_only EQUAL -1)
+      add_library(${pcl_component} @PCL_LIB_TYPE@ IMPORTED GLOBAL)
+      if(PCL_${COMPONENT}_LIBRARY_DEBUG)
+        set_target_properties(${pcl_component}
+          PROPERTIES
+            IMPORTED_CONFIGURATIONS "RELEASE;DEBUG"
+            IMPORTED_LOCATION_RELEASE "${PCL_${COMPONENT}_LIBRARY}"
+            IMPORTED_LOCATION_DEBUG "${PCL_${COMPONENT}_LIBRARY_DEBUG}"
+        )
+      else()
+        set_target_properties(${pcl_component}
+          PROPERTIES
+            IMPORTED_LOCATION "${PCL_${COMPONENT}_LIBRARY}"
+        )
+      endif()
+      foreach(def ${PCL_DEFINITIONS})
+        string(REPLACE " " ";" def2 ${def})
+        string(REGEX REPLACE "^-D" "" def3 "${def2}")
+        list(APPEND definitions ${def3})
+      endforeach()
+      set_target_properties(${pcl_component}
+        PROPERTIES
+          INTERFACE_COMPILE_DEFINITIONS "${definitions}"
+          INTERFACE_COMPILE_OPTIONS "${PCL_COMPILE_OPTIONS}"
+          INTERFACE_INCLUDE_DIRECTORIES "${PCL_${COMPONENT}_INCLUDE_DIRS}"
+          INTERFACE_LINK_LIBRARIES "${PCL_${COMPONENT}_LINK_LIBRARIES}"
+      )
+      set(PCL_${COMPONENT}_LIBRARIES ${pcl_component})
+    endif()
   endif(PCL_${COMPONENT}_FOUND)
 endforeach(component)

Haven't tested it yet, my laptop needs ages to build PCL.

[SergioRAgostinho]

Applied the patch

CMake Error at /home/sergio/Development/3rdparty/pcl/install/share/pcl-1.8/PCLConfig.cmake:609 (set_target_properties):
  Property INTERFACE_LINK_LIBRARIES may not contain link-type keyword
  "optimized".  The INTERFACE_LINK_LIBRARIES property may contain
  configuration-sensitive generator-expressions which may be used to specify
  per-configuration rules.
Call Stack (most recent call first):
  CMakeLists.txt:4 (find_package)

Edit: Here's the content of for octree for instance

-- PCL_OCTREE_LIBRARIES: optimized;/home/sergio/Development/3rdparty/pcl/install/lib/libpcl_octree.so;debug;/home/sergio/Development/3rdparty/pcl/install/lib/libpcl_octree.so;pcl_common

[taketwo]

Updated the diff. Should be fixed now; works on my machine. How this will behave on Windows I am not sure though.

[SergioRAgostinho]

👍 Looking good. The SSE compiler definitions options are not being propagated though.

/usr/bin/c++   -Dqh_QHpointer -isystem /usr/include/vtk-5.10 -isystem /usr/include/eigen3 -isystem /home/sergio/Development/3rdparty/pcl/install/include/pcl-1.8 -isystem /usr/include/ni -isystem /usr/include/openni2  -Wno-deprecated -g   -o CMakeFiles/gicp_double_free.dir/gicp_double_free.cpp.o -c /home/sergio/Development/pcl_test_cases/gicp_double_free/gicp_double_free.cpp

[taketwo]

This is how I set definitions/options:

+          COMPILE_DEFINITIONS "${PCL_${COMPONENT}_DEFINITIONS}"
+          COMPILE_OPTIONS ""

I suspect component definitions are actually empty. So just apply PCL_DEFINITIONS. (Updated the diff again).

[SergioRAgostinho]

I tried to set the compiler options in COMPILE_OPTIONS by passing the PCL_COMPILE_OPTIONS list but it is not cascading down.

[taketwo]

You are applying this diff on top of your pull request, right?

[SergioRAgostinho]

Yup. I'm even printing the contents of PCL_COMPILE_OPTIONS

[taketwo]

One more try, prefixed property names with INTERFACE_

[SergioRAgostinho]

We're getting somewhere but we need to extract that -D prefix

[ 50%] Building CXX object CMakeFiles/gicp_double_free.dir/gicp_double_free.cpp.o
/usr/bin/c++ -D-DDISABLE_DAVIDSDK -D-DDISABLE_DSSDK -D-DDISABLE_ENSENSO -D-DDISABLE_LIBUSB_1_0 -D-DDISABLE_PCAP -D-DDISABLE_PNG -Dqh_QHpointer -isystem /usr/include/vtk-5.10 -isystem /usr/include/eigen3 -isystem /home/sergio/Development/3rdparty/pcl/install/include/pcl-1.8 -isystem /usr/include/ni -isystem /usr/include/openni2 -Wno-deprecated -g -march=native -o CMakeFiles/gicp_double_free.dir/gicp_double_free.cpp.o -c /home/sergio/Development/pcl_test_cases/gicp_double_free/gicp_double_free.cpp

[taketwo]

Also, somebody suggested that options should be a list, not a string.

[SergioRAgostinho]

Yep. I confirmed that my sse flags are now passed the correctly but the definitions are messed up because they're being stored with "-D" prefix.

[taketwo]

That's a simple regex, wait a sec

[SergioRAgostinho]

We'll also need to define the pcl namespace to avoid target name collision with whoever works with our project downstream. Future people can solve that :)

[taketwo]

We'll also need to define the pcl namespace to avoid target name collision with whoever works with our project downstream.

Right now targets are prefixed, e.g. pcl_common, so collisions are very unlikely. If anyone creates such targets in his project, then it's... his fault :) But yeah, "the right way" will also have namespacing.

[taketwo]

Last try for today. (The filtering code will need to be factored into utility function later.)

[SergioRAgostinho]

It's working 👍 Gonna append your diff as your commit.

Super cool ending for this PR :D

[taketwo]

Is this on purpose that SSE became SSE3?

[SergioRAgostinho]

Bad copy paste.

[taketwo]

I think it's essential that these changes are tested on Windows before merging. @UnaNancyOwen can you give it a try? Important is to test if after building/installing PCL dependent projects can successfully link against. Further, switching between Debug/Release builds should be possible.

[UnaNancyOwen]

@taketwo Yes, I'm going to try this tonight.

[SergioRAgostinho]

Should be good now.

[taketwo]

I tried to build/install with and without PCL_ENABLE_SSE, as well as in "debian" mode (passing compiler flags explicitly). All works fine and fixes segfauls like reported in #2013.

From my side it's ready.

[taketwo]

I updated the PR description to make clear that only target_link_libraries is needed from now on.

[SergioRAgostinho]

We just fixed our Windows CI because of this one 🎉

[claudiofantacci]

What if we merge and we wait until windows users complain and help debug for a solution? When things work users are silent nothing changes. If there's a problem, users complain and they are more likely to help in reaching a solution. 🙄 😇

Here we are 🤣

I just came through the fact that with the following line
https://github.com/PointCloudLibrary/pcl/pull/2100/files#diff-af3b638bc2a3e6c650974192a53c7291R110
under Windows 10 and by using vcpkg toolchain, CMAKE_CXX_FLAGS is never empty and SSE options are never tested/enabled.

Is this still something that need to be addressed under Windows, or am I having an unexpected behaviour 😄?

[taketwo]

Which flags does vcpkg pass?

The currently implemented logic is as follows: if CMAKE_CXX_FLAGS are not set, then add some flags that PCL thinks are necessary (including architecture and SSE flags). Conversely, if CMAKE_CXX_FLAGS is set explicitly by the user, then don't touch everything, assume the user knows exactly what he wants.

This logic existed before this PR. Only thing it changed is fixed a potential nasty bug associated with it.

In general, the fact that vcpkg explicitly sets flags warrants a revision of the logic. Perhaps CMAKE_CXX_FLAGS should rather be treated as extra flags that are to be appended to all other flags set by PCL.

[claudiofantacci]

By default I have /DWIN32 /D_WINDOWS /W3 /GR /EHsc just before the if() statement preceding SSE macro. I'm trying to undesrtand whether this is due to vcpkg or by default (at least on my system).

Update: to be precise, those flags are set as soon as project() function is called, i.e. as soon as CMake checks my system for the compilare etc.

[taketwo]

I'm trying to undesrtand whether this is due to vcpkg or by default (at least on my system).

This would be very interesting to know indeed.

Edit: according to this comment these are defaults. So vcpkg is not passing any additional flags. Therefore, we have a flaw in our logic.

[claudiofantacci]

I'm trying to undesrtand whether this is due to vcpkg or by default (at least on my system).

This would be very interesting to know indeed.

Edit: according to this comment these are defaults. So vcpkg is not passing any additional flags. Therefore, we have a flaw in our logic.

You anticipated me 😁
This will give a great boost to programs under Windows.
I had some doubts becuase PCL-based programs were slower under new generation Windows-based machines compared to older generation Linux/macOS-based machines.
Thanks for the PR 👍