Attempt to find octomap using find_package #182
Conversation
Looks slightly awkward due to inconsistent casing of some variables.
| list(GET VERSION_LIST 1 OCTOMAP_MINOR_VERSION) | ||
| list(GET VERSION_LIST 2 OCTOMAP_PATCH_VERSION) | ||
| if(OCTOMAP_INCLUDE_DIRS AND OCTOMAP_LIBRARY_DIRS AND OCTOMAP_VERSION) | ||
| if(NOT OCTOMAP_MAJOR_VERSION AND NOT OCTOMAP_MINOR_VERSION AND NOT OCTOMAP_PATCH_VERSION) |
There was a problem hiding this comment.
I think the upstream change #137 should be merged first because we couldn't reach here without OCTOMAP_VERSION. Even it's merged this still wouldn't work if an octomap version without the change is installed.
Possible workaround I can think of now is we enforce the minimum required octomap version to 1.8.1 (or the minimum version that includes the version variable; we don't know which version is it yet, though) for CMake config mode like: find_package(octomap 1.8.1 QUIET) .
There was a problem hiding this comment.
What would you do here? Remove there and elsewhere the logic as redundant?
https://github.com/flexible-collision-library/fcl/blob/master/include/fcl/config.h.in#L59-L73
There was a problem hiding this comment.
The logic is still required. What I meant was that we enforce the minimum required octomap version only when we attempt to find octomap using find_package.
There was a problem hiding this comment.
The current second (non-pkg-config) way of finding octomap does not define OCTOMAP_VERSION. What do you currently do there?
| @@ -108,10 +108,11 @@ link_directories(${CCD_LIBRARY_DIRS}) | |||
| option(FCL_WITH_OCTOMAP "octomap library support" ON) | |||
| set(FCL_HAVE_OCTOMAP 0) | |||
| if(FCL_WITH_OCTOMAP) | |||
| if(PKG_CONFIG_FOUND) | |||
| find_package(octomap QUIET) | |||
| if(NOT OCTOMAP_INCLUDE_DIRS AND NOT OCTOMAP_LIBRARY_DIRS AND PKG_CONFIG_FOUND) | |||
There was a problem hiding this comment.
Wouldn't it be simpler to use octomap_FOUND here? Maybe you used OCTOMAP_INCLUDE_DIRS AND and OCTOMAP_LIBRARY_DIRS because of the case inconsistency? If so, it would be good to leave a short note here why we don't use XXX_FOUND for maintenance in the future. I think the case inconsistency should be fixed in the upstream, though.
There was a problem hiding this comment.
True, though this would allow you to specify OCTOMAP_INCLUDE_DIRS and OCTOMAP_LIBRARY_DIRS as cache variables if you are using an old version without the necessary config files.
| list(GET VERSION_LIST 2 OCTOMAP_PATCH_VERSION) | ||
| if(OCTOMAP_INCLUDE_DIRS AND OCTOMAP_LIBRARY_DIRS AND OCTOMAP_VERSION) | ||
| if(NOT OCTOMAP_MAJOR_VERSION AND NOT OCTOMAP_MINOR_VERSION AND NOT OCTOMAP_PATCH_VERSION) | ||
| string(REPLACE "." ";" VERSION_LIST "${OCTOMAP_VERSION}") |
There was a problem hiding this comment.
Note that without OCTOMAP_VERSION there is currently an error here in due to the lack of quotes around ${OCTOMAP_VERSION}
There was a problem hiding this comment.
My concern was the case that octomap is installed and found by find_package, but FCL doesn't use it only because OCTOMAP_VERSION is not defined (due to octomap-config.cmake doesn't define it).
There was a problem hiding this comment.
The configure will error out before that stage, so you need quoting and/or an OCTOMAP_VERSION guard.
There was a problem hiding this comment.
If OCTOMAP_VERSION is not defined, I guess I could still call pkg-config, which at least fixes that scenario. Sound good?
There was a problem hiding this comment.
For platforms with pkg-config the previous behavior sound now be preserved with the current state of octomap.
| list(GET VERSION_LIST 1 OCTOMAP_MINOR_VERSION) | ||
| list(GET VERSION_LIST 2 OCTOMAP_PATCH_VERSION) | ||
| if(OCTOMAP_INCLUDE_DIRS AND OCTOMAP_LIBRARY_DIRS AND OCTOMAP_VERSION) | ||
| if(NOT OCTOMAP_MAJOR_VERSION AND NOT OCTOMAP_MINOR_VERSION AND NOT OCTOMAP_PATCH_VERSION) |
There was a problem hiding this comment.
The logic is still required. What I meant was that we enforce the minimum required octomap version only when we attempt to find octomap using find_package.
| list(GET VERSION_LIST 2 OCTOMAP_PATCH_VERSION) | ||
| if(OCTOMAP_INCLUDE_DIRS AND OCTOMAP_LIBRARY_DIRS AND OCTOMAP_VERSION) | ||
| if(NOT OCTOMAP_MAJOR_VERSION AND NOT OCTOMAP_MINOR_VERSION AND NOT OCTOMAP_PATCH_VERSION) | ||
| string(REPLACE "." ";" VERSION_LIST "${OCTOMAP_VERSION}") |
There was a problem hiding this comment.
My concern was the case that octomap is installed and found by find_package, but FCL doesn't use it only because OCTOMAP_VERSION is not defined (due to octomap-config.cmake doesn't define it).
Looks slightly awkward due to inconsistent casing of some variables.