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

Add gulrak filesystem, a c++17 compliant std::filesystem polyfill. #50624

Merged
merged 5 commits into from
Aug 27, 2021

Conversation

akrieger
Copy link
Member

@akrieger akrieger commented Aug 9, 2021

Summary

None

Purpose of change

Add ghc filesystem, a c++17 compliant std::filesystem polyfill, and convert handwritten filesystem helpers to use it. This helps with writing more portable code and adds a functional path class too.

Testing

Create a new game, load a game, save and reload the game.

Additional context

Prerequisite for #50143.

@akrieger akrieger changed the title Ghc filesystem Add gulrak filesystem, a c++17 compliant std::filesystem polyfill. Aug 9, 2021
@actual-nh actual-nh added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Aug 9, 2021
@akrieger
Copy link
Member Author

akrieger commented Aug 9, 2021

I'm not sure the best way to exclude these sources from clang-tidy. Maybe appropriate to create a third_party directory to shove files like these and others which should be excluded from tidy and astyle?

@kevingranade
Copy link
Member

kevingranade commented Aug 9, 2021

This comment outlines how to set header-only libraries as system headers in cmake https://stackoverflow.com/a/64748887/280690
IIRC a separate directory won't work since it's going to trace the header inclusions anyway.

@akrieger
Copy link
Member Author

I would never have figured out that's how clang-tidy picks those up. Thanks!

@akrieger akrieger force-pushed the ghc_filesystem branch 6 times, most recently from 987d125 to d43e5a9 Compare August 12, 2021 16:28
@akrieger
Copy link
Member Author

akrieger commented Aug 12, 2021

Turns out I had to add extra arguments to clang-tidy-wrapper.sh because build.sh was just passing each file in isolation to clang-tidy without any extra arguments. At first it was complaining it couldn't find <ghc/fs_std.hpp>, adding -Isrc/third-party to the wrapper made it find the header but complained, changing it to -isystem src/third-party silenced the warnings.

@akrieger
Copy link
Member Author

+ ./build-scripts/files_changed
+ set +x
Analyzing changed files
No files to analyze

Hm. That seems wrong. But my changes to files_changed should've only been fixes or new behavior. HEAD^2 syntax didn't seem to work in my vm, changing it to ~2 did.

@akrieger
Copy link
Member Author

Reverted changes to build-scripts/files_changed, fixed tests Makefile compilation.

@akrieger
Copy link
Member Author

+ ./build-scripts/files_changed
LICENSE.txt
Makefile
build-scripts/clang-tidy-wrapper.sh
src/CMakeLists.txt
src/filesystem.cpp
src/filesystem.h
+ set +x
src/third-party/CMakeLists.txt
src/third-party/ghc/LICENSE
src/third-party/ghc/filesystem.hpp
src/third-party/ghc/fs_fwd.hpp
src/third-party/ghc/fs_impl.hpp
src/third-party/ghc/fs_std.hpp
src/third-party/ghc/fs_std_fwd.hpp
src/third-party/ghc/fs_std_impl.hpp
tests/Makefile
Analyzing changed files
3489 warnings generated.
Analyzing remaining files because build-scripts/clang-tidy-wrapper.sh was changed

Better, but now the whole codebase is going to barf.

@akrieger akrieger force-pushed the ghc_filesystem branch 4 times, most recently from 559a5ef to 5827466 Compare August 14, 2021 14:49
@akrieger
Copy link
Member Author

clang-tidy build is passing on the new code, it's just failing on the rest of the repo because I actually touched the clang-tidy script so it's rerunning the checks. I've fixed every build failure I've found so far, I just haven't managed to get all the builds to run without timing out eventually.

@akrieger
Copy link
Member Author

Reverted the change to the clang-tidy wrapper since it's not needed after fixing the cmake build finally.

@akrieger
Copy link
Member Author

This is finally ready and passing CI. The only thing I'm not sure about is the exact place in the Makefile that is best to add the include path.

Sourced from https://github.com/gulrak/filesystem/releases/tag/v1.5.8

This is a C++17 compliant `std::filesystem` polyfill. When included via
`ghc/fs_std.hpp`, it will delegate to the actual implementation when
available, populating the methods in the namespace `fs`. When not available,
its own implementations will be in the same namespace.
@akrieger
Copy link
Member Author

Rebased, moved include directory flags to CPPFLAGS.

@kevingranade kevingranade merged commit 95975cb into CleverRaven:master Aug 27, 2021
@ZhilkinSerg
Copy link
Contributor

Android build is broken - https://github.com/CleverRaven/Cataclysm-DDA/runs/3449081942

 /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/android/app/jni/src/../../../../src/filesystem.cpp:16:10: fatal error: 'ghc/fs_std_fwd.hpp' file not found
  #include <ghc/fs_std_fwd.hpp>

Probably src/third-party should be mentioned in Android Makefile.

@kevingranade
Copy link
Member

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/window-version-dosent-support-creating-world-use-chinese-characators/26929/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants