PCLConfig.cmake should be define QHULL_USE_STATIC and FLANN_USE_STATIC

[UnaNancyOwen]

New PCLConfig.cmake doesn't include settings of QHULL_USE_STATIC (that using in Modules/FindQHull.cmake) and FLANN_USE_STATIC (that using in Modules/FindFLANN.cmake).
Therefore, PCLConfig.cmake always find dynamic link library of QHull and FLANN when configure user own project.
I think that PCLConfig.cmake should be define QHULL_USE_STATIC and FLANN_USE_STATIC.
In generally, I think users should be use libraries that used build PCL from source code.

@SergioRAgostinho @taketwo What do you think about it?

[UnaNancyOwen]

I think that these options are necessary to define according to the build-time options.

 macro(find_qhull)
   if(PCL_ALL_IN_ONE_INSTALLER)
     set(QHULL_ROOT "${PCL_ROOT}/3rdParty/Qhull")
   elseif(NOT QHULL_ROOT)
     get_filename_component(QHULL_ROOT "@QHULL_INCLUDE_DIRS@" PATH)
   endif(PCL_ALL_IN_ONE_INSTALLER)
  
+  set(QHULL_USE_STATIC @QHULL_USE_STATIC@)
   find_package(Qhull)
 endmacro(find_qhull)

 macro(find_flann)
   if(PCL_ALL_IN_ONE_INSTALLER)
     set(FLANN_ROOT "${PCL_ROOT}/3rdParty/Flann")
   elseif(NOT FLANN_ROOT)
     get_filename_component(FLANN_ROOT "@FLANN_INCLUDE_DIRS@" PATH)
   endif(PCL_ALL_IN_ONE_INSTALLER)

+  set(FLANN_USE_STATIC @FLANN_USE_STATIC@)
   find_package(FLANN)
 endmacro(find_flann)

[taketwo]

Agree.

Speaking of static/dynamic switches, can you please explain PCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32 option? Why do we have this for FLANN, but not for QHull?

What about promoting xxx_USE_STATIC variables to proper CMake options that a user can toggle?

[SergioRAgostinho]

What about promoting xxx_USE_STATIC variables to proper CMake options that a user can toggle?

👍

[UnaNancyOwen]

PCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32 is an option to specify use dynamic link libray of FLANN for build PCL from source code when PCL_SHARED_LIBS is ON and on Windows. (In that case, PCL use static link library of FLANN on Windows by default.)
PCL has a similar option PCL_BUILD_WITH_BOOST_DYNAMIC_LINKING_WIN32.
I don't know the reason for not have a similar option for QHull.

What about promoting xxx_USE_STATIC variables to proper CMake options that a user can toggle?

👍

-  set(QHULL_USE_STATIC @QHULL_USE_STATIC@)
+  option(QHULL_USE_STATIC "Use Static Library of FLANN" @QHULL_USE_STATIC@)

-  set(FLANN_USE_STATIC @FLANN_USE_STATIC@)
+  option(FLANN_USE_STATIC "Use Static Library of QHull" @FLANN_USE_STATIC@)

[taketwo]

Oh, sorry, my proposal was misleading. I did not mean making xxx_USE_STATIC optional inside PCLConfig, I rather meant making it an option when configuring PCL before building. My thought was to replace PCL_BUILD_WITH_xxx_DYNAMIC_LINKING_WIN32. Because, why Windows only? Maybe people on Linux also want to be able to control this.

Now coming back to the situation in the PCLConfig file. Imagine PCL was built with static linking of FLANN. In this case we do not need to find and link FLANN in user project at all! It's already incorporated into PCL libraries.

[UnaNancyOwen]

Oh, sorry, my proposal was misleading. I did not mean making xxx_USE_STATIC optional inside PCLConfig, I rather meant making it an option when configuring PCL before building. My thought was to replace PCL_BUILD_WITH_xxx_DYNAMIC_LINKING_WIN32. Because, why Windows only? Maybe people on Linux also want to be able to control this.

I understood. Agree. 👍

Now coming back to the situation in the PCLConfig file. Imagine PCL was built with static linking of FLANN. In this case we do not need to find and link FLANN in user project at all! It's already incorporated into PCL libraries.

Boost, VTK(, and OpenNI2) always need to find libraries.
But, FLANN and QHull are not necessary when PCL static linked these libraries.
Currently, PCLConfig.cmake will always try to find dependency libraries (FLANN, QHull).
Should we fix this?

[taketwo]

But, FLANN and QHull are not necessary when PCL static linked these libraries.

Thinking about this more... we still need to find them to add their include directories, right?

Should we fix this?

I think your current PR is sufficient. In the case of static libraries we end up linking against them once again, but that does not hurt.

[SergioRAgostinho]

I think your current PR is sufficient. In the case of static libraries we end up linking against them once again, but that does not hurt.

Considering that we now have unified the find package scripts it would be complicated to address the "while building pcl" and the "while including pcl in your project" situations differently to prevent the libraries to be added.

During the static linking, the compiler only links what is required i.e., we don't risk bloated binaries if we have a second static linkage right?

[UnaNancyOwen]

Thinking about this more... we still need to find them to add their include directories, right?

OK, If include directories is necessary, It seems necessary to find those libraries.
I think this pull request is good to merge.

During the static linking, the compiler only links what is required i.e., we don't risk bloated binaries if we have a second static linkage right?

PCL that included in PCL All-in-one Installer is linked dependencies of static library (other than OpenNI2).
However, I have never received a report of issue caused by it.

[SergioRAgostinho]

Aren't you missing defining xxx_USE_STATIC as options during the build process?

[taketwo]

During the static linking, the compiler only links what is required i.e., we don't risk bloated binaries if we have a second static linkage right?

To the best of my knowledge, yes.

Aren't you missing defining xxx_USE_STATIC as options during the build process?

I can take care of this (and removing old _WIN32 options) in a separate PR.

[SergioRAgostinho]

Superseeded by #2089.