Remove rcpputils::fs dependencies in rosbag2 packages by MichaelOrlov · Pull Request #1740 · ros2/rosbag2

MichaelOrlov · 2024-07-09T23:35:58Z

This PR removes residual dependencies from the rcpputils::fs in all rosbag2 packages.
Rationale: The rcpputils::fs will be deprecated soon, and it is advised that std::filesystem be used instead.

Added own rosbag2_test_common::create_temp_directory(..) function in replacement of the analog from the rcpputils::fs since such functionality doesn't exists in the std::filesystem.

r7vme · 2024-07-10T06:46:07Z

@MichaelOrlov I looked thru the code and it looks ok from my side. I remember there were some Windows related problems back, when we tried to do similar change.

ahcorde

Just a question

ahcorde · 2024-07-10T10:50:59Z

rosbag2_test_common/include/rosbag2_test_common/temporary_directory_fixture.hpp

+/// \throws std::system_error If any OS APIs do not succeed.
+/// \throws std::runtime_error If the number of the iterations exceeds the maximum tries and
+/// a unique directory is not found.
+std::filesystem::path create_temp_directory(


why you don't use rcpputils::fs::create_temp_directory ?

Yeah, I think we should switch to using the rcpputils::fs::create_temp_directory here. If there are still problems with it, then we should fix them over in rcpputils.

@ahcorde @clalancette The reasons why I wrote my own implementation of the create_temp_directory is the following:

Because last time when we tried to use rcpputils::fs::create_temp_directory alongside with cleanup via std::filesystem::remove_all, the std::filesystem::remove_all was failing on Windows and also caused some test failures. See Use std::filesystem instead of rcpputils::fs #1576 (comment) and Use std::filesystem instead of rcpputils::fs #1576 (review) for details.
I have tried to triage this problem on Windows. It turns out that in failing tests the rosbag2 creating subfolder with files in the temporary created folder from rcpputils::fs::create_temp_directory. For some reason, likely because rcpputils::fs::create_temp_directory inside using platform-dependent function calls, i.e., _mktemp_s(..) and own implementation for create_directories(const path & p) the std::filesystem::remove_all fails when trying to delete such folder structures. However, if create a temp folder manually or use the temp folder returned by the std::filesystem::temp_directory_path() without creating a subfolder in it with rcpputils::fs::create_temp_directory, the std::filesystem::remove_all is not failing.

As far as I understand, it was a motion to deprecate functions from the rcpputils::fs and use alternatives from the std::filesystem.

I don't like the idea of usage std::filesystem::path with the rcpputils::fs::path. It would be more consistent and more robust to use only std::filesystem::path. If we decided to get rid from the rcpputils::fs let's do it! We are using rcpputils::fs::create_temp_directory only in two places in the core packages in the rosbag2 and in tests for the rcl_logging_spdlog.

Let's see how Windows CI will pass. If it will pass without failures, I don't mind to move my rosbag2_test_common::create_temp_directory(..) to the rcpputils::fs namespace.

I don't like the idea of usage std::filesystem::path with the rcpputils::fs::path. It would be more consistent and more robust to use only std::filesystem::path. If we decided to get rid from the rcpputils::fs let's do it! We are using rcpputils::fs::create_temp_directory only in two places in the core packages in the rosbag2 and in tests for the rcl_logging_spdlog.

Yes, we are actively doing this right now in ros2/rcpputils#197 and ros2/rcpputils#196 . That said, std::filesystem doesn't have any equivalent to rcpputils::fs::create_temp_directory, so we are going to have to carry around that function indefinitely.

@clalancette If CI will pass green I would like to move my implementation of the create_temp_directory to the rcpputils::fs as is but with the new name create_temporary_directory and deprecate the original rcpputils::fs::create_temp_directory eventually.

@clalancette If CI will pass green I would like to move my implementation of the create_temp_directory to the rcpputils::fs as is but with the new name create_temporary_directory and deprecate the original rcpputils::fs::create_temp_directory eventually.

I don't think we should do that. We are currently in the process of breaking the API for create_temp_directory anyway, so I think we should do this all at the same time. Please take a look at ros2/rcpputils#197 and give feedback on what we think the API should be.

BTW. The implementation in the https://github.com/ros2/rcpputils/pull/197/files has API-breaking changes and still uses platform-dependent code, which is not good.
Creating a new function with a different name and deprecating the old one would be better.

BTW. The implementation in the https://github.com/ros2/rcpputils/pull/197/files has API-breaking changes and still uses platform-dependent code, which is not good.

Yes, it has API-breaking changes. See the discussion in ros2/rcpputils#196 (review) about why we decided to do that.

@clalancette I see. However, I still don't like the idea of doing API-breaking changes, which is require to merge 3 PRs in synch.
Why not do it sequentially by creating a new function with create_temporary_directory name?
I've tried to look out, and it seems this name is not occupied in the ROS 2 core packages.

@clalancette FYI In Apex.AI, when we are doing synch with upstream ROS 2 by pulling PRs each time, such breaking changes, which require merging multiple PRs in different packages in synch, is a nightmare since it is breaking our CI.

We would be grateful if such breaking changes will be as minimum as possible.

MichaelOrlov · 2024-07-10T18:33:52Z

Pulls: #1740
Gist: https://gist.githubusercontent.com/MichaelOrlov/57ea65444857fed55ac99b2bdb80c0a1/raw/c8524ce3ec5433097215036dbc53c6c6f6aaea97/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_storage rosbag2_transport rosbag2_test_common rosbag2_tests
TEST args: --packages-above rosbag2_cpp rosbag2_storage rosbag2_transport rosbag2_test_common rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14211

Linux
Linux-aarch64
Linux-rhel
Windows

MichaelOrlov · 2024-07-10T22:49:42Z

The CI failed on rosbag2_storage_sqlite3.test_sqlite_storage. It looks like a failure in the test's destructor. It could be related to the failure in the std::filesystem::remove_all. I will try to reproduce it locally and rerun Windows CI.

MichaelOrlov · 2024-07-10T22:50:58Z

Re-run Windows CI

Windows

MichaelOrlov · 2024-07-11T05:27:45Z

Re-run Windows CI after attempting to fix test failure.

Windows

MichaelOrlov · 2024-07-12T02:42:48Z

Update: On the latest ROS 2 issues triage meeting we decided:

Move implementation of the newly added rosbag2_test_common::create_temp_directory(..) function to the rcpputils::fs with the new name create_temporary_directory(..). Will be done in the Replace create_temp_directory with the new create_temporary_directory rcpputils#198
Deprecate rcpputils::fs::create_temp_directory(..) and rcpputils::fs::temp_directory_path()
Move fix for the failing throws_on_invalid_pragma_in_config_file test to the separate PR. Will be handled in the Fix for failing throws_on_invalid_pragma_in_config_file test on Windows #1742
Get back to this PR when Fix for failing throws_on_invalid_pragma_in_config_file test on Windows #1742 and Replace create_temp_directory with the new create_temporary_directory rcpputils#198 will be merged.

- Added own rosbag2_test_common::create_temp_directory(..) function in replacement of the analog from the rcpputils::fs. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

rosbag2_test_common/include/rosbag2_test_common/temporary_directory_fixture.hpp

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde · 2024-07-22T08:31:20Z

Linux
Linux-aarch64
Linux-rhel
Windows

Included #1740 and ros2/rcl_logging#117

MichaelOrlov · 2024-08-09T19:54:41Z

https://github.com/Mergifyio backport jazzy

mergify · 2024-08-09T19:54:51Z

backport jazzy

✅ Backports have been created

Details

#1780 Remove rcpputils::fs dependencies in rosbag2 packages (backport #1740) has been created for branch jazzy

Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com> (cherry picked from commit 7a01d78)

MichaelOrlov force-pushed the morlov/get-rid-from-rcpputils-fs-dependencies branch 2 times, most recently from 6f39829 to 8116b85 Compare July 9, 2024 23:42

MichaelOrlov marked this pull request as ready for review July 9, 2024 23:48

MichaelOrlov requested a review from a team as a code owner July 9, 2024 23:48

MichaelOrlov requested review from gbiggs and hidmic and removed request for a team July 9, 2024 23:48

MichaelOrlov changed the title ~~Remove rcpputils::fs dependencies from rosbag2 packages~~ Remove rcpputils::fs dependencies in rosbag2 packages Jul 9, 2024

MichaelOrlov requested a review from clalancette July 9, 2024 23:49

ahcorde mentioned this pull request Jul 10, 2024

Use std::filesystem in create_temp_directory and temp_directory_path ros2/rcpputils#197

Closed

ahcorde requested changes Jul 10, 2024

View reviewed changes

MichaelOrlov mentioned this pull request Jul 12, 2024

Fix for failing throws_on_invalid_pragma_in_config_file test on Windows #1742

Merged

MichaelOrlov marked this pull request as draft July 12, 2024 02:35

MichaelOrlov changed the title ~~Remove rcpputils::fs dependencies in rosbag2 packages~~ [WIP] Remove rcpputils::fs dependencies in rosbag2 packages Jul 12, 2024

Remove rcpputils::fs dependencies from rosbag2 packages

0e7e581

- Added own rosbag2_test_common::create_temp_directory(..) function in replacement of the analog from the rcpputils::fs. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

MichaelOrlov force-pushed the morlov/get-rid-from-rcpputils-fs-dependencies branch from 7795660 to 0e7e581 Compare July 16, 2024 15:31

update code

e11b3a5

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde mentioned this pull request Jul 19, 2024

Replace create_temp_directory with the new create_temporary_directory ros2/rcpputils#198

Merged

MichaelOrlov commented Jul 19, 2024

View reviewed changes

rosbag2_test_common/include/rosbag2_test_common/temporary_directory_fixture.hpp Outdated Show resolved Hide resolved

Added feedback

67abb92

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde mentioned this pull request Jul 22, 2024

Updated deprecated API ros2/rcl_logging#117

Merged

ahcorde marked this pull request as ready for review July 22, 2024 08:31

ahcorde approved these changes Jul 22, 2024

View reviewed changes

ahcorde merged commit 7a01d78 into rolling Jul 22, 2024

ahcorde deleted the morlov/get-rid-from-rcpputils-fs-dependencies branch July 22, 2024 08:32

nicolaloi mentioned this pull request Jul 22, 2024

Add computation of size contribution to info verb #1726

Merged

MichaelOrlov changed the title ~~[WIP] Remove rcpputils::fs dependencies in rosbag2 packages~~ Remove rcpputils::fs dependencies in rosbag2 packages Jul 22, 2024

mergify bot mentioned this pull request Aug 9, 2024

[jazzy] Remove rcpputils::fs dependencies in rosbag2 packages (backport #1740) #1780

Closed

Conversation

MichaelOrlov commented Jul 9, 2024

Uh oh!

r7vme commented Jul 10, 2024

Uh oh!

ahcorde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelOrlov Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelOrlov Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelOrlov commented Jul 10, 2024

Uh oh!

MichaelOrlov commented Jul 10, 2024

Uh oh!

MichaelOrlov commented Jul 10, 2024

Uh oh!

MichaelOrlov commented Jul 11, 2024

Uh oh!

MichaelOrlov commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ahcorde commented Jul 22, 2024

Uh oh!

MichaelOrlov commented Aug 9, 2024

Uh oh!

mergify bot commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MichaelOrlov Jul 10, 2024 •

edited

Loading

MichaelOrlov Jul 10, 2024 •

edited

Loading

MichaelOrlov commented Jul 12, 2024 •

edited

Loading

mergify bot commented Aug 9, 2024 •

edited

Loading