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

[Humble] Add SplitBagfile recording service. (backport #1115) #1124

Closed
wants to merge 6 commits into from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Oct 14, 2022

This is an automatic backport of pull request #1115 done by Mergify.
Cherry-pick of e6f7fd7 has failed:

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

You are currently cherry-picking commit e6f7fd7.
  (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/include/rosbag2_cpp/writer.hpp
	modified:   rosbag2_cpp/include/rosbag2_cpp/writer_interfaces/base_writer_interface.hpp
	modified:   rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp
	new file:   rosbag2_interfaces/srv/SplitBagfile.srv
	modified:   rosbag2_transport/include/rosbag2_transport/recorder.hpp
	modified:   rosbag2_transport/src/rosbag2_transport/recorder.cpp
	modified:   rosbag2_transport/test/rosbag2_transport/mock_sequential_writer.hpp
	modified:   rosbag2_transport/test/rosbag2_transport/test_record_services.cpp

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	both modified:   rosbag2_cpp/src/rosbag2_cpp/writer.cpp
	deleted by us:   rosbag2_cpp/test/rosbag2_cpp/fake_data.cpp
	deleted by us:   rosbag2_cpp/test/rosbag2_cpp/fake_data.hpp
	both modified:   rosbag2_interfaces/CMakeLists.txt
	both modified:   rosbag2_transport/CMakeLists.txt

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


Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

@mergify mergify bot requested a review from a team as a code owner October 14, 2022 00:10
@mergify mergify bot requested review from emersonknapp and MichaelOrlov and removed request for a team October 14, 2022 00:10
@mergify mergify bot added the conflicts label Oct 14, 2022
@emersonknapp emersonknapp self-assigned this Oct 15, 2022
@rshanor
Copy link
Contributor

rshanor commented Oct 15, 2022

@emersonknapp Thanks! Let me know if there is anything I can do to help here.

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rshanor Please remove automatically added fake_data.cpp and fake_data.hpp by mergify.
In humble we don't need them, since we don't have tests which depends up on them.

As regards to the API and ABI stability IMO with this PR we are not breaking currently defined publicly available API and still will be ABI compatible since we are not changing any structures or public API only adding new.
Although would like to hear a second opinion.
@clalancette Please review/approve in regards API/ABI stability.
We are essentially propagating protected void split_bagfile()
To the public void split_bagfile() override; added in base_writer_interface.hpp.

@MichaelOrlov MichaelOrlov changed the title Add SplitBagfile recording service. (backport #1115) [Humble] Add SplitBagfile recording service. (backport #1115) Oct 18, 2022
@rshanor
Copy link
Contributor

rshanor commented Oct 18, 2022

@MichaelOrlov #1128

rshanor and others added 6 commits October 18, 2022 01:00
* feat(rosbag2_cpp): Add SplitBagfile recording service.

Fixes #1087

Tested from the command line and verified that below command closed one
log file and opened another.

ros2 service call /rosbag2_recorder/split_bagfile
rosbag2_interfaces/srv/SplitBagfile

Signed-off-by: Rick Shanor <[email protected]>

* feat(rosbag2_cpp): Add unit tests for SplitBagfile feature.

Also address PR comments from @MichaelOrlov and deal with rebase merge
conflicts.

Signed-off-by: Rick Shanor <[email protected]>

* fix(rosbag2_cpp): Remove unnecessary ManualSplitSequentialWriter.

After making split_bagfile public, this class was no longer necessary.

Signed-off-by: Rick Shanor <[email protected]>

* ci(rosbag2_transport): Mark test_record_services as xfail.

Signed-off-by: Rick Shanor <[email protected]>

Signed-off-by: Rick Shanor <[email protected]>
(cherry picked from commit e6f7fd7)

# Conflicts:
#	rosbag2_cpp/src/rosbag2_cpp/writer.cpp
#	rosbag2_cpp/test/rosbag2_cpp/fake_data.cpp
#	rosbag2_cpp/test/rosbag2_cpp/fake_data.hpp
#	rosbag2_interfaces/CMakeLists.txt
#	rosbag2_transport/CMakeLists.txt
Not required on humble branch.

Signed-off-by: Rick Shanor <[email protected]>

Signed-off-by: Rick Shanor <[email protected]>
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I've tried to fix DCO. But no luck.

@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/7784be09f2b6f86c8d6f3e6162aac106/raw/64bbaee1d49a8c11ce791748b9145e337ba575e7/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_cpp rosbag2_transport rosbag2_tests
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10998

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

/**
* Close the current bagfile and opens the next bagfile.
*/
virtual void split_bagfile() = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This almost certainly breaks ABI; it changes the vtable for the class, which means that everything else has to move around. So this PR is probably not ABI-safe, and we shouldn't merge it into Humble as-is.

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rshanor I am sorry we can't merge this PR to the Humble since it breaking ABI compatibility and we have strict requirement https://docs.ros.org/en/humble/How-To-Guides/Package-maintainer-guide.html#backporting-to-released-distributions to keep API and ABI compatibility in ROS2 release distros.

See explanation from @clalancette #1124 (comment)

@MichaelOrlov
Copy link
Contributor

@mergify mergify bot deleted the mergify/bp/humble/pr-1115 branch October 25, 2022 17:53
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.

4 participants