rosidl generator cpp depreceation warning failing builds when using const values in msg definitions

SteveMacenski commented 1 year ago

Bug report

Required Info:

Operating System: 22.04
Installation type: Binary
Version or commit hash: Iron testing, Rolling
DDS implementation: All
Client library (if applicable): N/A

Steps to reproduce issue

Build Nav2 (nav2_msgs will get you there) or other packages containing consts in the message definitions

colcon build --packages-select nav2_msgs nav2_common

Expected behavior

Builds without warnings

Actual behavior

--- stderr: nav2_msgs                                           
In file included from /home/steve/Documents/navigation2_ws/build/nav2_msgs/rosidl_typesupport_fastrtps_cpp/nav2_msgs/msg/detail/collision_monitor_state__rosidl_typesupport_fastrtps_cpp.hpp:11,
                 from /home/steve/Documents/navigation2_ws/build/nav2_msgs/rosidl_typesupport_fastrtps_cpp/nav2_msgs/msg/detail/dds_fastrtps/collision_monitor_state__type_support.cpp:4:
/home/steve/Documents/navigation2_ws/build/nav2_msgs/rosidl_generator_cpp/nav2_msgs/msg/detail/collision_monitor_state__struct.hpp:149:19: warning: redundant redeclaration of ‘constexpr’ static data member ‘nav2_msgs::msg::CollisionMonitorState_<ContainerAllocator>::DO_NOTHING’ [-Wdeprecated]
  149 | constexpr uint8_t CollisionMonitorState_<ContainerAllocator>::DO_NOTHING;
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/steve/Documents/navigation2_ws/build/nav2_msgs/rosidl_generator_cpp/nav2_msgs/msg/detail/collision_monitor_state__struct.hpp:80:28: note: previous declaration of ‘nav2_msgs::msg::CollisionMonitorState_<ContainerAllocator>::DO_NOTHING’
   80 |   static constexpr uint8_t DO_NOTHING =
      |                            ^~~~~~~~~~
/home/steve/Documents/navigation2_ws/build/nav2_msgs/rosidl_generator_cpp/nav2_msgs/msg/detail/collision_monitor_state__struct.hpp:151:19: warning: redundant redeclaration of ‘constexpr’ static data member ‘nav2_msgs::msg::CollisionMonitorState_<ContainerAllocator>::STOP’ [-Wdeprecated]
  151 | constexpr uint8_t CollisionMonitorState_<ContainerAllocator>::STOP;
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/steve/Documents/navigation2_ws/build/nav2_msgs/rosidl_generator_cpp/nav2_msgs/msg/detail/collision_monitor_state__struct.hpp:82:28: note: previous declaration of ‘nav2_msgs::msg::CollisionMonitorState_<ContainerAllocator>::STOP’
   82 |   static constexpr uint8_t STOP =
      |                            ^~~~
/home/steve/Documents/navigation2_ws/build/nav2_msgs/rosidl_generator_cpp/nav2_msgs/msg/detail/collision_monitor_state__struct.hpp:153:19: warning: redundant redeclaration of ‘constexpr’ static data member ‘nav2_msgs::msg::CollisionMonitorState_<ContainerAllocator>::SLOWDOWN’ [-Wdeprecated]
  153 | constexpr uint8_t CollisionMonitorState_<ContainerAllocator>::SLOWDOWN;
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/steve/Documents/navigation2_ws/build/nav2_msgs/rosidl_generator_cpp/nav2_msgs/msg/detail/collision_monitor_state__struct.hpp:84:28: note: previous declaration of ‘nav2_msgs::msg::CollisionMonitorState_<ContainerAllocator>::SLOWDOWN’
   84 |   static constexpr uint8_t SLOWDOWN =
      |                            ^~~~~~~~

And so on

Additional information

Using export CXXFLAGS="-Wno-error=deprecated" allows it to build without failure, but still with a seemingly serious warning

clalancette commented 1 year ago

This is similar to, but maybe not 100% the same as https://github.com/ros2/rosidl_typesupport_fastrtps/issues/28 (particularly the comment in https://github.com/ros2/rosidl_typesupport_fastrtps/issues/28#issuecomment-1511610906). I'll put it in the list to look at for Iron, but we may or may not fix it depending on how dangerous the fix ends up being.

emersonknapp commented 1 year ago

Note - I am currently investigating this as well

emersonknapp commented 1 year ago

This is not the same issue as ros2/rosidl_typesupport_fastrtps#28

Conclusions: At some point between 4 months -> 1 year ago, various dependencies of C++ rosidl message code generation upgraded from C++14 -> C++17. Unfortunately I found it difficult to point to exactly which change tipped things over so that e.g. nav2_msgs__rosidl_typesupport_fastrtps_cpp code generation started using -std=gnu++17, but that change in compile options is what made these warnings start appearing.

The reason the ROS 2 core message packages don't surface the warning is because they all declare -Wall -Wextra -Wpedantic, but NOT -Wdeprecated, which nav2_msgs does define. nav2_msgs should in fact also have been seeing this warning in the C++ targets rosidl_typesupport_cpp and rosidl_typesupport_introspection_cpp, but due to a CMake bug in those two, it was only surfacing in rosidl_typesupport_fastrtps_cpp.

The warning itself is due to a new semantic in C++17 that deprecated necessary code for C++14. This is for static constexpr member variables.

(until C++17) If a constexpr static data member is odr-used, a definition at namespace scope is still required, but it cannot have an initializer.

(since C++17) A constexpr static data member is implicitly inline and does not need to be redeclared at namespace scope. This redeclaration without an initializer (formerly required) is still permitted, but is deprecated.

(source https://en.cppreference.com/w/cpp/language/static#Constant_static_members)

In conclusion, I believe the correct fix is an ifdef check for C++ version around the redundant declaration. I also believe this change will be very safe to backport to Iron and Humble.

I have created https://github.com/ros2/rosidl/pull/750 and https://github.com/ros2/rosidl_typesupport/pull/145 to fix both the noted issues

emersonknapp commented 1 year ago

Completed in rolling, but I will want to backport all 3 PRs to Iron, Humble if possible. I don't have permissions for mergify bot on those repos @clalancette

ros2 / rosidl