[humble] Bugfix for rosbag2_cpp serialization converter (backport #1814…

…) (backport #1823) (#1824) * [iron] Bugfix for rosbag2_cpp serialization converter (backport #1814) (#1823) * Bugfix for rosbag2_cpp serialization converter (#1814) * Bugfix for rosbag2 serialization converter - Use rmw specific type support for rmw_serilize{deserialize} function calls. Note: It is ok for CycloneDDS to use introspection type support for rmw_serilize{deserialize} functions. However, for FastRTPS it must be rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for both CycloneDDS and FastRTPS rmw. Signed-off-by: Michael Orlov <[email protected]> * Add test coverage for default rmv serialization format converter Signed-off-by: Michael Orlov <[email protected]> * Run test_serialization_converter for each rmw implementation - Rationale: To make sure that the default serialization converter can serialize and deserialize messages with all supported rmw implementations. Since it uses rmw specific functions for serialization and deserialization inside. Signed-off-by: Michael Orlov <[email protected]> * Address uncrustify formating warnings Signed-off-by: Michael Orlov <[email protected]> * Enable sanitizer by default Signed-off-by: Michael Orlov <[email protected]> * Address Windows build warnings Signed-off-by: Michael Orlov <[email protected]> * Revert "Enable sanitizer by default" This reverts commit 7241963. Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> (cherry picked from commit 6e82f52) # Conflicts: # rosbag2_cpp/src/rosbag2_cpp/converter.cpp * Address merge conflicts Signed-off-by: Michael Orlov <[email protected]> * Fix uncrustfy warning due to the different versions of uncrustify Signed-off-by: Michael Orlov <[email protected]> * Replace ament_add_gmock_test with ament_add_gmock - Rationale: The ament_add_gmock_executable(..) and ament_add_gmock_test(..) macros are not available on Iron distro. Signed-off-by: Michael Orlov <[email protected]> * Add missing dependencies from the std_msgs Signed-off-by: Michael Orlov <[email protected]> * Adjust writer_->create_topic(..) call in tests - removed TopicId field initialization since it is absent on Iron Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: Michael Orlov <[email protected]> (cherry picked from commit a269cd4) # Conflicts: # rosbag2_cpp/CMakeLists.txt * Address merge conflicts after automatic backporting Signed-off-by: Michael Orlov <[email protected]> * Adjust tests for humble Signed-off-by: Michael Orlov <[email protected]> * Address linker errors on RHEL 8 - Use rcpputils::fs instead of the std::filesystem in tests. Since RHEL8 doesn't have adequate support for the std::filesystem. Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Michael Orlov <[email protected]>
ros2 · Oct 2, 2024 · d256a5b · d256a5b
1 parent f997e40
commit d256a5b
Show file tree

Hide file tree

Showing 10 changed files with 276 additions and 16 deletions.
diff --git a/rosbag2_cpp/CMakeLists.txt b/rosbag2_cpp/CMakeLists.txt
@@ -29,7 +29,7 @@ option(DISABLE_SANITIZERS "disables the use of gcc sanitizers" ON)
 if(NOT DISABLE_SANITIZERS AND CMAKE_COMPILER_IS_GNUCXX)
   include(CheckCXXSourceRuns)
   set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
-  set(CMAKE_REQUIRED_FLAGS "${OLD_CMAKE_REQUIRED_FLAGS} -fsanitize=leak")
+  set(CMAKE_REQUIRED_FLAGS "${OLD_CMAKE_REQUIRED_FLAGS} -fsanitize=address,leak,undefined")
   check_cxx_source_runs("int main() {}" HAVE_SANITIZERS)
   set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
   if(NOT HAVE_SANITIZERS)
@@ -131,7 +131,9 @@ ament_export_dependencies(
 if(BUILD_TESTING)
   find_package(ament_cmake_gmock REQUIRED)
   find_package(ament_lint_auto REQUIRED)
+  find_package(std_msgs REQUIRED)
   find_package(test_msgs REQUIRED)
+  find_package(rmw_implementation_cmake REQUIRED)
   ament_lint_auto_find_test_dependencies()
 
   add_library(
@@ -227,6 +229,39 @@ if(BUILD_TESTING)
     target_link_libraries(test_sequential_writer ${PROJECT_NAME})
   endif()
 
+  function(test_serialization_converter_for_rmw_implementation)
+    set(rmw_implementation_env_var RMW_IMPLEMENTATION=${rmw_implementation})
+    ament_add_gmock(test_serialization_converter${target_suffix}
+      test/rosbag2_cpp/test_serialization_converter.cpp
+      ENV ${rmw_implementation_env_var}
+    )
+    if(TARGET test_serialization_converter${target_suffix})
+      if(NOT DISABLE_SANITIZERS)
+        target_compile_options(test_serialization_converter${target_suffix}
+          PUBLIC $<$<CXX_COMPILER_ID:GNU>:-fsanitize=address,leak,undefined>
+        )
+        target_link_libraries(test_serialization_converter${target_suffix}
+          $<$<CXX_COMPILER_ID:GNU>:-fsanitize=address,leak,undefined>
+        )
+      endif()
+      ament_target_dependencies(test_serialization_converter${target_suffix}
+        rosbag2_storage
+        rcpputils
+        rclcpp
+        rmw
+        std_msgs
+      )
+      target_link_libraries(test_serialization_converter${target_suffix}
+        ${PROJECT_NAME}
+        rosbag2_storage::rosbag2_storage
+        ${std_msgs_TARGETS}
+      )
+    endif()
+  endfunction()
+  # Run test_serialization_converter for each rmw implementation because default serialization
+  # converter uses rmw specific functions for serialization and deserialization.
+  call_for_each_rmw_implementation(test_serialization_converter_for_rmw_implementation)
+
   ament_add_gmock(test_multifile_reader
     test/rosbag2_cpp/test_multifile_reader.cpp)
   if(TARGET test_multifile_reader)

diff --git a/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_converter.hpp b/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_converter.hpp
@@ -20,6 +20,7 @@
 
 #include "rosbag2_cpp/converter_interfaces/serialization_format_serializer.hpp"
 #include "rosbag2_cpp/converter_interfaces/serialization_format_deserializer.hpp"
+#include "rosbag2_cpp/visibility_control.hpp"
 
 /**
  * This is a convenience class for plugin developers.
@@ -31,7 +32,7 @@ namespace rosbag2_cpp
 namespace converter_interfaces
 {
 
-class SerializationFormatConverter
+class ROSBAG2_CPP_PUBLIC SerializationFormatConverter
   : public SerializationFormatSerializer, public SerializationFormatDeserializer
 {
 public:

diff --git a/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_deserializer.hpp b/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_deserializer.hpp
@@ -19,17 +19,16 @@
 #include <string>
 
 #include "rosbag2_cpp/types/introspection_message.hpp"
-
 #include "rosbag2_storage/serialized_bag_message.hpp"
-
 #include "rosidl_runtime_c/message_type_support_struct.h"
+#include "rosbag2_cpp/visibility_control.hpp"
 
 namespace rosbag2_cpp
 {
 namespace converter_interfaces
 {
 
-class SerializationFormatDeserializer
+class ROSBAG2_CPP_PUBLIC SerializationFormatDeserializer
 {
 public:
   virtual ~SerializationFormatDeserializer() = default;

diff --git a/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_serializer.hpp b/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_serializer.hpp
@@ -19,17 +19,16 @@
 #include <string>
 
 #include "rosbag2_cpp/types/introspection_message.hpp"
-
 #include "rosbag2_storage/serialized_bag_message.hpp"
-
 #include "rosidl_runtime_c/message_type_support_struct.h"
+#include "rosbag2_cpp/visibility_control.hpp"
 
 namespace rosbag2_cpp
 {
 namespace converter_interfaces
 {
 
-class SerializationFormatSerializer
+class ROSBAG2_CPP_PUBLIC SerializationFormatSerializer
 {
 public:
   virtual ~SerializationFormatSerializer() = default;

diff --git a/...mented_serialization_format_converter.hpp → ...mented_serialization_format_converter.hpp b/...mented_serialization_format_converter.hpp → ...mented_serialization_format_converter.hpp
@@ -19,6 +19,15 @@
 #include <string>
 
 #include "rosbag2_cpp/converter_interfaces/serialization_format_converter.hpp"
+#include "rosbag2_cpp/visibility_control.hpp"
+
+// This is necessary because of using stl types here. It is completely safe, because
+// a) the member is not accessible from the outside
+// b) there are no inline functions.
+#ifdef _WIN32
+# pragma warning(push)
+# pragma warning(disable:4251)
+#endif
 
 namespace rosbag2_cpp
 {
@@ -31,7 +40,7 @@ class RMWImplementedConverterImpl;
  * searches the system for an installed RMW implementation that understands the requested format,
  * loads that library if found, and uses its implementation for serialization.
  */
-class RMWImplementedConverter
+class ROSBAG2_CPP_PUBLIC RMWImplementedConverter
   : public rosbag2_cpp::converter_interfaces::SerializationFormatConverter
 {
 public:
@@ -40,7 +49,7 @@ class RMWImplementedConverter
    * \throws std::runtime_error if no RMW implementation was found supporting the format.
    */
   explicit RMWImplementedConverter(const std::string & format);
-  virtual ~RMWImplementedConverter();
+  ~RMWImplementedConverter() override;
 
   void deserialize(
     std::shared_ptr<const rosbag2_storage::SerializedBagMessage> serialized_message,

diff --git a/rosbag2_cpp/package.xml b/rosbag2_cpp/package.xml
@@ -33,6 +33,8 @@
   <test_depend>ament_cmake_gmock</test_depend>
   <test_depend>ament_lint_auto</test_depend>
   <test_depend>ament_lint_common</test_depend>
+  <test_depend>rmw_implementation_cmake</test_depend>
+  <test_depend>std_msgs</test_depend>
   <test_depend>test_msgs</test_depend>
   <test_depend>rosbag2_test_common</test_depend>
 

diff --git a/rosbag2_cpp/src/rosbag2_cpp/converter.cpp b/rosbag2_cpp/src/rosbag2_cpp/converter.cpp
@@ -67,22 +67,23 @@ std::shared_ptr<rosbag2_storage::SerializedBagMessage> Converter::convert(
   std::shared_ptr<const rosbag2_storage::SerializedBagMessage> message)
 {
   auto introspection_ts = topics_and_types_.at(message->topic_name).introspection_type_support;
+  auto rmw_ts = topics_and_types_.at(message->topic_name).rmw_type_support;
   auto allocator = rcutils_get_default_allocator();
   std::shared_ptr<rosbag2_introspection_message_t> allocated_ros_message =
     allocate_introspection_message(introspection_ts, &allocator);
-  auto output_message = std::make_shared<rosbag2_storage::SerializedBagMessage>();
 
   // deserialize
   rosbag2_cpp::introspection_message_set_topic_name(
     allocated_ros_message.get(), message->topic_name.c_str());
   allocated_ros_message->time_stamp = message->time_stamp;
-  input_converter_->deserialize(message, introspection_ts, allocated_ros_message);
+  input_converter_->deserialize(message, rmw_ts, allocated_ros_message);
 
-  // re-serialize
+  // re-serialize with the new serializer
+  auto output_message = std::make_shared<rosbag2_storage::SerializedBagMessage>();
   output_message->serialized_data = rosbag2_storage::make_empty_serialized_message(0);
   output_message->topic_name = std::string(allocated_ros_message->topic_name);
   output_message->time_stamp = allocated_ros_message->time_stamp;
-  output_converter_->serialize(allocated_ros_message, introspection_ts, output_message);
+  output_converter_->serialize(allocated_ros_message, rmw_ts, output_message);
   return output_message;
 }
 

diff --git a/rosbag2_cpp/src/rosbag2_cpp/rmw_implemented_serialization_format_converter.cpp b/rosbag2_cpp/src/rosbag2_cpp/rmw_implemented_serialization_format_converter.cpp
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include "rmw_implemented_serialization_format_converter.hpp"
+#include "rosbag2_cpp/rmw_implemented_serialization_format_converter.hpp"
 
 #include <memory>
 #include <sstream>

diff --git a/rosbag2_cpp/src/rosbag2_cpp/serialization_format_converter_factory_impl.hpp b/rosbag2_cpp/src/rosbag2_cpp/serialization_format_converter_factory_impl.hpp
@@ -28,7 +28,7 @@
 #include "rosbag2_cpp/logging.hpp"
 #include "rosbag2_cpp/visibility_control.hpp"
 
-#include "./rmw_implemented_serialization_format_converter.hpp"
+#include "rosbag2_cpp/rmw_implemented_serialization_format_converter.hpp"
 
 namespace rosbag2_cpp
 {