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

Feature/std filesystem #637

Merged
merged 3 commits into from
Jun 3, 2021
Merged

Feature/std filesystem #637

merged 3 commits into from
Jun 3, 2021

Conversation

markaren
Copy link
Contributor

This PR replaces usage of boost::filesystem with std::filesystem.
In order to support gcc7, the header fs_portability is used, which falls back to std::experimental::filesystem.
Thus, usage of std::filesystem should be done using the typealias cosim::filesystem

@markaren
Copy link
Contributor Author

The PR fails on file locking on windows.. Don't know how to fix..

@markaren
Copy link
Contributor Author

I hope this PR is welcome. No need to use boost when we got the std available. The only caveat is that we need the fallback header in order to support gcc 6 and 7. We also need to fix the file locking unit test. @kyllingstad need your expertise on that.

@kyllingstad
Copy link
Member

It's very welcome, in my opinion. I'll look into the file locking issue.

@kyllingstad
Copy link
Member

Hopefully, this does the trick. The file locking mechanism has an internal cache of mutexes associated with different files, and each time one tries to create a new one, it checks whether a mutex exists for the file already. Since different paths (relative vs. absolute, links, etc.) can refer to the same file, it uses filesystem::equivalent() to perform this check. But this requires that the file exists, which we previously didn't check.

Copy link
Member

@kyllingstad kyllingstad left a comment

Choose a reason for hiding this comment

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

Approving @markaren's changes here. I suppose he or @ljamt should have a look at mine.

@markaren markaren mentioned this pull request Jun 1, 2021
@ljamt
Copy link
Member

ljamt commented Jun 2, 2021

Approving @markaren's changes here. I suppose he or @ljamt should have a look at mine.

Looks good to me 👍

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.

3 participants