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

Introducing Reindexer CLI #699

Merged
merged 14 commits into from
Apr 2, 2021
Merged

Introducing Reindexer CLI #699

merged 14 commits into from
Apr 2, 2021

Conversation

jhdcs
Copy link
Contributor

@jhdcs jhdcs commented Mar 30, 2021 •
edited by emersonknapp
Loading

Fixes #395
Followup to #641

This PR adds the ros2bag/rosbag2_py CLI so that end users can reindex their bag files.

@jhdcs jhdcs requested a review from a team as a code owner March 30, 2021 13:21
@jhdcs jhdcs requested review from emersonknapp and Karsten1987 and removed request for a team March 30, 2021 13:21
emersonknapp
emersonknapp approved these changes Mar 30, 2021
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

LGTM once green CI

Small comment - did you intent to re-replace the multiple_files test, or do you want to revert that back to the fixed master version?

rosbag2_py/test/test_reindexer.py Outdated Show resolved Hide resolved
rosbag2_py/src/rosbag2_py/_reindexer.cpp Outdated Show resolved Hide resolved
@jhdcs
Copy link
Contributor Author

jhdcs commented Mar 30, 2021

Oh crud! No, I didn't intend to re-upload the broken test... Also, I might have had something funny go on with the rebasing I did. Looks like you found a place that might have occurred? I'll get your "nitpicks" fixed! (I quoted Nitpicks, because I don't think they're very nit-picky)

@emersonknapp
Copy link
Collaborator

Sorry - this'll need a quick rebase and conflict resolution - we've been touching some of the same files. It should be a simple resolution, though, we both added a python module to rosbag2_py

@jhdcs
Copy link
Contributor Author

jhdcs commented Apr 1, 2021

It looks like the rebase was successful on my end, so hopefully this works for you too?

@emersonknapp
Copy link
Collaborator

Ah - I'm realizing why those database files are re-added, and it's that we moved them to rosbag2_tests in the previous step, so that we could be sure to have access to rosbag2_storage_default_plugins in rosbag2_cpp. As this stands, we do have access to those plugins from rosbag2_py as a <test_depend>, so i'll just kick off CI now

@jhdcs
Copy link
Contributor Author

jhdcs commented Apr 1, 2021

Wait, I'm confused. Should I remove the database files? I thought I had stopped re-introducing them. Unless you meant the ones in rosbag2_py, which I'm just using as a test. If there's a way to just use one set of database files across all packages that would be nice... Or did I already do that before?

I think I'm making myself confused...

@emersonknapp
Copy link
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/a0f471c5073bd874049e9c10ddb2bfe9/raw/5eae459c861e4b2e910b1530f57b3ca917adc8c2/ros2.repos
BUILD args: --packages-up-to rosbag2_py ros2bag rosbag2_tests
TEST args: --packages-select rosbag2_py ros2bag rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8072

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

@emersonknapp
Copy link
Collaborator

emersonknapp commented Apr 1, 2021 •
edited
Loading

These multiple_files db3 files look like a duplicate of the ones that you originally introduced to rosbag2_cpp for the Reindexer class test. We then moved them to rosbag2_tests. I'm worried if they're exact copies of the originals, then they will also have a problem on OSX - we'll see when the above CI goes through. I'm ok with a little bit of the duplication right now - the alternatives i see are moving this reindexer test to rosbag2_tests, or moving the test bag assets to rosbag2_test_common as a common resource for packages to use. I don't feel too particular about fixing that now though.

Note: there's a failing test problem right now that those will show for rosbag2_py's Transport tests - see the conversation at #702. We'll need to make sure it's green before merging this, but I'll help push it through.

@jhdcs
Copy link
Contributor Author

jhdcs commented Apr 1, 2021

I thought I had copied over the ones you added. If there's a problem with the OSX test, then I'll delete them, and re-copy your database files again.

@jhdcs
Copy link
Contributor Author

jhdcs commented Apr 1, 2021

Looks like that might have been the old database! All right, swapping it out...

@emersonknapp
Copy link
Collaborator

Note: just waiting on #705 before moving this forward. I'll do a rebase and test asap (probably tomorrow morning after the WG meeting) then merge if it's green

Jacob Hassold added 12 commits April 2, 2021 00:54
Starting work on reindexer python interface
6e15bf9 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Adding test database files
0d71b58 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Remove dead imports
36ade5b 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Start creating unit test for reindexer ui
2922ffb 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
[WIP] Link up reindexer via CMake.
7d94a00 
Does not compile.
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
First working version of CLI
ba7b583 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Reindex is callable from terminal
9e2260c 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Fix linting errors
ba54ee1 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Enhance code clarity
4355d21 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Re-order variable creation
9801609 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Fix bag_path being a string when supposed to
9b9e2e0 
be a path
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Remove unused imports
7e9b124 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Jacob Hassold added 2 commits April 2, 2021 00:54
Replace database files with ones from
a995017 
rosbag2_tests
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Use the PROPER database. Also remove unneeded file
1b6606d 
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
@emersonknapp
Copy link
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/a848456e0c1996c86fb34abc646482bc/raw/5eae459c861e4b2e910b1530f57b3ca917adc8c2/ros2.repos
BUILD args: --packages-up-to rosbag2_py ros2bag rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_py ros2bag rosbag2_tests rosbag2
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8087

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

@emersonknapp
Copy link
Collaborator

Windows warnings are the following two:

@emersonknapp emersonknapp merged commit 8f7e42b into ros2:master Apr 2, 2021
MichaelOrlov pushed a commit to MichaelOrlov/rosbag2 that referenced this pull request Apr 3, 2021
@jhdcs @MichaelOrlov
Introducing Reindexer CLI (ros2#699)
1d226a7 
* CLI interface to the rosbag2_cpp Reindexer `ros2 bag reindex`
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Kettenhoax pushed a commit to Kettenhoax/rosbag2 that referenced this pull request Apr 9, 2021
@jhdcs
Introducing Reindexer CLI (ros2#699)
bbf9245 
* CLI interface to the rosbag2_cpp Reindexer `ros2 bag reindex`
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
emersonknapp pushed a commit that referenced this pull request Aug 31, 2021
@jhdcs @emersonknapp
Introducing Reindexer CLI (#699)
d0969eb 
* CLI interface to the rosbag2_cpp Reindexer `ros2 bag reindex`
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
@emersonknapp emersonknapp mentioned this pull request Aug 31, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp approved these changes

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

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

Successfully merging this pull request may close these issues.

DB3 bag is irrecoverable if metadata.yaml doesn't exist
2 participants
@jhdcs @emersonknapp