From 52b2e740de81d58fbe5fa535c1f66aa82e80951e Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Fri, 9 Aug 2019 12:00:43 -0700 Subject: [PATCH] [vcpkg] Fix build under /W4 I was building under /W3, because CMake hadn't been set up to build under /W4 -- therefore, I didn't see some warnings. We also decided to remove the niebloids and instead break ADL by using `= delete`, since otherwise we get warnings when we define a local variable with the same name as a niebloid. I also removed `status` and `symlink_status` from the `files` header, since it's unnecessary now, and they're just implementation details of `RealFilesystem`. I also removed some existing uses of unqualified `status(path)`, since that no longer compiles. I also added `Filesystem::canonical`, to remove another use of `fs::stdfs` in a function I was already working in. --- toolsrc/include/vcpkg/base/files.h | 112 ++++++--------- toolsrc/src/vcpkg-test/files.cpp | 6 +- toolsrc/src/vcpkg-test/util.cpp | 6 +- toolsrc/src/vcpkg/base/files.cpp | 216 +++++++++++++++-------------- toolsrc/src/vcpkg/build.cpp | 2 +- toolsrc/src/vcpkg/dependencies.cpp | 3 +- toolsrc/src/vcpkg/vcpkgpaths.cpp | 5 +- 7 files changed, 164 insertions(+), 186 deletions(-) diff --git a/toolsrc/include/vcpkg/base/files.h b/toolsrc/include/vcpkg/base/files.h index 19e4f78fd5..2b33835c0e 100644 --- a/toolsrc/include/vcpkg/base/files.h +++ b/toolsrc/include/vcpkg/base/files.h @@ -51,8 +51,14 @@ namespace fs #else - using stdfs::file_status; using stdfs::file_type; + // to set up ADL correctly on `file_status` objects, we are defining + // this in our own namespace + struct file_status : private stdfs::file_status { + using stdfs::file_status::file_status; + using stdfs::file_status::type; + using stdfs::file_status::permissions; + }; #endif @@ -61,91 +67,49 @@ namespace fs the presence of symlinks -- a symlink is treated as the object it points to for `symlink_status` and `symlink_type` */ + #if 0 + #endif - // we want to poison ADL with these niebloids - - namespace detail + inline bool is_symlink(file_status s) noexcept { - struct status_t - { - file_status operator()(const path& p, std::error_code& ec) const noexcept; - file_status operator()(vcpkg::LineInfo li, const path& p) const noexcept; - file_status operator()(const path& p) const; - }; - struct symlink_status_t - { - file_status operator()(const path& p, std::error_code& ec) const noexcept; - file_status operator()(vcpkg::LineInfo li, const path& p) const noexcept; - }; - struct is_symlink_t - { - bool operator()(file_status s) const - { #if defined(_WIN32) - return s.type() == file_type::directory_symlink || s.type() == file_type::symlink; -#else - return stdfs::is_symlink(s); + if (s.type() == file_type::directory_symlink) return true; #endif - } - }; - struct is_regular_file_t - { - inline bool operator()(file_status s) const - { -#if defined(_WIN32) - return s.type() == file_type::regular; -#else - return stdfs::is_regular_file(s); -#endif - } - }; - struct is_directory_t - { - inline bool operator()(file_status s) const - { -#if defined(_WIN32) - return s.type() == file_type::directory; -#else - return stdfs::is_directory(s); -#endif - } - }; - struct exists_t - { - inline bool operator()(file_status s) const - { -#if defined(_WIN32) - return s.type() != file_type::not_found && s.type() != file_type::none; -#else - return stdfs::exists(s); -#endif - } - }; + return s.type() == file_type::symlink; + } + inline bool is_regular_file(file_status s) + { + return s.type() == file_type::regular; + } + inline bool is_directory(file_status s) + { + return s.type() == file_type::directory; + } + inline bool exists(file_status s) + { + return s.type() != file_type::not_found && s.type() != file_type::none; } - - constexpr detail::status_t status{}; - constexpr detail::symlink_status_t symlink_status{}; - constexpr detail::is_symlink_t is_symlink{}; - constexpr detail::is_regular_file_t is_regular_file{}; - constexpr detail::is_directory_t is_directory{}; - constexpr detail::exists_t exists{}; } /* if someone attempts to use unqualified `symlink_status` or `is_symlink`, they might get the ADL version, which is broken. - Therefore, put `symlink_status` in the global namespace, so that they get - our symlink_status. + Therefore, put `(symlink_)?status` as deleted in the global namespace, so + that they get an error. We also want to poison the ADL on the other functions, because we don't want people calling these functions on paths */ -using fs::exists; -using fs::is_directory; -using fs::is_regular_file; -using fs::is_symlink; -using fs::status; -using fs::symlink_status; +void status(const fs::path& p) = delete; +void status(const fs::path& p, std::error_code& ec) = delete; +void symlink_status(const fs::path& p) = delete; +void symlink_status(const fs::path& p, std::error_code& ec) = delete; +void is_symlink(const fs::path& p) = delete; +void is_symlink(const fs::path& p, std::error_code& ec) = delete; +void is_regular_file(const fs::path& p) = delete; +void is_regular_file(const fs::path& p, std::error_code& ec) = delete; +void is_directory(const fs::path& p) = delete; +void is_directory(const fs::path& p, std::error_code& ec) = delete; namespace vcpkg::Files { @@ -192,6 +156,10 @@ namespace vcpkg::Files virtual void copy_symlink(const fs::path& oldpath, const fs::path& newpath, std::error_code& ec) = 0; virtual fs::file_status status(const fs::path& path, std::error_code& ec) const = 0; virtual fs::file_status symlink_status(const fs::path& path, std::error_code& ec) const = 0; + fs::file_status status(LineInfo li, const fs::path& p) const noexcept; + fs::file_status symlink_status(LineInfo li, const fs::path& p) const noexcept; + virtual fs::path canonical(const fs::path& path, std::error_code& ec) const = 0; + fs::path canonical(LineInfo li, const fs::path& path) const; virtual std::vector find_from_PATH(const std::string& name) const = 0; }; diff --git a/toolsrc/src/vcpkg-test/files.cpp b/toolsrc/src/vcpkg-test/files.cpp index a2faf455cd..d40edb3bda 100644 --- a/toolsrc/src/vcpkg-test/files.cpp +++ b/toolsrc/src/vcpkg-test/files.cpp @@ -145,7 +145,7 @@ namespace CHECK_EC_ON_FILE(base, ec); } - vcpkg::Files::Filesystem& setup(urbg_t& urbg) + vcpkg::Files::Filesystem& setup() { auto& fs = vcpkg::Files::get_real_filesystem(); @@ -161,7 +161,7 @@ TEST_CASE ("remove all", "[files]") { auto urbg = get_urbg(0); - auto& fs = setup(urbg); + auto& fs = setup(); fs::path temp_dir = base_temporary_directory() / get_random_filename(urbg); INFO("temp dir is: " << temp_dir); @@ -181,7 +181,7 @@ TEST_CASE ("remove all", "[files]") TEST_CASE ("remove all -- benchmarks", "[files][!benchmark]") { auto urbg = get_urbg(1); - auto& fs = setup(urbg); + auto& fs = setup(); struct { diff --git a/toolsrc/src/vcpkg-test/util.cpp b/toolsrc/src/vcpkg-test/util.cpp index 384954b4e7..379b253b18 100644 --- a/toolsrc/src/vcpkg-test/util.cpp +++ b/toolsrc/src/vcpkg-test/util.cpp @@ -141,7 +141,7 @@ namespace vcpkg::Test std::filesystem::path targetp = target.native(); std::filesystem::path filep = file.native(); - std::filesystem::create_symlink(targetp, filep); + std::filesystem::create_symlink(targetp, filep, ec); } else { @@ -153,6 +153,7 @@ namespace vcpkg::Test ec.assign(errno, std::system_category()); } #else + std::ignore = ec; vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, no_filesystem_message); #endif } @@ -165,7 +166,7 @@ namespace vcpkg::Test std::filesystem::path targetp = target.native(); std::filesystem::path filep = file.native(); - std::filesystem::create_directory_symlink(targetp, filep); + std::filesystem::create_directory_symlink(targetp, filep, ec); } else { @@ -174,6 +175,7 @@ namespace vcpkg::Test #elif FILESYSTEM_SYMLINK == FILESYSTEM_SYMLINK_UNIX ::vcpkg::Test::create_symlink(target, file, ec); #else + std::ignore = ec; vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, no_filesystem_message); #endif } diff --git a/toolsrc/src/vcpkg/base/files.cpp b/toolsrc/src/vcpkg/base/files.cpp index eb6119f180..583adbbb8c 100644 --- a/toolsrc/src/vcpkg/base/files.cpp +++ b/toolsrc/src/vcpkg/base/files.cpp @@ -21,114 +21,87 @@ #include #endif -namespace fs::detail -{ - static file_status status_implementation(bool follow_symlinks, const path& p, std::error_code& ec) - { -#if defined(_WIN32) - WIN32_FILE_ATTRIBUTE_DATA file_attributes; - file_type ft = file_type::unknown; - perms permissions = perms::unknown; - if (!GetFileAttributesExW(p.c_str(), GetFileExInfoStandard, &file_attributes)) - { - const auto err = GetLastError(); - if (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND) - { - ft = file_type::not_found; - } - else - { - ec.assign(err, std::system_category()); - } - } - else if (!follow_symlinks && file_attributes.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) - { - // this also gives junctions file_type::directory_symlink - if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) - { - ft = file_type::directory_symlink; - } - else - { - ft = file_type::symlink; - } - } - else if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) - { - ft = file_type::directory; - } - else - { - // otherwise, the file is a regular file - ft = file_type::regular; - } - - if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_READONLY) - { - constexpr auto all_write = perms::group_write | perms::owner_write | perms::others_write; - permissions = perms::all & ~all_write; - } - else if (ft != file_type::none && ft != file_type::none) - { - permissions = perms::all; - } - - return file_status(ft, permissions); - -#else - auto result = symlink ? stdfs::symlink_status(p, ec) : stdfs::status(p, ec); - // libstdc++ doesn't correctly not-set ec on nonexistent paths - if (ec.value() == ENOENT || ec.value() == ENOTDIR) - { - ec.clear(); - result = file_status(file_type::not_found, perms::unknown); - } - return result; -#endif - } - - file_status status_t::operator()(const path& p, std::error_code& ec) const noexcept - { - return status_implementation(false, p, ec); - } - file_status status_t::operator()(vcpkg::LineInfo li, const path& p) const noexcept - { - std::error_code ec; - auto result = (*this)(p, ec); - if (ec) vcpkg::Checks::exit_with_message(li, "error getting status of path %s: %s", p.string(), ec.message()); - - return result; - } - file_status status_t::operator()(const path& p) const - { -#if defined(_WIN32) - return (*this)(VCPKG_LINE_INFO, p); -#else - return fs::stdfs::status(p); -#endif - } - - file_status symlink_status_t::operator()(const path& p, std::error_code& ec) const noexcept - { - return status_implementation(true, p, ec); - } - - file_status symlink_status_t::operator()(vcpkg::LineInfo li, const path& p) const noexcept - { - std::error_code ec; - auto result = (*this)(p, ec); - if (ec) vcpkg::Checks::exit_with_message(li, "error getting status of path %s: %s", p.string(), ec.message()); - - return result; - } -} - namespace vcpkg::Files { static const std::regex FILESYSTEM_INVALID_CHARACTERS_REGEX = std::regex(R"([\/:*?"<>|])"); namespace { + fs::file_status status_implementation(bool follow_symlinks, const fs::path& p, std::error_code& ec) noexcept + { + using fs::file_type; + using fs::perms; +#if defined(_WIN32) + WIN32_FILE_ATTRIBUTE_DATA file_attributes; + auto ft = file_type::unknown; + auto permissions = perms::unknown; + if (!GetFileAttributesExW(p.c_str(), GetFileExInfoStandard, &file_attributes)) + { + const auto err = GetLastError(); + if (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND) + { + ft = file_type::not_found; + } + else + { + ec.assign(err, std::system_category()); + } + } + else if (!follow_symlinks && file_attributes.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) + { + // this also gives junctions file_type::directory_symlink + if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) + { + ft = file_type::directory_symlink; + } + else + { + ft = file_type::symlink; + } + } + else if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) + { + ft = file_type::directory; + } + else + { + // otherwise, the file is a regular file + ft = file_type::regular; + } + + if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_READONLY) + { + constexpr auto all_write = perms::group_write | perms::owner_write | perms::others_write; + permissions = perms::all & ~all_write; + } + else if (ft != file_type::none && ft != file_type::none) + { + permissions = perms::all; + } + + return fs::file_status(ft, permissions); + +#else + auto result = symlink ? stdfs::symlink_status(p, ec) : stdfs::status(p, ec); + // libstdc++ doesn't correctly not-set ec on nonexistent paths + if (ec.value() == ENOENT || ec.value() == ENOTDIR) + { + ec.clear(); + result = fs::file_status(file_type::not_found, perms::unknown); + } + return result; +#endif + } + + fs::file_status status(const fs::path& p, std::error_code& ec) noexcept + { + return status_implementation(false, p, ec); + } + fs::file_status symlink_status(const fs::path& p, std::error_code& ec) noexcept + { + return status_implementation(false, p, ec); + } + // does _not_ follow symlinks void set_writeable(const fs::path& path, std::error_code& ec) noexcept { @@ -220,6 +193,24 @@ namespace vcpkg::Files return exists(path, ec); } + fs::file_status Filesystem::status(vcpkg::LineInfo li, const fs::path& p) const noexcept + { + std::error_code ec; + auto result = this->status(p, ec); + if (ec) vcpkg::Checks::exit_with_message(li, "error getting status of path %s: %s", p.string(), ec.message()); + + return result; + } + + fs::file_status Filesystem::symlink_status(vcpkg::LineInfo li, const fs::path& p) const noexcept + { + std::error_code ec; + auto result = this->symlink_status(p, ec); + if (ec) vcpkg::Checks::exit_with_message(li, "error getting status of path %s: %s", p.string(), ec.message()); + + return result; + } + void Filesystem::write_lines(const fs::path& path, const std::vector& lines, LineInfo linfo) { std::error_code ec; @@ -244,6 +235,16 @@ namespace vcpkg::Files } } + fs::path Filesystem::canonical(LineInfo li, const fs::path& path) const + { + std::error_code ec; + + const auto result = this->canonical(path, ec); + + if (ec) Checks::exit_with_message(li, "Error getting canonicalization of %s: %s", path.string(), ec.message()); + return result; + } + struct RealFilesystem final : Filesystem { virtual Expected read_contents(const fs::path& file_path) const override @@ -463,7 +464,7 @@ namespace vcpkg::Files static void do_remove(const fs::path& current_path, ErrorInfo& err) { std::error_code ec; - const auto path_status = fs::symlink_status(current_path, ec); + const auto path_status = Files::symlink_status(current_path, ec); if (check_ec(ec, current_path, err)) return; if (!fs::exists(path_status)) return; @@ -593,11 +594,11 @@ namespace vcpkg::Files virtual fs::file_status status(const fs::path& path, std::error_code& ec) const override { - return fs::status(path, ec); + return Files::status(path, ec); } virtual fs::file_status symlink_status(const fs::path& path, std::error_code& ec) const override { - return fs::symlink_status(path, ec); + return Files::symlink_status(path, ec); } virtual void write_contents(const fs::path& file_path, const std::string& data, std::error_code& ec) override { @@ -628,6 +629,11 @@ namespace vcpkg::Files } } + virtual fs::path canonical(const fs::path& path, std::error_code& ec) const override + { + return fs::stdfs::canonical(path, ec); + } + virtual std::vector find_from_PATH(const std::string& name) const override { #if defined(_WIN32) diff --git a/toolsrc/src/vcpkg/build.cpp b/toolsrc/src/vcpkg/build.cpp index 5b93242a10..b809db0bc0 100644 --- a/toolsrc/src/vcpkg/build.cpp +++ b/toolsrc/src/vcpkg/build.cpp @@ -623,7 +623,7 @@ namespace vcpkg::Build std::vector port_files; for (auto& port_file : fs::stdfs::recursive_directory_iterator(config.port_dir)) { - if (fs::is_regular_file(status(port_file))) + if (fs::is_regular_file(fs.status(VCPKG_LINE_INFO, port_file))) { port_files.push_back(port_file); if (port_files.size() > max_port_file_count) diff --git a/toolsrc/src/vcpkg/dependencies.cpp b/toolsrc/src/vcpkg/dependencies.cpp index 65c5cc9854..9fccf950e2 100644 --- a/toolsrc/src/vcpkg/dependencies.cpp +++ b/toolsrc/src/vcpkg/dependencies.cpp @@ -312,6 +312,7 @@ namespace vcpkg::Dependencies const std::vector* ports_dirs_paths) : filesystem(paths.get_filesystem()) { + auto& fs = Files::get_real_filesystem(); if (ports_dirs_paths) { for (auto&& overlay_path : *ports_dirs_paths) @@ -326,7 +327,7 @@ namespace vcpkg::Dependencies overlay.string()); Checks::check_exit(VCPKG_LINE_INFO, - fs::is_directory(status(overlay)), + fs::is_directory(fs.status(VCPKG_LINE_INFO, overlay)), "Error: Path \"%s\" must be a directory", overlay.string()); diff --git a/toolsrc/src/vcpkg/vcpkgpaths.cpp b/toolsrc/src/vcpkg/vcpkgpaths.cpp index c5b5749ffd..bc46d2cfcc 100644 --- a/toolsrc/src/vcpkg/vcpkgpaths.cpp +++ b/toolsrc/src/vcpkg/vcpkgpaths.cpp @@ -18,8 +18,9 @@ namespace vcpkg const std::string& default_vs_path, const std::vector* triplets_dirs) { + auto& fs = Files::get_real_filesystem(); std::error_code ec; - const fs::path canonical_vcpkg_root_dir = fs::stdfs::canonical(vcpkg_root_dir, ec); + const fs::path canonical_vcpkg_root_dir = fs.canonical(vcpkg_root_dir, ec); if (ec) { return ec; @@ -42,7 +43,7 @@ namespace vcpkg if (auto odp = overriddenDownloadsPath.get()) { auto asPath = fs::u8path(*odp); - if (!fs::is_directory(status(asPath))) + if (!fs::is_directory(fs.status(VCPKG_LINE_INFO, asPath))) { Metrics::g_metrics.lock()->track_property("error", "Invalid VCPKG_DOWNLOADS override directory."); Checks::exit_with_message(