Fix: SequentialWriter incorrect metadata total duration for split bags

[emersonknapp]

Fixes #841

This fix should get backported to Foxy/Galactic/Humble, it's been persistent for a long time. The test infra might not port well but the fix itself is very simple.

[emersonknapp]

Gist: https://gist.githubusercontent.com/emersonknapp/692acfa17f9a48b555e93ef4072b9302/raw/ab57c1b00397503f9dee8ac39ff6e66945f97e70/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp
TEST args: --packages-above rosbag2_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10886

• Linux
• Linux-aarch64
• Windows

[emersonknapp]

@Mergifyio backport humble galactic foxy

[mergify]

backport humble galactic foxy

✅ Backports have been created

• #1101 Fix: SequentialWriter incorrect metadata total duration for split bags (backport #1098) has been created for branch humble
• #1102 Fix: SequentialWriter incorrect metadata total duration for split bags (backport #1098) has been created for branch galactic
• #1103 Fix: SequentialWriter incorrect metadata total duration for split bags (backport #1098) has been created for branch foxy

[MichaelOrlov]

@emersonknapp Thanks for fixing this issue!
LGTM.

[clalancette]

@emersonknapp @MichaelOrlov I'm pretty sure this change broke building on RHEL; see https://ci.ros2.org/view/nightly/job/nightly_linux-rhel_release/1273/console . My guess is because RHEL-8 has an older version of gcc, and thus the version of std::filesystem doesn't work as well as it should. Given that we are only using std::filesystem in a test, maybe we can slightly rewrite the test to be more friendly to RHEL-8?

[emersonknapp]

Yeah this can be easily patched to use rcpputils filsystem helper. Just didn't catch the issue because rhel not in the standard ci suite. Maybe we should add it to ci_launcher?

Regardless, I'm AFK until tomorrow afternoon so I can't fix it until then, reverting would be ok by me if wanted.

[MichaelOrlov]

@clalancette @emersonknapp I prepared fix in #1104 please review.

[clalancette]

Maybe we should add it to ci_launcher?

Yeah, we've gone back and forth on that question. RHEL is a Tier-2 platform, which means that we don't technically guarantee that it will work. On the other hand, we do have people using it, so we want to make sure it works as well as possible.

The problem with adding it to the ci_launcher is that it puts more strain on the CI workers, and we are already running kind of close to the limit. RHEL doesn't usually require special handling, but did in this case. I'm inclined to leave things as-is, and revisit this question if more RHEL-specific problems start to pop up.

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2022-10-13/28213/1