Consider making CMake variables case consistent

[jslee02]

The CMake config file for octomap is octomap-config.cmake. That means we find octomap in CMake using lower case name as find_package(octomap), and this defines octomap_FOUND in success. However, octomap-config.cmake defines other CMake variables using upper cases like OCTOMAP_INCLUDE_DIRS and OCTOMAP_LIBRARIES. So the usage would be like:

find_package(octomap)
if(octomap_FOUND)
  include_directories(${OCTOMAP_INCLUDE_DIRS})
  link_directories(${OCTOMAP_LIBRARY_DIRS})
endif()

It would be good to make them follow one consistent convention.

[ahornung]

Yes, that inconsistency can be found in many common libraries and partly the inconsistent style in CMake itself is to blame (e.g. https://cmake.org/cmake/help/v3.0/module/FindOpenMP.html / https://cmake.org/cmake/help/v3.0/module/FindBullet.html / https://cmake.org/cmake/help/v3.0/module/FindQt4.html), even the PackaceConfigHelpers docs use Foo_FOUND and FOO_INCLUDE_DIR.

octomap_FOUND is set according to the lowercase package name (the filename of the config file is irrelevant), so I think it's best to stick to that and provide ${octomap_INCLUDE_DIRS} etc in addition (like Boost does).

I'm not sure if manually setting OCTOMAP_FOUND is a good idea, but that's just a gut feeling.

[jslee02]

I think it's best to stick to that and provide ${octomap_INCLUDE_DIRS} etc in addition (like Boost does).

👍 to using the lowercase package name for all the variables.

I'm not sure if manually setting OCTOMAP_FOUND is a good idea

I agree with you. Having both of octomap_FOUND and OCTOMAP_FOUND would confuse people.

[jamiesnape]

octomap_FOUND is set according to the lowercase package name (the filename of the config file is irrelevant)

Partially relevant. Package names are not well defined in CMake. The key is the lowercase config file name. CMake looks for a case sensitive match XyzConfig.cmake and an all lowercase match xyz-config.cmake for the filename for a given call to find_package(Xyz). Therefore, find_package(OctoMap) and find_package(Octomap) would also resolve and would define OctoMap_FOUND and Octomap_FOUND rather than octomap_FOUND.

[ahornung]

Wow, that's quite a mess!

[ahornung]

Is there still something to take care of with this issue? If so, then please send a PR. I'm preparing a new release and now would be a good time to change things (backwards-compatible, if possible).

[jslee02]

My suggestion is using XYZConfig.cmake rather than xyz-config.cmake because of the point @jamiesnape made (xyz-config.cmake is resolved by any case cases such as find_package(XYZ), find_package(Xyz), find_package(xyz), which is confusing).

In short, it would be good to use one of

# (1) OCTOMAPConfig.cmake
find_package(OCTOMAP)
if(OCTOMAP_FOUND)
  include_directories(${OCTOMAP_INCLUDE_DIRS})
  link_directories(${OCTOMAP_LIBRARY_DIRS})
endif()

or

# (2) octomapConfig.cmake
find_package(octomap)
if(octomap_FOUND)
  include_directories(${octomap_INCLUDE_DIRS})
  link_directories(${octomap_LIBRARY_DIRS})
endif()

or

# (3) OctomapConfig.cmake
find_package(Octomap)
if(Octomap_FOUND)
  include_directories(${Octomap_INCLUDE_DIRS})
  link_directories(${Octomap_LIBRARY_DIRS})
endif()

or

# (4) OctoMapConfig.cmake
find_package(OctoMap)
if(OctoMap_FOUND)
  include_directories(${OctoMap_INCLUDE_DIRS})
  link_directories(${OctoMap_LIBRARY_DIRS})
endif()

Please note that this is a breaking change; Users need to change either find_package(...) or the octomap variables (e.g., <octomap>_INCLUDE_DIR) or both. So I prefer (1) since it only requires to change find_package(octomap) to find_package(OCTOMAP).

This issue is not a bug so it's totally up to you @ahornung . Personally, I might be happy with #140. If one of the options I suggest also makes sense to you, then I'll create an PR for it. It's totally fine even if you decline this change.

[jslee02]

Oh, wait. Let me take back my words. We don't need to change anything but the comment in octomap-config.cmake.in from FIND_PACKAGE(octomap REQUIRED ) to FIND_PACKAGE(OCTOMAP REQUIRED ) since it works and will eliminate the confusion.

[ahornung]

Now, that's an easy fix :)

[jslee02]

Thanks for the change!