Review fixes in rosbag2 composable player

- Cleanups in CMakeLists.txt
- Expose only the necessary constructors in rosbag2::player
- Add visibility control
- Add missing dependencies in package.xml
- Add component_player.hpp file

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

rosbag2/CMakeLists.txt

@@ -12,52 +12,37 @@ if(NOT CMAKE_CXX_STANDARD)
   set(CMAKE_CXX_STANDARD_REQUIRED ON)
 endif()
 
-if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
-  add_compile_options(-Wall -Wextra -Wpedantic)
-endif()
-if(CMAKE_BUILD_TYPE STREQUAL "Debug" AND MSVC)
-  # /bigobj is needed to avoid error C1128:
-  #   https://msdn.microsoft.com/en-us/library/8578y171.aspx
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj")
-endif()
-
-# Windows supplies macros for min and max by default. We should only use min and max from stl
-if(WIN32)
-  add_definitions(-DNOMINMAX)
-endif()
-
 find_package(ament_cmake REQUIRED)
+find_package(rclcpp REQUIRED)
 find_package(rclcpp_components REQUIRED)
 find_package(rosbag2_transport REQUIRED)
 
-add_library(${PROJECT_NAME} SHARED
+# Library for Rosbag2 composable nodes
+add_library(${PROJECT_NAME}_nodes SHARED
   src/rosbag2/component_player.cpp
+#  src/rosbag2/composable_recorder.cpp
 )
+target_include_directories(${PROJECT_NAME}_nodes PUBLIC
+  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
+  $<INSTALL_INTERFACE:include/${PROJECT_NAME}>)
 
-target_link_libraries(${PROJECT_NAME}
-  rclcpp::rclcpp
-  ${rclcpp_components_TARGETS}
-  rosbag2_transport::rosbag2_transport
-)
+ament_target_dependencies(${PROJECT_NAME}_nodes rclcpp rclcpp_components rosbag2_transport)
+
+rclcpp_components_register_node(${PROJECT_NAME}_nodes PLUGIN "rosbag2::Player" EXECUTABLE player)
 
-rclcpp_components_register_node(${PROJECT_NAME}
-  PLUGIN "rosbag2::Player"
-  EXECUTABLE player
+install(DIRECTORY include/
+  DESTINATION include/${PROJECT_NAME}
 )
 
 install(
-  TARGETS ${PROJECT_NAME}
-  EXPORT export_${PROJECT_NAME}
+  TARGETS ${PROJECT_NAME}_nodes
+  EXPORT export_${PROJECT_NAME}_nodes
   ARCHIVE DESTINATION lib
   LIBRARY DESTINATION lib
   RUNTIME DESTINATION bin
 )
 
-ament_export_libraries(${PROJECT_NAME})
-
-ament_export_dependencies(
-  rclcpp_components
-  rosbag2_transport
-)
+# Export modern CMake targets
+ament_export_targets(export_${PROJECT_NAME}_nodes)
 
 ament_package()

rosbag2/include/rosbag2/component_player.hpp

@@ -0,0 +1,56 @@
+// Copyright 2024 Open Source Robotics Foundation, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/* This header must be included by all rclcpp headers which declare symbols
+ * which are defined in the rclcpp library. When not building the rclcpp
+ * library, i.e. when using the headers in other package's code, the contents
+ * of this header change the visibility of certain symbols which the rclcpp
+ * library cannot have, but the consuming code must have inorder to link.
+ */
+
+#ifndef ROSBAG2__COMPONENT_PLAYER_HPP_
+#define ROSBAG2__COMPONENT_PLAYER_HPP_
+
+#include <string>
+
+#include "rclcpp/node_options.hpp"
+#include "rosbag2/visibility_control.hpp"
+#include "rosbag2_transport/player.hpp"
+
+namespace rosbag2
+{
+class Player : public rosbag2_transport::Player {
+public:
+  /// \brief Constructor and entry point for the composable player.
+  /// Will call Player(node_name, node_options) constructor with node_name = "rosbag2_player".
+  /// \param node_options Node options which will be used during construction of the underlying
+  /// node.
+  ROSBAG2_PUBLIC
+  explicit Player(const rclcpp::NodeOptions & node_options);
+
+  /// \brief Default constructor and entry point for the composable player.
+  /// Will construct Player class and initialize play_options, storage_options from node
+  /// parameters. At the end will call Player::play() to automatically start playback in a
+  /// separate thread.
+  /// \param node_name Name for the underlying node.
+  /// \param node_options Node options which will be used during construction of the underlying
+  /// node.
+  ROSBAG2_PUBLIC
+  explicit Player(
+    const std::string & node_name = "rosbag2_player",
+    const rclcpp::NodeOptions & node_options = rclcpp::NodeOptions());
+};
+}  // namespace rosbag2
+
+#endif  // ROSBAG2__COMPONENT_PLAYER_HPP_

rosbag2/include/rosbag2/visibility_control.hpp

@@ -0,0 +1,56 @@
+// Copyright 2024 Open Source Robotics Foundation, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/* This header must be included by all rclcpp headers which declare symbols
+ * which are defined in the rclcpp library. When not building the rclcpp
+ * library, i.e. when using the headers in other package's code, the contents
+ * of this header change the visibility of certain symbols which the rclcpp
+ * library cannot have, but the consuming code must have inorder to link.
+ */
+
+#ifndef ROSBAG2__VISIBILITY_CONTROL_HPP_
+#define ROSBAG2__VISIBILITY_CONTROL_HPP_
+
+// This logic was borrowed (then namespaced) from the examples on the gcc wiki:
+//     https://gcc.gnu.org/wiki/Visibility
+
+#if defined _WIN32 || defined __CYGWIN__
+#ifdef __GNUC__
+    #define ROSBAG2_EXPORT __attribute__ ((dllexport))
+    #define ROSBAG2_IMPORT __attribute__ ((dllimport))
+  #else
+    #define ROSBAG2_EXPORT __declspec(dllexport)
+    #define ROSBAG2_IMPORT __declspec(dllimport)
+  #endif
+  #ifdef ROSBAG2_BUILDING_LIBRARY
+    #define ROSBAG2_PUBLIC ROSBAG2_EXPORT
+  #else
+    #define ROSBAG2_PUBLIC ROSBAG2_IMPORT
+  #endif
+  #define ROSBAG2_PUBLIC_TYPE ROSBAG2_PUBLIC
+  #define ROSBAG2_LOCAL
+#else
+#define ROSBAG2_EXPORT __attribute__ ((visibility("default")))
+#define ROSBAG2_IMPORT
+#if __GNUC__ >= 4
+#define ROSBAG2_PUBLIC __attribute__ ((visibility("default")))
+#define ROSBAG2_LOCAL  __attribute__ ((visibility("hidden")))
+#else
+#define ROSBAG2_PUBLIC
+    #define ROSBAG2_LOCAL
+#endif
+#define ROSBAG2_PUBLIC_TYPE
+#endif
+
+#endif  // ROSBAG2__VISIBILITY_CONTROL_HPP_

rosbag2/package.xml

@@ -12,12 +12,15 @@
 
   <buildtool_depend>ament_cmake</buildtool_depend>
 
+  <depend>rclcpp</depend>
+  <depend>rclcpp_components</depend>
+  <depend>rosbag2_transport</depend>
+
   <exec_depend>ros2bag</exec_depend>
   <exec_depend>rosbag2_compression</exec_depend>
   <exec_depend>rosbag2_cpp</exec_depend>
   <exec_depend>rosbag2_py</exec_depend>
   <exec_depend>rosbag2_storage</exec_depend>
-  <exec_depend>rosbag2_transport</exec_depend>
 
   <!-- Default plugins -->
   <exec_depend>rosbag2_compression_zstd</exec_depend>

rosbag2/src/rosbag2/component_player.cpp

@@ -1,14 +1,39 @@
-#include "rosbag2_transport/player.hpp"
+// Copyright 2024 Open Source Robotics Foundation, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
 
-namespace rosbag2 {
-class Player : public rosbag2_transport::Player {
-    using rosbag2_transport::Player::Player;
-};
-}
+/* This header must be included by all rclcpp headers which declare symbols
+ * which are defined in the rclcpp library. When not building the rclcpp
+ * library, i.e. when using the headers in other package's code, the contents
+ * of this header change the visibility of certain symbols which the rclcpp
+ * library cannot have, but the consuming code must have inorder to link.
+ */
 
 #include "rclcpp_components/register_node_macro.hpp"
+#include "rosbag2/component_player.hpp"
+
+namespace rosbag2
+{
+Player::Player(const rclcpp::NodeOptions & node_options)
+: Player("rosbag2_player", node_options) {}
+
+Player::Player(
+  const std::string & node_name,
+  const rclcpp::NodeOptions & node_options)
+: rosbag2_transport::Player(node_name, node_options) {}
+}  // namespace rosbag2
 
 // Register the component with class_loader.
-// This acts as a sort of entry point, allowing the component to be
+// This acts as an entry point, allowing the component to be
 // discoverable when its library is being loaded into a running process.
-RCLCPP_COMPONENTS_REGISTER_NODE(rosbag2::Player)
+RCLCPP_COMPONENTS_REGISTER_NODE(rosbag2::Player)

rosbag2_transport/CMakeLists.txt

@@ -72,9 +72,11 @@ target_link_libraries(${PROJECT_NAME} PRIVATE
   yaml-cpp::yaml-cpp
 )
 
+# rosbag2_transport::Player composable node is deprecated. Please use rosbag2::Player instead
 rclcpp_components_register_node(
   ${PROJECT_NAME} PLUGIN "rosbag2_transport::Player" EXECUTABLE player)
 
+# rosbag2_transport::Recorder composable node is deprecated. Please use rosbag2::Recorder instead
 rclcpp_components_register_node(
   ${PROJECT_NAME} PLUGIN "rosbag2_transport::Recorder" EXECUTABLE recorder)