Skip to content

create_directories returns true if the path is empty#97

Closed
fujitatomoya wants to merge 2 commits intoros2:masterfrom
fujitatomoya:bugfix-20200925-issue-94
Closed

create_directories returns true if the path is empty#97
fujitatomoya wants to merge 2 commits intoros2:masterfrom
fujitatomoya:bugfix-20200925-issue-94

Conversation

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fix #94, and address #96

@fujitatomoya fujitatomoya requested a review from a team as a code owner September 25, 2020 10:07
@fujitatomoya fujitatomoya force-pushed the bugfix-20200925-issue-94 branch from e8140e0 to 6faac68 Compare September 25, 2020 10:13
…eate_directories (ros2#95)" (ros2#96)"

This reverts commit ba5d1e4.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya force-pushed the bugfix-20200925-issue-94 branch from 6faac68 to 8da0c7c Compare September 25, 2020 10:23
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

@wjwwood @christophebedard

I've added minor fix based on #95.

confirmed rosbag2 test does not detects any failures.

@christophebedard
Copy link
Copy Markdown
Member

thank you @fujitatomoya!

Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

The problem I have with this change is that this makes us act different than std::filesystem (which we eventually want to switch to). As far as I can tell, if you call:

std::filesystem::create_directories("");

It throws an exception:

terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
  what():  filesystem error: cannot create directories: Invalid argument []

Also, from a pure semantic point-of-view, the documentation says that it returns true if the directory is created, and false otherwise. Since it could not create the directory on an empty string, returning true seems wrong here.

My suggestion is that we actually fix the test in rosbag2 that is trying to call this API with an empty string. Once that is done, we can reapply #95 as it was, and then things shouldn't break on the farm.

@christophebedard
Copy link
Copy Markdown
Member

christophebedard commented Sep 25, 2020

That makes sense. I can do the rosbag2 changes (@fujitatomoya let me know if you've started working on it).

@christophebedard
Copy link
Copy Markdown
Member

My suggestion is that we actually fix the test in rosbag2 that is trying to call this API with an empty string. Once that is done, we can reapply #95 as it was, and then things shouldn't break on the farm.

I opened ros2/rosbag2#526

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

@christophebedard @clalancette

i will close this PR, you can revert #96 after ros2/rosbag2#526.

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.

rcpputils::fs::create_directories doesn't check if an existing path is a directory

3 participants