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

Remove relative paths from includes #405

Conversation

zmichaels11
Copy link
Contributor

@zmichaels11 zmichaels11 commented May 6, 2020

Description

Adds rosbag2_cpp/test path to includes in rosbag2_compression to replace relative includes.
Duplicate mocks from rosbag2_cpp in rosbag2_compression so relative includes can be removed.

This should resolve this issue: #404

Follow-up work should be done to de-duplicate this code.

Signed-off-by: Zachary Michaels zmichaels11@gmail.com

rosbag2_compression/CMakeLists.txt Outdated Show resolved Hide resolved
rosbag2_compression/CMakeLists.txt Outdated Show resolved Hide resolved
@piraka9011
Copy link
Contributor

Why not put these in rosbag2_test_common instead?

@zmichaels11
Copy link
Contributor Author

Why not put these in rosbag2_test_common instead?

I think it would create a circular dependency since the involved classes are mocks of objects declared in rosbag2_cpp and rosbag2_cpp already includes rosbag2_test_common.

…ncludes are not required

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 force-pushed the zmichaels11/compression/replace-relative-includes branch from 55a351e to 9cb71c3 Compare May 7, 2020 21:38
@zmichaels11 zmichaels11 marked this pull request as ready for review May 7, 2020 21:40
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Please make sure to run CI on it.

Copy link
Member

@jacobperron jacobperron 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. Thanks!

@jacobperron
Copy link
Member

Consider opening a ticket to track the follow-up task of de-duplicating code.

@Karsten1987
Copy link
Collaborator

Karsten1987 commented May 7, 2020

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

@Karsten1987
Copy link
Collaborator

CI comes back as expected. Going to merge and cut a new release to unblock the Foxy beta release.

@Karsten1987 Karsten1987 merged commit feb4164 into ros2:master May 8, 2020
@zmichaels11 zmichaels11 deleted the zmichaels11/compression/replace-relative-includes branch May 8, 2020 15:29
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.

None yet

5 participants