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

Use filesystem of C++17 #1345

Closed
wants to merge 4 commits into from

Conversation

nguyenpham
Copy link
Contributor

Use filesystem of C++17 to improve Lc0 filesystem functions:

  • CreateDirectory
  • GetFileList
  • GetFileSize
  • GetFileTime

This PR also fixed issue #831 since the improved function GetFileSize returns 0 instead of throwing an exception.

Tested with macOS. Need verifies with other OSs.

@nguyenpham
Copy link
Contributor Author

nguyenpham commented Jun 17, 2020

The test build system (build clang) printed out some similar blames such as:

/root/project/build-clang/../src/utils/filesystem.cc:37: undefined reference to std::filesystem::create_directory(std::filesystem::__cxx11::path const&)

I guess cxx11 is C++11 and I don't understand why it uses cxx11 for testing but not cxx17? Can someone help?

@ddobbelaere
Copy link
Contributor

The C++ 17 filesystem functionality is poorly supported on some platforms up to the point that it is close to unusable for our cross-platform engine. See e.g. the discussion in #1276 .

@borg323
Copy link
Member

borg323 commented Jun 17, 2020

The following works around the issue (tested with g++-7), but there are still issues with windows and android builds:

diff --git a/meson.build b/meson.build
index 19fe77c..42dfc74 100644
--- a/meson.build
+++ b/meson.build
@@ -501,6 +501,19 @@ endif
 
 
 #############################################################################
+## Workarounds
+#############################################################################
+if not cc.has_header('filesystem')
+  code = '''#include <experimental/filesystem>
+    int main() {return std::experimental::filesystem::exists(".");}'''
+  # Need more here
+  if cc.links(code, args : '-lstdc++fs', name : '<experimental/filesystem> test')
+    add_project_link_arguments(['-lstdc++fs'], language : 'cpp')
+  endif
+endif
+
+
+#############################################################################
 ## Main Executable
 #############################################################################
 
diff --git a/src/utils/filesystem.cc b/src/utils/filesystem.cc
index c59e8d9..128d0b8 100644
--- a/src/utils/filesystem.cc
+++ b/src/utils/filesystem.cc
@@ -28,7 +28,15 @@
 #include "utils/exception.h"
 #include "utils/filesystem.h"
 
+#if __has_include(<filesystem>)
 #include <filesystem>
+#else
+#include <experimental/filesystem>
+// This works for the compilers we care about.
+namespace std {
+namespace filesystem = experimental::filesystem;
+}
+#endif
 
 namespace lczero {
 
@@ -46,7 +54,7 @@ std::vector<std::string> GetFileList(const std::string& directory) {
     const std::filesystem::path p(directory);
     if (std::filesystem::exists(p)) {
       for (const auto & entry : std::filesystem::directory_iterator(directory)) {
-        if (!entry.is_symlink()) {
+        if (!std::filesystem::is_symlink(entry.path())) {
           result.push_back(entry.path().filename());
         }
       }

@borg323
Copy link
Member

borg323 commented Jun 17, 2020

Unfortunately, it seems library support for on android was merged less than a month ago, and will be available in the forthcoming NDK R22: android/ndk#609.

There is no point continuing work on this for the time being.

@nguyenpham
Copy link
Contributor Author

nguyenpham commented Jun 17, 2020

@borg323: Thanks for your effort. Let me try some workaround first, say create one version for Android, and one for the rest, in the hope the app for other OSs can get some benefit from the C++17 library.

@nguyenpham
Copy link
Contributor Author

I am going to close this PR. Doable but not nice and more trouble to create three versions, one for Win, one for Android and one for the rest.

@nguyenpham nguyenpham closed this Jun 18, 2020
@borg323
Copy link
Member

borg323 commented Jun 18, 2020

We can revisit this in a few months when android NDK R22 is released.

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