Express Humble/Rolling differences in #if

[tylerjw]

• Express Humble/Rolling differences in #if
• Remove commented out line of code

Description

Instead of disabling the warning for deprecations in CMake, I looked into a way to support both Humble and Rolling in main so we can continue to get the warnings that will help us stay up to date with Rolling.

I found this ROS Answers post about how to write an #if pre-processor statement for detecting the ROS version: https://answers.ros.org/question/9562/how-do-i-test-the-ros-version-in-c-code/

That lead me to this ament package that is used by rclcpp to create a variable based on its version here: https://github.com/ament/ament_cmake/tree/rolling/ament_cmake_gen_version_h/cmake

The code in that generated file has changed over time and isn't exactly what is in the first ROS Answers post and on my machine is this:

/// \def RCLCPP_VERSION_MAJOR
/// Defines RCLCPP major version number
#define RCLCPP_VERSION_MAJOR (17)

/// \def RCLCPP_VERSION_MINOR
/// Defines RCLCPP minor version number
#define RCLCPP_VERSION_MINOR (0)

/// \def RCLCPP_VERSION_PATCH
/// Defines RCLCPP version patch number
#define RCLCPP_VERSION_PATCH (0)

/// \def RCLCPP_VERSION_STR
/// Defines RCLCPP version string
#define RCLCPP_VERSION_STR "17.0.0"

/// \def RCLCPP_VERSION_GTE
/// Defines a macro to check whether the version of RCLCPP is greater than or equal to
/// the given version triple.
#define RCLCPP_VERSION_GTE(major, minor, patch) ( \
     (major < RCLCPP_VERSION_MAJOR) ? true \
     : (major > RCLCPP_VERSION_MAJOR) ? false \
     : (minor < RCLCPP_VERSION_MINOR) ? true \
     : (minor > RCLCPP_VERSION_MINOR) ? false \
     : (patch < RCLCPP_VERSION_PATCH) ? true \
     : (patch > RCLCPP_VERSION_PATCH) ? false \
     : true)

From this, I was able to update the code to support both Rolling and Humble by detecting the version of the function calls.

[codecov]

Codecov Report

Base: 51.08% // Head: 51.08% // No change to project coverage 👍

Coverage data is based on head (c6b7c86) compared to base (a8326ac).
Patch coverage: 57.50% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1620   +/-   ##
=======================================
  Coverage   51.08%   51.08%           
=======================================
  Files         381      381           
  Lines       31738    31738           
=======================================
  Hits        16209    16209           
  Misses      15529    15529           

Impacted Files	Coverage Δ
...ance_field/src/collision_common_distance_field.cpp	25.87% <16.67%> (ø)
...tance_field/src/collision_distance_field_types.cpp	35.65% <51.86%> (ø)
...istance_field/src/collision_env_distance_field.cpp	54.26% <58.98%> (ø)
...e/collision_detection_fcl/src/collision_common.cpp	73.76% <100.00%> (ø)
.../collision_detection_fcl/src/collision_env_fcl.cpp	91.52% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[henningkayser]

Thanks for the attempt to clean this up. What do you think of moving the implementation detail into a different header and only using the if statement as include guard. The header could provide something like a function rclcpp_compat::SystemDefaultsQoS(), returning the right value for each version.

[tylerjw]

Thanks for the attempt to clean this up. What do you think of moving the implementation detail into a different header and only using the if statement as include guard. The header could provide something like a function rclcpp_compat::SystemDefaultsQoS(), returning the right value for each version.

They are not the same type, so you can't just convert them. Disabling the warning for deprecated has to be the saddest "solution" to this.

[Abishalini]

Pretty cool! Thanks for taking over this issue :)

[henningkayser]

Thanks for the attempt to clean this up. What do you think of moving the implementation detail into a different header and only using the if statement as include guard. The header could provide something like a function rclcpp_compat::SystemDefaultsQoS(), returning the right value for each version.

They are not the same type, so you can't just convert them. Disabling the warning for deprecated has to be the saddest "solution" to this.

Why would it need to be? I'm not talking about conversion, I'm talking about defining the same function name in different headers, one for each version. Instead of wrapping the whole code block, we would only have to add include guards to select the right header. The return type can and must be different, that's the whole point. This would separate the compile-time logic from code in a more elegant way, imo.

[tylerjw]

Why would it need to be? I'm not talking about conversion, I'm talking about defining the same function name in different headers, one for each version. Instead of wrapping the whole code block, we would only have to add include guards to select the right header. The return type can and must be different, that's the whole point. This would separate the compile-time logic from code in a more elegant way, IMO.

I don't understand what you are asking. Would you like to make that change? I'm frustrated by what was merged and am trying to fix this so we can re-enable the warning.

I am not looking to develop some more generic solution to something that only shows up in this one file. I wish the person who merged disabling the warning for deprecations would have done almost anything else to fix this. Nowhere else in MoveIt are we specifying a callback group for services.

I assume what you mean is to do something like this:

#if RCLCPP_VERSION_GTE(17, 0, 0)
#include <moveit/rolling_interface.hpp>
#else
#include <moveit/humble_interface.hpp>
#endif

Then we now have three files that need to be changed when we drop support for Humble in the future, along with some dead code. I like this solution much better because it localizes it to the one file where we are using it. Once it gets used in more than one place, then maybe we consider some sort of abstraction?

[henningkayser]

Why would it need to be? I'm not talking about conversion, I'm talking about defining the same function name in different headers, one for each version. Instead of wrapping the whole code block, we would only have to add include guards to select the right header. The return type can and must be different, that's the whole point. This would separate the compile-time logic from code in a more elegant way, IMO.

I don't understand what you are asking. Would you like to make that change? I'm frustrated by what was merged and am trying to fix this so we can re-enable the warning.

I am not looking to develop some more generic solution to something that only shows up in this one file. I wish the person who merged disabling the warning for deprecations would have done almost anything else to fix this. Nowhere else in MoveIt are we specifying a callback group for services.

I assume what you mean is to do something like this:

#if RCLCPP_VERSION_GTE(17, 0, 0)
#include <moveit/rolling_interface.hpp>
#else
#include <moveit/humble_interface.hpp>
#endif

Then we now have three files that need to be changed when we drop support for Humble in the future, along with some dead code. I like this solution much better because it localizes it to the one file where we are using it. Once it gets used in more than one place, then maybe we consider some sort of abstraction?

Valid point about the multiple files. I think what I dislike is that the guard wraps a lot more code than the affected type. The last change looks much better to me, thanks!

[tylerjw]

I see that ros2_control has the same deprecation warnings right now on their master branch. I'm going to push this into RSL so we can hide these sorts of shims there.

I'll make a follow on pr here to use the RSL shim for this once it is easy to use.