Merge pull request #148 from MarkusRannare/filesystem-error-handling

Better handling of filesystem errors

Author: MarkusRannare

.github/workflows/ci.yml

@@ -9,6 +9,7 @@ jobs:
     strategy:
       fail-fast: true
       matrix:
+        action: ['run-tests', 'compile-examples']
         name:
           # - Ubuntu 16.04 GCC
           # - Ubuntu 16.04 Clang
@@ -93,11 +94,16 @@ jobs:
       - name: Checkout
         uses: actions/checkout@v2
 
+      - name: Compile for Tests
+        id: should-test
+        if: ${{matrix.action}} == 'run-tests'
+        run: echo "::set-output name=TEST-FLAGS::-DTEST=on"
+
       - name: Generate project files
         run: |
           mkdir ${{ matrix.build-dir || '.not-used' }}
           cd ${{ matrix.build-dir || '.' }}
-          cmake ${{ matrix.build-src-dir || '.' }} -Dtest=on ${{ matrix.cmake-args }}
+          cmake ${{ matrix.build-src-dir || '.' }} ${{ steps.should-test.TEST-FLAGS }} ${{ matrix.cmake-args }}
         env:
           CC: ${{ matrix.compiler }}
           CXX: ${{ matrix.cpp-compiler }}
@@ -111,12 +117,13 @@ jobs:
           cmake --build . --config ${{ matrix.build-config || 'Release' }}
 
       - name: Run test cases
+        if: matrix.action == 'run-tests'
         run: |
           cd ${{ matrix.build-dir || '.' }}
           ctest -C ${{ matrix.build-config || 'Release' }} --output-on-failure
 
       - name: Compile C examples
-        if: runner.os == 'Linux' || runner.os == 'macOS'
+        if: ( runner.os == 'Linux' || runner.os == 'macOS' ) && matrix.action == 'compile-examples'
         run: |
           cd examples/code-samples/c
           cmake .
@@ -129,7 +136,7 @@ jobs:
           LDFLAGS: ${{ matrix.ldflags || env.LDFLAGS }}
 
       - name: Compile C++ examples
-        if: runner.os == 'Linux' || runner.os == 'macOS'
+        if: ( runner.os == 'Linux' || runner.os == 'macOS' ) && matrix.action == 'compile-examples'
         run: |
           cd examples/code-samples/cpp
           cmake .

examples/code-samples/c/CMakeLists.txt

@@ -2,6 +2,8 @@ cmake_minimum_required(VERSION 3.5)
 project(examples)
 include_directories(../../../include)
 link_directories(../../../build)
+enable_language(C)
+enable_language(CXX)
 
 file( GLOB EXAMPLE_SOURCES *.c )
 

examples/code-samples/cpp/CMakeLists.txt

@@ -3,6 +3,8 @@ project(examples)
 include_directories(../../../include)
 include_directories(../../../additional_dependencies)
 set (CMAKE_CXX_STANDARD 11)
+enable_language(C)
+enable_language(CXX)
 link_directories(../../../build)
 
 file( GLOB EXAMPLE_SOURCES *.cpp )

src/Utility.cpp

