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

rosbag2_py: set defaults for config when bag rewriting #1121

Merged

Conversation

james-rms
Copy link
Contributor

@james-rms james-rms commented Oct 11, 2022

Signed-off-by: James Smith james@foxglove.dev

Fixes #1119

@james-rms james-rms marked this pull request as ready for review October 11, 2022 02:26
@james-rms james-rms requested a review from a team as a code owner October 11, 2022 02:26
@james-rms james-rms requested review from MichaelOrlov and hidmic and removed request for a team October 11, 2022 02:26
@james-rms james-rms force-pushed the jrms/convert-default-compression-queue branch 4 times, most recently from 790fd0e to a5872f0 Compare October 11, 2022 03:59
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.

@james-rms Thanks for proposed fix.
LGTM.

@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/37b035b3dac90791e769a837018b63ec/raw/838c31c160cc15706657ec6aecb273077a52baeb/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_py ros2bag rosbag2_tests
TEST args: --packages-above rosbag2_py ros2bag rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10967

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

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Oct 11, 2022

@james-rms I see a lot of new warnings on Windows build about multiple inclusions of the YAML_CPP_DLL.
Those warnings supposed to be fixed in the #964

I am curios about if your branch behind the latest rolling and doesn't contain fixes from #964 ?
If so could you please rebase your branch on top of latest rolling branch?

@james-rms james-rms force-pushed the jrms/convert-default-compression-queue branch 4 times, most recently from 1a1e813 to 73099fc Compare October 11, 2022 22:06
@james-rms
Copy link
Contributor Author

@MichaelOrlov I updated the patch to be type-safe, please let me know what you think.

@MichaelOrlov
Copy link
Contributor

@james-rms I am sorry I don't get it.
What are you trying to solve with type safe update?
And what a difference when calling YAML::convert::decode(bag_node, record_options); explicitly?

@james-rms
Copy link
Contributor Author

@MichaelOrlov here if compression_queue_size is removed or changes name, the code will no longer compile (rather than just setting a field on a YAML node that nobody reads).

@MichaelOrlov
Copy link
Contributor

@james-rms Ok good. Can we do the same for StorageOptions?
auto storage_options = bag_node.as<rosbag2_storage::StorageOptions>();

@MichaelOrlov
Copy link
Contributor

@james-rms BTW there are a couple new warnings from gcc
qualified-id in declaration before ‘(’ token 268 | YAML::convert::decode(bag_node, record_options); | ^
We need to address them before moving forward.

Signed-off-by: James Smith <james@foxglove.dev>
@james-rms james-rms force-pushed the jrms/convert-default-compression-queue branch from 73099fc to 75180f5 Compare October 12, 2022 01:57
@MichaelOrlov
Copy link
Contributor

Trying to re-run CI after updates:
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10974

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

@MichaelOrlov MichaelOrlov merged commit 5b8b658 into ros2:rolling Oct 12, 2022
@james-rms james-rms deleted the jrms/convert-default-compression-queue branch October 12, 2022 22:18
@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-2022-10-13/28213/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ros2 bag convert with compression can drop messages
3 participants