Xenial build broken

[gavanderhoorn]

The merge of #20 broke the build on Xenial:

Errors     << opw_kinematics:cmake /home/user/catkin_ws/logs/opw_kinematics/build.cmake.000.log
CMake Error at /home/user/catkin_ws/src/opw_kinematics/CMakeLists.txt:17 (set_target_properties):
  INTERFACE_LIBRARY targets may only have whitelisted properties.  The
  property "CXX_STANDARD" is not allowed.

CMake Error at /home/user/catkin_ws/src/opw_kinematics/CMakeLists.txt:17 (set_target_properties):
  INTERFACE_LIBRARY targets may only have whitelisted properties.  The
  property "CXX_STANDARD_REQUIRED" is not allowed.

[gavanderhoorn]

@Levi-Armstrong: you must've tested the work-around you proposed in 62124036, did you not run into this?

[haudren]

Moreover, testing on Bionic (ROS Melodic), gives us errors:

CMake Error at /opt/ros/melodic/share/catkin/cmake/catkinConfig.cmake:83 (find_package):
 Could not find a package configuration file provided by "opw_kinematics"
 with any of the following names:

Seems to me like the cmake configuration files are not properly installed: they should go in share/${PROJECT_NAME}/*.cmake, not in lib/things.cmake.

[haudren]

Note that this underlines the importance of using some form of CI, because those issues are easily hidden by old CMake config files left behind. Seems like catkin does not always clean them up properly.

[gavanderhoorn]

We already know that CI should be setup.

See #22 (and #19).

[gavanderhoorn]

Also note: this package is not released, so there are no guarantees about API or ABI stability. Things like this can and will happen.

For now I'll revert #20. I expect @Levi-Armstrong to take a look at this later today and then we can address the current issues.

[gavanderhoorn]

because those issues are easily hidden by old CMake config files left behind. Seems like catkin does not always clean them up properly.

catkin clean -y completely deletes build, devel and install directories. I'd be surprised if anything is left behind after that.

[gavanderhoorn]

Reverted #20 in #24.

[haudren]

No worries about ABI / API change, I understand that this is still fairly experimental. I actually am working on a fix, should be fairly straightforward for us to rely on a pure CMake package rather than through catkin (cause of our issues above).

Thanks for the quick action :)

[gavanderhoorn]

should be fairly straightforward for us to rely on a pure CMake package rather than through catkin (cause of our issues above).

If you're referring to caching of settings and CMake seemingly ignoring updated CMakeLists.txt then I believe nothing will change: the problem there would be CMake, not Catkin.

[haudren]

No sorry I was referring to issues we encounter on our Melodic builds. They still referred to opw_kinematics in CATKIN_DEPENDS.

[haudren]

So I can confirm that the following patch fixes the issue:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 75ad3e2..a1c95b0 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -14,7 +14,7 @@ target_link_libraries(${PROJECT_NAME} INTERFACE)
 target_compile_options(${PROJECT_NAME} INTERFACE -Wall -Wextra)

 if(CXX_FEATURE_FOUND EQUAL "-1")
-    set_target_properties(${PROJECT_NAME} PROPERTIES CXX_STANDARD 11 CXX_STANDARD_REQUIRED YES)
+    set(CMAKE_CXX_STANDARD 11)
 else()
     target_compile_features(${PROJECT_NAME} INTERFACE cxx_std_11)
 endif()
(END)

Obviously using CMAKE_CXX_STANDARD might not be entirely ideal, but I'm not sure how to handle this issue.

Furthermore, for people who are switching to this:

• You need to remove any mention of opw_kinematics from the calls to find_package catkin COMPONENTS ... and catkin_package( CATKIN_DEPENDS ...)
• Use instead find_package(opw_kinematics)
• Add the opw_kinematics_INCLUDE_DIR to your include_directories.
• I also needed to find and add include dirs for Eigen3.

[gavanderhoorn]

The problem here is that the need for C++11 needs to be exported to consumers of this library.

set(CMAKE_CXX_STANDARD ..) doesn't do that AFAIK.

The same issue affects the Eigen headers. That is what the interface library was for.

[haudren]

I understand that, that's why I'm not PRing it, it's more of a bandaid fix :(

Morevoer I'd like to add that if possible, we should wrap the exported targets in a namespace, that allows for easier diagnosing of issues: CMake will crash if a namespaced target is not found in target_link_libraries but will keep going if it's not and add the library to the -l flags.

[Levi-Armstrong]

Ideas on what namespace should be used?

[haudren]

I've seen a lot of people just repeat the project name. So opw_kinematics:: as a namespace sounds fine. We can then depend on opw_kinematics::opw_kinematics.

[gavanderhoorn]

Fixed in #25.