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

Prevent using message compression mode with mcap storage #1782

Merged

Conversation

defunctzombie
Copy link

MCAP does not support per-message compression. To prevent users from falling into the trap of recording such files, this change throws an error if per-message compression is used with the mcap storage plugin.

This is a backport of the same behavior present in iron onwards: (https://github.com/ros2/rosbag2/blob/iron/rosbag2_storage_mcap/src/mcap_storage.cpp#L821).

Continuation of #1705 which has gone dead.

@defunctzombie defunctzombie requested a review from a team as a code owner August 9, 2024 21:36
@defunctzombie defunctzombie requested review from gbiggs and jhdcs and removed request for a team August 9, 2024 21:36
@defunctzombie defunctzombie force-pushed the roman/humble-prevent-message-compression branch 3 times, most recently from 8be5d42 to fdf998d Compare August 9, 2024 21:48
@defunctzombie
Copy link
Author

@MichaelOrlov since #1705 seems to have gone dead - I've re-created the PR here with the changes and passing lint.

@defunctzombie
Copy link
Author

I don't understand the DCO check - if that's something I am supposed to do or if a maintainer does the sign-off.

@MichaelOrlov
Copy link
Contributor

@defunctzombie The DCO is a sort of signature from the committer claiming that the committer is the author of the changes.
Please see instruction how to sign your commits https://github.com/ros2/rosbag2/pull/1782/checks?check_run_id=28589105101

@MichaelOrlov
Copy link
Contributor

@defunctzombie Could you please retarget your branch to the rolling branch?
When we merge PR to rolling, we will backport it to all other active distros with a "simplified" approval workflow.

@defunctzombie
Copy link
Author

@defunctzombie Could you please retarget your branch to the rolling branch?
When we merge PR to rolling, we will backport it to all other active distros with a "simplified" approval workflow.

These changes are specifically against humble branch because the rolling branch has a completely different approach. I did what the original PR did - was that not the right thing to do?

@defunctzombie defunctzombie force-pushed the roman/humble-prevent-message-compression branch from fdf998d to 107c035 Compare August 9, 2024 23:38
@MichaelOrlov
Copy link
Contributor

@defunctzombie Sorry, I've already forgotten a point about the initial PR purpose.
Approving then.

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.

LGTM with green CI.

@MichaelOrlov
Copy link
Contributor

Pulls: #1782
Gist: https://gist.githubusercontent.com/MichaelOrlov/676291ba142aba4fe065dff7f011720d/raw/b5e0484e74012758a2f8cd24c092eed3377dd28f/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_compression rosbag2_cpp rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_compression rosbag2_cpp rosbag2_tests
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14401

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

@MichaelOrlov MichaelOrlov merged commit 9f766bf into ros2:humble Aug 10, 2024
14 checks passed
@defunctzombie defunctzombie deleted the roman/humble-prevent-message-compression branch August 12, 2024 15:55
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.

2 participants