Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iron] Bugfix for rosbag2_cpp serialization converter (backport #1814) #1823

Merged
merged 6 commits into from
Sep 26, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Sep 25, 2024


This is an automatic backport of pull request #1814 done by [Mergify](https://mergify.com).

* Bugfix for rosbag2 serialization converter

- Use rmw specific type support for rmw_serilize{deserialize} function
calls.
Note: It is ok for CycloneDDS to use introspection type support for
rmw_serilize{deserialize} functions. However, for FastRTPS it must be
rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for
both CycloneDDS and FastRTPS rmw.

Signed-off-by: Michael Orlov <[email protected]>

* Add test coverage for default rmv serialization format converter

Signed-off-by: Michael Orlov <[email protected]>

* Run test_serialization_converter for each rmw implementation

- Rationale: To make sure that the default serialization converter can
serialize and deserialize messages with all supported rmw
implementations. Since it uses rmw specific functions for serialization
and deserialization inside.

Signed-off-by: Michael Orlov <[email protected]>

* Address uncrustify formating warnings

Signed-off-by: Michael Orlov <[email protected]>

* Enable sanitizer by default

Signed-off-by: Michael Orlov <[email protected]>

* Address Windows build warnings

Signed-off-by: Michael Orlov <[email protected]>

* Revert "Enable sanitizer by default"

This reverts commit 7241963.

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit 6e82f52)

# Conflicts:
#	rosbag2_cpp/src/rosbag2_cpp/converter.cpp
@mergify mergify bot added the conflicts label Sep 25, 2024
@mergify mergify bot requested a review from a team as a code owner September 25, 2024 20:02
@mergify mergify bot requested review from emersonknapp and jhdcs and removed request for a team September 25, 2024 20:02
Copy link
Author

mergify bot commented Sep 25, 2024

Cherry-pick of 6e82f52 has failed:

On branch mergify/bp/iron/pr-1814
Your branch is up to date with 'origin/iron'.

