Conversation
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
| /** | ||
| * \brief Get a path to a location in the temporary directory, if it's available. | ||
| * | ||
| * This does not create any directories. | ||
| * On Windows, this uses "GetTempPathA" | ||
| * On non-Windows, this prefers the environment variable TMPDIR, falling back to /tmp | ||
| * | ||
| * \return A path to a directory for storing temporary files and directories. | ||
| */ | ||
| RCPPUTILS_PUBLIC std::filesystem::path temporary_directory_path(); | ||
|
|
||
| /** | ||
| * \brief Construct a uniquely named temporary directory, in "parent", with format base_nameXXXXXX | ||
| * | ||
| * The output, if successful, is guaranteed to be a newly-created directory. | ||
| * The underlying implementation keeps generating paths until one that does not exist is found. | ||
| * This guarantees that there will be no existing files in the returned directory. | ||
| * | ||
| * \param[in] base_name User-specified portion of the created directory | ||
| * \param[in] parent_path The parent path of the directory that will be created | ||
| * \return A path to a newly-created directory with base_name and a 6-character unique suffix | ||
| * | ||
| * \throws std::system_error If any OS APIs do not succeed. | ||
| */ | ||
| RCPPUTILS_PUBLIC std::filesystem::path create_temporary_directory( | ||
| const std::string & base_name, | ||
| const std::filesystem::path & parent_path = temporary_directory_path()); |
There was a problem hiding this comment.
I poked at this a while today with @mjcarroll , and tried to come up with a nicer way to do this. We really couldn't figure out a better way than making a new function name.
However, I pulled down all of the source code released into Rolling. And I found that rcpputils::fs::temp_directory_path and rcpputils::fs::create_temp_directory aren't used anywhere outside of the core. In the core, they are only used in:
rcpputils- the implementation and the testsrosbag2_compression- some testsrosbag2_cpp- some testsrosbag2_storage_mcap- a testrosbag2_storage_sqlite3- a testrosbag2_transport- a testrosbag2_test_common- a testrcl_logging_spdlog- a test
Thus, at the cost of only 3 PRs (this one, one in rcl_logging, and one in rosbag2), we can completely break this API. I think we should do that. In particular, I think we should remove temporary_directory_path and create_temporary_directory, and also remove temp_directory_path. Then we change create_temp_directory to take in a std::filesystem::path, and also return a std::filesystem::path. And then we update the tests here to use std::filesystem::path, as well as the tests in rcl_logging and rosbag2. At the end of those 3 PRs (which will have to be merged at the same time), we'll have the API we want going forward.
Does that make sense to you @ahcorde?
There was a problem hiding this comment.
Actually, in point of fact, that is a big enough change that I think we should do that on its own, separate from this PR. Once that is in, we can revisit deprecating rcpputils::fs::path separately (i.e. this PR).
fujitatomoya
left a comment
There was a problem hiding this comment.
this needs to be rebased on #198.
test/test_filesystem_helper_std.cpp
Outdated
| @@ -0,0 +1,519 @@ | |||
| // Copyright 2019 Open Source Robotics Foundation, Inc. | |||
There was a problem hiding this comment.
| // Copyright 2019 Open Source Robotics Foundation, Inc. | |
| // Copyright 2024 Open Source Robotics Foundation, Inc. |
There was a problem hiding this comment.
Sorry, just realized that @fujitatomoya's comment here is still open.
…precated_path_class
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Included ros2/rclcpp#2579 |
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
clalancette
left a comment
There was a problem hiding this comment.
One last thing to change here, then this should be good.
test/test_filesystem_helper_std.cpp
Outdated
| @@ -0,0 +1,519 @@ | |||
| // Copyright 2019 Open Source Robotics Foundation, Inc. | |||
There was a problem hiding this comment.
Sorry, just realized that @fujitatomoya's comment here is still open.
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
clalancette
left a comment
There was a problem hiding this comment.
Looks good to me with green CI.
|
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
We are using C++17, so we can deprecate
pathclass.This might require some changes downstream