Upstream quality changes from Apex.AI part-2 (#1924)

* Fix for incorrectly handling exit code in wait_until_completion(..)

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Address flakiness in test_play_services

- Increase timeout for `expect_messages(..)`
- Also add `player_->wait_for_playback_to_start();` in `setup_player()`

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Fix CPP_LINT warnings about missing include headers

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Address CPPLint warnings in the rosbag2_storage_sqlite3

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Adjust test_rosbag2_storage_api.cpp for bounded strings

- Reduce size of each std_string_msg to fit in 256 byte boundary

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Print warnings instead of errors for not found type definitions

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Add debug info to flaky `test_play_cancel` in rosbag2_py

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Fix for flaky play_respect_messages_timing_after_play_next

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Reduce runtime for the `PlayerTestFixture.playing_respects_delay` test

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Address review comment by using `delay_margin(0.5s)`

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Address linter warnings in the test_transport.py

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Use RESOURCES_PATH from envar in test_transport.py

 - Fix to avoid failure with remote test execution and to be consistent
  with oter similar tests.

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Mitigate flakiness in record_all_include_unpublished_false

Increase timeout when waiting to stop discovery at least to the 0.5 sec.
The record_all_include_unpublished_false_includes_later_published is
tend to be flaky due to the insufficient timeout for waiting to stop
discovery thread in the recorder.

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Fix linter errors in the `message_definition_encoding.md`

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Minor changes in rosbag2_examples_py

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Mark MessageProducer<T> as final to address clang16 warnings

- Address clang16 warning:
"destructor called on non-final 'msg_utils::MessageProducer<T>' that
has virtual functions but non-virtual destructor".

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Address unused variable warning on the clang16 build

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Fix for flaky `test_play_services`

Increase timeouts for service calls in rosbag2_transport
`test_play_services` tests

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Fix for record_if_topic_list_service_list_and_all_are_specified

- Flush output from Python print when printing warnings. Some tests rely
on the warning output to be in the std::cout. Without flushing the
warnings may not be captured by tests.

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Sort includes alphabetically in the `info.cpp` file

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Update rosbag2_cpp/test/rosbag2_cpp/test_service_utils.cpp

Sort includes alphabetically in test_service_utils.cpp

Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Sort includes in test_rosbag2_cpp_get_service_info.cpp

Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Michael Orlov <morlovmr@gmail.com>

* Sort includes in rosbag2_py/_info.cpp

Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Michael Orlov <morlovmr@gmail.com>

---------

Signed-off-by: Michael Orlov <morlovmr@gmail.com>
Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com>
(cherry picked from commit 6f44ee0)

# Conflicts:
#	ros2bag/ros2bag/verb/record.py
#	rosbag2_cpp/src/rosbag2_cpp/info.cpp

Author: MichaelOrlov

docs/message_definition_encoding.md

@@ -16,10 +16,14 @@ This set of definitions with all field types recursively included can be called
 
 ## `ros2msg` encoding
 
-This encoding consists of definitions in [.msg](https://docs.ros.org/en/rolling/Concepts/Basic/About-Interfaces.html#messages) and [.srv](https://docs.ros.org/en/rolling/Concepts/Basic/About-Interfaces.html#services) format, concatenated together in human-readable form with
+This encoding consists of definitions in
+[.msg](https://docs.ros.org/en/rolling/Concepts/Basic/About-Interfaces.html#messages) and
+[.srv](https://docs.ros.org/en/rolling/Concepts/Basic/About-Interfaces.html#services) format,
+concatenated together in human-readable form with
 a delimiter.
 
-The top-level message definition is present first, with no delimiter. All dependent .msg definitions are preceded by a two-line delimiter:
+The top-level message definition is present first, with no delimiter. All dependent .msg definitions
+are preceded by a two-line delimiter:
 
 * One line containing exactly 80 `=` characters
 * One line containing `MSG: <package resource name>` for that type. The space between MSG: and the
@@ -29,7 +33,7 @@ The top-level message definition is present first, with no delimiter. All depend
 
 For example, the complete message definition for `my_msgs/msg/ExampleMsg` in `ros2msg` form is:
 
-```
+```ini
 # defines a message that includes a field of a custom message type
 my_msgs/BasicMsg my_basic_field
 ================================================================================
@@ -40,7 +44,7 @@ float32 my_float
 
 Another example is a service message definition for `my_msgs/srv/ExampleSrv` in `ros2msg` form
 
-```
+```ini
 # defines a service message that includes a field of a custom message type
 my_msgs/BasicMsg request
 ---
@@ -53,16 +57,20 @@ float32 my_float
 
 ## `ros2idl` encoding
 
-The IDL definition of the type specified by name along with all dependent types are stored together. The IDL definitions can be stored in any order. Every definition is preceded by a two-line delimiter:
+The IDL definition of the type specified by name along with all dependent types are stored together.
+The IDL definitions can be stored in any order. Every definition is preceded by a two-line
+delimiter:
 
 * a line containing exactly 80 `=` characters, then
-* A line containing only `IDL: <package resource name>` for that definition. The space between IDL: and the package resource name is mandatory. The package resource name does not include a file extension.
+* A line containing only `IDL: <package resource name>` for that definition. The space between IDL:
+  and the package resource name is mandatory. The package resource name does not include a file
+  extension.
 
 ### `ros2idl` example
 
 For example, the complete message definition for `my_msgs/msg/ComplexMsg` in `ros2idl` form is:
 
-```
+```idl
 ================================================================================
 IDL: my_msgs/msg/ComplexMsg
 // generated from rosidl_adapter/resource/msg.idl.em

ros2bag/ros2bag/verb/record.py

@@ -229,6 +229,11 @@ def check_necessary_argument(args):
 
 def validate_parsed_arguments(args, uri) -> str:
     if args.topics_positional:
+<<<<<<< HEAD
+=======
+        print(print_warn('Positional "topics" argument deprecated. '
+                         'Please use optional "--topics" argument instead.'), flush=True)
+>>>>>>> 6f44ee0 (Upstream quality changes from Apex.AI part-2 (#1924))
         args.topics = args.topics_positional
 
     if not check_necessary_argument(args):
@@ -256,13 +261,22 @@ def validate_parsed_arguments(args, uri) -> str:
                            'or --regex')
 
     if (args.all or args.all_services) and args.services:
-        print(print_warn('--all or --all-services will override --services'))
+        print(print_warn('--all or --all-services will override --services'), flush=True)
 
     if (args.all or args.all_topics) and args.topics:
-        print(print_warn('--all or --all-topics will override --topics'))
+        print(print_warn('--all or --all-topics will override --topics'), flush=True)
 
+<<<<<<< HEAD
     if (args.all or args.all_topics or args.all_services) and args.regex:
         print(print_warn('--all, --all-topics or --all-services will override --regex'))
+=======
+    if (args.all or args.all_actions) and args.actions:
+        print(print_warn('--all or --all-actions will override --actions'))
+
+    if (args.all or args.all_topics or args.all_services or args.all_actions) and args.regex:
+        print(print_warn('--all, --all-topics --all-services or --all-actions will override '
+                         '--regex'),  flush=True)
+>>>>>>> 6f44ee0 (Upstream quality changes from Apex.AI part-2 (#1924))
 
     if os.path.isdir(uri):
         return print_error("Output folder '{}' already exists.".format(uri))

rosbag2_cpp/src/rosbag2_cpp/info.cpp

@@ -12,21 +12,29 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include "rosbag2_cpp/info.hpp"
-
 #include <filesystem>
-#include <unordered_map>
-#include <unordered_set>
+#include <memory>
 #include <stdexcept>
 #include <string>
+#include <unordered_map>
+#include <unordered_set>
+#include <utility>
+#include <vector>
 
 #include "rmw/rmw.h"
+<<<<<<< HEAD
 #include "rosidl_typesupport_cpp/message_type_support.hpp"
 #include "service_msgs/msg/service_event_info.hpp"
 
+=======
+#include "rosbag2_cpp/action_utils.hpp"
+#include "rosbag2_cpp/info.hpp"
+>>>>>>> 6f44ee0 (Upstream quality changes from Apex.AI part-2 (#1924))
 #include "rosbag2_cpp/service_utils.hpp"
 #include "rosbag2_storage/metadata_io.hpp"
 #include "rosbag2_storage/storage_factory.hpp"
+#include "rosidl_typesupport_cpp/message_type_support.hpp"
+#include "service_msgs/msg/service_event_info.hpp"
 
 namespace fs = std::filesystem;
 

rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp

@@ -184,7 +184,8 @@ const LocalMessageDefinitionSource::MessageSpec & LocalMessageDefinitionSource::
   try {
     share_dir = ament_index_cpp::get_package_share_directory(package);
   } catch (const ament_index_cpp::PackageNotFoundError & e) {
-    ROSBAG2_CPP_LOG_WARN("'%s'", e.what());
+    ROSBAG2_CPP_LOG_WARN(
+      "get_package_share_directory(%s) failed with error: '%s'", package.c_str(), e.what());
     throw DefinitionNotFoundError(definition_identifier.topic_type());
   }
   std::string dir = definition_identifier.format() == Format::MSG ||
@@ -245,7 +246,7 @@ rosbag2_storage::MessageDefinition LocalMessageDefinitionSource::get_full_text(
       result = (delimiter(root_definition_identifier) +
         append_recursive(root_definition_identifier, max_recursion_depth));
     } catch (const TypenameNotUnderstoodError & err) {
-      ROSBAG2_CPP_LOG_ERROR(
+      ROSBAG2_CPP_LOG_WARN(
         "Message type name '%s' not understood by type definition search, "
         "definition will be left empty in bag.", err.what());
       format = Format::UNKNOWN;
@@ -280,7 +281,7 @@ rosbag2_storage::MessageDefinition LocalMessageDefinitionSource::get_full_text(
             format = Format::IDL;
           }
         } catch (const TypenameNotUnderstoodError & err) {
-          ROSBAG2_CPP_LOG_ERROR(
+          ROSBAG2_CPP_LOG_WARN(
             "Message type name '%s' not understood by type definition search, "
             "definition will be left empty in bag.", err.what());
           format = Format::UNKNOWN;

rosbag2_cpp/src/rosbag2_cpp/service_utils.cpp

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include <cstring>
+#include <string>
 
 #include "rcl/service_introspection.h"
 

rosbag2_cpp/test/rosbag2_cpp/test_service_utils.cpp

@@ -15,6 +15,7 @@
 
 #include <string>
 #include <tuple>
+#include <utility>
 #include <vector>
 
 #include "rosbag2_cpp/service_utils.hpp"

rosbag2_examples/rosbag2_examples_py/rosbag2_examples_py/data_generator_executable.py

@@ -43,6 +43,8 @@ def main(args=None):
             serialize_message(data),
             time_stamp.nanoseconds)
         time_stamp += Duration(seconds=1)
+    writer.close()
+    print("Generated data saved into the '%s'" % storage_options.uri)
 
 
 if __name__ == '__main__':

rosbag2_examples/rosbag2_examples_py/rosbag2_examples_py/data_generator_node.py

@@ -28,7 +28,7 @@ def __init__(self):
 
         storage_options = rosbag2_py.StorageOptions(
             uri='timed_synthetic_bag',
-            storage_id='sqlite3')
+            storage_id='mcap')
         converter_options = rosbag2_py.ConverterOptions('', '')
         self.writer.open(storage_options, converter_options)
 

rosbag2_performance/rosbag2_performance_benchmarking/include/msg_utils/message_producer.hpp

@@ -33,7 +33,7 @@ class ProducerBase
 };
 
 template<typename T>
-class MessageProducer : public ProducerBase
+class MessageProducer final : public ProducerBase
 {
 public:
   MessageProducer(rclcpp::Node & node, std::string topic, const PublisherGroupConfig & config);

rosbag2_performance/rosbag2_performance_benchmarking/src/writer_benchmark.cpp

@@ -58,7 +58,6 @@ void WriterBenchmark::start_benchmark()
   RCLCPP_INFO(get_logger(), "Starting the WriterBenchmark");
   start_producers();
   while (rclcpp::ok()) {
-    int count = 0;
     unsigned int complete_count = 0;
 
     // TODO(adamdbrw) Performance can be improved. Use conditional variables
@@ -121,7 +120,6 @@ void WriterBenchmark::start_benchmark()
         } catch (const std::runtime_error & e) {
           RCLCPP_ERROR_STREAM(get_logger(), "Failed to record: " << e.what());
         }
-        ++count;
       }
     }
     if (complete_count == queues_.size()) {

rosbag2_py/src/rosbag2_py/_info.cpp

@@ -12,10 +12,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <algorithm>
 #include <iostream>
 #include <memory>
 #include <string>
-#include <algorithm>
+#include <vector>
 
 #include "info_sorting_method.hpp"
 #include "format_bag_metadata.hpp"

rosbag2_py/src/rosbag2_py/format_service_info.cpp

@@ -12,7 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <vector>
+#include <memory>
 #include <sstream>
+#include <string>
 
 #include "format_service_info.hpp"
 #include "rosbag2_cpp/service_utils.hpp"

rosbag2_py/test/test_transport.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import datetime
+import os
 from pathlib import Path
 import re
 import threading
@@ -28,7 +29,7 @@
 from std_msgs.msg import String
 
 
-RESOURCES_PATH = Path(__file__).parent / 'resources'
+RESOURCES_PATH = Path(os.environ['ROSBAG2_PY_TEST_RESOURCES_DIR'])
 PLAYBACK_UNTIL_TIMESTAMP_REGEX_STRING = r'\[rosbag2_player]: Playback until timestamp: -1'
 
 
@@ -117,6 +118,8 @@ def test_record_cancel(tmp_path, storage_id):
 @pytest.mark.parametrize('storage_id', TESTED_STORAGE_IDS)
 def test_play_cancel(storage_id, capfd):
     bag_path = str(RESOURCES_PATH / storage_id / 'talker')
+    assert os.path.exists(bag_path), 'Could not find test bag file: ' + bag_path
+
     storage_options, converter_options = get_rosbag_options(bag_path, storage_id)
 
     player = rosbag2_py.Player()
@@ -131,18 +134,24 @@ def test_play_cancel(storage_id, capfd):
         daemon=True)
     player_thread.start()
 
-    def check_playback_start_output(cumulative_out, cumulative_err):
+    def check_playback_start_output(cap_streams):
         out, err = capfd.readouterr()
-        cumulative_err += err
-        cumulative_out += out
+        cap_streams['err'] += err
+        cap_streams['out'] += out
         expected_string_regex = re.compile(PLAYBACK_UNTIL_TIMESTAMP_REGEX_STRING)
-        matches = expected_string_regex.search(cumulative_err)
+        matches = expected_string_regex.search(cap_streams['err'])
         return matches is not None
 
-    captured_out = ''
-    captured_err = ''
-    assert wait_for(lambda: check_playback_start_output(captured_out, captured_err),
-                    timeout=rclpy.duration.Duration(seconds=3))
+    captured_streams = {'out': '', 'err': ''}
+
+    if not wait_for(lambda: check_playback_start_output(captured_streams),
+                    timeout=rclpy.duration.Duration(seconds=5)):
+        with capfd.disabled():
+            print('\nCaptured stdout:', captured_streams['out'])
+            print('\nCaptured stderr:', captured_streams['err'])
+        player.cancel()
+        player_thread.join()
+        assert False
 
     player.cancel()
     player_thread.join(3)

rosbag2_storage_sqlite3/src/rosbag2_storage_sqlite3/sqlite_storage.cpp

@@ -19,9 +19,9 @@
 #include <atomic>
 #include <chrono>
 #include <cstring>
-#include <filesystem>
-#include <fstream>
+#include <filesystem>  // NOLINT cpplint: FP, filesystem is a C++17 system header
 #include <iostream>
+#include <limits>
 #include <memory>
 #include <string>
 #include <unordered_map>

rosbag2_storage_sqlite3/test/rosbag2_storage_sqlite3/storage_test_fixture.hpp

@@ -20,7 +20,7 @@
 #include <cstdio>
 #include <cstring>
 #include <iostream>
-#include <filesystem>
+#include <filesystem>  // NOLINT cpplint: FP, filesystem is a C++17 system header
 #include <fstream>
 #include <memory>
 #include <string>

rosbag2_storage_sqlite3/test/rosbag2_storage_sqlite3/test_sqlite_storage.cpp

@@ -15,7 +15,7 @@
 #include <gmock/gmock.h>
 
 #include <algorithm>
-#include <filesystem>
+#include <filesystem>  // NOLINT cpplint: FP, filesystem is a C++17 system header
 #include <limits>
 #include <memory>
 #include <string>

rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_unix.hpp

@@ -116,9 +116,13 @@ bool wait_until_completion(
     // WNOHANG - wait for processes without causing the caller to be blocked
     wait_ret_code = waitpid(process_id, &status, WNOHANG);
   }
+  EXPECT_NE(wait_ret_code, 0) << "Testing process " << process_id << " hangout.\n";
   EXPECT_NE(wait_ret_code, -1);
   EXPECT_EQ(wait_ret_code, process_id) << "status = " << status;
-  return wait_ret_code != 0;
+  EXPECT_EQ(WIFEXITED(status), true) << "status = " << status;
+  EXPECT_EQ(WIFSIGNALED(status), false) << "Process terminated by signal: " << WTERMSIG(status);
+  EXPECT_EQ(WEXITSTATUS(status), EXIT_SUCCESS) << "status = " << status;
+  return WIFSIGNALED(status) != true && WEXITSTATUS(status) == EXIT_SUCCESS;
 }
 
 /// @brief Force to stop process with signal if it's currently running

rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_get_service_info.cpp

@@ -14,11 +14,13 @@
 
 #include <gmock/gmock.h>
 
-#include <atomic>
 #include <chrono>
 #include <filesystem>
+#include <memory>
 #include <stdexcept>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "rosbag2_cpp/info.hpp"
 #include "rosbag2_cpp/writer.hpp"

rosbag2_tests/test/rosbag2_tests/test_rosbag2_storage_api.cpp

@@ -75,8 +75,8 @@ class Rosbag2StorageAPITests : public rosbag2_test_common::ParametrizedTemporary
       for (size_t msg_idx = 0; msg_idx < num_msgs_per_topic; msg_idx++) {
         auto std_string_msg = std::make_shared<std_msgs::msg::String>();
         std::stringstream ss;
-        for (int i = 0; i < 100; i++) {
-          ss << topic << "_long_long_string_message_" << msg_idx + 1 << "_repeat_";
+        for (int i = 0; i < 5; i++) {
+          ss << topic << "_long_string_message_" << msg_idx + 1 << "_repeat_";
         }
         std_string_msg->data = ss.str();
 
@@ -133,7 +133,7 @@ TEST_P(Rosbag2StorageAPITests, get_bagfile_size_read_write_interface)
     factory.open_read_write(options);
 
   std::vector<std::string> topics = {"topic1_topic1", "topic2_topic2"};
-  auto serialized_messages = prepare_serialized_messages(topics, 500);
+  auto serialized_messages = prepare_serialized_messages(topics, 500 * 20);
   create_topics(rw_storage, topics);
 
   rw_storage->write(serialized_messages);

rosbag2_transport/src/rosbag2_transport/recorder.cpp

@@ -456,7 +456,8 @@ void RecorderImpl::stop_discovery()
   std::lock_guard<std::mutex> state_lock(discovery_mutex_);
   if (discovery_running_.exchange(false)) {
     if (discovery_future_.valid()) {
-      auto status = discovery_future_.wait_for(2 * record_options_.topic_polling_interval);
+      auto status = discovery_future_.wait_for(
+        std::chrono::milliseconds(500) + record_options_.topic_polling_interval);
       if (status != std::future_status::ready) {
         RCLCPP_ERROR_STREAM(
           node->get_logger(),

rosbag2_transport/test/rosbag2_transport/test_play_next.cpp

@@ -200,8 +200,8 @@ TEST_F(RosBag2PlayTestFixture, play_respect_messages_timing_after_play_next) {
   ASSERT_TRUE(player->play_next());
   ASSERT_TRUE(player->play_next());
   ASSERT_TRUE(player->is_paused());
-  player->resume();
   auto start = std::chrono::steady_clock::now();
+  player->resume();
   player->wait_for_playback_to_finish();
   auto replay_time = std::chrono::steady_clock::now() - start;