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

Implement changes in io interfaces #7

Merged
merged 5 commits into from Nov 12, 2019
Merged

Implement changes in io interfaces #7

merged 5 commits into from Nov 12, 2019

Conversation

zmichaels11
Copy link
Contributor

@zmichaels11 zmichaels11 commented Oct 23, 2019

Implemented changes in IO interfaces that were introduced in recent commits to rosbag2.

Dependent PRs

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 marked this pull request as ready for review October 23, 2019 18:43
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

While the code generally LGTM, how could we cope with get_storage_identifier() and get_bagfile_size() being unimplemented here before? Are we in the process of expending the API?

Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

Hmmm the C++ code guidelines do not agree with me.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c139-use-final-sparingly
...so I guess override is ok.

@zmichaels11
Copy link
Contributor Author

While the code generally LGTM, how could we cope with get_storage_identifier() and get_bagfile_size() being unimplemented here before? Are we in the process of expending the API?

Yeah, both functions were added into the API in anticipation of supporting bagfile splitting. I'll link the merged PR numbers related to these changes.

@thomas-moulard
Copy link
Contributor

👍 Thanks!

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
…ag_v2_storage.cpp

Co-Authored-By: Thomas Moulard <thomas.moulard@gmail.com>
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@Karsten1987
Copy link
Contributor

does it make sense to enhance this PR to comply with ros2/rosbag2#184?

@zmichaels11
Copy link
Contributor Author

does it make sense to enhance this PR to comply with ros2/rosbag2#184?

Yes, unless you want to merge this commit first.

@Karsten1987
Copy link
Contributor

sounds good. Please go ahead and advance this PR with the changes needed and I we can run CI together on this.

@jacobperron
Copy link
Member

FYI, I've tried to use the current state of this PR and get the following compilation error:

In file included from /home/jacob/ws/ros1_bridge_ws/install/class_loader/include/class_loader/class_loader_core.hpp:56:0,
                 from /home/jacob/ws/ros1_bridge_ws/install/class_loader/include/class_loader/class_loader.hpp:55,
                 from /home/jacob/ws/ros1_bridge_ws/install/class_loader/include/class_loader/multi_library_class_loader.hpp:52,
                 from /home/jacob/ws/ros1_bridge_ws/install/pluginlib/include/pluginlib/class_loader.hpp:58,
                 from /home/jacob/ws/rosbag2_bag_v2_ws/install/include/rosbag/bag.h:67,
                 from /home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.hpp:23,
                 from /home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.cpp:15:
/home/jacob/ws/ros1_bridge_ws/install/class_loader/include/class_loader/meta_object.hpp: In instantiation of ‘B* class_loader::impl::MetaObject<C, B>::create() const [with C = rosbag2_bag_v2_plugins::RosbagV2Storage; B = rosbag2_storage::storage_interfaces::ReadOnlyInterface]’:
/home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.cpp:185:1:   required from here
/home/jacob/ws/ros1_bridge_ws/install/class_loader/include/class_loader/meta_object.hpp:193:12: error: invalid new-expression of abstract class type ‘rosbag2_bag_v2_plugins::RosbagV2Storage’
     return new C;
            ^~~~~
In file included from /home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.cpp:15:0:
/home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.hpp:29:7: note:   because the following virtual functions are pure within ‘rosbag2_bag_v2_plugins::RosbagV2Storage’:
 class RosbagV2Storage : public rosbag2_storage::storage_interfaces::ReadOnlyInterface
       ^~~~~~~~~~~~~~~
In file included from /home/jacob/ws/ros1_bridge_ws/install/rosbag2_storage/include/rosbag2_storage/storage_interfaces/read_only_interface.hpp:20:0,
                 from /home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.hpp:22,
                 from /home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.cpp:15:
/home/jacob/ws/ros1_bridge_ws/install/rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_info_interface.hpp:41:23: note: 	virtual std::__cxx11::string rosbag2_storage::storage_interfaces::BaseInfoInterface::get_relative_path() const
   virtual std::string get_relative_path() const = 0;
                       ^~~~~~~~~~~~~~~~~
make[2]: *** [CMakeFiles/rosbag2_bag_v2_plugins.dir/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/rosbag2_bag_v2_plugins.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [all] Error 2

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Oct 25, 2019

@jacobperron just resolved that issue; it was due to recent API changes introduced in ros2/rosbag2#184.

@Karsten1987 This PR is ready

@jacobperron
Copy link
Member

@zmichaels11 Yep, thanks. I'm able to compile now.

@zmichaels11
Copy link
Contributor Author

@Karsten1987 is there anything blocking this PR from merging?

@Karsten1987 Karsten1987 merged commit 39a0a84 into ros2:master Nov 12, 2019
@zmichaels11 zmichaels11 deleted the splitting/io-changes branch January 13, 2020 23:05
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