Use pkg-config to find Flann

[jspricke]

No description provided.

[jspricke]

Hey guys, this is a change I recently introduced in Debian, some details are covered here:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=911022#14
Would be great to have it in the 1.9 release (#2399) to make sure PCL compiles on future Debian and Ubuntu distros. If someone could test this on other systems, that would be great.

P.S.: I just merged this from the simpler patch I did for Debian and the old version, will hopefully have time tomorrow to actually test it. Just wanted to hand it in so you are aware of it already. Feel free to fix my bugs ;).

[claudiofantacci]

Thanks for the PR, I can try to test this under Windows with vcpkg in the next days 👍

[taketwo]

Ubuntu 16.04 is fine, I can configure/build a downstream project that uses PCL/KdTrees.

[SergioRAgostinho]

Confirmed the same on OS X.

[taketwo]

I'm taking back my words, it's not fine, more details later.

[taketwo]

In my case pkgconfig succeeds in find the library, so commands in the if(NOT FLANN_FOUND) branch are never executed. This means that FLANN_INCLUDE_DIRS variable is not set. Because of that, PCL thinks that FLANN is missing and does not build any of the modules that depend on FLANN (including transitive dependencies). You can observe this in the Travis build, which succeeded in 10 minutes since it did not need to build much.

In the case when pkgconfig succeeds a variable called FLANN_INCLUDEDIR is set (at least on Ubuntu 16.04). So we need to assign FLANN_INCLUDE_DIRS to it.

[SergioRAgostinho]

This behavior definitely needs to be patched. The find_package_handle_standard_args needs to move outside the conditional branch.

[jspricke]

@taketwo I added a workaround the the FLANN_FOUND. The FLANN_INCLUDE_DIRS should be set by pkg_check_modules (It's probably empty on Ubuntu). Can you try?
@SergioRAgostinho find_package_handle_standard_args is not needed.

[SergioRAgostinho]

My comment was based on a practice which goes as following: when something doesn't fail and it should have, you first write code to ensure it fails and then you prevent the failure from happening.

Furthermore this patch 12dc5a2 will appear slightly cryptic to someone who just sees this file. FLANN_FOUND is already being implicitly evaluated as TRUE in the previous line. I tried to look for other occurrences of FLANN_FOUND in the repo but saw no explicit comparison of the type

FLANN_FOUND STREQUAL TRUE

[jspricke]

It's one of those here: https://github.com/PointCloudLibrary/pcl/blob/master/cmake/pcl_targets.cmake#L113

[taketwo]

There was no problem with FLANN_FOUND variable. The problem is that FLANN_INCLUDE_DIRS is not set by pkg_check_modules on Ubuntu 16.04. Instead, a variable called FLANN_INCLUDEDIR is set.

[jspricke]

I tested it on Ubuntu and it works.
Read here if you want to know more about FLANN_INCLUDE_DIRS https://cmake.org/cmake/help/v3.5/module/FindPkgConfig.html (hint, it's all fine).

[taketwo]

Hm, you are right, it indeed works after your latest commit. Nevertheless, I don't quite get the situation with the include directory. Here is the relevant CMake output:

-- Checking for module 'flann>=1.7.0'
--   Found flann, version 1.8.4
-- FLANN found (include: , lib: flann_cpp)

As you can see, include directory is missing because FLANN_INCLUDE_DIRS is not set. On a system where FLANN is installed system-wide that's okay since /usr/include is anyway included. But what if FLANN is installed in an unconventional location?

[jspricke]

In other cases it should be set in the pkg-config file and pkg_check_modules should pick it up ;):

[taketwo]

Is this still WIP, do you plan to change anything else?

[jspricke]

@taketwo it's fine with me (if it works for everyone). I would love to get rid of the FLANN_FOUND TRUE thing, but I fear that would change too much of our cmake internals this late in the release process.
Thanks everyone for helping with getting this into the release.

[claudiofantacci]

It works under Windows 10 with vcpkg:

FLANN found (include: C:/Users/cfantacci/vcpkg/installed/x64-windows/include, lib: optimized;C:/Users/cfantacci/vcpkg/installed/x64-windows/lib/flann_cpp.lib;debug;C:/Users/cfantacci/vcpkg/installed/x64-windows/debug/lib/flann_cpp-gd.lib)

[taketwo]

@jspricke Actually, do you see any reason for this test you referenced:

pcl/cmake/pcl_targets.cmake

Line 113 in a602b19

if(NOT ${EXT_DEP_FOUND} OR (NOT (${EXT_DEP_FOUND} STREQUAL "TRUE")))

Why isn't the first part of the condition sufficient? Looking at the Git history, this was added as a single commit and does not provide any motivation. Maybe we just drop it? Or better after the release?

[claudiofantacci]

@taketwo, my two cents here: there are part of CMake codes where some variables are set/evaluated ON/OFF (Bool), other where they are considered as literals TRUE/FALSE (string). My guess is that this may depends on the package configurations file. It may be risky to change this.

[SergioRAgostinho]

I'm pro getting rid of that second condition after the release. One more task for the CMake rework.

[jspricke]

I looked into this a little more and I think OR should be AND:

-            if(NOT ${EXT_DEP_FOUND} OR (NOT (${EXT_DEP_FOUND} STREQUAL "TRUE")))
+            if(NOT ${EXT_DEP_FOUND} AND (NOT (${EXT_DEP_FOUND} STREQUAL "TRUE")))

[jspricke]

I updated the logic, please squash before merging.

[taketwo]

You forgot to remove this comment.

[jspricke]

Updated, thx.

[SergioRAgostinho]

My output on OS X also seems to be fine. 👍

-- Checking for module 'flann>=1.7.0'
--   Found flann, version 1.9.1
-- FLANN found (include: /usr/local/Cellar/flann/1.9.1_3/include, lib: flann;flann_cpp)