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 relative metadata paths in SequentialCompressionWriter #613

Merged
merged 6 commits into from
Jan 27, 2021
Merged

Fix relative metadata paths in SequentialCompressionWriter #613

merged 6 commits into from
Jan 27, 2021

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Jan 25, 2021 •
edited
Loading

Fixes #557
Related to #612

This makes the SequentialCompressionWriter write in the bag metadata with correct relative paths in the V4 format.

  • Use common init_metadata logic to get the correct relative path there
  • In the file compression, be much more careful about what the paths truly mean, given that the metadata requires a path relative to the bag, but the compressor just needs to find and compress a file on the filesystem
  • Factor out some common test logic so the new test can be a bit more readable

@emersonknapp emersonknapp requested review from mjeronimo and a team as code owners January 25, 2021 19:16
@emersonknapp emersonknapp requested review from zmichaels11 and prajakta-gokhale and removed request for a team January 25, 2021 19:16
@emersonknapp emersonknapp force-pushed the emersonknapp/compression-writer-correct-relative-paths branch from 5504e9a to aa0dbf1 Compare January 25, 2021 21:05
@emersonknapp emersonknapp changed the title Fix relative metadata path writing in compression by deduplicating business logic Fix relative metadata paths in SequentialCompressionWriter Jan 25, 2021
@emersonknapp emersonknapp force-pushed the emersonknapp/compression-writer-correct-relative-paths branch from 247c48c to 03d26d5 Compare January 25, 2021 23:04
@emersonknapp
Copy link
Collaborator Author

Gist: https://gist.githubusercontent.com/emersonknapp/48d4065acf325d5ca483591e2588d687/raw/cc4ce0d6c338621125898050f630578454945852/ros2.repos
BUILD args: --packages-up-to rosbag2_compression rosbag2_tests
TEST args: --packages-select rosbag2_compression rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/7513

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

@jikawa-az jikawa-az self-requested a review January 26, 2021 03:33
Emerson Knapp added 3 commits January 26, 2021 17:05
Fix relative metadata path writing in compression by deduplicating bu…
03f09d3 
…siness logic

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Address review comment by making error message even more explicit
cd41944 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Old end to end test enforced wrong behavior
bf9bd04 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/compression-writer-correct-relative-paths branch from c593177 to bf9bd04 Compare January 27, 2021 01:06
Use a local temporary directory, rather than the global one, for more…
028f859 
… portability.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/compression-writer-correct-relative-paths branch 4 times, most recently from 5f39bbf to c50ec86 Compare January 27, 2021 19:36
WIP more aggressive loggingto try and understand if this code is bein…
8506827 
…g run properly in the github test

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/compression-writer-correct-relative-paths branch from c50ec86 to 8506827 Compare January 27, 2021 19:36
Revert debugging changes and fix the synchronization problem
9990110 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/compression-writer-correct-relative-paths branch from 2f0406d to 9990110 Compare January 27, 2021 22:21
@emersonknapp
Copy link
Collaborator Author

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

@emersonknapp emersonknapp merged commit c4912cd into master Jan 27, 2021
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/compression-writer-correct-relative-paths branch January 27, 2021 23:11
emersonknapp added a commit that referenced this pull request Jan 28, 2021
@emersonknapp
[foxy backport] Fix relative metadata paths in SequentialCompressionW…
eb02132 
…riter (#613)

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp mentioned this pull request Jan 28, 2021
Closed
emersonknapp added a commit that referenced this pull request Feb 2, 2021
@emersonknapp
Fix relative metadata paths in SequentialCompressionWriter (#613)
59aa96a 
* Fix relative metadata path writing in compression by deduplicating business logic

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
emersonknapp added a commit that referenced this pull request Feb 17, 2021
@emersonknapp
Fix relative metadata paths in SequentialCompressionWriter (#613)
320659a 
* Fix relative metadata path writing in compression by deduplicating business logic

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@prajakta-gokhale prajakta-gokhale prajakta-gokhale approved these changes

@mjeronimo mjeronimo Awaiting requested review from mjeronimo

@jikawa-az jikawa-az Awaiting requested review from jikawa-az

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

Successfully merging this pull request may close these issues.

Recording with compression creates non-standard relative filepaths in metadata
3 participants
@emersonknapp @zmichaels11 @prajakta-gokhale