Skip to content

Use std::filesystem in create_temp_directory and temp_directory_path#197

Closed
ahcorde wants to merge 3 commits intorollingfrom
ahcorde/rolling/temp_directories
Closed

Use std::filesystem in create_temp_directory and temp_directory_path#197
ahcorde wants to merge 3 commits intorollingfrom
ahcorde/rolling/temp_directories

Conversation

@ahcorde
Copy link
Copy Markdown
Contributor

@ahcorde ahcorde commented Jul 10, 2024

Related to this comment #196 (comment)

update create_temp_directory and temp_directory_path to use std::filesystem::path

Related PRs

@ahcorde
Use std::filesystem in create_temp_directory and temp_directory_path
5fae8d3 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from clalancette July 10, 2024 10:46
@ahcorde ahcorde self-assigned this Jul 10, 2024
@ahcorde ahcorde mentioned this pull request Jul 10, 2024
Merged
clalancette
clalancette requested changes Jul 10, 2024
View reviewed changes
@ahcorde
Removed create_temp_path and create_temp_directory implementations
a9c79bd 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
clalancette
clalancette requested changes Jul 10, 2024
View reviewed changes
@ahcorde
restore create temp directory
82b20b3 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from clalancette July 10, 2024 15:22
@clalancette clalancette mentioned this pull request Jul 10, 2024
Merged
clalancette
clalancette reviewed Jul 10, 2024
View reviewed changes
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.

This implementation generally looks good to me. I'm going to hold off on approving it until we figure out what is going on with ros2/rosbag2#1740

@clalancette clalancette mentioned this pull request Jul 10, 2024
Merged
MichaelOrlov
MichaelOrlov reviewed Jul 10, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

The implementation 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.

@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Jul 11, 2024

still uses platform-dependent code

I don't believe there is a way around this. Using posix mkdtmp is probably as good as we are going to get here.

Should we perhaps throw or return error code on non-implemented platforms?

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Jul 11, 2024

I don't believe there is a way around this. Using posix mkdtmp is probably as good as we are going to get here.

Yes and no. I took a look at the implementation of mkdtemp in glibc, and it is not really security sensitive. I think we could make our own cross-platform version (similar to what is in ros2/rosbag2#1740) without too much trouble.

@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Jul 11, 2024

As discussed in the waffle meeting, we will close this issue and @MichaelOrlov will create a new PR.

@ahcorde ahcorde closed this Jul 11, 2024
@MichaelOrlov MichaelOrlov mentioned this pull request Jul 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clalancette clalancette clalancette requested changes

@MichaelOrlov MichaelOrlov MichaelOrlov left review comments

@methylDragon methylDragon Awaiting requested review from methylDragon

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees

@ahcorde ahcorde

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ahcorde @mjcarroll @clalancette @MichaelOrlov