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

Add the ability to record any key/value pair in 'custom' field in metadata.yaml #1038

Merged
merged 2 commits into from
Jul 7, 2022
Merged

Add the ability to record any key/value pair in 'custom' field in metadata.yaml #1038

merged 2 commits into from
Jul 7, 2022

Conversation

allenh1
Copy link
Contributor

@allenh1 allenh1 commented Jul 5, 2022

This restores the changes from #976 that were reverted in #984.

Additionally, a fix was added to correct the destruction order in SequentialCompressionWriterTest.

rosbag2/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp

Lines 61 to 64 in e1f4349

ON_CALL(*metadata_io_, write_metadata).WillByDefault(
[this](const std::string &, const rosbag2_storage::BagMetadata & metadata) {
intercepted_metadata_ = metadata;
});

The write_metadata call registered by the above hook is being invoked by the destructor for writer_. At this point, however, the intercepted_metadata_ object has already been destructed, leading to a segv.

@allenh1 allenh1 requested a review from a team as a code owner July 5, 2022 20:46
@allenh1 allenh1 self-assigned this Jul 5, 2022
@allenh1 allenh1 requested review from gbiggs, emersonknapp and MichaelOrlov and removed request for a team July 5, 2022 20:46
@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/f369c45f7fc66b7ec4d59dfa09eb0893/raw/e532c13f7dc3a8293cf61bfe23856d7524753c4a/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_compression rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_compression rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10475

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

MichaelOrlov
MichaelOrlov approved these changes Jul 5, 2022
View reviewed changes
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.

@allenh1 Thanks for your contribution and fixing issue with Windows build!
LGTM.

MichaelOrlov
MichaelOrlov requested changes Jul 5, 2022
View reviewed changes
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.

@allenh1 Unfortunately CI build got failed, but now in another place.
SequentialWriterTest.split_event_calls_callback
Could you please try to figure out why it fails.

@gbiggs Could you please help with analysis in SequentialWriterTest.split_event_calls_callback test failure, it was recently added by you.

@allenh1 allenh1 force-pushed the restore-ability-to-record-key-value-pair branch from 88c48c0 to ead8856 Compare July 6, 2022 19:51
@allenh1 allenh1 requested a review from MichaelOrlov July 6, 2022 19:51
@allenh1
Copy link
Contributor Author

allenh1 commented Jul 6, 2022

@MichaelOrlov Should be fixed now.

allenh1 added 2 commits July 6, 2022 18:58
@allenh1
Revert "Revert "Add the ability to record any key/value pair in the '…
e2ac968 
…custom' field in metadata.yaml (#976)" (#984)"

This reverts commit e7d4975.

Signed-off-by: Hunter L. Allen <hunter.allen@apex.ai>
@allenh1
Ensure writer_ is destructed before intercepted_metadata_
ead8856 
Signed-off-by: Hunter L. Allen <hunter.allen@apex.ai>
@MichaelOrlov
Copy link
Contributor

Re-run CI:
Gist: https://gist.githubusercontent.com/MichaelOrlov/f369c45f7fc66b7ec4d59dfa09eb0893/raw/e532c13f7dc3a8293cf61bfe23856d7524753c4a/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_compression rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_compression rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10486

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

@MichaelOrlov MichaelOrlov merged commit 5dc9e7f into rolling Jul 7, 2022
@delete-merged-branch delete-merged-branch bot deleted the restore-ability-to-record-key-value-pair branch July 7, 2022 02:15
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-8-18-2022/27050/1

@roncapat roncapat mentioned this pull request Nov 4, 2023
Merged
emersonknapp pushed a commit that referenced this pull request May 14, 2024
@allenh1 @emersonknapp
Add the ability to record any key/value pair in 'custom' field in met…
f8f5a5c 
…adata.yaml (#1038)

* Revert "Revert "Add the ability to record any key/value pair in the 'custom' field in metadata.yaml (#976)" (#984)"

This reverts commit e7d4975.

Signed-off-by: Hunter L. Allen <hunter.allen@apex.ai>

* Ensure writer_ is destructed before intercepted_metadata_

Signed-off-by: Hunter L. Allen <hunter.allen@apex.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

@gbiggs gbiggs Awaiting requested review from gbiggs gbiggs was automatically assigned from ros2/tooling-reviewers

@emersonknapp emersonknapp Awaiting requested review from emersonknapp emersonknapp was automatically assigned from ros2/tooling-reviewers

Assignees

@allenh1 allenh1

Labels
needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@allenh1 @MichaelOrlov @ros-discourse