Skip to content

fix(rosbag): default storage format sqlite3 -> mcap#252

Merged
mfaferek93 merged 3 commits intomainfrom
fix/rosbag-mcap-default
Mar 5, 2026
Merged

fix(rosbag): default storage format sqlite3 -> mcap#252
mfaferek93 merged 3 commits intomainfrom
fix/rosbag-mcap-default

Conversation

@mfaferek93
Copy link
Collaborator

Summary

  • Default rosbag storage format changed from sqlite3 to mcap
  • rosbag2_storage_mcap promoted from <test_depend> to <depend> in package.xml
  • Storage format validation now tests any format uniformly instead of assuming sqlite3 is always available

On Jazzy/Noble the rosbag2_storage_default_plugins package only ships the mcap plugin. The rosbag2_storage_sqlite3 package is not available, causing rosbag capture and integration tests to fail on the buildfarm (build #2).

Closes #251

Test plan

  • Verify buildfarm build passes with mcap as default
  • Test with explicit snapshots.rosbag.format: sqlite3 on a system where sqlite3 plugin is installed
  • Run integration tests locally

On Jazzy/Noble the rosbag2_storage_default_plugins package only ships the
mcap plugin. The sqlite3 plugin (rosbag2_storage_sqlite3) is not available,
causing integration tests and rosbag capture to fail on the buildfarm.

Changes:
- Default rosbag storage format changed from "sqlite3" to "mcap"
- rosbag2_storage_mcap promoted from test_depend to depend in package.xml
- Storage format validation now tests any format uniformly instead of
  assuming sqlite3 is always available

Closes #251
Copilot AI review requested due to automatic review settings March 5, 2026 12:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the fault manager’s rosbag snapshot capture defaults to use MCAP storage by default, aligning runtime behavior with ROS 2 Jazzy/Noble packaging where only the MCAP storage plugin is shipped by default.

Changes:

  • Switch default rosbag storage format from sqlite3 to mcap (both in config struct default and declared ROS parameter default).
  • Promote rosbag2_storage_mcap from a test-only dependency to a runtime dependency.
  • Change storage-format validation to probe plugin availability by opening a test bag for the selected format (instead of assuming sqlite3 is always present).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/ros2_medkit_fault_manager/src/rosbag_capture.cpp Validates selected storage format by attempting to open a test bag with the requested storage plugin.
src/ros2_medkit_fault_manager/src/fault_manager_node.cpp Changes default snapshots.rosbag.format parameter from sqlite3 to mcap.
src/ros2_medkit_fault_manager/package.xml Adds rosbag2_storage_mcap as a runtime dependency (removes it from test deps).
src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/snapshot_capture.hpp Changes RosbagConfig::format default to mcap and updates the comment accordingly.
Comments suppressed due to low confidence (1)

src/ros2_medkit_fault_manager/src/rosbag_capture.cpp:703

  • The catch block always reports that the storage format is "not available", but Writer::open() can also fail for non-plugin reasons (e.g., temp dir permissions, existing path collisions). Consider using a more unique temp path (timestamp/random + pid and/or remove_any existing path before open), and adjust the error text to include the test path / underlying failure so it doesn’t incorrectly imply a missing plugin.
  std::string test_path =
      std::filesystem::temp_directory_path().string() + "/.rosbag_format_test_" + std::to_string(getpid());

  try {
    rosbag2_cpp::Writer writer;
    rosbag2_storage::StorageOptions opts;
    opts.uri = test_path;
    opts.storage_id = config_.format;
    writer.open(opts);
    // Success - plugin is available
  } catch (const std::exception & e) {
    // Clean up any partial test files
    std::error_code ec;
    std::filesystem::remove_all(test_path, ec);

    throw std::runtime_error(
        "Rosbag storage format '" + config_.format +
        "' is not available. "
        "Install the plugin or use a different format. "
        "Error: " +
        std::string(e.what()));

Update unit and integration tests to use mcap instead of sqlite3 as the
default storage format. Remove sqlite3-specific acceptance tests since
the plugin is not available on all platforms (e.g. Jazzy/Noble).
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/ros2_medkit_fault_manager/src/rosbag_capture.cpp:684

  • validate_storage_format() uses a fixed temp test_path based only on PID. If multiple RosbagCapture instances validate concurrently within the same process (e.g., composed nodes or multi-threaded startup), they can race on the same path and cause flaky failures or partial cleanup. Consider incorporating additional uniqueness (timestamp + thread id / random suffix) and building the path with std::filesystem::path to avoid collisions.
  // Verify the selected storage plugin is available by trying to create a test bag
  std::string test_path =
      std::filesystem::temp_directory_path().string() + "/.rosbag_format_test_" + std::to_string(getpid());

src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp:301

  • The change makes sqlite3 format validation depend on plugin availability (by attempting to open the writer), but the unit tests no longer exercise the sqlite3 path at all. Add a robust test that covers format="sqlite3" and asserts the expected behavior based on whether the plugin can be opened on the current system (e.g., detect availability first, then expect throw vs no-throw), so this regression stays covered across distros.
// Format tests

TEST_F(RosbagCaptureTest, McapFormatAccepted) {
  auto rosbag_config = create_rosbag_config();
  rosbag_config.format = "mcap";
  auto snapshot_config = create_snapshot_config();
  EXPECT_NO_THROW(RosbagCapture(node_.get(), storage_.get(), rosbag_config, snapshot_config));
}

Align string continuation in throw statement to match clang-format rules.
@mfaferek93 mfaferek93 merged commit 7fa8454 into main Mar 5, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Buildfarm integration tests fail - rosbag2 sqlite3 storage plugin unavailable on Jazzy/Noble

3 participants