-
Notifications
You must be signed in to change notification settings - Fork 417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attempt to find octomap using find_package #182
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,10 +108,16 @@ 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) | ||
# octomap-config.cmake may not define OCTOMAP_VERSION so fall back to | ||
# pkgconfig | ||
if(NOT OCTOMAP_VERSION AND PKG_CONFIG_FOUND) | ||
pkg_check_modules(OCTOMAP QUIET octomap) | ||
endif() | ||
if(NOT OCTOMAP_FOUND) | ||
# whether octomap_FOUND and/or OCTOMAP_FOUND are set is inconsistent, but | ||
# OCTOMAP_INCLUDE_DIRS and OCTOMAP_LIBRARY_DIRS should always be set when | ||
# octomap is found | ||
if(NOT OCTOMAP_INCLUDE_DIRS AND NOT OCTOMAP_LIBRARY_DIRS) | ||
# if pkgconfig is not installed, then fall back on more fragile detection | ||
# of octomap | ||
find_path(OCTOMAP_INCLUDE_DIRS octomap.h | ||
|
@@ -122,11 +128,13 @@ if(FCL_WITH_OCTOMAP) | |
set(OCTOMAP_LIBRARIES "octomap;octomath") | ||
endif() | ||
endif() | ||
if (OCTOMAP_FOUND OR (OCTOMAP_INCLUDE_DIRS AND OCTOMAP_LIBRARY_DIRS)) | ||
string(REPLACE "." ";" VERSION_LIST ${OCTOMAP_VERSION}) | ||
list(GET VERSION_LIST 0 OCTOMAP_MAJOR_VERSION) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
string(REPLACE "." ";" VERSION_LIST "${OCTOMAP_VERSION}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern was the case that octomap is installed and found by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The configure will error out before that stage, so you need quoting and/or an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For platforms with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good! Thanks. |
||
list(GET VERSION_LIST 0 OCTOMAP_MAJOR_VERSION) | ||
list(GET VERSION_LIST 1 OCTOMAP_MINOR_VERSION) | ||
list(GET VERSION_LIST 2 OCTOMAP_PATCH_VERSION) | ||
endif() | ||
include_directories(${OCTOMAP_INCLUDE_DIRS}) | ||
link_directories(${OCTOMAP_LIBRARY_DIRS}) | ||
set(FCL_HAVE_OCTOMAP 1) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current second (non-pkg-config) way of finding octomap does not define
OCTOMAP_VERSION
. What do you currently do there?