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

Switch from rcpputils::fs to std::filesystem #2438

Closed

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented Feb 26, 2024

Part of ros2/rcpputils#164

I split the PR into two commits: internal changes and API changes.

@christophebedard christophebedard self-assigned this Feb 26, 2024
@christophebedard
Copy link
Member Author

christophebedard commented Feb 26, 2024

I opened this as a draft because I'm not sure how to deal with the public API change for rclcpp::get_logging_directory(). I changed the return type from rcpputils::fs::path to std::filesystem::path because we can't have both, but they're functionally the same. I can't find any non-test calls to rclcpp::get_logging_directory() in the ROS 2 core.

The "cleanest" option is probably to deprecate the current function and create a new one with a different name, but that's unfortunate.

Comment on lines 88 to 90
RCLCPP_PUBLIC
rcpputils::fs::path
std::filesystem::path
get_logging_directory();
Copy link
Collaborator

Choose a reason for hiding this comment

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

so your concern is, this breaks the followings, right? i mean basically we need to provide deprecation period for the user space, but we can not define both of the functions here.

// Compile Error Case-1
rcpputils::fs::path my_path = rclcpp::get_logging_directory();
// Compile Error Case-2
auto your_path = rclcpp::get_logging_directory();
bool directory_flag = your_path.is_directory(); // std::filesystem::path does not have this method.

just idea, i am not even sure if this is actually feasible.

  • keep this function rclcpp as implemented, just change into std::filesystem:path.
  • add copy constructor and copy assignment operator to rcpputils::fs::path to take std::filesystem::path.
  • (as planned) add deprecation to rcpputils::fs::path, so that user can see that rcpputils::fs::path will be removed in next release?

again, this might not be doable idea...

I can't find any non-test calls to rclcpp::get_logging_directory() in the ROS 2 core.

the other part of me, asking myself if we need the deprecation period for this. but that is not something we cannot guarantee.

Copy link
Member

Choose a reason for hiding this comment

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

I think that should work? It's a relatively cheap copy-constructor (basically string to string)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll give it a shot and will probably add a small test for it. I'm just wondering if it's too ambiguous if someone uses auto.

the other part of me, asking myself if we need the deprecation period for this. but that is not something we cannot guarantee.

Yeah it doesn't have a lot of users: https://github.com/search?q=rclcpp%3A%3Aget_logging_directory%28%29&type=code. Let's see if we can properly deprecate it first.

Copy link
Member Author

@christophebedard christophebedard Feb 27, 2024

Choose a reason for hiding this comment

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

It might be too ambiguous, since both rcpputils::fs::path and std::filesystem::path have a conversion constructor for std::string.

@christophebedard
Copy link
Member Author

Superseded by #2579

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.

4 participants