From 8e932304d0f4c0654901e81b140fae0e9246b6c2 Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Thu, 22 Apr 2021 17:15:42 -0700
Subject: [PATCH 01/11] Reformulate unix-specific file operations with
 std::filesystem

Issues #307 #765

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 .github/workflows/windows-compile.yml |   4 +-
 common/util/file_util.cc              | 170 ++++++++++----------------
 common/util/file_util_test.cc         |  22 ++--
 3 files changed, 82 insertions(+), 114 deletions(-)

diff --git a/.github/workflows/windows-compile.yml b/.github/workflows/windows-compile.yml
index 25f2edb24..d2a06afc3 100644
--- a/.github/workflows/windows-compile.yml
+++ b/.github/workflows/windows-compile.yml
@@ -31,7 +31,9 @@ jobs:
     # Windows support is in progress, only run "working" tests
     - name: Run known-to-work tests
       run: |
-        bazel test  //common/analysis:command_file_lexer_test `
+        bazel test  --test_output=errors `
+                    //common/util:file_util_test `
+                    //common/analysis:command_file_lexer_test `
                     //common/analysis:file_analyzer_test `
                     //common/analysis:line_linter_test `
                     //common/analysis:lint_rule_status_test `
diff --git a/common/util/file_util.cc b/common/util/file_util.cc
index a5433a4fe..76e836aab 100644
--- a/common/util/file_util.cc
+++ b/common/util/file_util.cc
@@ -14,19 +14,17 @@
 
 #include "common/util/file_util.h"
 
-#include <dirent.h>
 #include <errno.h>
 #include <stdlib.h>
 #include <string.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <unistd.h>
 
 #include <algorithm>
+#include <filesystem>
 #include <fstream>
 #include <iostream>
 #include <streambuf>
 #include <string>
+#include <system_error>
 
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
@@ -36,6 +34,8 @@
 #include "absl/strings/string_view.h"
 #include "common/util/logging.h"
 
