(API) Generate bagfile metadata in Writer

[zmichaels11]

This is part of an effort to rework PR #158 into multiple, smaller PRs.

Changes

• Writer can now create BagMetadata, this enables splitting of bagfiles without having to merge intermediate bagfile metadata.

Dependent PRs

• PR Add bagfile splitting support to storage_options and Writer #182
• PR Add get_identifier to io-interfaces for support in bagfile splitting #183

Issues

• ros-security/aws-roadmap#11

[Karsten1987]

looks good to me except one comment.
Will reiterate once the dependent PRs are addressed.

[zmichaels11]

@Karsten1987 I merged #184 into this PR and rebased onto master.
Please review this one.

[Karsten1987]

So I think I stick to my point that having a metadata class instance makes things easier. In the generate_metadata() function, we can then tackle remaining things like the duration, but in general I think it's easier to update the metadata on the fly.

---

With that, are you going to remove the function get_metadata() from the storage interfaces here as well? I feel like that should be part of that PR as well.

[zmichaels11]

@Karsten1987 ok I reworked it so that it can build as much of BagMetadata inline.
I replaced generate_metadata() with finalize_metadata. The only operations that occur within that function are summing up bagfile size and moving the TopicInformation from the Writer owned hashmap to the metadata's vector.

[Karsten1987]

really only a few nitpicks. But generally looks good.

[thomas-moulard]

Left a few comments, I am worried that we may not have proper testing here around the implicit FSM declared in this class (ctor / open / create_topic / remote_topic / read / write / close / dtor being states and not all transitions being allowed).
edit: ^ this is a general concern, probably too big to be addressed in this PR.

The dtor also is doing more than calling close, which makes me nervous (storage factory reset being put aside, which anyway is not necessary here IIUC).

We should also add test to this PR

[zmichaels11]

Opened Issue #195 regarding tests on FSM since the logic in Writer's FSM was not changed in this PR.

[piraka9011]

• Windows

[zmichaels11]

The problem in Windows builds was due to Windows including min and max as macros.
The exact same problem was found here: nlohmann/json#506.

Edit:
Solution was to add to all the CMakelists.txt:

if(WIN32)
  add_definitions(-DNOMINMAX)
endif()

[piraka9011]

• Windows

[piraka9011]

• Windows

[thomas-moulard]

The code LGTM, we should just resolve Karsten's comment on returning dummy paths and testing for those.

[thomas-moulard]

LGTM 🚀

[zmichaels11]

@piraka9011 one last time, but with more unit tests

[Karsten1987]

This looks good to me (with green CI).
Just a nitpick with renaming the constants header file.

[zmichaels11]

• Renamed plugin_constants -> test_constants
• Moved default_max_bagfile_size into test_constants

[Karsten1987]

Please run a new round of CI on this. Also make sure you build packages up to ros2bag and rosbag2_tests and test with packages-select-regex rosbag2

[piraka9011]

Gist: https://gist.githubusercontent.com/piraka9011/d15a12ab0ff8cd4c3f20c7c1f14180b8/raw/7cda0198d9bee189ba8f61a3da0f8801c1ee378e/ros2.repos
BUILD args: --packages-up-to rosbag2 rosbag2_tests
TEST args: --packages-select rosbag2
Job: ci_launcher

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

Given that this PR is touching more than one package, I think we should test all rosbag2 related packages.
Therefore I proposed to set packages-select-regex rosbag2 as test args instead of just testing rosbag2 to avoid regression.

[piraka9011]

Sorry I missed that. Here's the new build:

Gist: https://gist.githubusercontent.com/piraka9011/d15a12ab0ff8cd4c3f20c7c1f14180b8/raw/7cda0198d9bee189ba8f61a3da0f8801c1ee378e/ros2.repos
BUILD args: --packages-up-to rosbag2 rosbag2_tests
TEST args: --packages-select-regex rosbag2
Job: ci_launcher

• Linux
• Linux-aarch64
• macOS
• Windows

[piraka9011]

Gist: https://gist.githubusercontent.com/piraka9011/21c04ad8e74d0f75a3f6bfae01264072/raw/7cda0198d9bee189ba8f61a3da0f8801c1ee378e/ros2.repos
BUILD args: --packages-up-to rosbag2 rosbag2_tests
TEST args: --packages-select-regex rosbag2
Job: ci_launcher

• Linux
• Linux-aarch64
• macOS
• Windows

[zmichaels11]

@Karsten1987 we're green!