Make rosbag2 a metapackage

[Karsten1987]

fixes #174

As discussed in the top level ticket, this PR renames the existing rosbag2 package to rosbag2_cpp and introduces rosbag2 as a simple metapackage with only listing runtime dependencies.
The renaming also entails changing the namespace accordingly, which makes that PR pretty brutal.

Signed-off-by: Karsten Knese karsten@openrobotics.org

[Karsten1987]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

windows again after fixing visibility macro:

[Karsten1987]

And a final round of windows CI:

As listed in the CI output, compiling & testing up to rosbag2 encompasses now the following packages:

8. test
8.1. rosbag2_test_common
8.2. rosbag2_storage
8.3. rosbag2_compression
8.4. rosbag2_cpp
8.5. rosbag2_storage_default_plugins
8.6. rosbag2_converter_default_plugins
8.7. rosbag2_transport
8.8. rosbag2_tests
8.9. rosbag2

Putting this up for review.

[zmichaels11]

A couple comments:

• The removal of type aliases in types.hpp could have been done separately
• Could the code in rosbag2_cpp use the namespace rosbag2?

I want to give this PR another run through since its large.

[Karsten1987]

The namespace should always reflect the package name otherwise this would lead to confusion IMO. Also discussed here: #174 (comment)

As for the types, I agree this could have been separately, but really, this PR is already changing so many files and namespaces, that removing the aliases for the types doesn't make a huge impact either.

[piraka9011]

This is probably for another PR, but shouldn't these be found via the CMakeLists.txt?

[wjwwood]

This is ok since the package's structure is known to the tests, but it can be fragile. You could create a relative path in CMake and pass it as a compiler definition instead. At least then it wouldn't be hardcoded in so many places. But the use of a relative path here is in principle safe.

[piraka9011]

LGTM. It seems all the packages have been renamed correctly.

[zmichaels11]

@Karsten1987 I think you should get more people's input before merging due to how big this is.
Besides that, LGTM

[wjwwood]

Looks reasonable to me. It's a lot of churn, but now's the time to do it.

[zmichaels11]

@Karsten1987 the build is probably going to fail due to a new unit test I added in the compression metadata PR that was just merged. I can add the fix if you want.

The changes needed are:

diff --git a/rosbag2_cpp/test/rosbag2_cpp/test_info.cpp b/rosbag2_cpp/test/rosbag2_cpp/test_info.cpp
index 40608fc..b3cec52 100644
--- a/rosbag2_cpp/test/rosbag2_cpp/test_info.cpp
+++ b/rosbag2_cpp/test/rosbag2_cpp/test_info.cpp
@@ -64,7 +64,7 @@ TEST_F(TemporaryDirectoryFixture, read_metadata_supports_version_2) {
     fout << bagfile;
   }
 
-  rosbag2::Info info;
+  rosbag2_cpp::Info info;
   const auto metadata = info.read_metadata(temporary_dir_path_, "sqlite3");
 
   EXPECT_EQ(metadata.version, 2);

and

diff --git a/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp b/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp
index 278913c..a77c46d 100644
--- a/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp
+++ b/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp
@@ -141,9 +141,9 @@ TEST_F(SequentialWriterTest, open_throws_error_if_converter_plugin_does_not_exis
 }
 
 TEST_F(SequentialWriterTest, open_throws_error_on_invalid_splitting_size) {
-  auto sequential_writer = std::make_unique<rosbag2::writers::SequentialWriter>(
+  auto sequential_writer = std::make_unique<rosbag2_cpp::writers::SequentialWriter>(
     std::move(storage_factory_), converter_factory_, std::move(metadata_io_));
-  writer_ = std::make_unique<rosbag2::Writer>(std::move(sequential_writer));
+  writer_ = std::make_unique<rosbag2_cpp::Writer>(std::move(sequential_writer));
 
   // Set minimum file size greater than max bagfile size option
   const uint64_t min_split_file_size = 10;

[Karsten1987]

• Linux
• Linux-aarch64
• macOS
• Windows // buildfarm hiccup

[piraka9011]

Running another windows build:

• Windows

[Karsten1987]

windows:

[zmichaels11]

@Karsten1987 I think we're good for merge on green CI.