@@ -289,8 +289,8 @@ std::string getFileExtension(std::string path)
 bool isDirectory(const std::string& directory)
 {
   std::error_code ec;
-  ghc::filesystem::directory_entry directoryEntry(directory, ec);
-  if( directoryEntry.exists( ec ) && !ec )
+  const ghc::filesystem::directory_entry directoryEntry(directory, ec);
+  if( !ec && directoryEntry.exists( ec ) )
   {
       return directoryEntry.is_directory( ec ) && !ec;
   }
@@ -319,19 +319,54 @@ std::string getDirectoryPath(const std::string &filename)
 
 bool fileExists(const std::string &directory)
 {
-  return ghc::filesystem::is_regular_file(directory);
+  std::error_code ec;
+  bool result = ghc::filesystem::exists(directory, ec);
+  if(ec)
+  {
+    writeLogLine("Couldn't check if file '" + directory + "' existes with error code: " + std::to_string(ec.value()) + " and message\n" + ec.message(), MODIO_DEBUGLEVEL_ERROR);
+    return false;
+  }
+  if(result)
+  {
+    result = ghc::filesystem::is_regular_file(directory, ec);
+    if(ec)
+    {
+      writeLogLine("Couldn't check if '" + directory + "' is regular file  with error code: " + std::to_string(ec.value()) + " and message\n" + ec.message(), MODIO_DEBUGLEVEL_ERROR);
+      return false;
+    }
+  }
+  return result;
 }
 
 std::vector<std::string> getFilenames(const std::string &directory)
 {
   std::vector<std::string> files;
 
-  ghc::filesystem::path directoryPath( directory );
-  for( auto& p : ghc::filesystem::recursive_directory_iterator(directoryPath) )
+  const ghc::filesystem::path directoryPath( directory );
+  if(!ghc::filesystem::exists(directoryPath))
+  {
+      writeLogLine("getFilenames: Directory '" + ghc::filesystem::absolute(directory).generic_u8string() + "' doesn't exist!", MODIO_DEBUGLEVEL_ERROR);
+      return files;
+  }
+
+  const ghc::filesystem::path abosolute = ghc::filesystem::absolute(directory);
+
+  // @todo: This might throw. If we get crash reports, we need to convert this to a while loop and pass in a ec to the iterator to handle the exception
+  // See: https://stackoverflow.com/a/16101173
+  for( const auto& p : ghc::filesystem::recursive_directory_iterator(directoryPath) )
   {
     if( !p.is_directory() )
     {
-      files.push_back( ghc::filesystem::relative( p.path(), directoryPath ).generic_u8string() );
+      std::error_code ec;
+      ghc::filesystem::path file = ghc::filesystem::relative(p.path(), directoryPath, ec).generic_u8string();
+      if (!ec)
+      {
+        files.push_back(file);
+      }
+      else
+      {
+        writeLogLine("Couldn't list file '" + directory + "/" + p.path().generic_u8string() + "' with error code: " + std::to_string(ec.value()) + " and message\n" + ec.message(), MODIO_DEBUGLEVEL_ERROR);
+      }
     }
   }
 
@@ -342,12 +377,30 @@ std::vector<std::string> getDirectoryNames(const std::string &root_directory)
 {
   std::vector<std::string> files;
 
-  ghc::filesystem::path rootDirectoryPath(root_directory);
-  for (auto& p : ghc::filesystem::directory_iterator(rootDirectoryPath))
+  const ghc::filesystem::path rootDirectoryPath(root_directory);
+
+  if(!ghc::filesystem::exists(rootDirectoryPath))
+  {
+    writeLogLine("getFilenames: Directory '" + ghc::filesystem::absolute(rootDirectoryPath).generic_u8string() + "' doesn't exist!", MODIO_DEBUGLEVEL_ERROR);
+    return files;
+  }
+
+  // @todo: This might throw. If we get crash reports, we need to convert this to a while loop and pass in a ec to the iterator to handle the exception
+  // See: https://stackoverflow.com/a/16101173
+  for (const auto& p : ghc::filesystem::directory_iterator(rootDirectoryPath))
   {
     if (p.is_directory())
     {
-      files.push_back(ghc::filesystem::relative(p.path(), rootDirectoryPath).generic_u8string());
+      std::error_code ec;
+      ghc::filesystem::path directory = ghc::filesystem::relative(p.path(), rootDirectoryPath, ec).generic_u8string();
+      if (!ec)
+      {
+        files.push_back(directory);
+      }
+      else
+      {
+        writeLogLine("Couldn't list directory '" + root_directory + "/" + p.path().generic_u8string() + "' with error code: " + std::to_string(ec.value()) + " and message\n" + ec.message(), MODIO_DEBUGLEVEL_ERROR);
+      }
     }
   }
 

src/wrappers/MinizipWrapper.cpp

@@ -288,13 +288,13 @@ void compressFiles(std::string root_directory, std::vector<std::string> filename
                                   password, crcFile, 36, UTF8_FLAG,zip64);
 
     if (err != ZIP_OK)
-      writeLogLine(std::string("Could not open ") + filenameinzip + " in zipfile, zlib error: " + toString(err), MODIO_DEBUGLEVEL_ERROR);
+      writeLogLine(std::string("Could not open '") + savefilenameinzip + "' in zipfile, zlib error: " + toString(err), MODIO_DEBUGLEVEL_ERROR);
     else
     {
       fin = modio::platform::fopen(complete_file_path.c_str(), "rb");
       if (fin == NULL)
       {
-        writeLogLine(std::string("Could not open ") + filenameinzip + " for reading", MODIO_DEBUGLEVEL_ERROR);
+        writeLogLine(std::string("Could not open '") + complete_file_path + "' for reading", MODIO_DEBUGLEVEL_ERROR);
       }
     }
 

test/Fixture_CleanupFolders.cpp

@@ -14,7 +14,7 @@ void Fixture_CleanupFolders::DeleteTransientDirectories()
   for (auto& it : ghc::filesystem::directory_iterator(ghc::filesystem::current_path()))
   {
     std::vector<ghc::filesystem::path> toDelete;
-    if (it.is_directory() && !isAllowedDirectory(it.path()))
+    if (it.is_directory() && !isPersistentDirectory(it.path()))
     {
       toDelete.push_back(it.path());
     }

test/Fixture_CleanupFolders.h

@@ -10,7 +10,7 @@ class Fixture_CleanupFolders : public ::testing::Test
   void setFilePermission(std::string fileName, bool access);
 
   // Used for teardown to delete directories that we created
-  static bool isAllowedDirectory(const ghc::filesystem::path& path)
+  static bool isPersistentDirectory(const ghc::filesystem::path& path)
   {
     for (auto& allowedFolder : allowedFolders)
     {

test/test_filesystem.cpp

@@ -174,6 +174,85 @@ TEST_F(FolderBase, CreateRelativePathSuccess)
   createRelativePathTest(u8"unicode/バックソン/");
 }
 
+TEST_F(FolderBase, getDirectoryName)
+{
+  // Create testing dataset
+  ghc::filesystem::create_directory("base_dir");
+  ghc::filesystem::create_directory("base_dir/subdir_1");
+  ghc::filesystem::create_directory("base_dir/subdir_2");
+  ghc::filesystem::create_directory(u8"base_dir/чизкейк");
+
+  std::vector<std::string> directories = modio::getDirectoryNames("base_dir");
+  EXPECT_EQ(directories.size(), 3);
+  EXPECT_NE(std::find(directories.begin(), directories.end(), "subdir_1"), directories.end());
+  EXPECT_NE(std::find(directories.begin(), directories.end(), "subdir_2"), directories.end());
+  const std::string cyrillicFolderName = u8"чизкейк";
+  EXPECT_NE(std::find(directories.begin(), directories.end(), cyrillicFolderName), directories.end());
+}
+
+
+TEST_F(FolderBase, getDirectoryNameBaseDirUTF8)
+{
+  // Create testing dataset
+  ghc::filesystem::create_directory(u8"чизкейк");
+  ghc::filesystem::create_directory(u8"чизкейк/subdir_1");
+  ghc::filesystem::create_directory(u8"чизкейк/subdir_2");
+  ghc::filesystem::create_directory(u8"чизкейк/чизкейк");
+
+  std::vector<std::string> directories = modio::getDirectoryNames(u8"чизкейк");
+  EXPECT_EQ(directories.size(), 3);
+  EXPECT_NE(std::find(directories.begin(), directories.end(), "subdir_1"), directories.end());
+  EXPECT_NE(std::find(directories.begin(), directories.end(), "subdir_2"), directories.end());
+  const std::string cyrillicFolderName = u8"чизкейк";
+  EXPECT_NE(std::find(directories.begin(), directories.end(), cyrillicFolderName), directories.end());
+}
+
+TEST_F(FolderBase, getDirectoryNameBadFolder)
+{
+    std::vector<std::string> directories;
+    EXPECT_NO_THROW( directories = modio::getDirectoryNames("BOGUS"));
+    EXPECT_EQ(directories.size(), 0);
+}
+
+TEST_F(FolderBase, getFilenames)
+{
+    // Create testing dataset
+    ghc::filesystem::create_directory("base_dir");
+    std::ofstream("base_dir/file1");
+    std::ofstream("base_dir/file2");
+    std::ofstream(u8"base_dir/чизкейк");
+
+    std::vector<std::string> files = modio::getFilenames("base_dir");
+    EXPECT_EQ(files.size(), 3);
+    EXPECT_NE(std::find(files.begin(), files.end(), "file1"), files.end());
+    EXPECT_NE(std::find(files.begin(), files.end(), "file2"), files.end());
+    const std::string cyrillicFileName = u8"чизкейк";
+    EXPECT_NE(std::find(files.begin(), files.end(), cyrillicFileName), files.end());
+}
+
+TEST_F(FolderBase, getFilenamesUTF8)
+{
+    // Create testing dataset
+    ghc::filesystem::create_directory(u8"чизкейк");
+    std::ofstream(u8"чизкейк/file1");
+    std::ofstream(u8"чизкейк/file2");
+    std::ofstream(u8"чизкейк/чизкейк");
+
+    std::vector<std::string> files = modio::getFilenames(u8"чизкейк");
+    EXPECT_EQ(files.size(), 3);
+    EXPECT_NE(std::find(files.begin(), files.end(), "file1"), files.end());
+    EXPECT_NE(std::find(files.begin(), files.end(), "file2"), files.end());
+    const std::string cyrillicFileName = u8"чизкейк";
+    EXPECT_NE(std::find(files.begin(), files.end(), cyrillicFileName), files.end());
+}
+
+TEST_F(FolderBase, getFilenamesBadFolder)
+{
+    std::vector<std::string> files;
+    EXPECT_NO_THROW( files = modio::getFilenames("BOGUS"));
+    EXPECT_EQ(files.size(), 0);
+}
+
 static void createRelativePathWithFilename(const std::string& relativePath, const std::string& parentFolder)
 {
   bool result = modio::createPath(relativePath);

test/test_modio.cpp

@@ -209,7 +209,7 @@ TEST_F(Modio, TestGetMod)
     modioShutdown();
 }
 
-static std::string downloadedImageFilename = u8"downloaded/logo_original.png";
+//static std::string downloadedImageFilename = u8"downloaded/logo_original.png";
 
 u32 calculateCRCOfFile( const std::string& file )
 {
@@ -246,7 +246,7 @@ u32 calculateCRCOfFile( const std::string& file )
   return crcValue;
 }
 
-void onDownloadImage_TestDownloadImage(void* object, ModioResponse response)
+/*void onDownloadImage_TestDownloadImage(void* object, ModioResponse response)
 {
   bool* wait = (bool*)object;
 
@@ -332,7 +332,7 @@ TEST_F(Modio, TestDownloadImageUnicode)
   }
 
   modioShutdown();
-}
+}*/
 
 TEST_F(Modio, TestInstallMods)
 {