Use std::filesystem instead of rcpputils::fs

[r7vme]

Based on PRs #1478 and #1479

Use std::filesystem instead of rcpputils::fs (ros2/rcpputils#164) in all packages of rosbag2.

The only place where it is still used is TemporaryDirectoryFixture in

rosbag2/rosbag2_test_common/include/rosbag2_test_common/temporary_directory_fixture.hpp

Line 34 in 1564687

temporary_dir_path_ = rcpputils::fs::create_temp_directory("tmp_test_dir_").string();

Because there is no alternative in the standard library

[clalancette]

Because there is no alternative in the standard library

For what it is worth, the only users of rcpputils::fs::create_temp_directory are here in rosbag2:

rosbag2/rosbag2_test_common/include/rosbag2_test_common/temporary_directory_fixture.hpp

Line 34 in 1564687

temporary_dir_path_ = rcpputils::fs::create_temp_directory("tmp_test_dir_").string();

and

rosbag2/rosbag2_transport/test/rosbag2_transport/test_rewrite.cpp

Line 54 in 1564687

: output_dir_(rcpputils::fs::create_temp_directory("test_bag_rewrite"))

.

Since the goal is to eventually remove rcpputils::fs, I think it is reasonable enough to copy https://github.com/ros2/rcpputils/blob/292be119c13edb8baa1e78c348637c460ad7a846/src/filesystem_helper.cpp#L310-L342 somewhere into this repository.

[r7vme]

@clalancette thanks, done. Do we run CI on Windows? Just want to be sure it works there too

@MichaelOrlov could you also please take a look?

[r7vme]

@fujitatomoya thanks for the comments, addressed

[MichaelOrlov]

@r7vme Thank you for picking up and finishing this PR. It turns out to be a large PR.
In general, it looks good to me. However, I have a few findings which need to be addressed.
Among a few minor findings which I pointed out in the inline comments there is some missed usage of the rcutils_calculate_directory_size(uri.c_str(), &metadata.bag_size, allocator)) in the

rosbag2/rosbag2_storage/src/rosbag2_storage/metadata_io.cpp

