ros2 / rosbag2

Apache License 2.0
285 stars 251 forks source link

Bugfix for rosbag2_cpp serialization converter #1814

Closed MichaelOrlov closed 1 month ago

MichaelOrlov commented 1 month ago
MichaelOrlov commented 1 month ago

Pulls: ros2/rosbag2#1814 Gist: https://gist.githubusercontent.com/MichaelOrlov/7edb3db78c6c16f382dfbce73551159b/raw/8b648a41eb4c66a65d673f6944b28d3ccb59dd37/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/14590

MichaelOrlov commented 1 month ago

Re-run Windows CI build after an attempt to fix warnings

MichaelOrlov commented 1 month ago

@clalancette We are getting a new warning on RHEL build from the

Sanitizers aren't supported by the compiler or environment - disabling

It is from the: https://github.com/ros2/rosbag2/blob/e1c0cf9794868f3bc7929907b27a9d86dc8a57f2/rosbag2_cpp/CMakeLists.txt#L38

This warning appeared because I enabled sanitizer by default and RHEL build doesn't support them for some reason.

Please correct me if I am wrong, but if I understood correctly, merging with such a warning is not acceptable. However, I don't want to remove such a warning at all or disable sanitizers by default on CI.
Meanwhile, I will try to implement a workaround by detecting the RHEL build in CMake and turning the sanitizers off before that check. Please let me know if there are other options for a workaround or solution to those warning.

clalancette commented 1 month ago

Please correct me if I am wrong, but if I understood correctly, merging with such a warning is not acceptable.

Yes, that's correct; we shouldn't merge with that warning.

However, I don't want to remove such a warning at all or disable sanitizers by default on CI.

I have a slightly different suggestion here, which is to not enable sanitizers in this PR. It is not integral to this PR, so I think it should be done separately.

In that separate PR, I think it is fine to skip the sanitizer on RHEL (or just downgrade the WARNING to STATUS), but at least it will be separate and easier to change/revert if we decide to do that later.

MichaelOrlov commented 1 month ago

Okay, I disabled sanitizers by default, as @clalancette suggested. We will enable them and try to workaround the warning on the RHEL build in a separate PR. Rerunning RHEL CI build

MichaelOrlov commented 1 month ago

https://github.com/Mergifyio backport jazzy iron

mergify[bot] commented 1 month ago

backport jazzy iron

✅ Backports have been created

* [#1822 Bugfix for rosbag2_cpp serialization converter (backport #1814)](https://github.com/ros2/rosbag2/pull/1822) has been created for branch `jazzy` * [#1823 Bugfix for rosbag2_cpp serialization converter (backport #1814)](https://github.com/ros2/rosbag2/pull/1823) has been created for branch `iron` but encountered conflicts