diff --git a/.github/workflows/windows-compile.yml b/.github/workflows/windows-compile.yml index d095d31f5..7990375b1 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..1967ebd78 100644 --- a/common/util/file_util.cc +++ b/common/util/file_util.cc @@ -14,13 +14,9 @@ #include "common/util/file_util.h" -#include #include #include #include -#include -#include -#include #include #include @@ -36,6 +32,29 @@ #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 +#include +namespace fs = std::filesystem; +#else +#include +#include +#include +#include +#endif + namespace verible { namespace file { absl::string_view Basename(absl::string_view filename) { @@ -64,10 +83,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,9 +103,39 @@ 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); +} + +#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); + if (err.value() != 0) + return CreateErrorStatusFromErr("invalid config path specified.", err); + + fs::path probe_dir = absolute_path; + for (;;) { + *result = (probe_dir / search_file).string(); + 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; + } + return absl::NotFoundError("No matching file found."); +#else + // Unix specific implementation. static constexpr char dir_separator[] = "/"; /* Convert to absolute path */ @@ -123,9 +174,28 @@ absl::Status UpwardFileSearch(absl::string_view start, } 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); + + if (err.value() != 0) { + return absl::NotFoundError(absl::StrCat(filename, ":", err.message())); + } + + if (fs::is_regular_file(stat) || fs::is_fifo(stat)) { + return absl::OkStatus(); + } + + if (fs::is_directory(stat)) { + 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.")); @@ -137,6 +207,7 @@ absl::Status FileExists(const std::string &filename) { return absl::InvalidArgumentError( absl::StrCat(filename, ": is a directory, not a file")); } +#endif return absl::InvalidArgumentError( absl::StrCat(filename, ": not a regular file.")); } @@ -144,11 +215,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); @@ -174,9 +243,22 @@ 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 +#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 + while (!name.empty() && name[0] == '/') { + name = name.substr(1); + } + + 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]) { @@ -186,16 +268,48 @@ std::string JoinPath(absl::string_view base, absl::string_view name) { 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 ListDir(absl::string_view dir) { +#if USE_CPP_17_FILESYSTEM + std::error_code err; + Directory d; + + d.path = dir.empty() ? "." : std::string(dir); + + 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")); + } + + for (const fs::directory_entry entry : fs::directory_iterator(d.path)) { + const std::string entry_name = entry.path().string(); + if (entry.is_directory()) { + d.directories.push_back(entry_name); + } else { + d.files.push_back(entry_name); + } + } +#else Directory d; d.path = dir.empty() ? "." : std::string(dir); @@ -203,9 +317,8 @@ absl::StatusOr ListDir(absl::string_view dir) { 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)); + if (errno == ENOTDIR) return absl::InvalidArgumentError(d.path); + return CreateErrorStatusFromErrno("Failure to open directory"); } struct stat statbuf; while (true) { @@ -251,6 +364,8 @@ absl::StatusOr ListDir(absl::string_view dir) { } } closedir(handle); +#endif + std::sort(d.files.begin(), d.files.end()); std::sort(d.directories.begin(), d.directories.end()); return d; @@ -259,7 +374,7 @@ absl::StatusOr 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, diff --git a/common/util/file_util_test.cc b/common/util/file_util_test.cc index d4e7c69c8..5ba904d67 100644 --- a/common/util/file_util_test.cc +++ b/common/util/file_util_test.cc @@ -24,12 +24,29 @@ #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 +#endif #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; \ + } + +using testing::HasSubstr; namespace verible { namespace util { @@ -64,21 +81,33 @@ 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) { +#ifdef USE_CPP_17_FILESYSTEM + return std::filesystem::path(path).lexically_normal().string(); +#else + return path; +#endif +} + TEST(FileUtil, JoinPath) { - EXPECT_EQ(file::JoinPath("foo", ""), "foo/"); - EXPECT_EQ(file::JoinPath("", "bar"), "/bar"); - EXPECT_EQ(file::JoinPath("foo", "bar"), "foo/bar"); + EXPECT_EQ(file::JoinPath("foo", ""), PlatformPath("foo/")); + EXPECT_EQ(file::JoinPath("", "bar"), "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"), "/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"); +#endif } TEST(FileUtil, CreateDir) { @@ -87,6 +116,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; @@ -107,10 +137,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) { @@ -121,17 +156,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); } @@ -203,17 +227,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"), @@ -240,7 +264,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 +272,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 +300,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); }