Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Attempt to find octomap using find_package #182

Merged
merged 2 commits into from
Sep 28, 2016
Merged

Attempt to find octomap using find_package #182

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af0a215
Attempt to find octomap using find_package
Sep 27, 2016
83aad77
Add comments and fall back to pkg-config if OCTOMAP_VERSION is not set
Sep 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Copy link
Member

@jslee02 jslee02 Sep 28, 2016 •
edited
Loading

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) .

Copy link
Contributor Author

@jamiesnape jamiesnape Sep 28, 2016 •
edited
Loading

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

Copy link
Member

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.

Copy link
Contributor Author

@jamiesnape jamiesnape Sep 28, 2016 •
edited
Loading

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?

Copy link
Member

@jslee02 jslee02 Sep 28, 2016 •
edited
Loading

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.

string(REPLACE "." ";" VERSION_LIST "${OCTOMAP_VERSION}")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that without OCTOMAP_VERSION there is currently an error here in due to the lack of quotes around ${OCTOMAP_VERSION}

Copy link
Member

@jslee02 jslee02 Sep 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 OCTOMAP_VERSION guard.

Copy link
Contributor Author

@jamiesnape jamiesnape Sep 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If OCTOMAP_VERSION is not defined, I guess I could still call pkg-config, which at least fixes that scenario. Sound good?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For platforms with pkg-config the previous behavior sound now be preserved with the current state of octomap.

Copy link
Member

@jslee02 jslee02 Sep 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The 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)
Expand Down