Lines 38 to 48 in a2283ab

	BagMetadata MetadataIo::read_metadata(const std::string & uri)
	{
	try {
	YAML::Node yaml_file = YAML::LoadFile(get_metadata_file_name(uri));
	auto metadata = yaml_file["rosbag2_bagfile_information"].as<rosbag2_storage::BagMetadata>();
	rcutils_allocator_t allocator = rcutils_get_default_allocator();
	if (RCUTILS_RET_OK !=
	rcutils_calculate_directory_size(uri.c_str(), &metadata.bag_size, allocator))
	{
	throw std::runtime_error(
	std::string("Exception on calculating the size of directory :") + uri);

which could be replaced by using https://en.cppreference.com/w/cpp/filesystem/file_size instead

[r7vme]

@MichaelOrlov addressed your comments here 0b53db4

Should I squash last 4 commits into one?

[MichaelOrlov]

@r7vme Thank you for addressing most of the comments from the review.
It seems one comment was missed

Among a few minor findings which I pointed out in the inline comments there is some missed usage of the rcutils_calculate_directory_size(uri.c_str(), &metadata.bag_size, allocator)) in the

rosbag2/rosbag2_storage/src/rosbag2_storage/metadata_io.cpp

Lines 38 to 48 in a2283ab

	BagMetadata MetadataIo::read_metadata(const std::string & uri)
	{
	try {
	YAML::Node yaml_file = YAML::LoadFile(get_metadata_file_name(uri));
	auto metadata = yaml_file["rosbag2_bagfile_information"].as<rosbag2_storage::BagMetadata>();
	rcutils_allocator_t allocator = rcutils_get_default_allocator();
	if (RCUTILS_RET_OK !=
	rcutils_calculate_directory_size(uri.c_str(), &metadata.bag_size, allocator))
	{
	throw std::runtime_error(
	std::string("Exception on calculating the size of directory :") + uri);

which could be replaced by using https://en.cppreference.com/w/cpp/filesystem/file_size instead

[r7vme]

@MichaelOrlov regarding rcutils_calculate_directory_size. Unfortunately there is no alteranative in std::filesystem. std::filesystem::file_size will return filesystem error: cannot get file size: Is a directory on Linux if run for directrory. I would leave this piece as is. But I definitely missed this comment in the first place.

[r7vme]

@clalancette @MichaelOrlov looks like Rpr__rosbag2__ubuntu_noble_amd64 fails on all latest PRs with The command '/bin/sh -c pip3 install -U setuptools==59.6.0' returned a non-zero code: 1

[MichaelOrlov]

@r7vme Yeah, we are aware of it. This is a generic issue due to the transition to the Ubuntu 24.04 on Rolling. This issue is going to be addressed sometime soon.

[MichaelOrlov]

@r7vme As regards

Should I squash last 4 commits into one?

Please do not squash commits in the future. It is very helpful for the reviewer to see changes in the separate commits.
We have automatically mandatory squash for all commits in the PR during the final merge to the target branch.

[MichaelOrlov]

@r7vme Thanks, LGTM!

[MichaelOrlov]

Gist: https://gist.githubusercontent.com/MichaelOrlov/f323f6647c62a2395c5006aa68777d14/raw/fa6e1966d9c3fc82d4d4eeb990dd9e3461fe97b8/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_compression_zstd rosbag2_cpp rosbag2_storage rosbag2_test_common rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_compression rosbag2_compression_zstd rosbag2_cpp rosbag2_storage rosbag2_test_common rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13467

• Linux
• Linux-aarch64
• Windows

[r7vme]

@MichaelOrlov thanks, is there anything required from my side? Or you can restart failed CI job Rpr__rosbag2__ubuntu_noble_amd64 ?

[clalancette]

@MichaelOrlov thanks, is there anything required from my side? Or you can restart failed CI job Rpr__rosbag2__ubuntu_noble_amd64 ?

These aren't going to work right at the moment. We are close to making them work, but need a bit more time. The regular CI above is good enough.

[MichaelOrlov]

@r7vme Some tests failing on Windows CI https://ci.ros2.org/job/ci_windows/21225/
At first glance, it seems failure relates to the filesystem operation.

C:\ci\ws\src\r7vme\rosbag2\rosbag2_cpp\test\rosbag2_cpp\test_sequential_reader.cpp:224
Expected equality of these values:
  closed_file
    Which is: "C:/Users/ContainerAdministrator/AppData/Local/Temp/some/folder/bag_file1"
  bag_file_1_path_.string()
    Which is: "C:/Users/ContainerAdministrator/AppData/Local/Temp/some/folder\\bag_file1"

C:\ci\ws\src\r7vme\rosbag2\rosbag2_cpp\test\rosbag2_cpp\test_sequential_reader.cpp:225
Expected equality of these values:
  opened_file
    Which is: "C:/Users/ContainerAdministrator/AppData/Local/Temp/some/folder/bag_file2"
  bag_file_2_path_.string()
    Which is: "C:/Users/ContainerAdministrator/AppData/Local/Temp/some/folder\\bag_file2"

Could you please take a look at failures and test use cases? It seems we have forgotten to use .string_generic() instead of .string() for fs::path somewhere.

[MichaelOrlov]

Re-run CI afer attempt to fix test errors on Windows build
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13484

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

@r7vme We have one more test failure on Windows

[ RUN      ] StorageTestFixture.storage_preset_profile_applies_over_defaults
[INFO] [1710819321.702947900] [rosbag2_storage]: Opened database 'C:/Users/ContainerAdministrator/AppData/Local/Temp/tmp_test_dir_a05376/rosbag.db3' for READ_WRITE.
[       OK ] StorageTestFixture.storage_preset_profile_applies_over_defaults (32 ms)
[ RUN      ] StorageTestFixture.throws_on_invalid_pragma_in_config_file

Could you please take a look on it?

[r7vme]

@MichaelOrlov I reverted remove_all in test fixture. This part is used in failed test. Let's see it if helps. No other ideas unforunately

[MichaelOrlov]

Trying to re-run Windows CI after reverting remove_all in the test fixture to see if it is a root cause of the CI failure

• Windows

[r7vme]

@clalancette could you please take another look? your comment was addressed

[clalancette]

@MichaelOrlov I reverted remove_all in test fixture. This part is used in failed test. Let's see it if helps. No other ideas unforunately

I have to say that I'm surprised that this fixed the problem. We are using a similar pattern in https://github.com/ros2/rcl_logging/blob/rolling/rcl_logging_spdlog/test/test_logging_interface.cpp , and it is working fine by using std::filesystem::remove_all.

That said, I'm still approving since I'm OK with putting this in as-is. It just means that we won't be able to remove rcpputils::fs until we do another PR to fix whatever bug std::filesystem::remove_all is hitting.

[r7vme]

@MichaelOrlov I guess can be merged finally?

[MichaelOrlov]

@r7vme Yes we can merge I think. However, please hold off for a couple of days. I need to wrap up some other things and I want to mitigate the potential merge or rebase conflicts.