+namespace fs = std::filesystem;
+
 namespace verible {
 namespace file {
 absl::string_view Basename(absl::string_view filename) {
@@ -64,10 +64,12 @@ absl::string_view Stem(absl::string_view filename) {
 
 // This will always return an error, even if we can't determine anything
 // from errno. Returns "fallback_msg" in that case.
-static absl::Status CreateErrorStatusFromErrno(const char *fallback_msg) {
+static absl::Status CreateErrorStatusFromSysError(const char *fallback_msg,
+                                                  int sys_error) {
   using absl::StatusCode;
-  const char *const system_msg = errno == 0 ? fallback_msg : strerror(errno);
-  switch (errno) {
+  const char *const system_msg = sys_error == 0
+    ? fallback_msg : strerror(sys_error);
+  switch (sys_error) {
     case EPERM:
     case EACCES:
       return absl::Status(StatusCode::kPermissionDenied, system_msg);
@@ -82,58 +84,49 @@ static absl::Status CreateErrorStatusFromErrno(const char *fallback_msg) {
   }
 }
 
-// TODO (after bump to c++17) rewrite this function to use std::filesystem
+static absl::Status CreateErrorStatusFromErrno(const char *fallback_msg) {
+  return CreateErrorStatusFromSysError(fallback_msg, errno);
+}
+static absl::Status CreateErrorStatusFromErr(const char *fallback_msg,
+                                             const std::error_code& err) {
+  // TODO: this assumes that err.value() returns errno-like values. Might not
+  // always be the case.
+  return CreateErrorStatusFromSysError(fallback_msg, err.value());
+}
+
 absl::Status UpwardFileSearch(absl::string_view start,
                               absl::string_view filename, std::string *result) {
-  static constexpr char dir_separator[] = "/";
-
-  /* Convert to absolute path */
-  char absolute_path[PATH_MAX];
-  if (realpath(std::string(start).c_str(), absolute_path) == nullptr) {
-    return absl::InvalidArgumentError("Invalid config path specified.");
-  }
+  std::error_code err;
+  const std::string search_file(filename);
+  fs::path absolute_path = fs::absolute(std::string(start), err);
+  if (err.value() != 0)
+    return CreateErrorStatusFromErr("invalid config path specified.", err);
 
-  /* Add trailing slash */
-  const std::string full_path = absl::StrCat(absolute_path, dir_separator);
-
-  VLOG(1) << "Upward search for " << filename << ", starting in " << start;
-
-  size_t search_up_to = full_path.length();
+  fs::path probe_dir = absolute_path;
   for (;;) {
-    const size_t separator_pos = full_path.rfind(dir_separator, search_up_to);
-
-    if (separator_pos == search_up_to || separator_pos == std::string::npos) {
-      break;
-    }
-
-    const std::string candidate =
-        absl::StrCat(full_path.substr(0, separator_pos + 1), filename);
-
-    if (access(candidate.c_str(), R_OK) != -1) {
-      *result = candidate;
-      VLOG(1) << "Found: " << *result;
+    *result = (probe_dir / search_file).string();
+    if (FileExists(*result).ok())
       return absl::OkStatus();
-    }
-
-    search_up_to = separator_pos - 1;
-
-    if (separator_pos == 0) {
-      break;
-    }
+    fs::path one_up = probe_dir.parent_path();
+    if (one_up == probe_dir) break;
+    probe_dir = one_up;
   }
-
   return absl::NotFoundError("No matching file found.");
 }
 
 absl::Status FileExists(const std::string &filename) {
-  struct stat file_info;
-  if (stat(filename.c_str(), &file_info) != 0) {
-    return absl::NotFoundError(absl::StrCat(filename, ": No such file."));
+  std::error_code err;
+  fs::file_status stat = fs::status(filename, err);
+
+  if (err.value() != 0) {
+    return absl::NotFoundError(absl::StrCat(filename, ":", err.message()));
   }
-  if (S_ISREG(file_info.st_mode) || S_ISFIFO(file_info.st_mode)) {
+
+  if (fs::is_regular_file(stat) || fs::is_fifo(stat)) {
     return absl::OkStatus();
   }
-  if (S_ISDIR(file_info.st_mode)) {
+
+  if (fs::is_directory(stat)) {
     return absl::InvalidArgumentError(
         absl::StrCat(filename, ": is a directory, not a file"));
   }
@@ -174,83 +167,48 @@ absl::Status SetContents(absl::string_view filename,
 }
 
 std::string JoinPath(absl::string_view base, absl::string_view name) {
-  // TODO: this will certainly be different on Windows
-  static constexpr absl::string_view kFileSeparator = "/";
-
-  // Make sure that we don't concatenate multiple superfluous separators.
-  // Note, since we're using string_view, the substr() operations are cheap.
-  while (!base.empty() && base[base.length() - 1] == kFileSeparator[0]) {
-    base = base.substr(0, base.length() - 1);
-  }
-  while (!name.empty() && name[0] == kFileSeparator[0]) {
+  // Make sure the second element is not already absolute, otherwise
+  // the fs::path() uses this as toplevel path. This is only an issue with
+  // Unix paths. Windows paths will have a c:\ for explicit full-pathness
+  while (!name.empty() && name[0] == '/') {
     name = name.substr(1);
   }
-  return absl::StrCat(base, kFileSeparator, name);
+
+  fs::path p = fs::path(std::string(base)) / fs::path(std::string(name));
+  return p.lexically_normal().string();
 }
 
 absl::Status CreateDir(absl::string_view dir) {
   const std::string path(dir);
-  int ret = mkdir(path.c_str(), 0755);
-  if (ret == 0 || errno == EEXIST) return absl::OkStatus();
-  return CreateErrorStatusFromErrno("can't create directory");
+  std::error_code err;
+  if (fs::create_directory(path, err) || err.value() == 0)
+    return absl::OkStatus();
+
+  return CreateErrorStatusFromErr("can't create directory", err);
 }
 
 absl::StatusOr<Directory> ListDir(absl::string_view dir) {
+  std::error_code err;
   Directory d;
+
   d.path = dir.empty() ? "." : std::string(dir);
 
-  // Reset the errno and open the directory
-  errno = 0;
-  DIR *handle = opendir(d.path.c_str());
-  if (handle == nullptr) {
-    if (errno == ENOTDIR) return absl::NotFoundError(d.path);
-    return absl::InternalError(absl::StrCat("Failed to open the directory '",
-                                            d.path, "'. Got error: ", errno));
+  fs::file_status stat = fs::status(d.path, err);
+  if (err.value() != 0)
+    return CreateErrorStatusFromErr("Opening directory", err);
+  if (!fs::is_directory(stat)) {
+    return absl::InvalidArgumentError(absl::StrCat(dir, ": not a directory"));
   }
-  struct stat statbuf;
-  while (true) {
-    errno = 0;
-    auto *entry = readdir(handle);
-    if (errno) {
-      closedir(handle);
-      return absl::InternalError(
-          absl::StrCat("Failed to read the contents of directory '", d.path,
-                       "'. Got error: ", errno));
-    }
-    // Finished listing the directory.
-    if (entry == nullptr) {
-      break;
-    }
-    absl::string_view d_name(entry->d_name);
-    // Skip '.' and '..' directory links
-    if (d_name == "." || d_name == "..") {
-      continue;
-    }
 
-    std::string full_path = verible::file::JoinPath(d.path, d_name);
-    auto d_type = entry->d_type;
-    if (d_type == DT_LNK || d_type == DT_UNKNOWN) {
-      // Try to resolve symlinks and unknown nodes.
-      if (stat(full_path.c_str(), &statbuf) == -1) {
-        LOG(WARNING) << "Stat failed. Ignoring " << d_name;
-        continue;
-      }
-      if (S_ISDIR(statbuf.st_mode)) {
-        d.directories.push_back(full_path);
-      } else if (S_ISREG(statbuf.st_mode)) {
-        d.files.push_back(full_path);
-      } else {
-        LOG(INFO) << "Ignoring " << d_name
-                  << " because st_mode == " << statbuf.st_mode;
-        continue;
-      }
-    } else if (d_type == DT_DIR) {
-      d.directories.push_back(full_path);
+  for (const fs::directory_entry entry : fs::directory_iterator(d.path)) {
+    const std::string entry_name = entry.path();
+    if (entry.is_directory()) {
+      d.directories.push_back(entry_name);
     } else {
-      d.files.push_back(full_path);
+      d.files.push_back(entry_name);
     }
   }
-  closedir(handle);
+
   std::sort(d.files.begin(), d.files.end());
   std::sort(d.directories.begin(), d.directories.end());
   return d;
diff --git a/common/util/file_util_test.cc b/common/util/file_util_test.cc
index d4e7c69c8..cf64f9dcd 100644
--- a/common/util/file_util_test.cc
+++ b/common/util/file_util_test.cc
@@ -27,9 +27,9 @@
 using testing::HasSubstr;
 
 #undef EXPECT_OK
-#define EXPECT_OK(value) EXPECT_TRUE((value).ok())
+#define EXPECT_OK(value) { auto s = (value); EXPECT_TRUE(s.ok()) << s; }
 #undef ASSERT_OK
-#define ASSERT_OK(value) ASSERT_TRUE((value).ok())
+#define ASSERT_OK(value) { auto s = (value); ASSERT_TRUE(s.ok()) << s; }
 
 namespace verible {
 namespace util {
@@ -66,19 +66,22 @@ TEST(FileUtil, Stem) {
 
 TEST(FileUtil, JoinPath) {
   EXPECT_EQ(file::JoinPath("foo", ""), "foo/");
-  EXPECT_EQ(file::JoinPath("", "bar"), "/bar");
+  EXPECT_EQ(file::JoinPath("", "bar"), "bar");
   EXPECT_EQ(file::JoinPath("foo", "bar"), "foo/bar");
 
   // Assemble an absolute path
-  EXPECT_EQ(file::JoinPath("", "bar"), "/bar");
+  EXPECT_EQ(file::JoinPath("", "bar"), "bar");
   EXPECT_EQ(file::JoinPath("/", "bar"), "/bar");
-  EXPECT_EQ(file::JoinPath("///", "///bar"), "/bar");
 
   // Lightly canonicalize multiple consecutive slashes
   EXPECT_EQ(file::JoinPath("foo/", "bar"), "foo/bar");
   EXPECT_EQ(file::JoinPath("foo///", "bar"), "foo/bar");
   EXPECT_EQ(file::JoinPath("foo/", "/bar"), "foo/bar");
   EXPECT_EQ(file::JoinPath("foo/", "///bar"), "foo/bar");
+
+#ifdef _WIN32
+  EXPECT_EQ(file::JoinPath("C:\\foo", "bar"), "C:\\foo\\bar");
+#endif
 }
 
 TEST(FileUtil, CreateDir) {
@@ -87,6 +90,7 @@ TEST(FileUtil, CreateDir) {
   const absl::string_view test_content = "directory create test";
 
   EXPECT_OK(file::CreateDir(test_dir));
+  EXPECT_OK(file::CreateDir(test_dir));  // Creating twice should succeed
 
   EXPECT_OK(file::SetContents(test_file, test_content));
   std::string read_back_content;
@@ -240,7 +244,7 @@ TEST(FileUtil, ReadNonexistentDirectory) {
 
   auto dir_or = file::ListDir(test_dir);
   EXPECT_FALSE(dir_or.ok());
-  EXPECT_EQ(dir_or.status().code(), absl::StatusCode::kInternal);
+  EXPECT_EQ(dir_or.status().code(), absl::StatusCode::kNotFound);
 }
 
 TEST(FileUtil, ListNotADirectory) {
@@ -248,7 +252,7 @@ TEST(FileUtil, ListNotADirectory) {
 
   auto dir_or = file::ListDir(tempfile.filename());
   EXPECT_FALSE(dir_or.ok());
-  EXPECT_EQ(dir_or.status().code(), absl::StatusCode::kNotFound);
+  EXPECT_EQ(dir_or.status().code(), absl::StatusCode::kInvalidArgument);
 }
 
 TEST(FileUtil, ReadDirectory) {
@@ -276,7 +280,11 @@ TEST(FileUtil, ReadDirectory) {
 
   const auto& dir = *dir_or;
   EXPECT_EQ(dir.path, test_dir);
+
+  EXPECT_EQ(dir.directories.size(), test_directories.size());
   EXPECT_EQ(dir.directories, test_directories);
+
+  EXPECT_EQ(dir.files.size(), test_files.size());
   EXPECT_EQ(dir.files, test_files);
 }
 

From 5c13263d625a296223c321e413e9b1e849cf1742 Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Thu, 22 Apr 2021 17:24:46 -0700
Subject: [PATCH 02/11] Remove fragile stdin GetContents() test.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 common/util/file_util.cc      |  9 ++++-----
 common/util/file_util_test.cc | 23 ++++++++++-------------
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/common/util/file_util.cc b/common/util/file_util.cc
index 76e836aab..03649e858 100644
--- a/common/util/file_util.cc
+++ b/common/util/file_util.cc
@@ -67,8 +67,8 @@ absl::string_view Stem(absl::string_view filename) {
 static absl::Status CreateErrorStatusFromSysError(const char *fallback_msg,
                                                   int sys_error) {
   using absl::StatusCode;
-  const char *const system_msg = sys_error == 0
-    ? fallback_msg : strerror(sys_error);
+  const char *const system_msg =
+      sys_error == 0 ? fallback_msg : strerror(sys_error);
   switch (sys_error) {
     case EPERM:
     case EACCES:
@@ -88,7 +88,7 @@ static absl::Status CreateErrorStatusFromErrno(const char *fallback_msg) {
   return CreateErrorStatusFromSysError(fallback_msg, errno);
 }
 static absl::Status CreateErrorStatusFromErr(const char *fallback_msg,
-                                             const std::error_code& err) {
+                                             const std::error_code &err) {
   // TODO: this assumes that err.value() returns errno-like values. Might not
   // always be the case.
   return CreateErrorStatusFromSysError(fallback_msg, err.value());
@@ -105,8 +105,7 @@ absl::Status UpwardFileSearch(absl::string_view start,
   fs::path probe_dir = absolute_path;
   for (;;) {
     *result = (probe_dir / search_file).string();
-    if (FileExists(*result).ok())
-      return absl::OkStatus();
+    if (FileExists(*result).ok()) return absl::OkStatus();
     fs::path one_up = probe_dir.parent_path();
     if (one_up == probe_dir) break;
     probe_dir = one_up;
diff --git a/common/util/file_util_test.cc b/common/util/file_util_test.cc
index cf64f9dcd..84edd2280 100644
--- a/common/util/file_util_test.cc
+++ b/common/util/file_util_test.cc
@@ -27,9 +27,17 @@
 using testing::HasSubstr;
 
 #undef EXPECT_OK
-#define EXPECT_OK(value) { auto s = (value); EXPECT_TRUE(s.ok()) << s; }
+#define EXPECT_OK(value)      \
+  {                           \
+    auto s = (value);         \
+    EXPECT_TRUE(s.ok()) << s; \
+  }
 #undef ASSERT_OK
-#define ASSERT_OK(value) { auto s = (value); ASSERT_TRUE(s.ok()) << s; }
+#define ASSERT_OK(value)      \
+  {                           \
+    auto s = (value);         \
+    ASSERT_TRUE(s.ok()) << s; \
+  }
 
 namespace verible {
 namespace util {
@@ -125,17 +133,6 @@ TEST(FileUtil, ScopedTestFile) {
   EXPECT_EQ(test_content, read_back_content);
 }
 
-TEST(FileUtil, ScopedTestFileStdin) {
-  // When running interactively, skip this test to avoid waiting for stdin.
-  if (isatty(STDIN_FILENO)) return;
-  // Testing is non-interactive, so reading from stdin will be immediately
-  // closed, resulting in an empty string.
-  const absl::string_view test_content = "";
-  std::string read_back_content;
-  EXPECT_OK(file::GetContents("-", &read_back_content));
-  EXPECT_EQ(test_content, read_back_content);
-}
-
 static ScopedTestFile TestFileGenerator(absl::string_view content) {
   return ScopedTestFile(testing::TempDir(), content);
 }

From fa7de211e87cc2a10613a9f0b1a3e876d9d0b029 Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Thu, 22 Apr 2021 17:35:56 -0700
Subject: [PATCH 03/11] Avoid unix specific getpid(), isatty().

Also proper std::filesystem::path string conversion.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 common/util/file_util.cc | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/common/util/file_util.cc b/common/util/file_util.cc
index 03649e858..97cc28d33 100644
--- a/common/util/file_util.cc
+++ b/common/util/file_util.cc
@@ -136,11 +136,9 @@ absl::Status FileExists(const std::string &filename) {
 absl::Status GetContents(absl::string_view filename, std::string *content) {
   std::ifstream fs;
   std::istream *stream = nullptr;
-  const bool use_stdin = filename == "-";
+  const bool use_stdin = filename == "-";  // convention: honor "-" as stdin
   if (use_stdin) {
-    // convention: honor "-" as stdin
     stream = &std::cin;
-    if (isatty(0)) std::cerr << "Enter input (terminate with Ctrl-D):\n";
   } else {
     const std::string filename_str = std::string(filename);
     absl::Status usable_file = FileExists(filename_str);
@@ -200,7 +198,7 @@ absl::StatusOr<Directory> ListDir(absl::string_view dir) {
   }
 
   for (const fs::directory_entry entry : fs::directory_iterator(d.path)) {
-    const std::string entry_name = entry.path();
+    const std::string entry_name = entry.path().string();
     if (entry.is_directory()) {
       d.directories.push_back(entry_name);
     } else {
@@ -216,7 +214,7 @@ absl::StatusOr<Directory> ListDir(absl::string_view dir) {
 namespace testing {
 
 std::string RandomFileBasename(absl::string_view prefix) {
-  return absl::StrCat(prefix, "-", getpid(), "-", random());
+  return absl::StrCat(prefix, "-", rand());
 }
 
 ScopedTestFile::ScopedTestFile(absl::string_view base_dir,

From c886964ed9f871ea3d2b45f989d9c293189e29cb Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Thu, 22 Apr 2021 18:08:52 -0700
Subject: [PATCH 04/11] Adapt the expected path string outputs depending on
 platform.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 common/util/file_util_test.cc | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/common/util/file_util_test.cc b/common/util/file_util_test.cc
index 84edd2280..f27a7bc8a 100644
--- a/common/util/file_util_test.cc
+++ b/common/util/file_util_test.cc
@@ -15,6 +15,7 @@
 #include "common/util/file_util.h"
 
 #include <algorithm>
+#include <filesystem>
 #include <string>
 #include <vector>
 
@@ -72,20 +73,25 @@ TEST(FileUtil, Stem) {
   EXPECT_EQ(file::Stem("/foo/bar"), "/foo/bar");
 }
 
+// Returns the forward-slashed path in platform-specific notation.
+static std::string PlatformPath(const std::string& path) {
+  return std::filesystem::path(path).lexically_normal().string();
+}
+
 TEST(FileUtil, JoinPath) {
-  EXPECT_EQ(file::JoinPath("foo", ""), "foo/");
+  EXPECT_EQ(file::JoinPath("foo", ""), PlatformPath("foo/"));
   EXPECT_EQ(file::JoinPath("", "bar"), "bar");
-  EXPECT_EQ(file::JoinPath("foo", "bar"), "foo/bar");
+  EXPECT_EQ(file::JoinPath("foo", "bar"), PlatformPath("foo/bar"));
 
   // Assemble an absolute path
   EXPECT_EQ(file::JoinPath("", "bar"), "bar");
-  EXPECT_EQ(file::JoinPath("/", "bar"), "/bar");
+  EXPECT_EQ(file::JoinPath("/", "bar"), PlatformPath("/bar"));
 
   // Lightly canonicalize multiple consecutive slashes
-  EXPECT_EQ(file::JoinPath("foo/", "bar"), "foo/bar");
-  EXPECT_EQ(file::JoinPath("foo///", "bar"), "foo/bar");
-  EXPECT_EQ(file::JoinPath("foo/", "/bar"), "foo/bar");
-  EXPECT_EQ(file::JoinPath("foo/", "///bar"), "foo/bar");
+  EXPECT_EQ(file::JoinPath("foo/", "bar"), PlatformPath("foo/bar"));
+  EXPECT_EQ(file::JoinPath("foo///", "bar"), PlatformPath("foo/bar"));
+  EXPECT_EQ(file::JoinPath("foo/", "/bar"), PlatformPath("foo/bar"));
+  EXPECT_EQ(file::JoinPath("foo/", "///bar"), PlatformPath("foo/bar"));
 
 #ifdef _WIN32
   EXPECT_EQ(file::JoinPath("C:\\foo", "bar"), "C:\\foo\\bar");
@@ -204,17 +210,17 @@ TEST(FileUtil, UpwardFileSearchTest) {
   // We don't compare the full path, as the UpwardFileSearch() makes it an
   // realpath which might not entirely match our root_dir prefix anymore; but
   // we do know that the suffix should be the same.
-  EXPECT_TRUE(absl::EndsWith(result, "up-search/foo/foo-file"));
+  EXPECT_TRUE(absl::EndsWith(result, PlatformPath("up-search/foo/foo-file")));
 
   // Somewhere below
   EXPECT_OK(file::UpwardFileSearch(file::JoinPath(root_dir, "foo/bar/baz"),
                                    "foo-file", &result));
-  EXPECT_TRUE(absl::EndsWith(result, "up-search/foo/foo-file"));
+  EXPECT_TRUE(absl::EndsWith(result, PlatformPath("up-search/foo/foo-file")));
 
   // Find toplevel file
   EXPECT_OK(file::UpwardFileSearch(file::JoinPath(root_dir, "foo/bar/baz"),
                                    "toplevel-file", &result));
-  EXPECT_TRUE(absl::EndsWith(result, "up-search/toplevel-file"));
+  EXPECT_TRUE(absl::EndsWith(result, PlatformPath("up-search/toplevel-file")));
 
   // Negative test.
   auto status = file::UpwardFileSearch(file::JoinPath(root_dir, "foo/bar/baz"),

From 92553c538ce787e64902ea9aada739d138677b32 Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Thu, 22 Apr 2021 18:24:53 -0700
Subject: [PATCH 05/11] chmod() does not work on Win32

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 common/util/file_util_test.cc | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/common/util/file_util_test.cc b/common/util/file_util_test.cc
index f27a7bc8a..814f52ddb 100644
--- a/common/util/file_util_test.cc
+++ b/common/util/file_util_test.cc
@@ -125,10 +125,15 @@ TEST(FileUtil, StatusErrorReporting) {
   EXPECT_OK(file::GetContents(test_file, &content));
   EXPECT_EQ(content, "foo");
 
+#ifndef _WIN32
+  // The following chmod() is not working on Win32. So let's not use
+  // this test here.
+  // TODO: Can we make permission-denied test that works on Windows ?
   chmod(test_file.c_str(), 0);  // Enforce a permission denied situation
   status = file::GetContents(test_file, &content);
   EXPECT_FALSE(status.ok());
   EXPECT_EQ(status.code(), absl::StatusCode::kPermissionDenied) << status;
+#endif
 }
 
 TEST(FileUtil, ScopedTestFile) {

From 3aca9d4fa259027c9815d0742a6b6a788be5cfeb Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Thu, 22 Apr 2021 18:39:17 -0700
Subject: [PATCH 06/11] Temporarily enable all artifacts build to make sure
 <filesystem> builds there as well.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 .github/workflows/verible-ci.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/verible-ci.yml b/.github/workflows/verible-ci.yml
index f22468456..6c91065c3 100644
--- a/.github/workflows/verible-ci.yml
+++ b/.github/workflows/verible-ci.yml
@@ -155,7 +155,7 @@ jobs:
     runs-on: ubuntu-20.04
     # Github actions resources are limited; don't build artifacts for all
     # platforms on every pull request, only on push.
-    if: ${{github.event_name == 'push'}}
+    #if: ${{github.event_name == 'push'}}
     strategy:
       fail-fast: false
       matrix:

From 0a30bb69cee2bc37c4162a199b946447b4ede7f6 Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Fri, 23 Apr 2021 08:48:34 -0700
Subject: [PATCH 07/11] Testing if newer compilers exist on older platforms.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 releasing/ubuntu/common/compiler.dockerstage | 7 +++++--
 releasing/ubuntu/xenial/compiler.dockerstage | 8 ++++----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/releasing/ubuntu/common/compiler.dockerstage b/releasing/ubuntu/common/compiler.dockerstage
index a28bbc83c..cd8609b74 100644
--- a/releasing/ubuntu/common/compiler.dockerstage
+++ b/releasing/ubuntu/common/compiler.dockerstage
@@ -1,5 +1,8 @@
 # Install compiler
 RUN apt-get install -y  \
     build-essential     \
-    g++                 \
-    gcc
+    g++-9               \
+    gcc-9
+
+RUN ln -sf /usr/bin/gcc-9 /usr/bin/gcc
+RUN ln -sf /usr/bin/g++-9 /usr/bin/g++
diff --git a/releasing/ubuntu/xenial/compiler.dockerstage b/releasing/ubuntu/xenial/compiler.dockerstage
index 086eef19b..ff58c9661 100644
--- a/releasing/ubuntu/xenial/compiler.dockerstage
+++ b/releasing/ubuntu/xenial/compiler.dockerstage
@@ -1,11 +1,11 @@
 RUN add-apt-repository ppa:ubuntu-toolchain-r/test; apt-get update
 RUN apt-get install -y  \
     build-essential     \
-    g++-7               \
-    gcc-7
+    g++-8               \
+    gcc-8
 
-RUN ln -sf /usr/bin/gcc-7 /usr/bin/gcc
-RUN ln -sf /usr/bin/g++-7 /usr/bin/g++
+RUN ln -sf /usr/bin/gcc-8 /usr/bin/gcc
+RUN ln -sf /usr/bin/g++-8 /usr/bin/g++
 
 # Link libstdc++ statically
 ENV BAZEL_LINKOPTS "-static-libstdc++:-lm"

From d3c5d3dc02580e08c630ed9779c524419c74eb40 Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Fri, 23 Apr 2021 12:59:51 -0700
Subject: [PATCH 08/11] Make file_utils dual implementation for now.

The old compilers for ancient Linux distributions can't deal with
std::filesystem yet. Until that is resolved, let's have a dual
implementation that chooses std::filesystem only on Windows for
now.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 common/util/file_util.cc      | 164 +++++++++++++++++++++++++++++++++-
 common/util/file_util_test.cc |  16 +++-
 2 files changed, 176 insertions(+), 4 deletions(-)

diff --git a/common/util/file_util.cc b/common/util/file_util.cc
index 97cc28d33..068f2c2cc 100644
--- a/common/util/file_util.cc
+++ b/common/util/file_util.cc
@@ -19,12 +19,10 @@
 #include <string.h>
 
 #include <algorithm>
-#include <filesystem>
 #include <fstream>
 #include <iostream>
 #include <streambuf>
 #include <string>
-#include <system_error>
 
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
@@ -34,7 +32,28 @@
 #include "absl/strings/string_view.h"
 #include "common/util/logging.h"
 
+/*
+ * We're in the transition to use std::filesystem for all file operations.
+ * However some ancient compilers in releasing/ can't handle that yet. Until
+ * that is figured out, have a dual implementation.
+ * For now, only enable the std::filesystem implementation on Windows
+ * TODO(hzeller): remove the non-std::filesystem implementation once the release
+ *                issues have been resolved.
+ */
+#ifdef _WIN32
+#  define USE_CPP_17_FILESYSTEM 1
+#endif
+
+#ifdef USE_CPP_17_FILESYSTEM
+#  include <filesystem>
+#  include <system_error>
 namespace fs = std::filesystem;
+#else
+#  include <dirent.h>
+#  include <sys/stat.h>
+#  include <sys/types.h>
+#  include <unistd.h>
+#endif
 
 namespace verible {
 namespace file {
@@ -87,15 +106,19 @@ static absl::Status CreateErrorStatusFromSysError(const char *fallback_msg,
 static absl::Status CreateErrorStatusFromErrno(const char *fallback_msg) {
   return CreateErrorStatusFromSysError(fallback_msg, errno);
 }
+
+#ifdef USE_CPP_17_FILESYSTEM
 static absl::Status CreateErrorStatusFromErr(const char *fallback_msg,
                                              const std::error_code &err) {
   // TODO: this assumes that err.value() returns errno-like values. Might not
   // always be the case.
   return CreateErrorStatusFromSysError(fallback_msg, err.value());
 }
+#endif
 
 absl::Status UpwardFileSearch(absl::string_view start,
                               absl::string_view filename, std::string *result) {
+#if USE_CPP_17_FILESYSTEM
   std::error_code err;
   const std::string search_file(filename);
   fs::path absolute_path = fs::absolute(std::string(start), err);
@@ -111,9 +134,51 @@ absl::Status UpwardFileSearch(absl::string_view start,
     probe_dir = one_up;
   }
   return absl::NotFoundError("No matching file found.");
+#else
+  // Unix specific implementation.
+  static constexpr char dir_separator[] = "/";
+
+  /* Convert to absolute path */
+  char absolute_path[PATH_MAX];
+  if (realpath(std::string(start).c_str(), absolute_path) == nullptr) {
+    return absl::InvalidArgumentError("Invalid config path specified.");
+  }
+
+  /* Add trailing slash */
+  const std::string full_path = absl::StrCat(absolute_path, dir_separator);
+
+  VLOG(1) << "Upward search for " << filename << ", starting in " << start;
+
+  size_t search_up_to = full_path.length();
+  for (;;) {
+    const size_t separator_pos = full_path.rfind(dir_separator, search_up_to);
+
+    if (separator_pos == search_up_to || separator_pos == std::string::npos) {
+      break;
+    }
+
+    const std::string candidate =
+        absl::StrCat(full_path.substr(0, separator_pos + 1), filename);
+
+    if (access(candidate.c_str(), R_OK) != -1) {
+      *result = candidate;
+      VLOG(1) << "Found: " << *result;
+      return absl::OkStatus();
+    }
+
+    search_up_to = separator_pos - 1;
+
+    if (separator_pos == 0) {
+      break;
+    }
+  }
+
+  return absl::NotFoundError("No matching file found.");
+#endif
 }
 
 absl::Status FileExists(const std::string &filename) {
+#if USE_CPP_17_FILESYSTEM
   std::error_code err;
   fs::file_status stat = fs::status(filename, err);
 
@@ -129,6 +194,20 @@ absl::Status FileExists(const std::string &filename) {
     return absl::InvalidArgumentError(
         absl::StrCat(filename, ": is a directory, not a file"));
   }
+#else
+  // Unix specific implementation.
+  struct stat file_info;
+  if (stat(filename.c_str(), &file_info) != 0) {
+    return absl::NotFoundError(absl::StrCat(filename, ": No such file."));
+  }
+  if (S_ISREG(file_info.st_mode) || S_ISFIFO(file_info.st_mode)) {
+    return absl::OkStatus();
+  }
+  if (S_ISDIR(file_info.st_mode)) {
+    return absl::InvalidArgumentError(
+        absl::StrCat(filename, ": is a directory, not a file"));
+  }
+#endif
   return absl::InvalidArgumentError(
       absl::StrCat(filename, ": not a regular file."));
 }
@@ -164,6 +243,7 @@ absl::Status SetContents(absl::string_view filename,
 }
 
 std::string JoinPath(absl::string_view base, absl::string_view name) {
+#if USE_CPP_17_FILESYSTEM
   // Make sure the second element is not already absolute, otherwise
   // the fs::path() uses this as toplevel path. This is only an issue with
   // Unix paths. Windows paths will have a c:\ for explicit full-pathness
@@ -173,18 +253,42 @@ std::string JoinPath(absl::string_view base, absl::string_view name) {
 
   fs::path p = fs::path(std::string(base)) / fs::path(std::string(name));
   return p.lexically_normal().string();
+#else
+  // Unix specific implementation
+  static constexpr absl::string_view kFileSeparator = "/";
+
+  if (base.empty()) return std::string(name);
+
+  // Make sure that we don't concatenate multiple superfluous separators.
+  // Note, since we're using string_view, the substr() operations are cheap.
+  while (!base.empty() && base[base.length() - 1] == kFileSeparator[0]) {
+    base = base.substr(0, base.length() - 1);
+  }
+  while (!name.empty() && name[0] == kFileSeparator[0]) {
+    name = name.substr(1);
+  }
+  return absl::StrCat(base, kFileSeparator, name);
+#endif
 }
 
 absl::Status CreateDir(absl::string_view dir) {
+#if USE_CPP_17_FILESYSTEM
   const std::string path(dir);
   std::error_code err;
   if (fs::create_directory(path, err) || err.value() == 0)
     return absl::OkStatus();
 
   return CreateErrorStatusFromErr("can't create directory", err);
+#else
+  const std::string path(dir);
+  int ret = mkdir(path.c_str(), 0755);
+  if (ret == 0 || errno == EEXIST) return absl::OkStatus();
+  return CreateErrorStatusFromErrno("can't create directory");
+#endif
 }
 
 absl::StatusOr<Directory> ListDir(absl::string_view dir) {
+#if USE_CPP_17_FILESYSTEM
   std::error_code err;
   Directory d;
 
@@ -205,6 +309,62 @@ absl::StatusOr<Directory> ListDir(absl::string_view dir) {
       d.files.push_back(entry_name);
     }
   }
+#else
+  Directory d;
+  d.path = dir.empty() ? "." : std::string(dir);
+
+  // Reset the errno and open the directory
+  errno = 0;
+  DIR *handle = opendir(d.path.c_str());
+  if (handle == nullptr) {
+    if (errno == ENOTDIR) return absl::InvalidArgumentError(d.path);
+    return CreateErrorStatusFromErrno("Failure to open directory");
+  }
+  struct stat statbuf;
+  while (true) {
+    errno = 0;
+    auto *entry = readdir(handle);
+    if (errno) {
+      closedir(handle);
+      return absl::InternalError(
+          absl::StrCat("Failed to read the contents of directory '", d.path,
+                       "'. Got error: ", errno));
+    }
+    // Finished listing the directory.
+    if (entry == nullptr) {
+      break;
+    }
+    absl::string_view d_name(entry->d_name);
+    // Skip '.' and '..' directory links
+    if (d_name == "." || d_name == "..") {
+      continue;
+    }
+
+    std::string full_path = verible::file::JoinPath(d.path, d_name);
+    auto d_type = entry->d_type;
+    if (d_type == DT_LNK || d_type == DT_UNKNOWN) {
+      // Try to resolve symlinks and unknown nodes.
+      if (stat(full_path.c_str(), &statbuf) == -1) {
+        LOG(WARNING) << "Stat failed. Ignoring " << d_name;
+        continue;
+      }
+      if (S_ISDIR(statbuf.st_mode)) {
+        d.directories.push_back(full_path);
+      } else if (S_ISREG(statbuf.st_mode)) {
+        d.files.push_back(full_path);
+      } else {
+        LOG(INFO) << "Ignoring " << d_name
+                  << " because st_mode == " << statbuf.st_mode;
+        continue;
+      }
+    } else if (d_type == DT_DIR) {
+      d.directories.push_back(full_path);
+    } else {
+      d.files.push_back(full_path);
+    }
+  }
+  closedir(handle);
+#endif
 
   std::sort(d.files.begin(), d.files.end());
   std::sort(d.directories.begin(), d.directories.end());
diff --git a/common/util/file_util_test.cc b/common/util/file_util_test.cc
index 814f52ddb..42776da69 100644
--- a/common/util/file_util_test.cc
+++ b/common/util/file_util_test.cc
@@ -15,7 +15,6 @@
 #include "common/util/file_util.h"
 
 #include <algorithm>
-#include <filesystem>
 #include <string>
 #include <vector>
 
@@ -25,7 +24,14 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-using testing::HasSubstr;
+// Temporary until we have all platforms on C++17, see comment in file_util.cc
+#ifdef _WIN32
+#  define USE_CPP_17_FILESYSTEM 1
+#endif
+
+#ifdef USE_CPP_17_FILESYSTEM
+#  include <filesystem>
+#endif
 
 #undef EXPECT_OK
 #define EXPECT_OK(value)      \
@@ -40,6 +46,8 @@ using testing::HasSubstr;
     ASSERT_TRUE(s.ok()) << s; \
   }
 
+using testing::HasSubstr;
+
 namespace verible {
 namespace util {
 namespace {
@@ -75,7 +83,11 @@ TEST(FileUtil, Stem) {
 
 // Returns the forward-slashed path in platform-specific notation.
 static std::string PlatformPath(const std::string& path) {
+#ifdef USE_CPP_17_FILESYSTEM
   return std::filesystem::path(path).lexically_normal().string();
+#else
+  return path;
+#endif
 }
 
 TEST(FileUtil, JoinPath) {

From f7af1a4c83fd9be8571937b3832acda7ffc7a736 Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Fri, 23 Apr 2021 13:02:57 -0700
Subject: [PATCH 09/11] Undo the releasing/ experiments.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 releasing/ubuntu/common/compiler.dockerstage | 7 ++-----
 releasing/ubuntu/xenial/compiler.dockerstage | 8 ++++----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/releasing/ubuntu/common/compiler.dockerstage b/releasing/ubuntu/common/compiler.dockerstage
index cd8609b74..a28bbc83c 100644
--- a/releasing/ubuntu/common/compiler.dockerstage
+++ b/releasing/ubuntu/common/compiler.dockerstage
@@ -1,8 +1,5 @@
 # Install compiler
 RUN apt-get install -y  \
     build-essential     \
-    g++-9               \
-    gcc-9
-
-RUN ln -sf /usr/bin/gcc-9 /usr/bin/gcc
-RUN ln -sf /usr/bin/g++-9 /usr/bin/g++
+    g++                 \
+    gcc
diff --git a/releasing/ubuntu/xenial/compiler.dockerstage b/releasing/ubuntu/xenial/compiler.dockerstage
index ff58c9661..086eef19b 100644
--- a/releasing/ubuntu/xenial/compiler.dockerstage
+++ b/releasing/ubuntu/xenial/compiler.dockerstage
@@ -1,11 +1,11 @@
 RUN add-apt-repository ppa:ubuntu-toolchain-r/test; apt-get update
 RUN apt-get install -y  \
     build-essential     \
-    g++-8               \
-    gcc-8
+    g++-7               \
+    gcc-7
 
-RUN ln -sf /usr/bin/gcc-8 /usr/bin/gcc
-RUN ln -sf /usr/bin/g++-8 /usr/bin/g++
+RUN ln -sf /usr/bin/gcc-7 /usr/bin/gcc
+RUN ln -sf /usr/bin/g++-7 /usr/bin/g++
 
 # Link libstdc++ statically
 ENV BAZEL_LINKOPTS "-static-libstdc++:-lm"

From 2dc696b542f79cc6f7393f0922b8659bebeaf55f Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Fri, 23 Apr 2021 13:07:49 -0700
Subject: [PATCH 10/11] Run formatting.

... which unfortunately undoes the properly formatted
preprocessing tokens.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 common/util/file_util.cc      | 14 +++++++-------
 common/util/file_util_test.cc |  4 ++--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/common/util/file_util.cc b/common/util/file_util.cc
index 068f2c2cc..1967ebd78 100644
--- a/common/util/file_util.cc
+++ b/common/util/file_util.cc
@@ -41,18 +41,18 @@
  *                issues have been resolved.
  */
 #ifdef _WIN32
-#  define USE_CPP_17_FILESYSTEM 1
+#define USE_CPP_17_FILESYSTEM 1
 #endif
 
 #ifdef USE_CPP_17_FILESYSTEM
-#  include <filesystem>
-#  include <system_error>
+#include <filesystem>
+#include <system_error>
 namespace fs = std::filesystem;
 #else
-#  include <dirent.h>
-#  include <sys/stat.h>
-#  include <sys/types.h>
-#  include <unistd.h>
+#include <dirent.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
 #endif
 
 namespace verible {
diff --git a/common/util/file_util_test.cc b/common/util/file_util_test.cc
index 42776da69..5ba904d67 100644
--- a/common/util/file_util_test.cc
+++ b/common/util/file_util_test.cc
@@ -26,11 +26,11 @@
 
 // Temporary until we have all platforms on C++17, see comment in file_util.cc
 #ifdef _WIN32
-#  define USE_CPP_17_FILESYSTEM 1
+#define USE_CPP_17_FILESYSTEM 1
 #endif
 
 #ifdef USE_CPP_17_FILESYSTEM
-#  include <filesystem>
+#include <filesystem>
 #endif
 
 #undef EXPECT_OK

From a4bf53477d43af0413d1ea6d544ae5a22b1bf95c Mon Sep 17 00:00:00 2001
From: Henner Zeller <h.zeller@acm.org>
Date: Fri, 23 Apr 2021 13:52:35 -0700
Subject: [PATCH 11/11] Limit release building to pushes again.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
---
 .github/workflows/verible-ci.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/verible-ci.yml b/.github/workflows/verible-ci.yml
index 6c91065c3..f22468456 100644
--- a/.github/workflows/verible-ci.yml
+++ b/.github/workflows/verible-ci.yml
@@ -155,7 +155,7 @@ jobs:
     runs-on: ubuntu-20.04
     # Github actions resources are limited; don't build artifacts for all
     # platforms on every pull request, only on push.
-    #if: ${{github.event_name == 'push'}}
+    if: ${{github.event_name == 'push'}}
     strategy:
       fail-fast: false
       matrix: