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.

Sign up for GitHub

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

Jump to bottom

Fix ros2 bag play for split bags #268

Merged
merged 1 commit into from
Jan 31, 2020
Merged

Fix ros2 bag play for split bags #268

merged 1 commit into from
Jan 31, 2020

Conversation

zmichaels11
Copy link
Contributor

Fixes problem where SequentialReader is not loading storage from split bagfiles.

@zmichaels11 zmichaels11 force-pushed the zmichaels11/fix-ros2bag-play-split-bags branch from 142a59d to 5f74acb Compare January 27, 2020 18:27
@zmichaels11 zmichaels11 requested review from Karsten1987, piraka9011, dabonnie and mm318 and removed request for Karsten1987 January 27, 2020 18:35
piraka9011
piraka9011 approved these changes Jan 27, 2020
View reviewed changes
Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

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

LGTM pending green CI.

@zmichaels11
Copy link
Contributor Author

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/d6621d10346dd152fb6ad47c25f2c6dd/raw/60a6f549e55489a079ca4039e0f12b5bcd733c1d/ros2.repos
BUILD args: --packages-up-to rosbag2_storage_default_plugins rosbag2_tests
TEST args: --packages-select rosbag2_storage_default_plugins rosbag2_tests
Job: ci_launcher

@zmichaels11
Copy link
Contributor Author

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

Karsten1987
Karsten1987 reviewed Jan 28, 2020
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

can you link to an issue / failing tests which is indicating this problem? This very much sounds like a regression to me and I wonder why we haven't figured this one out earlier.
If there's no test available, I very much require to extend this PR to have one.

Karsten1987
Karsten1987 approved these changes Jan 28, 2020
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

with green CI

@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Jan 28, 2020 •
edited
Loading

It looks like there's also some sort of regression in rosbag2_tests where the test directory is sometimes not cleared out on Windows. I've opened a PR to resolve this.

@piraka9011 piraka9011 force-pushed the zmichaels11/fix-ros2bag-play-split-bags branch from 5f74acb to f00c4c7 Compare January 31, 2020 18:35
@zmichaels11
Fix ros2 bag play on split bags
c342edb 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@piraka9011 piraka9011 force-pushed the zmichaels11/fix-ros2bag-play-split-bags branch from f00c4c7 to c342edb Compare January 31, 2020 19:14
@zmichaels11
Copy link
Contributor Author

CI job
Gist: https://gist.githubusercontent.com/zmichaels11/4d5742701cd4972bfa0d89729f20c261/raw/60a6f549e55489a079ca4039e0f12b5bcd733c1d/ros2.repos
BUILD args: --packages-up-to rosbag2_cpp rosbag2_tests
TEST args: --packages-select rosbag2_cpp rosbag2_tests
Job: ci_launcher

@zmichaels11
Copy link
Contributor Author

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

@zmichaels11 zmichaels11 merged commit 57b6b2f into ros2:master Jan 31, 2020
@zmichaels11 zmichaels11 deleted the zmichaels11/fix-ros2bag-play-split-bags branch January 31, 2020 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@piraka9011 piraka9011 piraka9011 approved these changes

@dabonnie dabonnie Awaiting requested review from dabonnie

@mm318 mm318 Awaiting requested review from mm318

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zmichaels11 @Karsten1987 @piraka9011