You are currently cherry-picking commit 6e82f52f.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   rosbag2_cpp/CMakeLists.txt
	modified:   rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_converter.hpp
	modified:   rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_deserializer.hpp
	modified:   rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_serializer.hpp
	renamed:    rosbag2_cpp/src/rosbag2_cpp/rmw_implemented_serialization_format_converter.hpp -> rosbag2_cpp/include/rosbag2_cpp/rmw_implemented_serialization_format_converter.hpp
	modified:   rosbag2_cpp/package.xml
	modified:   rosbag2_cpp/src/rosbag2_cpp/rmw_implemented_serialization_format_converter.cpp
	modified:   rosbag2_cpp/src/rosbag2_cpp/serialization_format_converter_factory_impl.hpp
	new file:   rosbag2_cpp/test/rosbag2_cpp/test_serialization_converter.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   rosbag2_cpp/src/rosbag2_cpp/converter.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@MichaelOrlov MichaelOrlov changed the title Bugfix for rosbag2_cpp serialization converter (backport #1814) [iron] Bugfix for rosbag2_cpp serialization converter (backport #1814) Sep 25, 2024
Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov
Copy link
Contributor

The changes in the PR are ABI/API compatible because we moved RMWImplementedConverter class declaration from the src folder to the include folder and made it publicaly available in the dll.
This is equivalent to adding a new class to the dll.
According to the https://acodersjourney.com/20-abi-breaking-changes/ - this is an ABI compatible changes.

@MichaelOrlov
Copy link
Contributor

Pulls: #1823
Gist: https://gist.githubusercontent.com/MichaelOrlov/26ced40d72ff85483c4f0e7fc9427dab/raw/d255e6babae5f6a9027a5beb3b1d2415e4d15413/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp
TEST args: --packages-above rosbag2_cpp
ROS Distro: iron
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14616

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor

Pulls: #1823
Gist: https://gist.githubusercontent.com/MichaelOrlov/889d3a9b53433b3f1f0d4d6dd29ef20c/raw/d255e6babae5f6a9027a5beb3b1d2415e4d15413/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp
TEST args: --packages-above rosbag2_cpp
ROS Distro: iron
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14617

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

- Rationale:
 The ament_add_gmock_executable(..) and ament_add_gmock_test(..) macros
are not available on Iron distro.

Signed-off-by: Michael Orlov <[email protected]>
- removed TopicId field initialization since it is absent on Iron

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov
Copy link
Contributor

Pulls: #1823
Gist: https://gist.githubusercontent.com/MichaelOrlov/19b3219cfeebd91be9631b8c46933f5a/raw/d255e6babae5f6a9027a5beb3b1d2415e4d15413/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp
TEST args: --packages-above rosbag2_cpp
ROS Distro: iron
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14619

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor

Warnings on RHEL build no CONNEXTDDS_DIR nor NDDSHOME specified is a known issue and unrelated to the changes from this PR.

@MichaelOrlov MichaelOrlov merged commit a269cd4 into iron Sep 26, 2024
14 checks passed
@MichaelOrlov MichaelOrlov deleted the mergify/bp/iron/pr-1814 branch September 26, 2024 16:53
@MichaelOrlov
Copy link
Contributor

https://github.com/Mergifyio backport humble

Copy link
Author

mergify bot commented Sep 26, 2024

backport humble

✅ Backports have been created

mergify bot added a commit that referenced this pull request Sep 26, 2024
#1823)

* Bugfix for rosbag2_cpp serialization converter (#1814)

* Bugfix for rosbag2 serialization converter

- Use rmw specific type support for rmw_serilize{deserialize} function
calls.
Note: It is ok for CycloneDDS to use introspection type support for
rmw_serilize{deserialize} functions. However, for FastRTPS it must be
rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for
both CycloneDDS and FastRTPS rmw.

Signed-off-by: Michael Orlov <[email protected]>

* Add test coverage for default rmv serialization format converter

Signed-off-by: Michael Orlov <[email protected]>

* Run test_serialization_converter for each rmw implementation

- Rationale: To make sure that the default serialization converter can
serialize and deserialize messages with all supported rmw
implementations. Since it uses rmw specific functions for serialization
and deserialization inside.

Signed-off-by: Michael Orlov <[email protected]>

* Address uncrustify formating warnings

Signed-off-by: Michael Orlov <[email protected]>

* Enable sanitizer by default

Signed-off-by: Michael Orlov <[email protected]>

* Address Windows build warnings

Signed-off-by: Michael Orlov <[email protected]>

* Revert "Enable sanitizer by default"

This reverts commit 7241963.

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit 6e82f52)

# Conflicts:
#	rosbag2_cpp/src/rosbag2_cpp/converter.cpp

* Address merge conflicts

Signed-off-by: Michael Orlov <[email protected]>

* Fix uncrustfy warning due to the different versions of uncrustify

Signed-off-by: Michael Orlov <[email protected]>

* Replace ament_add_gmock_test with ament_add_gmock

- Rationale:
 The ament_add_gmock_executable(..) and ament_add_gmock_test(..) macros
are not available on Iron distro.

Signed-off-by: Michael Orlov <[email protected]>

* Add missing dependencies from the std_msgs

Signed-off-by: Michael Orlov <[email protected]>

* Adjust writer_->create_topic(..) call in tests

- removed TopicId field initialization since it is absent on Iron

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Michael Orlov <[email protected]>
(cherry picked from commit a269cd4)

# Conflicts:
#	rosbag2_cpp/CMakeLists.txt
MichaelOrlov added a commit that referenced this pull request Oct 2, 2024
…) (backport #1823) (#1824)

* [iron] Bugfix for rosbag2_cpp serialization converter (backport #1814) (#1823)

* Bugfix for rosbag2_cpp serialization converter (#1814)

* Bugfix for rosbag2 serialization converter

- Use rmw specific type support for rmw_serilize{deserialize} function
calls.
Note: It is ok for CycloneDDS to use introspection type support for
rmw_serilize{deserialize} functions. However, for FastRTPS it must be
rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for
both CycloneDDS and FastRTPS rmw.

Signed-off-by: Michael Orlov <[email protected]>

* Add test coverage for default rmv serialization format converter

Signed-off-by: Michael Orlov <[email protected]>

* Run test_serialization_converter for each rmw implementation

- Rationale: To make sure that the default serialization converter can
serialize and deserialize messages with all supported rmw
implementations. Since it uses rmw specific functions for serialization
and deserialization inside.

Signed-off-by: Michael Orlov <[email protected]>

* Address uncrustify formating warnings

Signed-off-by: Michael Orlov <[email protected]>

* Enable sanitizer by default

Signed-off-by: Michael Orlov <[email protected]>

* Address Windows build warnings

Signed-off-by: Michael Orlov <[email protected]>

* Revert "Enable sanitizer by default"

This reverts commit 7241963.

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit 6e82f52)

# Conflicts:
#	rosbag2_cpp/src/rosbag2_cpp/converter.cpp

* Address merge conflicts

Signed-off-by: Michael Orlov <[email protected]>

* Fix uncrustfy warning due to the different versions of uncrustify

Signed-off-by: Michael Orlov <[email protected]>

* Replace ament_add_gmock_test with ament_add_gmock

- Rationale:
 The ament_add_gmock_executable(..) and ament_add_gmock_test(..) macros
are not available on Iron distro.

Signed-off-by: Michael Orlov <[email protected]>

* Add missing dependencies from the std_msgs

Signed-off-by: Michael Orlov <[email protected]>

* Adjust writer_->create_topic(..) call in tests

- removed TopicId field initialization since it is absent on Iron

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Michael Orlov <[email protected]>
(cherry picked from commit a269cd4)

# Conflicts:
#	rosbag2_cpp/CMakeLists.txt

* Address merge conflicts after automatic backporting

Signed-off-by: Michael Orlov <[email protected]>

* Adjust tests for humble

Signed-off-by: Michael Orlov <[email protected]>

* Address linker errors on RHEL 8

- Use rcpputils::fs instead of the std::filesystem in tests. Since RHEL8
doesn't have adequate support for the std::filesystem.

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Michael Orlov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant