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

Deprecate rcpputils::fs in favor of C++17 std::filesystem #164

Closed
emersonknapp opened this issue Aug 4, 2022 · 6 comments
Closed

Deprecate rcpputils::fs in favor of C++17 std::filesystem #164

emersonknapp opened this issue Aug 4, 2022 · 6 comments

Comments

@emersonknapp
Copy link
Contributor

Now that it's available, there's no reason to provide a wrapper for the std functionality (IMO).

Related to #125
@clalancette
Copy link
Contributor

I agree.

To get there, what would need to happen is the following:

  1. Go through the ROS 2 core, changing any uses of rcpputils::fs to std::filesystem.
  2. Mark the various functions and classes in rcpputils::fs as deprecated for Iron.
  3. Put a release note into the release page for Iron.
  4. For J-Turtle, remove rcpputils::fs

This was referenced Sep 25, 2023
Merged
Merged
Merged
Closed
@yoneken yoneken mentioned this issue Oct 6, 2023
Merged
@MichaelOrlov
Copy link
Contributor

@clalancette @emersonknapp I remember we had problems on the on RHEL-8 due to the lack of adequate support for std::filesystem::path(..)
See ros2/rosbag2#1104 for details.
Has it already been fixed or not an issue anymore?

@clalancette
Copy link
Contributor

Has it already been fixed or not an issue anymore?

It's not an issue anymore since we moved to RHEL-9 for Rolling (and Iron, for that matter). So we can go ahead with reviews on this.

MichaelOrlov pushed a commit to ros2/rosbag2 that referenced this issue Nov 7, 2023
@morlov-apexai
Use std::filesystem instead of rcpputils::fs functions
975e075 
- Rationale: Deprecation of the rcpputils::fs in future
See ros2/rcpputils#164 for details

Signed-off-by: Michael Orlov <[email protected]>
This was referenced Nov 7, 2023
Merged
Merged
Barry-Xu-2018 pushed a commit to Barry-Xu-2018/rosbag2 that referenced this issue Nov 14, 2023
@morlov-apexai @Barry-Xu-2018
Use std::filesystem instead of rcpputils::fs functions
13baf49 
- Rationale: Deprecation of the rcpputils::fs in future
See ros2/rcpputils#164 for details

Signed-off-by: Michael Orlov <[email protected]>
Barry-Xu-2018 pushed a commit to Barry-Xu-2018/rosbag2 that referenced this issue Dec 13, 2023
@morlov-apexai @Barry-Xu-2018
Use std::filesystem instead of rcpputils::fs functions
a21c216 
- Rationale: Deprecation of the rcpputils::fs in future
See ros2/rcpputils#164 for details

Signed-off-by: Michael Orlov <[email protected]>
MichaelOrlov pushed a commit to ros2/rosbag2 that referenced this issue Dec 21, 2023
@Barry-Xu-2018 @morlov-apexai
Implement service recording and display info about recorded services (#…
3286736 
…1480)

* Implement service record and recorded service info display

Signed-off-by: Barry Xu <[email protected]>

* Address MichaelOrlov's review comments

Signed-off-by: Barry Xu <[email protected]>

* Add tests for rosbag2_cpp::Info::read_service_info()

Signed-off-by: Barry Xu <[email protected]>

* Updated code on exclude parameters

Refer to #1483

Signed-off-by: Barry Xu <[email protected]>

* Fix flake8 errors

Signed-off-by: Barry Xu <[email protected]>

* Update code to remove hard-coded values to make the code more readable

Signed-off-by: Barry Xu <[email protected]>

* Fix a bug on exclude parameter

Signed-off-by: Barry Xu <[email protected]>

* Update codes on rosbag2 QoS for service event topic

Signed-off-by: Barry Xu <[email protected]>

* Address the second round of review comments

Signed-off-by: Barry Xu <[email protected]>

* Updated info test on getting service info

Signed-off-by: Barry Xu <[email protected]>

* Rename exclude_services to exclude_service_event

Signed-off-by: Barry Xu <[email protected]>

* Address flakiness in newly added rosbag2_transport tests

- Got rid form ambient sleep for waiting for messages to be recorded
- Exclude "/rosout" topic from recording

Signed-off-by: Michael Orlov <[email protected]>

* Use wait_for_srvice_to_be_ready() instead check_service_ready()

- Also add default timeout parameter for ClientManager::send_request(..)

Signed-off-by: Michael Orlov <[email protected]>

* Add rosbag2_test_common::wait_until_condition(..)

Signed-off-by: Michael Orlov <[email protected]>

* Rewrite get_service_info tests to be more deterministic

- Also parameterize tests to use default supported storage plugins

Signed-off-by: Michael Orlov <[email protected]>

* Renames in the get_service_info tests

Signed-off-by: Michael Orlov <[email protected]>

* Use `std::filesystem` instead of `rcpputils::fs` functions

- Rationale: Deprecation of the rcpputils::fs in future
See ros2/rcpputils#164 for details

Signed-off-by: Michael Orlov <[email protected]>

* Avoid extra metadata readout in `ros2 bag info` verb with `--verbose`

Signed-off-by: Michael Orlov <[email protected]>

* Add info_end_to_end test with `--verbose` parameter

Signed-off-by: Michael Orlov <[email protected]>

* Fix bugs in info_with_verbose_option_end_to_end_test

Signed-off-by: Barry Xu <[email protected]>

* Fix segment fault issue in test_rosbag2_cpp_get_service_info.cpp

Signed-off-by: Barry Xu <[email protected]>

* Update a fix for info_with_verbose_option_end_to_end_test

Signed-off-by: Barry Xu <[email protected]>

* Output a warning while setting both '-t' and 'v' for info command

Signed-off-by: Barry Xu <[email protected]>

* Address comments from Fujita-san

Signed-off-by: Barry Xu <[email protected]>

* Use topic_in_list instead of service_in_list

Signed-off-by: Michael Orlov <[email protected]>

* Handle cases when dependent service msg definitions could be in IDL file

Signed-off-by: Michael Orlov <[email protected]>

* Fix rebase errors

- Delete "rosbag2_transport/qos.cpp" which was moved to the
rosbag2_storage package during prior PR before rebase

Signed-off-by: Michael Orlov <[email protected]>

* Fix the path of a library

Signed-off-by: Barry Xu <[email protected]>

* Resolve the conflicts caused by the rebase

Signed-off-by: Barry Xu <[email protected]>

* Replace sizeof() with strlen()

Signed-off-by: Barry Xu <[email protected]>

* Use consistent variable name

Signed-off-by: Barry Xu <[email protected]>

* Fix a link error on Windows

Signed-off-by: Barry Xu <[email protected]>

* Use visibility control for format library

Signed-off-by: Barry Xu <[email protected]>

* Replace rosbag2_format_output by direct source files linkage

- Also put `format_service_info(..)` and `format_bag_meta_data(..)`
under the rosbag2_py namespace

Signed-off-by: Michael Orlov <[email protected]>

* Remove header files from pybind modules and delete visibility_control.hpp

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Barry Xu <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Michael Orlov <[email protected]>
This was referenced Feb 22, 2024
Merged
Merged
Closed
Merged
@r7vme r7vme mentioned this issue Feb 28, 2024
Merged
@fujitatomoya fujitatomoya mentioned this issue Mar 10, 2024
Closed
@felixf4xu
Copy link

just a heads-up to check the status of this issue :-)

It's not an urgent issue, but I think it's a valid issue.

@clalancette
Copy link
Contributor

We are still having issues around ros2/rclcpp#2438, so we can't deprecate it until that is fixed.

@ahcorde
Copy link
Contributor

ahcorde commented Aug 21, 2024

we can close this issue the deprecation is done here ros2/rclcpp#2579.

@ahcorde ahcorde closed this as completed Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@clalancette @emersonknapp @ahcorde @MichaelOrlov @felixf4xu