From 5857e2c680fde9e37abc8f799f8d5509dd47ed62 Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Mon, 8 Jul 2019 16:45:27 -0700 Subject: [PATCH 01/17] initial remove-in-parallel doesn't actually do parallel remove yet --- toolsrc/include/vcpkg/base/rng.h | 96 ++++++++++++++++++++++++++++ toolsrc/include/vcpkg/base/strings.h | 32 ++++++++++ toolsrc/src/vcpkg/base/files.cpp | 68 +++++++++++++++----- toolsrc/src/vcpkg/base/rng.cpp | 14 ++++ 4 files changed, 194 insertions(+), 16 deletions(-) create mode 100644 toolsrc/include/vcpkg/base/rng.h create mode 100644 toolsrc/src/vcpkg/base/rng.cpp diff --git a/toolsrc/include/vcpkg/base/rng.h b/toolsrc/include/vcpkg/base/rng.h new file mode 100644 index 0000000000..1bcab05b3f --- /dev/null +++ b/toolsrc/include/vcpkg/base/rng.h @@ -0,0 +1,96 @@ +#pragma once + +#include +#include +#include + +namespace vcpkg { + + /* + NOTE(ubsan): taken from the xoshiro paper + initialized from random_device by default + actual code is copied from wikipedia, since I wrote that code + */ + struct splitmix64_engine { + splitmix64_engine() noexcept; + + constexpr splitmix64_engine(std::uint64_t seed) noexcept + : state(seed) {} + + constexpr std::uint64_t operator()() noexcept { + state += 0x9E3779B97F4A7C15; + + std::uint64_t result = state; + result = (result ^ (result >> 30)) * 0xBF58476D1CE4E5B9; + result = (result ^ (result >> 27)) * 0x94D049BB133111EB; + + return result ^ (result >> 31); + } + + constexpr std::uint64_t max() const noexcept { + return std::numeric_limits::max(); + } + + constexpr std::uint64_t min() const noexcept { + return std::numeric_limits::min(); + } + + private: + std::uint64_t state; + }; + + // Sebastian Vigna's xorshift-based xoshiro xoshiro256** engine + // fast and really good + // uses the splitmix64_engine to initialize state + struct xoshiro256ss_engine { + // splitmix64_engine will be initialized with random_device + xoshiro256ss_engine() noexcept { + splitmix64_engine sm64{}; + + for (std::uint64_t& s : this->state) { + s = sm64(); + } + } + + constexpr xoshiro256ss_engine(std::uint64_t seed) noexcept : state() { + splitmix64_engine sm64{seed}; + + for (std::uint64_t& s : this->state) { + s = sm64(); + } + } + + constexpr std::uint64_t operator()() noexcept { + std::uint64_t const result = rol(state[1] * 5, 7) * 9; + + std::uint64_t const t = state[1] << 17; + + // state[i] = state[i] ^ state[i + 4 mod 4] + state[2] ^= state[0]; + state[3] ^= state[1]; + state[1] ^= state[2]; + state[0] ^= state[3]; + + state[2] ^= t; + state[3] ^= rol(state[3], 45); + + return result; + } + + constexpr std::uint64_t max() const noexcept { + return std::numeric_limits::max(); + } + + constexpr std::uint64_t min() const noexcept { + return std::numeric_limits::min(); + } + private: + // rotate left + constexpr std::uint64_t rol(std::uint64_t x, int k) { + return (x << k) | (x >> (64 - k)); + } + + std::uint64_t state[4]; + }; + +} diff --git a/toolsrc/include/vcpkg/base/strings.h b/toolsrc/include/vcpkg/base/strings.h index d553d1d417..423ea2096a 100644 --- a/toolsrc/include/vcpkg/base/strings.h +++ b/toolsrc/include/vcpkg/base/strings.h @@ -184,4 +184,36 @@ namespace vcpkg::Strings const char* search(StringView haystack, StringView needle); bool contains(StringView haystack, StringView needle); + + // base 64 encoding with URL and filesafe alphabet (base64url) + // based on IETF RFC 4648 + // ignores padding, since one implicitly knows the length from the size of x + template + std::string b64url_encode(Integral x) { + static_assert(std::is_integral_v); + auto value = static_cast>(x); + + // 64 values, plus the implicit \0 + constexpr static char map[0x41] = + /* 0123456789ABCDEF */ + /*0*/ "ABCDEFGHIJKLMNOP" + /*1*/ "QRSTUVWXYZabcdef" + /*2*/ "ghijklmnopqrstuv" + /*3*/ "wxyz0123456789-_" + ; + + constexpr static std::make_unsigned_t mask = 0x3F; + constexpr static int shift = 5; + + std::string result; + // reserve ceiling(number of bits / 3) + result.reserve((sizeof(value) * 8 + 2) / 3); + + while (value != 0) { + char mapped_value = map[value & mask]; + result.push_back(mapped_value); + } + + return result; + } } diff --git a/toolsrc/src/vcpkg/base/files.cpp b/toolsrc/src/vcpkg/base/files.cpp index 5099795e9e..d0926bb4c5 100644 --- a/toolsrc/src/vcpkg/base/files.cpp +++ b/toolsrc/src/vcpkg/base/files.cpp @@ -1,6 +1,7 @@ #include "pch.h" #include +#include #include #include #include @@ -256,26 +257,61 @@ namespace vcpkg::Files virtual bool remove(const fs::path& path, std::error_code& ec) override { return fs::stdfs::remove(path, ec); } virtual std::uintmax_t remove_all(const fs::path& path, std::error_code& ec) override { - // Working around the currently buggy remove_all() - std::uintmax_t out = fs::stdfs::remove_all(path, ec); + /* + does not use the std::filesystem call since it is buggy, and can + have spurious errors before VS 2017 update 6, and on later versions + (as well as on macOS and Linux), this is just as fast and will have + fewer spurious errors due to locks. + */ + struct recursive { + const fs::path& tmp_directory; + std::error_code& ec; + xoshiro256ss_engine& rng; - for (int i = 0; i < 5 && this->exists(path); i++) - { - using namespace std::chrono_literals; - std::this_thread::sleep_for(i * 100ms); - out += fs::stdfs::remove_all(path, ec); + void operator()(const fs::path& current) const { + const auto type = fs::stdfs::symlink_status(current, ec).type(); + if (ec) return; + + const auto tmp_name = Strings::b64url_encode(rng()); + const auto tmp_path = tmp_directory / tmp_name; + + switch (type) { + case fs::file_type::directory: { + fs::stdfs::rename(current, tmp_path, ec); + if (ec) return; + for (const auto& entry : fs::stdfs::directory_iterator(tmp_path)) { + (*this)(entry); + } + fs::stdfs::remove(tmp_path, ec); + } break; + case fs::file_type::symlink: + case fs::file_type::regular: { + fs::stdfs::rename(current, tmp_path, ec); + fs::stdfs::remove(current, ec); + } break; + case fs::file_type::not_found: return; + case fs::file_type::none: { + Checks::exit_with_message(VCPKG_LINE_INFO, "Error occurred when evaluating file type of file: %s", current); + } + default: { + Checks::exit_with_message(VCPKG_LINE_INFO, "Attempted to delete special file: %s", current); + } + } + } + }; + + auto const real_path = fs::stdfs::absolute(path); + + if (! real_path.has_parent_path()) { + Checks::exit_with_message(VCPKG_LINE_INFO, "Attempted to remove_all the base directory"); } - if (this->exists(path)) - { - System::print2( - System::Color::warning, - "Some files in ", - path.u8string(), - " were unable to be removed. Close any editors operating in this directory and retry.\n"); - } + // thoughts: is this fine? or should we do something different? + // maybe a temporary directory? + auto const base_path = real_path.parent_path(); - return out; + xoshiro256ss_engine rng{}; + recursive{base_path, ec, rng}(real_path); } virtual bool exists(const fs::path& path) const override { return fs::stdfs::exists(path); } virtual bool is_directory(const fs::path& path) const override { return fs::stdfs::is_directory(path); } diff --git a/toolsrc/src/vcpkg/base/rng.cpp b/toolsrc/src/vcpkg/base/rng.cpp new file mode 100644 index 0000000000..9fe2ea3b40 --- /dev/null +++ b/toolsrc/src/vcpkg/base/rng.cpp @@ -0,0 +1,14 @@ +#include + +namespace vcpkg { + namespace { + std::random_device system_entropy{}; + } + + splitmix64_engine::splitmix64_engine() { + std::uint64_t top_half = system_entropy(); + std::uint64_t bottom_half = system_entropy(); + + state = (top_half << 32) | bottom_half; + } +} From 2d6df16849ebcf237d17c919727756d90974daba Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Wed, 10 Jul 2019 14:35:10 -0700 Subject: [PATCH 02/17] remove_all parallelized, and fix the issues with symlink --- toolsrc/include/vcpkg/base/files.h | 38 ++++- toolsrc/include/vcpkg/base/rng.h | 165 ++++++++++++++++---- toolsrc/include/vcpkg/base/work_queue.h | 183 +++++++++++++++++++++++ toolsrc/src/vcpkg/base/files.cpp | 190 +++++++++++++++++++----- toolsrc/src/vcpkg/base/rng.cpp | 4 +- 5 files changed, 508 insertions(+), 72 deletions(-) create mode 100644 toolsrc/include/vcpkg/base/work_queue.h diff --git a/toolsrc/include/vcpkg/base/files.h b/toolsrc/include/vcpkg/base/files.h index 3ea0d60366..178fae5410 100644 --- a/toolsrc/include/vcpkg/base/files.h +++ b/toolsrc/include/vcpkg/base/files.h @@ -12,14 +12,50 @@ namespace fs using stdfs::copy_options; using stdfs::file_status; using stdfs::file_type; + using stdfs::perms; using stdfs::path; using stdfs::u8path; + /* + std::experimental::filesystem's file_status and file_type are broken in + the presence of symlinks -- a symlink is treated as the object it points + to for `symlink_status` and `symlink_type` + */ + + using stdfs::status; + + // we want to poison ADL with these niebloids + constexpr struct { + file_status operator()(const path& p, std::error_code& ec) const noexcept; + file_status operator()(const path& p) const noexcept; + } symlink_status{}; + + constexpr struct { + inline bool operator()(file_status s) const { + return stdfs::is_symlink(s); + } + + inline bool operator()(const path& p) const { + return stdfs::is_symlink(symlink_status(p)); + } + inline bool operator()(const path& p, std::error_code& ec) const { + return stdfs::is_symlink(symlink_status(p, ec)); + } + } is_symlink{}; + inline bool is_regular_file(file_status s) { return stdfs::is_regular_file(s); } inline bool is_directory(file_status s) { return stdfs::is_directory(s); } - inline bool is_symlink(file_status s) { return stdfs::is_symlink(s); } } +/* + 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. +*/ +using fs::symlink_status; +using fs::is_symlink; + namespace vcpkg::Files { struct Filesystem diff --git a/toolsrc/include/vcpkg/base/rng.h b/toolsrc/include/vcpkg/base/rng.h index 1bcab05b3f..4a0411f644 100644 --- a/toolsrc/include/vcpkg/base/rng.h +++ b/toolsrc/include/vcpkg/base/rng.h @@ -4,17 +4,56 @@ #include #include -namespace vcpkg { +namespace vcpkg::Rng { + + namespace detail { + template + constexpr std::size_t bitsize = sizeof(T) * CHAR_BITS; + + template + constexpr bool is_valid_shift(int k) { + return 0 <= k && k <= bitsize; + } + + // precondition: 0 <= k < bitsize + template + constexpr T ror(T x, int k) { + if (k == 0) { + return x; + } + return (x >> k) | (x << (bitsize - k)); + } + + // precondition: 0 <= k < bitsize + template + constexpr T rol(T x, int k) { + if (k == 0) { + return x; + } + return (x << k) | (x >> (bitsize - k)); + } + + // there _is_ a way to do this generally, but I don't know how to + template + struct XoshiroJumpTable; + + template <> + struct XoshiroJumpTable { + constexpr static std::uint64_t value[4] = { + 0x180ec6d33cfd0aba, 0xd5a61266f0c9392c, 0xa9582618e03fc9aa, 0x39abdc4529b1661c + }; + }; + } /* NOTE(ubsan): taken from the xoshiro paper initialized from random_device by default actual code is copied from wikipedia, since I wrote that code */ - struct splitmix64_engine { - splitmix64_engine() noexcept; + struct splitmix { + splitmix() noexcept; - constexpr splitmix64_engine(std::uint64_t seed) noexcept + constexpr splitmix(std::uint64_t seed) noexcept : state(seed) {} constexpr std::uint64_t operator()() noexcept { @@ -35,62 +74,126 @@ namespace vcpkg { return std::numeric_limits::min(); } + template + constexpr void fill(T* first, T* last) { + constexpr auto mask = + static_cast(std::numeric_limits::max()); + + const auto remaining = + (last - first) % (sizeof(std::uint64_t) / sizeof(T)); + + for (auto it = first; it != last - remaining;) { + const auto item = (*this)(); + for ( + int shift = 0; + shift < 64; + shift += detail::bitsize, ++it + ) { + *it = static_cast((item >> shift) & mask); + } + } + + if (remaining == 0) return; + + int shift = 0; + const auto item = (*this)(); + for (auto it = last - remaining; + it != last; + shift += detail::bitsize, ++it + ) { + *it = static_cast((item >> shift) & mask); + } + } + private: std::uint64_t state; }; - // Sebastian Vigna's xorshift-based xoshiro xoshiro256** engine + template + struct starstar_scrambler { + constexpr static UIntType scramble(UIntType n) noexcept { + return detail::rol(n * S, R) * T; + } + }; + + // Sebastian Vigna's xorshift-based xoshiro engine // fast and really good - // uses the splitmix64_engine to initialize state - struct xoshiro256ss_engine { - // splitmix64_engine will be initialized with random_device - xoshiro256ss_engine() noexcept { - splitmix64_engine sm64{}; + // uses the splitmix to initialize state + template + struct xoshiro_engine { + static_assert(detail::is_valid_shift(A)); + static_assert(detail::is_valid_shift(B)); + static_assert(std::is_unsigned_v); - for (std::uint64_t& s : this->state) { - s = sm64(); - } + // splitmix will be initialized with random_device + xoshiro_engine() noexcept { + splitmix sm{}; + + sm.fill(&state[0], &state[4]); } - constexpr xoshiro256ss_engine(std::uint64_t seed) noexcept : state() { - splitmix64_engine sm64{seed}; + constexpr xoshiro_engine(std::uint64_t seed) noexcept : state() { + splitmix sm{seed}; - for (std::uint64_t& s : this->state) { - s = sm64(); - } + sm.fill(&state[0], &state[4]); } - constexpr std::uint64_t operator()() noexcept { - std::uint64_t const result = rol(state[1] * 5, 7) * 9; + constexpr UIntType operator()() noexcept { + const UIntType result = Scrambler::scramble(state[0]); - std::uint64_t const t = state[1] << 17; + const UIntType t = state[1] << A; - // state[i] = state[i] ^ state[i + 4 mod 4] state[2] ^= state[0]; state[3] ^= state[1]; state[1] ^= state[2]; state[0] ^= state[3]; state[2] ^= t; - state[3] ^= rol(state[3], 45); + state[3] ^= detail::rol(state[3], B); return result; } - constexpr std::uint64_t max() const noexcept { - return std::numeric_limits::max(); + constexpr UIntType max() const noexcept { + return std::numeric_limits::max(); } constexpr std::uint64_t min() const noexcept { - return std::numeric_limits::min(); + return std::numeric_limits::min(); + } + + // quickly jump ahead 2^e steps + // takes 4 * bitsize rng next operations + template + constexpr void discard_e() noexcept { + using JT = detail::XoshiroJumpTable; + + UIntType s[4] = {}; + for (const auto& jump : JT::value) { + for (std::size_t i = 0; i < bitsize; ++i) { + if ((jump >> i) & 1) { + s[0] ^= state[0]; + s[1] ^= state[1]; + s[2] ^= state[2]; + s[3] ^= state[3]; + } + (*this)(); + } + } + + state[0] = s[0]; + state[1] = s[1]; + state[2] = s[2]; + state[3] = s[3]; } private: // rotate left - constexpr std::uint64_t rol(std::uint64_t x, int k) { - return (x << k) | (x >> (64 - k)); - } - - std::uint64_t state[4]; + UIntType state[4]; }; + using xoshiro256ss = xoshiro_engine< + std::uint64_t, + starstar_scrambler, + 17, + 45>; } diff --git a/toolsrc/include/vcpkg/base/work_queue.h b/toolsrc/include/vcpkg/base/work_queue.h new file mode 100644 index 0000000000..4db167fa64 --- /dev/null +++ b/toolsrc/include/vcpkg/base/work_queue.h @@ -0,0 +1,183 @@ +#pragma once + +#include +#include + +namespace vcpkg { + namespace detail { + template + auto call_action( + Action& action, + const WorkQueue& work_queue, + ThreadLocalData& tld + ) -> decltype(static_cast(std::move(action)(tld, work_queue))) + { + std::move(action)(tld, work_queue); + } + + template + auto call_action( + Action& action, + const WorkQueue&, + ThreadLocalData& tld + ) -> decltype(static_cast(std::move(action)(tld))) + { + std::move(action)(tld); + } + } + + template + struct WorkQueue { + template + explicit WorkQueue(const F& initializer) noexcept { + state = State::Joining; + + std::size_t num_threads = std::thread::hardware_concurrency(); + if (num_threads == 0) { + num_threads = 4; + } + + m_threads.reserve(num_threads); + for (std::size_t i = 0; i < num_threads; ++i) { + m_threads.emplace_back(this, initializer); + } + } + + WorkQueue(WorkQueue const&) = delete; + WorkQueue(WorkQueue&&) = delete; + + ~WorkQueue() = default; + + // runs all remaining tasks, and blocks on their finishing + // if this is called in an existing task, _will block forever_ + // DO NOT DO THAT + // thread-unsafe + void join() { + { + auto lck = std::unique_lock(m_mutex); + if (m_state == State::Running) { + m_state = State::Joining; + } else if (m_state == State::Joining) { + Checks::exit_with_message(VCPKG_LINE_INFO, "Attempted to join more than once"); + } + } + for (auto& thrd : m_threads) { + thrd.join(); + } + } + + // useful in the case of errors + // doesn't stop any existing running tasks + // returns immediately, so that one can call this in a task + void terminate() const { + { + auto lck = std::unique_lock(m_mutex); + m_state = State::Terminated; + } + m_cv.notify_all(); + } + + void enqueue_action(Action a) const { + { + auto lck = std::unique_lock(m_mutex); + m_actions.push_back(std::move(a)); + } + m_cv.notify_one(); + } + + template + void enqueue_all_actions_by_move(Rng&& rng) const { + { + using std::begin; + using std::end; + + auto lck = std::unique_lock(m_mutex); + + auto first = begin(rng); + auto last = end(rng); + + m_actions.reserve(m_actions.size() + (end - begin)); + + std::move(first, last, std::back_insert_iterator(rng)); + } + + m_cv.notify_all(); + } + + template + void enqueue_all_actions(Rng&& rng) const { + { + using std::begin; + using std::end; + + auto lck = std::unique_lock(m_mutex); + + auto first = begin(rng); + auto last = end(rng); + + m_actions.reserve(m_actions.size() + (end - begin)); + + std::copy(first, last, std::back_insert_iterator(rng)); + } + + m_cv.notify_all(); + } + + private: + friend struct WorkQueueWorker { + const WorkQueue* work_queue; + ThreadLocalData tld; + + template + WorkQueueWorker(const WorkQueue* work_queue, const F& initializer) + : work_queue(work_queue), tld(initializer()) + { } + + void operator()() { + for (;;) { + auto lck = std::unique_lock(work_queue->m_mutex); + ++work_queue->running_workers; + + const auto state = work_queue->m_state; + + if (state == State::Terminated) { + --work_queue->running_workers; + return; + } + + if (work_queue->m_actions.empty()) { + --work_queue->running_workers; + if (state == State::Running || work_queue->running_workers > 0) { + work_queue->m_cv.wait(lck); + continue; + } + + // state == State::Joining and we are the only worker + // no more work! + return; + } + + Action action = work_queue->m_actions.pop_back(); + lck.unlock(); + + detail::call_action(action, *work_queue, tld); + } + } + }; + + enum class State : std::uint16_t { + Running, + Joining, + Terminated, + }; + + mutable std::mutex m_mutex; + // these four are under m_mutex + mutable State m_state; + mutable std::uint16_t running_workers; + mutable std::vector m_actions; + mutable std::condition_variable condition_variable; + + std::vector m_threads; + }; +} diff --git a/toolsrc/src/vcpkg/base/files.cpp b/toolsrc/src/vcpkg/base/files.cpp index d0926bb4c5..e89c531be7 100644 --- a/toolsrc/src/vcpkg/base/files.cpp +++ b/toolsrc/src/vcpkg/base/files.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #if defined(__linux__) || defined(__APPLE__) @@ -21,6 +22,45 @@ #include #endif +namespace fs { + file_status decltype(symlink_status)::operator()(const path& p, std::error_code& ec) const noexcept { +#if defined(_WIN32) + /* + do not find the permissions of the file -- it's unnecessary for the + things that vcpkg does. + if one were to add support for this in the future, one should look + into GetFileSecurityW + */ + perms permissions = perms::unknown; + + WIN32_FILE_ATTRIBUTE_DATA file_attributes; + file_type ft = file_type::unknown; + if (!GetFileAttributesExW(p.c_str(), GetFileExInfoStandard, &file_attributes)) { + ft = file_type::not_found; + } else if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) { + // check for reparse point -- if yes, then symlink + 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; + } + + return file_status(ft, permissions); + +#else + return stdfs::symlink_status(p, ec); +#endif + } + + file_status decltype(symlink_status)::operator()(const path& p) const noexcept { + std::error_code ec; + auto result = symlink_status(p, ec); + if (ec) vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "error getting status of path %s: %s", p, ec.message()); + } +} + namespace vcpkg::Files { static const std::regex FILESYSTEM_INVALID_CHARACTERS_REGEX = std::regex(R"([\/:*?"<>|])"); @@ -263,55 +303,129 @@ namespace vcpkg::Files (as well as on macOS and Linux), this is just as fast and will have fewer spurious errors due to locks. */ - struct recursive { - const fs::path& tmp_directory; - std::error_code& ec; - xoshiro256ss_engine& rng; - void operator()(const fs::path& current) const { - const auto type = fs::stdfs::symlink_status(current, ec).type(); - if (ec) return; + /* + `remove` doesn't actually remove anything -- it simply moves the + files into a parent directory (which ends up being at `path`), + and then inserts `actually_remove{current_path}` into the work + queue. + */ + struct remove { + struct tld { + const fs::path& tmp_directory; + std::uint64_t index; - const auto tmp_name = Strings::b64url_encode(rng()); - const auto tmp_path = tmp_directory / tmp_name; + std::atomic& files_deleted; - switch (type) { - case fs::file_type::directory: { - fs::stdfs::rename(current, tmp_path, ec); - if (ec) return; - for (const auto& entry : fs::stdfs::directory_iterator(tmp_path)) { - (*this)(entry); + std::mutex& ec_mutex; + std::error_code& ec; + }; + + struct actually_remove; + using queue = WorkQueue; + + /* + if `current_path` is a directory, first `remove`s all + elements of the directory, then calls remove. + + else, just calls remove. + */ + struct actually_remove { + fs::path current_path; + + void operator()(tld& info, const queue& queue) const { + std::error_code ec; + const auto path_type = fs::symlink_status(current_path, ec).type(); + + if (check_ec(ec, info, queue)) return; + + if (path_type == fs::file_type::directory) { + for (const auto& entry : fs::stdfs::directory_iterator(current_path)) { + remove{}(entry, info, queue); + } + } + + if (fs::stdfs::remove(current_path, ec)) { + info.files_deleted.fetch_add(1, std::memory_order_relaxed); + } else { + check_ec(ec, info, queue); } - fs::stdfs::remove(tmp_path, ec); - } break; - case fs::file_type::symlink: - case fs::file_type::regular: { - fs::stdfs::rename(current, tmp_path, ec); - fs::stdfs::remove(current, ec); - } break; - case fs::file_type::not_found: return; - case fs::file_type::none: { - Checks::exit_with_message(VCPKG_LINE_INFO, "Error occurred when evaluating file type of file: %s", current); - } - default: { - Checks::exit_with_message(VCPKG_LINE_INFO, "Attempted to delete special file: %s", current); } + }; + + static bool check_ec(const std::error_code& ec, tld& info, const queue& queue) { + if (ec) { + queue.terminate(); + + auto lck = std::unique_lock(info.ec_mutex); + if (!info.ec) { + info.ec = ec; + } + + return true; + } else { + return false; } } + + void operator()(const fs::path& current_path, tld& info, const queue& queue) const { + std::error_code ec; + + const auto type = fs::symlink_status(current_path, ec).type(); + if (check_ec(ec, info, queue)) return; + + const auto tmp_name = Strings::b64url_encode(info.index++); + const auto tmp_path = info.tmp_directory / tmp_name; + + fs::stdfs::rename(current_path, tmp_path, ec); + if (check_ec(ec, info, queue)) return; + + queue.enqueue_action(actually_remove{std::move(tmp_path)}); + } }; - auto const real_path = fs::stdfs::absolute(path); + const auto path_type = fs::symlink_status(path, ec).type(); - if (! real_path.has_parent_path()) { - Checks::exit_with_message(VCPKG_LINE_INFO, "Attempted to remove_all the base directory"); + std::atomic files_deleted = 0; + + if (path_type == fs::file_type::directory) { + std::uint64_t index = 0; + std::mutex ec_mutex; + + auto queue = remove::queue([&] { + index += 1 << 32; + return remove::tld{path, index, files_deleted, ec_mutex, ec}; + }); + + index += 1 << 32; + auto main_tld = remove::tld{path, index, files_deleted, ec_mutex, ec}; + for (const auto& entry : fs::stdfs::directory_iterator(path)) { + remove{}(entry, main_tld, queue); + } + + queue.join(); } - // thoughts: is this fine? or should we do something different? - // maybe a temporary directory? - auto const base_path = real_path.parent_path(); + /* + we need to do backoff on the removal of the top level directory, + since we need to place all moved files into that top level + directory, and so we can only delete the directory after all the + lower levels have been deleted. + */ + for (int backoff = 0; backoff < 5; ++backoff) { + if (backoff) { + using namespace std::chrono_literals; + auto backoff_time = 100ms * backoff; + std::this_thread::sleep_for(backoff_time); + } - xoshiro256ss_engine rng{}; - recursive{base_path, ec, rng}(real_path); + if (fs::stdfs::remove(path, ec)) { + files_deleted.fetch_add(1, std::memory_order_relaxed); + break; + } + } + + return files_deleted; } virtual bool exists(const fs::path& path) const override { return fs::stdfs::exists(path); } virtual bool is_directory(const fs::path& path) const override { return fs::stdfs::is_directory(path); } @@ -343,11 +457,11 @@ namespace vcpkg::Files virtual fs::file_status status(const fs::path& path, std::error_code& ec) const override { - return fs::stdfs::status(path, ec); + return fs::status(path, ec); } virtual fs::file_status symlink_status(const fs::path& path, std::error_code& ec) const override { - return fs::stdfs::symlink_status(path, ec); + return fs::symlink_status(path, ec); } virtual void write_contents(const fs::path& file_path, const std::string& data, std::error_code& ec) override { diff --git a/toolsrc/src/vcpkg/base/rng.cpp b/toolsrc/src/vcpkg/base/rng.cpp index 9fe2ea3b40..40ff646b70 100644 --- a/toolsrc/src/vcpkg/base/rng.cpp +++ b/toolsrc/src/vcpkg/base/rng.cpp @@ -1,11 +1,11 @@ #include -namespace vcpkg { +namespace vcpkg::Rng { namespace { std::random_device system_entropy{}; } - splitmix64_engine::splitmix64_engine() { + splitmix::splitmix() { std::uint64_t top_half = system_entropy(); std::uint64_t bottom_half = system_entropy(); From 43493b56df7c8f7aab02256ab7f65135d4dd1d4c Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Wed, 10 Jul 2019 15:18:44 -0700 Subject: [PATCH 03/17] delete the random number generator --- toolsrc/include/vcpkg/base/rng.h | 199 ------------------------------- toolsrc/src/vcpkg/base/rng.cpp | 14 --- 2 files changed, 213 deletions(-) delete mode 100644 toolsrc/include/vcpkg/base/rng.h delete mode 100644 toolsrc/src/vcpkg/base/rng.cpp diff --git a/toolsrc/include/vcpkg/base/rng.h b/toolsrc/include/vcpkg/base/rng.h deleted file mode 100644 index 4a0411f644..0000000000 --- a/toolsrc/include/vcpkg/base/rng.h +++ /dev/null @@ -1,199 +0,0 @@ -#pragma once - -#include -#include -#include - -namespace vcpkg::Rng { - - namespace detail { - template - constexpr std::size_t bitsize = sizeof(T) * CHAR_BITS; - - template - constexpr bool is_valid_shift(int k) { - return 0 <= k && k <= bitsize; - } - - // precondition: 0 <= k < bitsize - template - constexpr T ror(T x, int k) { - if (k == 0) { - return x; - } - return (x >> k) | (x << (bitsize - k)); - } - - // precondition: 0 <= k < bitsize - template - constexpr T rol(T x, int k) { - if (k == 0) { - return x; - } - return (x << k) | (x >> (bitsize - k)); - } - - // there _is_ a way to do this generally, but I don't know how to - template - struct XoshiroJumpTable; - - template <> - struct XoshiroJumpTable { - constexpr static std::uint64_t value[4] = { - 0x180ec6d33cfd0aba, 0xd5a61266f0c9392c, 0xa9582618e03fc9aa, 0x39abdc4529b1661c - }; - }; - } - - /* - NOTE(ubsan): taken from the xoshiro paper - initialized from random_device by default - actual code is copied from wikipedia, since I wrote that code - */ - struct splitmix { - splitmix() noexcept; - - constexpr splitmix(std::uint64_t seed) noexcept - : state(seed) {} - - constexpr std::uint64_t operator()() noexcept { - state += 0x9E3779B97F4A7C15; - - std::uint64_t result = state; - result = (result ^ (result >> 30)) * 0xBF58476D1CE4E5B9; - result = (result ^ (result >> 27)) * 0x94D049BB133111EB; - - return result ^ (result >> 31); - } - - constexpr std::uint64_t max() const noexcept { - return std::numeric_limits::max(); - } - - constexpr std::uint64_t min() const noexcept { - return std::numeric_limits::min(); - } - - template - constexpr void fill(T* first, T* last) { - constexpr auto mask = - static_cast(std::numeric_limits::max()); - - const auto remaining = - (last - first) % (sizeof(std::uint64_t) / sizeof(T)); - - for (auto it = first; it != last - remaining;) { - const auto item = (*this)(); - for ( - int shift = 0; - shift < 64; - shift += detail::bitsize, ++it - ) { - *it = static_cast((item >> shift) & mask); - } - } - - if (remaining == 0) return; - - int shift = 0; - const auto item = (*this)(); - for (auto it = last - remaining; - it != last; - shift += detail::bitsize, ++it - ) { - *it = static_cast((item >> shift) & mask); - } - } - - private: - std::uint64_t state; - }; - - template - struct starstar_scrambler { - constexpr static UIntType scramble(UIntType n) noexcept { - return detail::rol(n * S, R) * T; - } - }; - - // Sebastian Vigna's xorshift-based xoshiro engine - // fast and really good - // uses the splitmix to initialize state - template - struct xoshiro_engine { - static_assert(detail::is_valid_shift(A)); - static_assert(detail::is_valid_shift(B)); - static_assert(std::is_unsigned_v); - - // splitmix will be initialized with random_device - xoshiro_engine() noexcept { - splitmix sm{}; - - sm.fill(&state[0], &state[4]); - } - - constexpr xoshiro_engine(std::uint64_t seed) noexcept : state() { - splitmix sm{seed}; - - sm.fill(&state[0], &state[4]); - } - - constexpr UIntType operator()() noexcept { - const UIntType result = Scrambler::scramble(state[0]); - - const UIntType t = state[1] << A; - - state[2] ^= state[0]; - state[3] ^= state[1]; - state[1] ^= state[2]; - state[0] ^= state[3]; - - state[2] ^= t; - state[3] ^= detail::rol(state[3], B); - - return result; - } - - constexpr UIntType max() const noexcept { - return std::numeric_limits::max(); - } - - constexpr std::uint64_t min() const noexcept { - return std::numeric_limits::min(); - } - - // quickly jump ahead 2^e steps - // takes 4 * bitsize rng next operations - template - constexpr void discard_e() noexcept { - using JT = detail::XoshiroJumpTable; - - UIntType s[4] = {}; - for (const auto& jump : JT::value) { - for (std::size_t i = 0; i < bitsize; ++i) { - if ((jump >> i) & 1) { - s[0] ^= state[0]; - s[1] ^= state[1]; - s[2] ^= state[2]; - s[3] ^= state[3]; - } - (*this)(); - } - } - - state[0] = s[0]; - state[1] = s[1]; - state[2] = s[2]; - state[3] = s[3]; - } - private: - // rotate left - UIntType state[4]; - }; - - using xoshiro256ss = xoshiro_engine< - std::uint64_t, - starstar_scrambler, - 17, - 45>; -} diff --git a/toolsrc/src/vcpkg/base/rng.cpp b/toolsrc/src/vcpkg/base/rng.cpp deleted file mode 100644 index 40ff646b70..0000000000 --- a/toolsrc/src/vcpkg/base/rng.cpp +++ /dev/null @@ -1,14 +0,0 @@ -#include - -namespace vcpkg::Rng { - namespace { - std::random_device system_entropy{}; - } - - splitmix::splitmix() { - std::uint64_t top_half = system_entropy(); - std::uint64_t bottom_half = system_entropy(); - - state = (top_half << 32) | bottom_half; - } -} From 3b6d6b3465e0e79999e5995f0104a6e8c021088c Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Wed, 10 Jul 2019 15:42:13 -0700 Subject: [PATCH 04/17] actually get the code compiling --- toolsrc/include/vcpkg/base/work_queue.h | 42 ++++++++++++++----------- toolsrc/src/vcpkg/base/files.cpp | 9 +++--- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/toolsrc/include/vcpkg/base/work_queue.h b/toolsrc/include/vcpkg/base/work_queue.h index 4db167fa64..71e00a2abb 100644 --- a/toolsrc/include/vcpkg/base/work_queue.h +++ b/toolsrc/include/vcpkg/base/work_queue.h @@ -1,10 +1,15 @@ #pragma once +#include #include #include namespace vcpkg { + template + struct WorkQueue; + namespace detail { + // for SFINAE purposes, keep out of the class template auto call_action( Action& action, @@ -29,8 +34,8 @@ namespace vcpkg { template struct WorkQueue { template - explicit WorkQueue(const F& initializer) noexcept { - state = State::Joining; + explicit WorkQueue(const F& tld_init) noexcept { + m_state = State::Running; std::size_t num_threads = std::thread::hardware_concurrency(); if (num_threads == 0) { @@ -39,7 +44,7 @@ namespace vcpkg { m_threads.reserve(num_threads); for (std::size_t i = 0; i < num_threads; ++i) { - m_threads.emplace_back(this, initializer); + m_threads.push_back(std::thread(Worker{this, tld_init()})); } } @@ -93,10 +98,10 @@ namespace vcpkg { auto lck = std::unique_lock(m_mutex); - auto first = begin(rng); - auto last = end(rng); + const auto first = begin(rng); + const auto last = end(rng); - m_actions.reserve(m_actions.size() + (end - begin)); + m_actions.reserve(m_actions.size() + (last - first)); std::move(first, last, std::back_insert_iterator(rng)); } @@ -112,10 +117,10 @@ namespace vcpkg { auto lck = std::unique_lock(m_mutex); - auto first = begin(rng); - auto last = end(rng); + const auto first = begin(rng); + const auto last = end(rng); - m_actions.reserve(m_actions.size() + (end - begin)); + m_actions.reserve(m_actions.size() + (last - first)); std::copy(first, last, std::back_insert_iterator(rng)); } @@ -124,18 +129,15 @@ namespace vcpkg { } private: - friend struct WorkQueueWorker { + struct Worker { const WorkQueue* work_queue; ThreadLocalData tld; - template - WorkQueueWorker(const WorkQueue* work_queue, const F& initializer) - : work_queue(work_queue), tld(initializer()) - { } - void operator()() { + // unlocked when waiting, or when in the `call_action` block + // locked otherwise + auto lck = std::unique_lock(work_queue->m_mutex); for (;;) { - auto lck = std::unique_lock(work_queue->m_mutex); ++work_queue->running_workers; const auto state = work_queue->m_state; @@ -157,10 +159,12 @@ namespace vcpkg { return; } - Action action = work_queue->m_actions.pop_back(); - lck.unlock(); + Action action = std::move(work_queue->m_actions.back()); + work_queue->m_actions.pop_back(); + lck.unlock(); detail::call_action(action, *work_queue, tld); + lck.lock(); } } }; @@ -176,7 +180,7 @@ namespace vcpkg { mutable State m_state; mutable std::uint16_t running_workers; mutable std::vector m_actions; - mutable std::condition_variable condition_variable; + mutable std::condition_variable m_cv; std::vector m_threads; }; diff --git a/toolsrc/src/vcpkg/base/files.cpp b/toolsrc/src/vcpkg/base/files.cpp index e89c531be7..d8a9821646 100644 --- a/toolsrc/src/vcpkg/base/files.cpp +++ b/toolsrc/src/vcpkg/base/files.cpp @@ -1,7 +1,6 @@ #include "pch.h" #include -#include #include #include #include @@ -57,7 +56,9 @@ namespace fs { file_status decltype(symlink_status)::operator()(const path& p) const noexcept { std::error_code ec; auto result = symlink_status(p, ec); - if (ec) vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "error getting status of path %s: %s", p, ec.message()); + if (ec) vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "error getting status of path %s: %s", p.string(), ec.message()); + + return result; } } @@ -393,11 +394,11 @@ namespace vcpkg::Files std::mutex ec_mutex; auto queue = remove::queue([&] { - index += 1 << 32; + index += static_cast(1) << 32; return remove::tld{path, index, files_deleted, ec_mutex, ec}; }); - index += 1 << 32; + index += static_cast(1) << 32; auto main_tld = remove::tld{path, index, files_deleted, ec_mutex, ec}; for (const auto& entry : fs::stdfs::directory_iterator(path)) { remove{}(entry, main_tld, queue); From 5b76f24f35976739991941d3b6289fb78fd93648 Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Wed, 10 Jul 2019 16:28:56 -0700 Subject: [PATCH 05/17] make this compile on macos --- toolsrc/include/vcpkg/base/files.h | 35 ++++++++++++++++------------ toolsrc/include/vcpkg/base/strings.h | 2 +- toolsrc/src/vcpkg/base/files.cpp | 6 ++--- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/toolsrc/include/vcpkg/base/files.h b/toolsrc/include/vcpkg/base/files.h index 178fae5410..33f4647792 100644 --- a/toolsrc/include/vcpkg/base/files.h +++ b/toolsrc/include/vcpkg/base/files.h @@ -25,23 +25,28 @@ namespace fs using stdfs::status; // we want to poison ADL with these niebloids - constexpr struct { - file_status operator()(const path& p, std::error_code& ec) const noexcept; - file_status operator()(const path& p) const noexcept; - } symlink_status{}; - constexpr struct { - inline bool operator()(file_status s) const { - return stdfs::is_symlink(s); - } + namespace detail { + struct symlink_status_t { + file_status operator()(const path& p, std::error_code& ec) const noexcept; + file_status operator()(const path& p) const noexcept; + }; + struct is_symlink_t { + inline bool operator()(file_status s) const { + return stdfs::is_symlink(s); + } - inline bool operator()(const path& p) const { - return stdfs::is_symlink(symlink_status(p)); - } - inline bool operator()(const path& p, std::error_code& ec) const { - return stdfs::is_symlink(symlink_status(p, ec)); - } - } is_symlink{}; + inline bool operator()(const path& p) const { + return stdfs::is_symlink(symlink_status(p)); + } + inline bool operator()(const path& p, std::error_code& ec) const { + return stdfs::is_symlink(symlink_status(p, ec)); + } + }; + } + + constexpr detail::symlink_status_t symlink_status{}; + constexpr detail::is_symlink_t is_symlink{}; inline bool is_regular_file(file_status s) { return stdfs::is_regular_file(s); } inline bool is_directory(file_status s) { return stdfs::is_directory(s); } diff --git a/toolsrc/include/vcpkg/base/strings.h b/toolsrc/include/vcpkg/base/strings.h index 423ea2096a..82145b826c 100644 --- a/toolsrc/include/vcpkg/base/strings.h +++ b/toolsrc/include/vcpkg/base/strings.h @@ -190,7 +190,7 @@ namespace vcpkg::Strings // ignores padding, since one implicitly knows the length from the size of x template std::string b64url_encode(Integral x) { - static_assert(std::is_integral_v); + static_assert(std::is_integral::value, "b64url_encode must take an integer type"); auto value = static_cast>(x); // 64 values, plus the implicit \0 diff --git a/toolsrc/src/vcpkg/base/files.cpp b/toolsrc/src/vcpkg/base/files.cpp index d8a9821646..f4c2106d41 100644 --- a/toolsrc/src/vcpkg/base/files.cpp +++ b/toolsrc/src/vcpkg/base/files.cpp @@ -21,8 +21,8 @@ #include #endif -namespace fs { - file_status decltype(symlink_status)::operator()(const path& p, std::error_code& ec) const noexcept { +namespace fs::detail { + file_status symlink_status_t::operator()(const path& p, std::error_code& ec) const noexcept { #if defined(_WIN32) /* do not find the permissions of the file -- it's unnecessary for the @@ -53,7 +53,7 @@ namespace fs { #endif } - file_status decltype(symlink_status)::operator()(const path& p) const noexcept { + file_status symlink_status_t::operator()(const path& p) const noexcept { std::error_code ec; auto result = symlink_status(p, ec); if (ec) vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "error getting status of path %s: %s", p.string(), ec.message()); From bb579072077153fabfa74acec852bce222265357 Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Wed, 10 Jul 2019 17:39:04 -0700 Subject: [PATCH 06/17] make it compile on macos under g++6 --- toolsrc/include/vcpkg/base/strings.h | 6 ++++-- toolsrc/include/vcpkg/base/work_queue.h | 4 ++-- toolsrc/src/vcpkg/base/files.cpp | 13 +++++-------- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/toolsrc/include/vcpkg/base/strings.h b/toolsrc/include/vcpkg/base/strings.h index 82145b826c..9890bedbc3 100644 --- a/toolsrc/include/vcpkg/base/strings.h +++ b/toolsrc/include/vcpkg/base/strings.h @@ -191,7 +191,8 @@ namespace vcpkg::Strings template std::string b64url_encode(Integral x) { static_assert(std::is_integral::value, "b64url_encode must take an integer type"); - auto value = static_cast>(x); + using Unsigned = std::make_unsigned_t; + auto value = static_cast(x); // 64 values, plus the implicit \0 constexpr static char map[0x41] = @@ -202,8 +203,8 @@ namespace vcpkg::Strings /*3*/ "wxyz0123456789-_" ; - constexpr static std::make_unsigned_t mask = 0x3F; constexpr static int shift = 5; + constexpr static auto mask = (static_cast(1) << shift) - 1; std::string result; // reserve ceiling(number of bits / 3) @@ -212,6 +213,7 @@ namespace vcpkg::Strings while (value != 0) { char mapped_value = map[value & mask]; result.push_back(mapped_value); + value >>= shift; } return result; diff --git a/toolsrc/include/vcpkg/base/work_queue.h b/toolsrc/include/vcpkg/base/work_queue.h index 71e00a2abb..8a3d275384 100644 --- a/toolsrc/include/vcpkg/base/work_queue.h +++ b/toolsrc/include/vcpkg/base/work_queue.h @@ -103,7 +103,7 @@ namespace vcpkg { m_actions.reserve(m_actions.size() + (last - first)); - std::move(first, last, std::back_insert_iterator(rng)); + std::move(first, last, std::back_inserter(rng)); } m_cv.notify_all(); @@ -122,7 +122,7 @@ namespace vcpkg { m_actions.reserve(m_actions.size() + (last - first)); - std::copy(first, last, std::back_insert_iterator(rng)); + std::copy(first, last, std::back_inserter(rng)); } m_cv.notify_all(); diff --git a/toolsrc/src/vcpkg/base/files.cpp b/toolsrc/src/vcpkg/base/files.cpp index f4c2106d41..8bc37819af 100644 --- a/toolsrc/src/vcpkg/base/files.cpp +++ b/toolsrc/src/vcpkg/base/files.cpp @@ -129,7 +129,7 @@ namespace vcpkg::Files file_stream.read(&output[0], length); file_stream.close(); - return std::move(output); + return output; } virtual Expected> read_lines(const fs::path& file_path) const override { @@ -147,7 +147,7 @@ namespace vcpkg::Files } file_stream.close(); - return std::move(output); + return output; } virtual fs::path find_file_recursively_up(const fs::path& starting_dir, const std::string& filename) const override @@ -372,9 +372,6 @@ namespace vcpkg::Files void operator()(const fs::path& current_path, tld& info, const queue& queue) const { std::error_code ec; - const auto type = fs::symlink_status(current_path, ec).type(); - if (check_ec(ec, info, queue)) return; - const auto tmp_name = Strings::b64url_encode(info.index++); const auto tmp_path = info.tmp_directory / tmp_name; @@ -387,16 +384,16 @@ namespace vcpkg::Files const auto path_type = fs::symlink_status(path, ec).type(); - std::atomic files_deleted = 0; + std::atomic files_deleted{0}; if (path_type == fs::file_type::directory) { std::uint64_t index = 0; std::mutex ec_mutex; - auto queue = remove::queue([&] { + remove::queue queue{[&] { index += static_cast(1) << 32; return remove::tld{path, index, files_deleted, ec_mutex, ec}; - }); + }}; index += static_cast(1) << 32; auto main_tld = remove::tld{path, index, files_deleted, ec_mutex, ec}; From 319023587558a9f8de9d7eabeb7441ef2e7ee277 Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Thu, 11 Jul 2019 10:53:32 -0700 Subject: [PATCH 07/17] fix some comments from code reviewers --- toolsrc/include/vcpkg/base/strings.h | 10 ++++++---- toolsrc/include/vcpkg/base/work_queue.h | 15 +++++++-------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/toolsrc/include/vcpkg/base/strings.h b/toolsrc/include/vcpkg/base/strings.h index 9890bedbc3..25cd302b01 100644 --- a/toolsrc/include/vcpkg/base/strings.h +++ b/toolsrc/include/vcpkg/base/strings.h @@ -208,11 +208,13 @@ namespace vcpkg::Strings std::string result; // reserve ceiling(number of bits / 3) - result.reserve((sizeof(value) * 8 + 2) / 3); + result.resize((sizeof(value) * 8 + 2) / 3, map[0]); - while (value != 0) { - char mapped_value = map[value & mask]; - result.push_back(mapped_value); + for (char& c: result) { + if (value == 0) { + break; + } + c = map[value & mask]; value >>= shift; } diff --git a/toolsrc/include/vcpkg/base/work_queue.h b/toolsrc/include/vcpkg/base/work_queue.h index 8a3d275384..b6f070cd87 100644 --- a/toolsrc/include/vcpkg/base/work_queue.h +++ b/toolsrc/include/vcpkg/base/work_queue.h @@ -11,7 +11,7 @@ namespace vcpkg { namespace detail { // for SFINAE purposes, keep out of the class template - auto call_action( + auto call_moved_action( Action& action, const WorkQueue& work_queue, ThreadLocalData& tld @@ -21,7 +21,7 @@ namespace vcpkg { } template - auto call_action( + auto call_moved_action( Action& action, const WorkQueue&, ThreadLocalData& tld @@ -134,21 +134,18 @@ namespace vcpkg { ThreadLocalData tld; void operator()() { - // unlocked when waiting, or when in the `call_action` block + // unlocked when waiting, or when in the `call_moved_action` + // block // locked otherwise auto lck = std::unique_lock(work_queue->m_mutex); for (;;) { - ++work_queue->running_workers; - const auto state = work_queue->m_state; if (state == State::Terminated) { - --work_queue->running_workers; return; } if (work_queue->m_actions.empty()) { - --work_queue->running_workers; if (state == State::Running || work_queue->running_workers > 0) { work_queue->m_cv.wait(lck); continue; @@ -162,9 +159,11 @@ namespace vcpkg { Action action = std::move(work_queue->m_actions.back()); work_queue->m_actions.pop_back(); + ++work_queue->running_workers; lck.unlock(); - detail::call_action(action, *work_queue, tld); + detail::call_moved_action(action, *work_queue, tld); lck.lock(); + --work_queue->running_workers; } } }; From 510b0c5cc0233311b6993b89cd5ce488218ed78d Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Thu, 11 Jul 2019 15:01:29 -0700 Subject: [PATCH 08/17] fix more comments --- toolsrc/include/vcpkg/base/files.h | 29 +++-- toolsrc/include/vcpkg/base/strings.h | 35 +----- toolsrc/include/vcpkg/base/work_queue.h | 102 +++++++++++++---- toolsrc/src/vcpkg/archives.cpp | 5 +- toolsrc/src/vcpkg/base/files.cpp | 133 ++++++++++++++++------- toolsrc/src/vcpkg/base/strings.cpp | 42 +++++++ toolsrc/src/vcpkg/build.cpp | 12 +- toolsrc/src/vcpkg/commands.exportifw.cpp | 16 ++- toolsrc/src/vcpkg/commands.portsdiff.cpp | 2 +- toolsrc/src/vcpkg/export.cpp | 6 +- toolsrc/src/vcpkg/install.cpp | 3 +- toolsrc/src/vcpkg/remove.cpp | 3 +- 12 files changed, 269 insertions(+), 119 deletions(-) diff --git a/toolsrc/include/vcpkg/base/files.h b/toolsrc/include/vcpkg/base/files.h index 33f4647792..6cad3d4613 100644 --- a/toolsrc/include/vcpkg/base/files.h +++ b/toolsrc/include/vcpkg/base/files.h @@ -29,27 +29,29 @@ namespace fs namespace detail { struct symlink_status_t { file_status operator()(const path& p, std::error_code& ec) const noexcept; - file_status operator()(const path& p) const noexcept; + file_status operator()(const path& p, vcpkg::LineInfo li) const noexcept; }; struct is_symlink_t { inline bool operator()(file_status s) const { return stdfs::is_symlink(s); } - - inline bool operator()(const path& p) const { - return stdfs::is_symlink(symlink_status(p)); + }; + struct is_regular_file_t { + inline bool operator()(file_status s) const { + return stdfs::is_regular_file(s); } - inline bool operator()(const path& p, std::error_code& ec) const { - return stdfs::is_symlink(symlink_status(p, ec)); + }; + struct is_directory_t { + inline bool operator()(file_status s) const { + return stdfs::is_directory(s); } }; } constexpr detail::symlink_status_t symlink_status{}; constexpr detail::is_symlink_t is_symlink{}; - - inline bool is_regular_file(file_status s) { return stdfs::is_regular_file(s); } - inline bool is_directory(file_status s) { return stdfs::is_directory(s); } + constexpr detail::is_regular_file_t is_regular_file{}; + constexpr detail::is_directory_t is_directory{}; } /* @@ -57,9 +59,14 @@ namespace fs they might get the ADL version, which is broken. Therefore, put `symlink_status` in the global namespace, so that they get our symlink_status. + + We also want to poison the ADL on is_regular_file and is_directory, because + we don't want people calling these functions on paths */ using fs::symlink_status; using fs::is_symlink; +using fs::is_regular_file; +using fs::is_directory; namespace vcpkg::Files { @@ -85,7 +92,9 @@ namespace vcpkg::Files std::error_code& ec) = 0; bool remove(const fs::path& path, LineInfo linfo); virtual bool remove(const fs::path& path, std::error_code& ec) = 0; - virtual std::uintmax_t remove_all(const fs::path& path, std::error_code& ec) = 0; + + virtual std::uintmax_t remove_all(const fs::path& path, std::error_code& ec, fs::path& failure_point) = 0; + std::uintmax_t remove_all(const fs::path& path, LineInfo li); virtual bool exists(const fs::path& path) const = 0; virtual bool is_directory(const fs::path& path) const = 0; virtual bool is_regular_file(const fs::path& path) const = 0; diff --git a/toolsrc/include/vcpkg/base/strings.h b/toolsrc/include/vcpkg/base/strings.h index 25cd302b01..625c0240fd 100644 --- a/toolsrc/include/vcpkg/base/strings.h +++ b/toolsrc/include/vcpkg/base/strings.h @@ -188,36 +188,13 @@ namespace vcpkg::Strings // base 64 encoding with URL and filesafe alphabet (base64url) // based on IETF RFC 4648 // ignores padding, since one implicitly knows the length from the size of x - template - std::string b64url_encode(Integral x) { - static_assert(std::is_integral::value, "b64url_encode must take an integer type"); - using Unsigned = std::make_unsigned_t; - auto value = static_cast(x); + namespace detail { - // 64 values, plus the implicit \0 - constexpr static char map[0x41] = - /* 0123456789ABCDEF */ - /*0*/ "ABCDEFGHIJKLMNOP" - /*1*/ "QRSTUVWXYZabcdef" - /*2*/ "ghijklmnopqrstuv" - /*3*/ "wxyz0123456789-_" - ; + struct b64url_encode_t { + std::string operator()(std::uint64_t x) const noexcept; + }; - constexpr static int shift = 5; - constexpr static auto mask = (static_cast(1) << shift) - 1; - - std::string result; - // reserve ceiling(number of bits / 3) - result.resize((sizeof(value) * 8 + 2) / 3, map[0]); - - for (char& c: result) { - if (value == 0) { - break; - } - c = map[value & mask]; - value >>= shift; - } - - return result; } + + constexpr detail::b64url_encode_t b64url_encode{}; } diff --git a/toolsrc/include/vcpkg/base/work_queue.h b/toolsrc/include/vcpkg/base/work_queue.h index b6f070cd87..1836404ca6 100644 --- a/toolsrc/include/vcpkg/base/work_queue.h +++ b/toolsrc/include/vcpkg/base/work_queue.h @@ -29,18 +29,19 @@ namespace vcpkg { { std::move(action)(tld); } + + struct immediately_run_t {}; } + constexpr detail::immediately_run_t immediately_run{}; + + template struct WorkQueue { template - explicit WorkQueue(const F& tld_init) noexcept { - m_state = State::Running; - - std::size_t num_threads = std::thread::hardware_concurrency(); - if (num_threads == 0) { - num_threads = 4; - } + WorkQueue(std::size_t num_threads, LineInfo li, const F& tld_init) noexcept { + m_line_info = li; + m_state = State::BeforeRun; m_threads.reserve(num_threads); for (std::size_t i = 0; i < num_threads; ++i) { @@ -48,22 +49,52 @@ namespace vcpkg { } } + template + WorkQueue( + detail::immediately_run_t, + std::size_t num_threads, + LineInfo li, + const F& tld_init + ) noexcept : WorkQueue(num_threads, li, tld_init) { + m_state = State::Running; + } + WorkQueue(WorkQueue const&) = delete; WorkQueue(WorkQueue&&) = delete; - ~WorkQueue() = default; + ~WorkQueue() { + auto lck = std::unique_lock(m_mutex); + if (m_state == State::Running) { + Checks::exit_with_message(m_line_info, "Failed to call join() on a WorkQueue that was destroyed"); + } + } + + // should only be called once; anything else is an error + void run(LineInfo li) { + // this should _not_ be locked before `run()` is called; however, we + // want to terminate if someone screws up, rather than cause UB + auto lck = std::unique_lock(m_mutex); + + if (m_state != State::BeforeRun) { + Checks::exit_with_message(li, "Attempted to run() twice"); + } + + m_state = State::Running; + } // runs all remaining tasks, and blocks on their finishing // if this is called in an existing task, _will block forever_ // DO NOT DO THAT // thread-unsafe - void join() { + void join(LineInfo li) { { auto lck = std::unique_lock(m_mutex); - if (m_state == State::Running) { - m_state = State::Joining; - } else if (m_state == State::Joining) { - Checks::exit_with_message(VCPKG_LINE_INFO, "Attempted to join more than once"); + if (is_joined(m_state)) { + Checks::exit_with_message(li, "Attempted to call join() more than once"); + } else if (m_state == State::Terminated) { + m_state = State::TerminatedJoined; + } else { + m_state = State::Joined; } } for (auto& thrd : m_threads) { @@ -77,7 +108,11 @@ namespace vcpkg { void terminate() const { { auto lck = std::unique_lock(m_mutex); - m_state = State::Terminated; + if (is_joined(m_state)) { + m_state = State::TerminatedJoined; + } else { + m_state = State::Terminated; + } } m_cv.notify_all(); } @@ -86,6 +121,8 @@ namespace vcpkg { { auto lck = std::unique_lock(m_mutex); m_actions.push_back(std::move(a)); + + if (m_state == State::BeforeRun) return; } m_cv.notify_one(); } @@ -104,6 +141,8 @@ namespace vcpkg { m_actions.reserve(m_actions.size() + (last - first)); std::move(first, last, std::back_inserter(rng)); + + if (m_state == State::BeforeRun) return; } m_cv.notify_all(); @@ -123,6 +162,8 @@ namespace vcpkg { m_actions.reserve(m_actions.size() + (last - first)); std::copy(first, last, std::back_inserter(rng)); + + if (m_state == State::BeforeRun) return; } m_cv.notify_all(); @@ -134,24 +175,30 @@ namespace vcpkg { ThreadLocalData tld; void operator()() { - // unlocked when waiting, or when in the `call_moved_action` - // block + // unlocked when waiting, or when in the action // locked otherwise auto lck = std::unique_lock(work_queue->m_mutex); + + work_queue->m_cv.wait(lck, [&] { + return work_queue->m_state != State::BeforeRun; + }); + for (;;) { const auto state = work_queue->m_state; - if (state == State::Terminated) { + if (is_terminated(state)) { return; } if (work_queue->m_actions.empty()) { if (state == State::Running || work_queue->running_workers > 0) { + --work_queue->running_workers; work_queue->m_cv.wait(lck); + ++work_queue->running_workers; continue; } - // state == State::Joining and we are the only worker + // the queue isn't running, and we are the only worker // no more work! return; } @@ -159,21 +206,31 @@ namespace vcpkg { Action action = std::move(work_queue->m_actions.back()); work_queue->m_actions.pop_back(); - ++work_queue->running_workers; lck.unlock(); detail::call_moved_action(action, *work_queue, tld); lck.lock(); - --work_queue->running_workers; } } }; - enum class State : std::uint16_t { + enum class State : std::int16_t { + // can only exist upon construction + BeforeRun = -1, + Running, - Joining, + Joined, Terminated, + TerminatedJoined, }; + static bool is_terminated(State st) { + return st == State::Terminated || st == State::TerminatedJoined; + } + + static bool is_joined(State st) { + return st != State::Joined || st == State::TerminatedJoined; + } + mutable std::mutex m_mutex; // these four are under m_mutex mutable State m_state; @@ -182,5 +239,6 @@ namespace vcpkg { mutable std::condition_variable m_cv; std::vector m_threads; + LineInfo m_line_info; }; } diff --git a/toolsrc/src/vcpkg/archives.cpp b/toolsrc/src/vcpkg/archives.cpp index 69a916828d..d22e841de3 100644 --- a/toolsrc/src/vcpkg/archives.cpp +++ b/toolsrc/src/vcpkg/archives.cpp @@ -15,9 +15,10 @@ namespace vcpkg::Archives #endif ; + fs.remove_all(to_path, VCPKG_LINE_INFO); + fs.remove_all(to_path_partial, VCPKG_LINE_INFO); + // TODO: check this error code std::error_code ec; - fs.remove_all(to_path, ec); - fs.remove_all(to_path_partial, ec); fs.create_directories(to_path_partial, ec); const auto ext = archive.extension(); #if defined(_WIN32) diff --git a/toolsrc/src/vcpkg/base/files.cpp b/toolsrc/src/vcpkg/base/files.cpp index 8bc37819af..a4e67a1421 100644 --- a/toolsrc/src/vcpkg/base/files.cpp +++ b/toolsrc/src/vcpkg/base/files.cpp @@ -5,8 +5,8 @@ #include #include #include -#include #include +#include #if defined(__linux__) || defined(__APPLE__) #include @@ -21,8 +21,10 @@ #include #endif -namespace fs::detail { - file_status symlink_status_t::operator()(const path& p, std::error_code& ec) const noexcept { +namespace fs::detail +{ + file_status symlink_status_t::operator()(const path& p, std::error_code& ec) const noexcept + { #if defined(_WIN32) /* do not find the permissions of the file -- it's unnecessary for the @@ -34,14 +36,21 @@ namespace fs::detail { WIN32_FILE_ATTRIBUTE_DATA file_attributes; file_type ft = file_type::unknown; - if (!GetFileAttributesExW(p.c_str(), GetFileExInfoStandard, &file_attributes)) { + if (!GetFileAttributesExW(p.c_str(), GetFileExInfoStandard, &file_attributes)) + { ft = file_type::not_found; - } else if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) { + } + else if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) + { // check for reparse point -- if yes, then symlink ft = file_type::symlink; - } else if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { + } + else if (file_attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) + { ft = file_type::directory; - } else { + } + else + { // otherwise, the file is a regular file ft = file_type::regular; } @@ -53,12 +62,13 @@ namespace fs::detail { #endif } - file_status symlink_status_t::operator()(const path& p) const noexcept { + file_status symlink_status_t::operator()(const path& p, vcpkg::LineInfo li) const noexcept + { std::error_code ec; auto result = symlink_status(p, ec); - if (ec) vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "error getting status of path %s: %s", p.string(), ec.message()); + if (ec) vcpkg::Checks::exit_with_message(li, "error getting status of path %s: %s", p.string(), ec.message()); - return result; + return result; } } @@ -105,6 +115,25 @@ namespace vcpkg::Files if (ec) Checks::exit_with_message(linfo, "error writing lines: %s: %s", path.u8string(), ec.message()); } + std::uintmax_t Filesystem::remove_all(const fs::path& path, LineInfo li) + { + std::error_code ec; + fs::path failure_point; + + const auto result = this->remove_all(path, ec, failure_point); + + if (ec) + { + Checks::exit_with_message(li, + "Failure to remove_all(%s) due to file %s: %s", + path.string(), + failure_point.string(), + ec.message()); + } + + return result; + } + struct RealFilesystem final : Filesystem { virtual Expected read_contents(const fs::path& file_path) const override @@ -296,7 +325,7 @@ namespace vcpkg::Files #endif } virtual bool remove(const fs::path& path, std::error_code& ec) override { return fs::stdfs::remove(path, ec); } - virtual std::uintmax_t remove_all(const fs::path& path, std::error_code& ec) override + virtual std::uintmax_t remove_all(const fs::path& path, std::error_code& ec, fs::path& failure_point) override { /* does not use the std::filesystem call since it is buggy, and can @@ -311,8 +340,10 @@ namespace vcpkg::Files and then inserts `actually_remove{current_path}` into the work queue. */ - struct remove { - struct tld { + struct remove + { + struct tld + { const fs::path& tmp_directory; std::uint64_t index; @@ -320,6 +351,7 @@ namespace vcpkg::Files std::mutex& ec_mutex; std::error_code& ec; + fs::path& failure_point; }; struct actually_remove; @@ -331,52 +363,68 @@ namespace vcpkg::Files else, just calls remove. */ - struct actually_remove { + struct actually_remove + { fs::path current_path; - void operator()(tld& info, const queue& queue) const { + void operator()(tld& info, const queue& queue) const + { std::error_code ec; const auto path_type = fs::symlink_status(current_path, ec).type(); - if (check_ec(ec, info, queue)) return; + if (check_ec(ec, info, queue, current_path)) return; - if (path_type == fs::file_type::directory) { - for (const auto& entry : fs::stdfs::directory_iterator(current_path)) { + if (path_type == fs::file_type::directory) + { + for (const auto& entry : fs::stdfs::directory_iterator(current_path)) + { remove{}(entry, info, queue); } } - if (fs::stdfs::remove(current_path, ec)) { + if (fs::stdfs::remove(current_path, ec)) + { info.files_deleted.fetch_add(1, std::memory_order_relaxed); - } else { - check_ec(ec, info, queue); + } + else + { + check_ec(ec, info, queue, current_path); } } }; - static bool check_ec(const std::error_code& ec, tld& info, const queue& queue) { - if (ec) { + static bool check_ec(const std::error_code& ec, + tld& info, + const queue& queue, + const fs::path& failure_point) + { + if (ec) + { queue.terminate(); auto lck = std::unique_lock(info.ec_mutex); - if (!info.ec) { + if (!info.ec) + { info.ec = ec; } return true; - } else { + } + else + { return false; } } - void operator()(const fs::path& current_path, tld& info, const queue& queue) const { + void operator()(const fs::path& current_path, tld& info, const queue& queue) const + { std::error_code ec; const auto tmp_name = Strings::b64url_encode(info.index++); const auto tmp_path = info.tmp_directory / tmp_name; fs::stdfs::rename(current_path, tmp_path, ec); - if (check_ec(ec, info, queue)) return; + if (check_ec(ec, info, queue, current_path)) return; queue.enqueue_action(actually_remove{std::move(tmp_path)}); } @@ -386,22 +434,28 @@ namespace vcpkg::Files std::atomic files_deleted{0}; - if (path_type == fs::file_type::directory) { + if (path_type == fs::file_type::directory) + { std::uint64_t index = 0; std::mutex ec_mutex; - remove::queue queue{[&] { + auto const tld_gen = [&] { index += static_cast(1) << 32; - return remove::tld{path, index, files_deleted, ec_mutex, ec}; - }}; + return remove::tld{path, index, files_deleted, ec_mutex, ec, failure_point}; + }; - index += static_cast(1) << 32; - auto main_tld = remove::tld{path, index, files_deleted, ec_mutex, ec}; - for (const auto& entry : fs::stdfs::directory_iterator(path)) { + remove::queue queue{4, VCPKG_LINE_INFO, tld_gen}; + + // note: we don't actually start the queue running until the + // `join()`. This allows us to rename all the top-level files in + // peace, so that we don't get collisions. + auto main_tld = tld_gen(); + for (const auto& entry : fs::stdfs::directory_iterator(path)) + { remove{}(entry, main_tld, queue); } - queue.join(); + queue.join(VCPKG_LINE_INFO); } /* @@ -410,14 +464,17 @@ namespace vcpkg::Files directory, and so we can only delete the directory after all the lower levels have been deleted. */ - for (int backoff = 0; backoff < 5; ++backoff) { - if (backoff) { + for (int backoff = 0; backoff < 5; ++backoff) + { + if (backoff) + { using namespace std::chrono_literals; auto backoff_time = 100ms * backoff; std::this_thread::sleep_for(backoff_time); } - if (fs::stdfs::remove(path, ec)) { + if (fs::stdfs::remove(path, ec)) + { files_deleted.fetch_add(1, std::memory_order_relaxed); break; } diff --git a/toolsrc/src/vcpkg/base/strings.cpp b/toolsrc/src/vcpkg/base/strings.cpp index 54a74a7a16..16543046ea 100644 --- a/toolsrc/src/vcpkg/base/strings.cpp +++ b/toolsrc/src/vcpkg/base/strings.cpp @@ -288,3 +288,45 @@ bool Strings::contains(StringView haystack, StringView needle) { return Strings::search(haystack, needle) != haystack.end(); } + +namespace vcpkg::Strings::detail { + + template + std::string b64url_encode_implementation(Integral x) { + static_assert(std::is_integral::value, "b64url_encode must take an integer type"); + using Unsigned = std::make_unsigned_t; + auto value = static_cast(x); + + // 64 values, plus the implicit \0 + constexpr static char map[0x41] = + /* 0123456789ABCDEF */ + /*0*/ "ABCDEFGHIJKLMNOP" + /*1*/ "QRSTUVWXYZabcdef" + /*2*/ "ghijklmnopqrstuv" + /*3*/ "wxyz0123456789-_" + ; + + constexpr static int shift = 5; + constexpr static auto mask = (static_cast(1) << shift) - 1; + + std::string result; + // reserve ceiling(number of bits / 3) + result.resize((sizeof(value) * 8 + 2) / 3, map[0]); + + for (char& c: result) { + if (value == 0) { + break; + } + c = map[value & mask]; + value >>= shift; + } + + return result; + } + + std::string b64url_encode_t::operator()(std::uint64_t x) const noexcept{ + return b64url_encode_implementation(x); + } + +} + diff --git a/toolsrc/src/vcpkg/build.cpp b/toolsrc/src/vcpkg/build.cpp index 68df1f965a..c50acce7e3 100644 --- a/toolsrc/src/vcpkg/build.cpp +++ b/toolsrc/src/vcpkg/build.cpp @@ -363,7 +363,8 @@ namespace vcpkg::Build const Triplet& triplet = spec.triplet(); const auto& triplet_file_path = paths.get_triplet_file_path(spec.triplet()).u8string(); - if (!Strings::case_insensitive_ascii_starts_with(triplet_file_path, paths.triplets.u8string())) + if (!Strings::case_insensitive_ascii_starts_with(triplet_file_path, + paths.triplets.u8string())) { System::printf("-- Loading triplet configuration from: %s\n", triplet_file_path); } @@ -495,7 +496,8 @@ namespace vcpkg::Build if (fs.is_directory(file)) // Will only keep the logs { std::error_code ec; - fs.remove_all(file, ec); + fs::path failure_point; + fs.remove_all(file, ec, failure_point); } } } @@ -610,8 +612,8 @@ namespace vcpkg::Build auto& fs = paths.get_filesystem(); auto pkg_path = paths.package_dir(spec); + fs.remove_all(pkg_path, VCPKG_LINE_INFO); std::error_code ec; - fs.remove_all(pkg_path, ec); fs.create_directories(pkg_path, ec); auto files = fs.get_files_non_recursive(pkg_path); Checks::check_exit(VCPKG_LINE_INFO, files.empty(), "unable to clear path: %s", pkg_path.u8string()); @@ -794,7 +796,7 @@ namespace vcpkg::Build fs.rename_or_copy(tmp_failure_zip, archive_tombstone_path, ".tmp", ec); // clean up temporary directory - fs.remove_all(tmp_log_path, ec); + fs.remove_all(tmp_log_path, VCPKG_LINE_INFO); } } @@ -1018,7 +1020,7 @@ namespace vcpkg::Build hash += "-"; hash += Hash::get_file_hash(fs, *p, "SHA1"); } - else if (pre_build_info.cmake_system_name.empty() || + else if (pre_build_info.cmake_system_name.empty() || pre_build_info.cmake_system_name == "WindowsStore") { hash += "-"; diff --git a/toolsrc/src/vcpkg/commands.exportifw.cpp b/toolsrc/src/vcpkg/commands.exportifw.cpp index f0946110c2..3d963a2976 100644 --- a/toolsrc/src/vcpkg/commands.exportifw.cpp +++ b/toolsrc/src/vcpkg/commands.exportifw.cpp @@ -352,13 +352,15 @@ namespace vcpkg::Export::IFW System::print2("Generating repository ", repository_dir.generic_u8string(), "...\n"); std::error_code ec; + fs::path failure_point; Files::Filesystem& fs = paths.get_filesystem(); - fs.remove_all(repository_dir, ec); + fs.remove_all(repository_dir, ec, failure_point); Checks::check_exit(VCPKG_LINE_INFO, !ec, - "Could not remove outdated repository directory %s", - repository_dir.generic_u8string()); + "Could not remove outdated repository directory %s due to file %s", + repository_dir.generic_u8string(), + failure_point.string()); const auto cmd_line = Strings::format(R"("%s" --packages "%s" "%s" > nul)", repogen_exe.u8string(), @@ -414,16 +416,18 @@ namespace vcpkg::Export::IFW const VcpkgPaths& paths) { std::error_code ec; + fs::path failure_point; Files::Filesystem& fs = paths.get_filesystem(); // Prepare packages directory const fs::path ifw_packages_dir_path = get_packages_dir_path(export_id, ifw_options, paths); - fs.remove_all(ifw_packages_dir_path, ec); + fs.remove_all(ifw_packages_dir_path, ec, failure_point); Checks::check_exit(VCPKG_LINE_INFO, !ec, - "Could not remove outdated packages directory %s", - ifw_packages_dir_path.generic_u8string()); + "Could not remove outdated packages directory %s due to file %s", + ifw_packages_dir_path.generic_u8string(), + failure_point.string()); fs.create_directory(ifw_packages_dir_path, ec); Checks::check_exit( diff --git a/toolsrc/src/vcpkg/commands.portsdiff.cpp b/toolsrc/src/vcpkg/commands.portsdiff.cpp index b30c38f43a..cddc274b89 100644 --- a/toolsrc/src/vcpkg/commands.portsdiff.cpp +++ b/toolsrc/src/vcpkg/commands.portsdiff.cpp @@ -105,7 +105,7 @@ namespace vcpkg::Commands::PortsDiff std::map names_and_versions; for (auto&& port : all_ports) names_and_versions.emplace(port->core_paragraph->name, port->core_paragraph->version); - fs.remove_all(temp_checkout_path, ec); + fs.remove_all(temp_checkout_path, VCPKG_LINE_INFO); return names_and_versions; } diff --git a/toolsrc/src/vcpkg/export.cpp b/toolsrc/src/vcpkg/export.cpp index 88c1526c5c..f306bf4e69 100644 --- a/toolsrc/src/vcpkg/export.cpp +++ b/toolsrc/src/vcpkg/export.cpp @@ -400,8 +400,10 @@ namespace vcpkg::Export Files::Filesystem& fs = paths.get_filesystem(); const fs::path export_to_path = paths.root; const fs::path raw_exported_dir_path = export_to_path / export_id; + fs.remove_all(raw_exported_dir_path, VCPKG_LINE_INFO); + + // TODO: error handling std::error_code ec; - fs.remove_all(raw_exported_dir_path, ec); fs.create_directory(raw_exported_dir_path, ec); // execute the plan @@ -476,7 +478,7 @@ With a project open, go to Tools->NuGet Package Manager->Package Manager Console if (!opts.raw) { - fs.remove_all(raw_exported_dir_path, ec); + fs.remove_all(raw_exported_dir_path, VCPKG_LINE_INFO); } } diff --git a/toolsrc/src/vcpkg/install.cpp b/toolsrc/src/vcpkg/install.cpp index de19c360a9..981a9233fc 100644 --- a/toolsrc/src/vcpkg/install.cpp +++ b/toolsrc/src/vcpkg/install.cpp @@ -355,8 +355,7 @@ namespace vcpkg::Install { auto& fs = paths.get_filesystem(); const fs::path package_dir = paths.package_dir(action.spec); - std::error_code ec; - fs.remove_all(package_dir, ec); + fs.remove_all(package_dir, VCPKG_LINE_INFO); } if (action.build_options.clean_downloads == Build::CleanDownloads::YES) diff --git a/toolsrc/src/vcpkg/remove.cpp b/toolsrc/src/vcpkg/remove.cpp index a40b27bd74..84ec6c981b 100644 --- a/toolsrc/src/vcpkg/remove.cpp +++ b/toolsrc/src/vcpkg/remove.cpp @@ -179,8 +179,7 @@ namespace vcpkg::Remove { System::printf("Purging package %s...\n", display_name); Files::Filesystem& fs = paths.get_filesystem(); - std::error_code ec; - fs.remove_all(paths.packages / action.spec.dir(), ec); + fs.remove_all(paths.packages / action.spec.dir(), VCPKG_LINE_INFO); System::printf(System::Color::success, "Purging package %s... done\n", display_name); } } From a0fe40ea5842006c0da901a99ae07d7a25531175 Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Thu, 11 Jul 2019 18:16:10 -0700 Subject: [PATCH 09/17] add tests! Also, fix all the bugs I found when I wrote the tests! --- toolsrc/include/vcpkg/base/files.h | 35 ++-- toolsrc/include/vcpkg/base/strings.h | 10 +- toolsrc/include/vcpkg/base/work_queue.h | 182 ++++++++++-------- toolsrc/src/tests.files.cpp | 80 ++++++++ toolsrc/src/tests.strings.cpp | 38 ++++ toolsrc/src/vcpkg/base/files.cpp | 3 +- toolsrc/src/vcpkg/base/strings.cpp | 62 +++--- toolsrc/vcpkg/vcpkg.vcxproj | 4 +- toolsrc/vcpkglib/vcpkglib.vcxproj | 4 +- .../vcpkgmetricsuploader.vcxproj | 4 +- toolsrc/vcpkgtest/vcpkgtest.vcxproj | 10 +- toolsrc/vcpkgtest/vcpkgtest.vcxproj.filters | 6 + 12 files changed, 290 insertions(+), 148 deletions(-) create mode 100644 toolsrc/src/tests.files.cpp create mode 100644 toolsrc/src/tests.strings.cpp diff --git a/toolsrc/include/vcpkg/base/files.h b/toolsrc/include/vcpkg/base/files.h index 6cad3d4613..a5e04db251 100644 --- a/toolsrc/include/vcpkg/base/files.h +++ b/toolsrc/include/vcpkg/base/files.h @@ -12,8 +12,8 @@ namespace fs using stdfs::copy_options; using stdfs::file_status; using stdfs::file_type; - using stdfs::perms; using stdfs::path; + using stdfs::perms; using stdfs::u8path; /* @@ -26,25 +26,24 @@ namespace fs // we want to poison ADL with these niebloids - namespace detail { - struct symlink_status_t { + namespace detail + { + struct symlink_status_t + { file_status operator()(const path& p, std::error_code& ec) const noexcept; file_status operator()(const path& p, vcpkg::LineInfo li) const noexcept; }; - struct is_symlink_t { - inline bool operator()(file_status s) const { - return stdfs::is_symlink(s); - } + struct is_symlink_t + { + inline bool operator()(file_status s) const { return stdfs::is_symlink(s); } }; - struct is_regular_file_t { - inline bool operator()(file_status s) const { - return stdfs::is_regular_file(s); - } + struct is_regular_file_t + { + inline bool operator()(file_status s) const { return stdfs::is_regular_file(s); } }; - struct is_directory_t { - inline bool operator()(file_status s) const { - return stdfs::is_directory(s); - } + struct is_directory_t + { + inline bool operator()(file_status s) const { return stdfs::is_directory(s); } }; } @@ -63,10 +62,10 @@ namespace fs We also want to poison the ADL on is_regular_file and is_directory, because we don't want people calling these functions on paths */ -using fs::symlink_status; -using fs::is_symlink; -using fs::is_regular_file; using fs::is_directory; +using fs::is_regular_file; +using fs::is_symlink; +using fs::symlink_status; namespace vcpkg::Files { diff --git a/toolsrc/include/vcpkg/base/strings.h b/toolsrc/include/vcpkg/base/strings.h index 625c0240fd..a1906790f0 100644 --- a/toolsrc/include/vcpkg/base/strings.h +++ b/toolsrc/include/vcpkg/base/strings.h @@ -188,13 +188,5 @@ namespace vcpkg::Strings // base 64 encoding with URL and filesafe alphabet (base64url) // based on IETF RFC 4648 // ignores padding, since one implicitly knows the length from the size of x - namespace detail { - - struct b64url_encode_t { - std::string operator()(std::uint64_t x) const noexcept; - }; - - } - - constexpr detail::b64url_encode_t b64url_encode{}; + std::string b64url_encode(std::uint64_t x) noexcept; } diff --git a/toolsrc/include/vcpkg/base/work_queue.h b/toolsrc/include/vcpkg/base/work_queue.h index 1836404ca6..d6666770b9 100644 --- a/toolsrc/include/vcpkg/base/work_queue.h +++ b/toolsrc/include/vcpkg/base/work_queue.h @@ -4,78 +4,67 @@ #include #include -namespace vcpkg { - template +namespace vcpkg +{ + template struct WorkQueue; - namespace detail { + namespace detail + { // for SFINAE purposes, keep out of the class - template - auto call_moved_action( - Action& action, - const WorkQueue& work_queue, - ThreadLocalData& tld - ) -> decltype(static_cast(std::move(action)(tld, work_queue))) + template + auto call_moved_action(Action& action, + const WorkQueue& work_queue, + ThreadLocalData& tld) -> decltype(static_cast(std::move(action)(tld, work_queue))) { std::move(action)(tld, work_queue); } - template - auto call_moved_action( - Action& action, - const WorkQueue&, - ThreadLocalData& tld - ) -> decltype(static_cast(std::move(action)(tld))) + template + auto call_moved_action(Action& action, const WorkQueue&, ThreadLocalData& tld) + -> decltype(static_cast(std::move(action)(tld))) { std::move(action)(tld); } - - struct immediately_run_t {}; } - constexpr detail::immediately_run_t immediately_run{}; - - - template - struct WorkQueue { - template - WorkQueue(std::size_t num_threads, LineInfo li, const F& tld_init) noexcept { + template + struct WorkQueue + { + template + WorkQueue(std::uint16_t num_threads, LineInfo li, const F& tld_init) noexcept + { m_line_info = li; - m_state = State::BeforeRun; + m_unjoined_workers = num_threads; m_threads.reserve(num_threads); - for (std::size_t i = 0; i < num_threads; ++i) { + for (std::size_t i = 0; i < num_threads; ++i) + { m_threads.push_back(std::thread(Worker{this, tld_init()})); } } - template - WorkQueue( - detail::immediately_run_t, - std::size_t num_threads, - LineInfo li, - const F& tld_init - ) noexcept : WorkQueue(num_threads, li, tld_init) { - m_state = State::Running; - } - WorkQueue(WorkQueue const&) = delete; WorkQueue(WorkQueue&&) = delete; - ~WorkQueue() { + ~WorkQueue() + { auto lck = std::unique_lock(m_mutex); - if (m_state == State::Running) { + if (!is_joined(m_state)) + { Checks::exit_with_message(m_line_info, "Failed to call join() on a WorkQueue that was destroyed"); } } // should only be called once; anything else is an error - void run(LineInfo li) { + void run(LineInfo li) + { // this should _not_ be locked before `run()` is called; however, we // want to terminate if someone screws up, rather than cause UB auto lck = std::unique_lock(m_mutex); - if (m_state != State::BeforeRun) { + if (m_state != State::BeforeRun) + { Checks::exit_with_message(li, "Attempted to run() twice"); } @@ -86,18 +75,40 @@ namespace vcpkg { // if this is called in an existing task, _will block forever_ // DO NOT DO THAT // thread-unsafe - void join(LineInfo li) { + void join(LineInfo li) + { { auto lck = std::unique_lock(m_mutex); - if (is_joined(m_state)) { + if (is_joined(m_state)) + { Checks::exit_with_message(li, "Attempted to call join() more than once"); - } else if (m_state == State::Terminated) { + } + else if (m_state == State::Terminated) + { m_state = State::TerminatedJoined; - } else { + } + else + { m_state = State::Joined; } } - for (auto& thrd : m_threads) { + + for (;;) + { + auto lck = std::unique_lock(m_mutex); + if (!m_unjoined_workers) + break; + + else if (!m_running_workers) + { + lck.unlock(); + m_cv.notify_all(); + } + } + + // all threads have returned -- now, it's time to join them + for (auto& thrd : m_threads) + { thrd.join(); } } @@ -105,19 +116,24 @@ namespace vcpkg { // useful in the case of errors // doesn't stop any existing running tasks // returns immediately, so that one can call this in a task - void terminate() const { + void terminate() const + { { auto lck = std::unique_lock(m_mutex); - if (is_joined(m_state)) { + if (is_joined(m_state)) + { m_state = State::TerminatedJoined; - } else { + } + else + { m_state = State::Terminated; } } m_cv.notify_all(); } - void enqueue_action(Action a) const { + void enqueue_action(Action a) const + { { auto lck = std::unique_lock(m_mutex); m_actions.push_back(std::move(a)); @@ -127,8 +143,9 @@ namespace vcpkg { m_cv.notify_one(); } - template - void enqueue_all_actions_by_move(Rng&& rng) const { + template + void enqueue_all_actions_by_move(Rng&& rng) const + { { using std::begin; using std::end; @@ -148,8 +165,9 @@ namespace vcpkg { m_cv.notify_all(); } - template - void enqueue_all_actions(Rng&& rng) const { + template + void enqueue_all_actions(Rng&& rng) const + { { using std::begin; using std::end; @@ -170,37 +188,41 @@ namespace vcpkg { } private: - struct Worker { + struct Worker + { const WorkQueue* work_queue; ThreadLocalData tld; - void operator()() { + void operator()() + { // unlocked when waiting, or when in the action // locked otherwise auto lck = std::unique_lock(work_queue->m_mutex); - work_queue->m_cv.wait(lck, [&] { - return work_queue->m_state != State::BeforeRun; - }); + work_queue->m_cv.wait(lck, [&] { return work_queue->m_state != State::BeforeRun; }); - for (;;) { + for (;;) + { const auto state = work_queue->m_state; - if (is_terminated(state)) { - return; + if (is_terminated(state)) + { + break; } - if (work_queue->m_actions.empty()) { - if (state == State::Running || work_queue->running_workers > 0) { - --work_queue->running_workers; + if (work_queue->m_actions.empty()) + { + if (state == State::Running || work_queue->m_running_workers > 1) + { + --work_queue->m_running_workers; work_queue->m_cv.wait(lck); - ++work_queue->running_workers; + ++work_queue->m_running_workers; continue; } // the queue isn't running, and we are the only worker // no more work! - return; + break; } Action action = std::move(work_queue->m_actions.back()); @@ -210,10 +232,13 @@ namespace vcpkg { detail::call_moved_action(action, *work_queue, tld); lck.lock(); } + + --work_queue->m_unjoined_workers; } }; - enum class State : std::int16_t { + enum class State : std::int16_t + { // can only exist upon construction BeforeRun = -1, @@ -223,22 +248,19 @@ namespace vcpkg { TerminatedJoined, }; - static bool is_terminated(State st) { - return st == State::Terminated || st == State::TerminatedJoined; - } + static bool is_terminated(State st) { return st == State::Terminated || st == State::TerminatedJoined; } - static bool is_joined(State st) { - return st != State::Joined || st == State::TerminatedJoined; - } + static bool is_joined(State st) { return st == State::Joined || st == State::TerminatedJoined; } - mutable std::mutex m_mutex; - // these four are under m_mutex - mutable State m_state; - mutable std::uint16_t running_workers; - mutable std::vector m_actions; - mutable std::condition_variable m_cv; + mutable std::mutex m_mutex{}; + // these are all under m_mutex + mutable State m_state = State::BeforeRun; + mutable std::uint16_t m_running_workers = 0; + mutable std::uint16_t m_unjoined_workers = 0; // num_threads + mutable std::vector m_actions{}; + mutable std::condition_variable m_cv{}; - std::vector m_threads; + std::vector m_threads{}; LineInfo m_line_info; }; } diff --git a/toolsrc/src/tests.files.cpp b/toolsrc/src/tests.files.cpp new file mode 100644 index 0000000000..73c7eb5bca --- /dev/null +++ b/toolsrc/src/tests.files.cpp @@ -0,0 +1,80 @@ +#include "tests.pch.h" + +#include +#include + +#include +#include + +#include + +using namespace Microsoft::VisualStudio::CppUnitTestFramework; + +namespace UnitTest1 { + class FilesTest : public TestClass { + using uid = std::uniform_int_distribution; + + std::string get_random_filename() + { + std::random_device rd; + return vcpkg::Strings::b64url_encode(uid{}(rd)); + } + + void create_directory_tree( + vcpkg::Files::Filesystem& fs, + std::uint64_t depth, + const fs::path& base) + { + std::random_device rd; + constexpr auto max_depth = std::uint64_t(3); + const auto width = depth ? uid{0, (max_depth - depth) * 3 / 2}(rd) : 5; + + std::error_code ec; + if (width == 0) { + fs.write_contents(base, "", ec); + Assert::IsFalse(bool(ec)); + + return; + } + + fs.create_directory(base, ec); + Assert::IsFalse(bool(ec)); + + for (int i = 0; i < width; ++i) { + create_directory_tree(fs, depth + 1, base / get_random_filename()); + } + } + + TEST_METHOD(remove_all) { + fs::path temp_dir; + + { + wchar_t* tmp = static_cast(calloc(32'767, 2)); + + if (!GetEnvironmentVariableW(L"TEMP", tmp, 32'767)) { + Assert::Fail(L"GetEnvironmentVariable(\"TEMP\") failed"); + } + + temp_dir = tmp; + + std::string dir_name = "vcpkg-tmp-dir-"; + dir_name += get_random_filename(); + + temp_dir /= dir_name; + } + + auto& fs = vcpkg::Files::get_real_filesystem(); + + std::cout << "temp dir is: " << temp_dir << '\n'; + + std::error_code ec; + create_directory_tree(fs, 0, temp_dir); + + fs::path fp; + fs.remove_all(temp_dir, ec, fp); + Assert::IsFalse(bool(ec)); + + Assert::IsFalse(fs.exists(temp_dir)); + } + }; +} diff --git a/toolsrc/src/tests.strings.cpp b/toolsrc/src/tests.strings.cpp new file mode 100644 index 0000000000..14b449e861 --- /dev/null +++ b/toolsrc/src/tests.strings.cpp @@ -0,0 +1,38 @@ +#include "tests.pch.h" + +#include + +#include +#include +#include + +using namespace Microsoft::VisualStudio::CppUnitTestFramework; + +namespace UnitTest1 { + class StringsTest : public TestClass { + TEST_METHOD(b64url_encode) + { + using u64 = std::uint64_t; + + std::vector> map; + + map.emplace_back(0, "AAAAAAAAAAA"); + map.emplace_back(1, "BAAAAAAAAAA"); + + map.emplace_back(u64(1) << 32, "AAAAAEAAAAA"); + map.emplace_back((u64(1) << 32) + 1, "BAAAAEAAAAA"); + + map.emplace_back(0xE4D0'1065'D11E'0229, "pIgHRXGEQTO"); + map.emplace_back(0xA626'FE45'B135'07FF, "_fQNxWk_mYK"); + map.emplace_back(0xEE36'D228'0C31'D405, "FQdMMgi024O"); + map.emplace_back(0x1405'64E7'FE7E'A88C, "Miqf-fOZFQB"); + map.emplace_back(0xFFFF'FFFF'FFFF'FFFF, "__________P"); + + std::string result; + for (const auto& pr : map) { + result = vcpkg::Strings::b64url_encode(pr.first); + Assert::AreEqual(result, pr.second); + } + } + }; +} diff --git a/toolsrc/src/vcpkg/base/files.cpp b/toolsrc/src/vcpkg/base/files.cpp index a4e67a1421..4e36fe9901 100644 --- a/toolsrc/src/vcpkg/base/files.cpp +++ b/toolsrc/src/vcpkg/base/files.cpp @@ -23,7 +23,7 @@ namespace fs::detail { - file_status symlink_status_t::operator()(const path& p, std::error_code& ec) const noexcept + file_status symlink_status_t::operator()(const path& p, std::error_code&) const noexcept { #if defined(_WIN32) /* @@ -406,6 +406,7 @@ namespace vcpkg::Files if (!info.ec) { info.ec = ec; + info.failure_point = failure_point; } return true; diff --git a/toolsrc/src/vcpkg/base/strings.cpp b/toolsrc/src/vcpkg/base/strings.cpp index 16543046ea..ade4384a97 100644 --- a/toolsrc/src/vcpkg/base/strings.cpp +++ b/toolsrc/src/vcpkg/base/strings.cpp @@ -289,44 +289,46 @@ bool Strings::contains(StringView haystack, StringView needle) return Strings::search(haystack, needle) != haystack.end(); } -namespace vcpkg::Strings::detail { +namespace vcpkg::Strings +{ + namespace + { + template + std::string b64url_encode_implementation(Integral x) + { + static_assert(std::is_integral::value, "b64url_encode must take an integer type"); + using Unsigned = std::make_unsigned_t; + auto value = static_cast(x); - template - std::string b64url_encode_implementation(Integral x) { - static_assert(std::is_integral::value, "b64url_encode must take an integer type"); - using Unsigned = std::make_unsigned_t; - auto value = static_cast(x); + // 64 values, plus the implicit \0 + constexpr static char map[65] = + /* 0123456789ABCDEF */ + /*0*/ "ABCDEFGHIJKLMNOP" + /*1*/ "QRSTUVWXYZabcdef" + /*2*/ "ghijklmnopqrstuv" + /*3*/ "wxyz0123456789-_"; - // 64 values, plus the implicit \0 - constexpr static char map[0x41] = - /* 0123456789ABCDEF */ - /*0*/ "ABCDEFGHIJKLMNOP" - /*1*/ "QRSTUVWXYZabcdef" - /*2*/ "ghijklmnopqrstuv" - /*3*/ "wxyz0123456789-_" - ; + // log2(64) + constexpr static int shift = 6; + // 64 - 1 + constexpr static auto mask = 63; - constexpr static int shift = 5; - constexpr static auto mask = (static_cast(1) << shift) - 1; + // ceiling(bitsize(Integral) / log2(64)) + constexpr static auto result_size = (sizeof(value) * 8 + shift - 1) / shift; - std::string result; - // reserve ceiling(number of bits / 3) - result.resize((sizeof(value) * 8 + 2) / 3, map[0]); + std::string result; + result.reserve(result_size); - for (char& c: result) { - if (value == 0) { - break; + for (std::size_t i = 0; i < result_size; ++i) + { + result.push_back(map[value & mask]); + value >>= shift; } - c = map[value & mask]; - value >>= shift; + + return result; } - - return result; } - std::string b64url_encode_t::operator()(std::uint64_t x) const noexcept{ - return b64url_encode_implementation(x); - } + std::string b64url_encode(std::uint64_t x) noexcept { return b64url_encode_implementation(x); } } - diff --git a/toolsrc/vcpkg/vcpkg.vcxproj b/toolsrc/vcpkg/vcpkg.vcxproj index 8edea2244f..06d849a740 100644 --- a/toolsrc/vcpkg/vcpkg.vcxproj +++ b/toolsrc/vcpkg/vcpkg.vcxproj @@ -21,8 +21,8 @@ {34671B80-54F9-46F5-8310-AC429C11D4FB} vcpkg - 8.1 - v140 + 10.0 + v142 diff --git a/toolsrc/vcpkglib/vcpkglib.vcxproj b/toolsrc/vcpkglib/vcpkglib.vcxproj index 2eff1abee2..9312dd627d 100644 --- a/toolsrc/vcpkglib/vcpkglib.vcxproj +++ b/toolsrc/vcpkglib/vcpkglib.vcxproj @@ -21,8 +21,8 @@ {B98C92B7-2874-4537-9D46-D14E5C237F04} vcpkglib - 8.1 - v140 + 10.0 + v142 diff --git a/toolsrc/vcpkgmetricsuploader/vcpkgmetricsuploader.vcxproj b/toolsrc/vcpkgmetricsuploader/vcpkgmetricsuploader.vcxproj index e533d0e15d..2e689c7fca 100644 --- a/toolsrc/vcpkgmetricsuploader/vcpkgmetricsuploader.vcxproj +++ b/toolsrc/vcpkgmetricsuploader/vcpkgmetricsuploader.vcxproj @@ -21,8 +21,8 @@ {7D6FDEEB-B299-4A23-85EE-F67C4DED47BE} vcpkgmetricsuploader - 8.1 - v140 + 10.0 + v142 diff --git a/toolsrc/vcpkgtest/vcpkgtest.vcxproj b/toolsrc/vcpkgtest/vcpkgtest.vcxproj index 4cda294618..ac7675ba5e 100644 --- a/toolsrc/vcpkgtest/vcpkgtest.vcxproj +++ b/toolsrc/vcpkgtest/vcpkgtest.vcxproj @@ -22,6 +22,7 @@ + @@ -32,6 +33,7 @@ + @@ -48,8 +50,8 @@ {F27B8DB0-1279-4AF8-A2E3-1D49C4F0220D} Win32Proj vcpkgtest - 8.1 - v140 + 10.0 + v142 @@ -84,7 +86,7 @@ - + $(SolutionDir)msbuild.x86.debug\$(ProjectName)\ $(SolutionDir)msbuild.x86.debug\ @@ -193,4 +195,4 @@ - \ No newline at end of file + diff --git a/toolsrc/vcpkgtest/vcpkgtest.vcxproj.filters b/toolsrc/vcpkgtest/vcpkgtest.vcxproj.filters index 0f799426e2..057c50a992 100644 --- a/toolsrc/vcpkgtest/vcpkgtest.vcxproj.filters +++ b/toolsrc/vcpkgtest/vcpkgtest.vcxproj.filters @@ -45,6 +45,12 @@ Source Files + + Source Files + + + Source Files + From 771e23c665f1960ef4e7fd9816e0d21c943a7903 Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Thu, 11 Jul 2019 18:26:42 -0700 Subject: [PATCH 10/17] forgot to test on macos >.< --- toolsrc/src/vcpkg/base/files.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/toolsrc/src/vcpkg/base/files.cpp b/toolsrc/src/vcpkg/base/files.cpp index 4e36fe9901..e40822705a 100644 --- a/toolsrc/src/vcpkg/base/files.cpp +++ b/toolsrc/src/vcpkg/base/files.cpp @@ -23,9 +23,11 @@ namespace fs::detail { - file_status symlink_status_t::operator()(const path& p, std::error_code&) const noexcept + file_status symlink_status_t::operator()(const path& p, std::error_code& ec) const noexcept { #if defined(_WIN32) + static_cast(ec); + /* do not find the permissions of the file -- it's unnecessary for the things that vcpkg does. From 02c977186e890e4090d91c171b42d20729e668af Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Mon, 15 Jul 2019 16:43:55 -0700 Subject: [PATCH 11/17] modify files test to include symlinks --- toolsrc/src/tests.files.cpp | 147 ++++++++++++++++++++++++------------ 1 file changed, 100 insertions(+), 47 deletions(-) diff --git a/toolsrc/src/tests.files.cpp b/toolsrc/src/tests.files.cpp index 73c7eb5bca..c99dbb286c 100644 --- a/toolsrc/src/tests.files.cpp +++ b/toolsrc/src/tests.files.cpp @@ -10,28 +10,112 @@ using namespace Microsoft::VisualStudio::CppUnitTestFramework; -namespace UnitTest1 { - class FilesTest : public TestClass { +namespace UnitTest1 +{ + class FilesTest : public TestClass + { using uid = std::uniform_int_distribution; - - std::string get_random_filename() + + public: + FilesTest() { - std::random_device rd; - return vcpkg::Strings::b64url_encode(uid{}(rd)); + HKEY key; + const auto status = RegOpenKeyExW( + HKEY_LOCAL_MACHINE, LR"(SOFTWARE\Microsoft\Windows\CurrentVersion\AppModelUnlock)", 0, KEY_READ, &key); + + if (!status) + { + ALLOW_SYMLINKS = false; + std::clog << "Symlinks are not allowed on this system\n"; + } + else + { + ALLOW_SYMLINKS = true; + RegCloseKey(key); + } } - - void create_directory_tree( - vcpkg::Files::Filesystem& fs, - std::uint64_t depth, - const fs::path& base) + + private: + TEST_METHOD(remove_all) + { + auto urbg = get_urbg(0); + + fs::path temp_dir; + + { + wchar_t* tmp = static_cast(calloc(32'767, 2)); + + if (!GetEnvironmentVariableW(L"TEMP", tmp, 32'767)) + { + Assert::Fail(L"GetEnvironmentVariable(\"TEMP\") failed"); + } + + temp_dir = tmp; + + std::string dir_name = "vcpkg-tmp-dir-"; + dir_name += get_random_filename(urbg); + + temp_dir /= dir_name; + } + + auto& fs = vcpkg::Files::get_real_filesystem(); + + std::clog << "temp dir is: " << temp_dir << '\n'; + + std::error_code ec; + create_directory_tree(urbg, fs, 0, temp_dir); + + fs::path fp; + fs.remove_all(temp_dir, ec, fp); + Assert::IsFalse(bool(ec)); + + Assert::IsFalse(fs.exists(temp_dir)); + } + + bool ALLOW_SYMLINKS; + + std::mt19937_64 get_urbg(std::uint64_t index) + { + // smallest prime > 2**63 - 1 + return std::mt19937_64{index + 9223372036854775837}; + } + + std::string get_random_filename(std::mt19937_64& urbg) { return vcpkg::Strings::b64url_encode(uid{}(urbg)); } + + void create_directory_tree(std::mt19937_64& urbg, + vcpkg::Files::Filesystem& fs, + std::uint64_t depth, + const fs::path& base) { std::random_device rd; constexpr auto max_depth = std::uint64_t(3); - const auto width = depth ? uid{0, (max_depth - depth) * 3 / 2}(rd) : 5; + const auto width = depth ? uid{0, (max_depth - depth) * 3 / 2}(urbg) : 5; std::error_code ec; - if (width == 0) { - fs.write_contents(base, "", ec); + if (width == 0) + { + // I don't want to move urbg forward conditionally + const auto type = uid{0, 3}(urbg); + if (type == 0 || !ALLOW_SYMLINKS) + { + // 0 is a regular file + fs.write_contents(base, "", ec); + } + else if (type == 1) + { + // 1 is a regular symlink + fs.write_contents(base, "", ec); + Assert::IsFalse(bool(ec)); + fs::path base_link = base; + base_link.append("-link"); + fs::stdfs::create_symlink(base, base_link, ec); + } + else + { + // 2 is a directory symlink + fs::stdfs::create_directory_symlink(".", base, ec); + } + Assert::IsFalse(bool(ec)); return; @@ -40,41 +124,10 @@ namespace UnitTest1 { fs.create_directory(base, ec); Assert::IsFalse(bool(ec)); - for (int i = 0; i < width; ++i) { - create_directory_tree(fs, depth + 1, base / get_random_filename()); - } - } - - TEST_METHOD(remove_all) { - fs::path temp_dir; - + for (int i = 0; i < width; ++i) { - wchar_t* tmp = static_cast(calloc(32'767, 2)); - - if (!GetEnvironmentVariableW(L"TEMP", tmp, 32'767)) { - Assert::Fail(L"GetEnvironmentVariable(\"TEMP\") failed"); - } - - temp_dir = tmp; - - std::string dir_name = "vcpkg-tmp-dir-"; - dir_name += get_random_filename(); - - temp_dir /= dir_name; + create_directory_tree(urbg, fs, depth + 1, base / get_random_filename(urbg)); } - - auto& fs = vcpkg::Files::get_real_filesystem(); - - std::cout << "temp dir is: " << temp_dir << '\n'; - - std::error_code ec; - create_directory_tree(fs, 0, temp_dir); - - fs::path fp; - fs.remove_all(temp_dir, ec, fp); - Assert::IsFalse(bool(ec)); - - Assert::IsFalse(fs.exists(temp_dir)); } }; } From 65d34c5e55ef30a6572dfb3b79d212196fbadc0d Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Mon, 15 Jul 2019 18:51:03 -0700 Subject: [PATCH 12/17] wheeeee more fixes --- toolsrc/include/vcpkg/base/strings.h | 8 +-- toolsrc/include/vcpkg/base/work_queue.h | 89 ++++++++----------------- toolsrc/src/tests.files.cpp | 64 +++++++++++------- toolsrc/src/tests.strings.cpp | 20 +++--- toolsrc/src/vcpkg/base/files.cpp | 2 +- toolsrc/src/vcpkg/base/strings.cpp | 23 +++---- 6 files changed, 90 insertions(+), 116 deletions(-) diff --git a/toolsrc/include/vcpkg/base/strings.h b/toolsrc/include/vcpkg/base/strings.h index a1906790f0..aa4c4d6900 100644 --- a/toolsrc/include/vcpkg/base/strings.h +++ b/toolsrc/include/vcpkg/base/strings.h @@ -185,8 +185,8 @@ namespace vcpkg::Strings bool contains(StringView haystack, StringView needle); - // base 64 encoding with URL and filesafe alphabet (base64url) - // based on IETF RFC 4648 - // ignores padding, since one implicitly knows the length from the size of x - std::string b64url_encode(std::uint64_t x) noexcept; + // base 32 encoding, since base64 encoding requires lowercase letters, + // which are not distinct from uppercase letters on macOS or Windows filesystems. + // follows RFC 4648 + std::string b32_encode(std::uint64_t x) noexcept; } diff --git a/toolsrc/include/vcpkg/base/work_queue.h b/toolsrc/include/vcpkg/base/work_queue.h index d6666770b9..69ca387f3a 100644 --- a/toolsrc/include/vcpkg/base/work_queue.h +++ b/toolsrc/include/vcpkg/base/work_queue.h @@ -36,7 +36,7 @@ namespace vcpkg { m_line_info = li; - m_unjoined_workers = num_threads; + set_unjoined_workers(num_threads); m_threads.reserve(num_threads); for (std::size_t i = 0; i < num_threads; ++i) { @@ -93,20 +93,16 @@ namespace vcpkg } } - for (;;) + while (unjoined_workers()) { - auto lck = std::unique_lock(m_mutex); - if (!m_unjoined_workers) - break; - - else if (!m_running_workers) + if (!running_workers()) { - lck.unlock(); - m_cv.notify_all(); - } + m_cv.notify_one(); + + } } - // all threads have returned -- now, it's time to join them + // wait for all threads to join for (auto& thrd : m_threads) { thrd.join(); @@ -143,50 +139,6 @@ namespace vcpkg m_cv.notify_one(); } - template - void enqueue_all_actions_by_move(Rng&& rng) const - { - { - using std::begin; - using std::end; - - auto lck = std::unique_lock(m_mutex); - - const auto first = begin(rng); - const auto last = end(rng); - - m_actions.reserve(m_actions.size() + (last - first)); - - std::move(first, last, std::back_inserter(rng)); - - if (m_state == State::BeforeRun) return; - } - - m_cv.notify_all(); - } - - template - void enqueue_all_actions(Rng&& rng) const - { - { - using std::begin; - using std::end; - - auto lck = std::unique_lock(m_mutex); - - const auto first = begin(rng); - const auto last = end(rng); - - m_actions.reserve(m_actions.size() + (last - first)); - - std::copy(first, last, std::back_inserter(rng)); - - if (m_state == State::BeforeRun) return; - } - - m_cv.notify_all(); - } - private: struct Worker { @@ -201,6 +153,7 @@ namespace vcpkg work_queue->m_cv.wait(lck, [&] { return work_queue->m_state != State::BeforeRun; }); + work_queue->increment_running_workers(); for (;;) { const auto state = work_queue->m_state; @@ -212,15 +165,15 @@ namespace vcpkg if (work_queue->m_actions.empty()) { - if (state == State::Running || work_queue->m_running_workers > 1) + if (state == State::Running || work_queue->running_workers() > 1) { - --work_queue->m_running_workers; + work_queue->decrement_running_workers(); work_queue->m_cv.wait(lck); - ++work_queue->m_running_workers; + work_queue->increment_running_workers(); continue; } - // the queue isn't running, and we are the only worker + // the queue is joining, and we are the only worker running // no more work! break; } @@ -229,11 +182,13 @@ namespace vcpkg work_queue->m_actions.pop_back(); lck.unlock(); + work_queue->m_cv.notify_one(); detail::call_moved_action(action, *work_queue, tld); lck.lock(); } - --work_queue->m_unjoined_workers; + work_queue->decrement_running_workers(); + work_queue->decrement_unjoined_workers(); } }; @@ -255,11 +210,21 @@ namespace vcpkg mutable std::mutex m_mutex{}; // these are all under m_mutex mutable State m_state = State::BeforeRun; - mutable std::uint16_t m_running_workers = 0; - mutable std::uint16_t m_unjoined_workers = 0; // num_threads mutable std::vector m_actions{}; mutable std::condition_variable m_cv{}; + mutable std::atomic m_workers; + // = unjoined_workers << 16 | running_workers + + void set_unjoined_workers(std::uint16_t threads) { m_workers = std::uint32_t(threads) << 16; } + void decrement_unjoined_workers() const { m_workers -= 1 << 16; } + + std::uint16_t unjoined_workers() const { return std::uint16_t(m_workers >> 16); } + + void increment_running_workers() const { ++m_workers; } + void decrement_running_workers() const { --m_workers; } + std::uint16_t running_workers() const { return std::uint16_t(m_workers); } + std::vector m_threads{}; LineInfo m_line_info; }; diff --git a/toolsrc/src/tests.files.cpp b/toolsrc/src/tests.files.cpp index c99dbb286c..56b0ceac6e 100644 --- a/toolsrc/src/tests.files.cpp +++ b/toolsrc/src/tests.files.cpp @@ -4,6 +4,7 @@ #include #include +#include // required for filesystem::create_{directory_}symlink #include #include @@ -21,18 +22,20 @@ namespace UnitTest1 { HKEY key; const auto status = RegOpenKeyExW( - HKEY_LOCAL_MACHINE, LR"(SOFTWARE\Microsoft\Windows\CurrentVersion\AppModelUnlock)", 0, KEY_READ, &key); + HKEY_LOCAL_MACHINE, LR"(SOFTWARE\Microsoft\Windows\CurrentVersion\AppModelUnlock)", 0, 0, &key); - if (!status) + if (status == ERROR_FILE_NOT_FOUND) { ALLOW_SYMLINKS = false; std::clog << "Symlinks are not allowed on this system\n"; } else { + // if we get a permissions error, we still know that we're in developer mode ALLOW_SYMLINKS = true; - RegCloseKey(key); } + + if (status == ERROR_SUCCESS) RegCloseKey(key); } private: @@ -62,9 +65,9 @@ namespace UnitTest1 std::clog << "temp dir is: " << temp_dir << '\n'; - std::error_code ec; create_directory_tree(urbg, fs, 0, temp_dir); + std::error_code ec; fs::path fp; fs.remove_all(temp_dir, ec, fp); Assert::IsFalse(bool(ec)); @@ -80,7 +83,7 @@ namespace UnitTest1 return std::mt19937_64{index + 9223372036854775837}; } - std::string get_random_filename(std::mt19937_64& urbg) { return vcpkg::Strings::b64url_encode(uid{}(urbg)); } + std::string get_random_filename(std::mt19937_64& urbg) { return vcpkg::Strings::b32_encode(uid{}(urbg)); } void create_directory_tree(std::mt19937_64& urbg, vcpkg::Files::Filesystem& fs, @@ -88,46 +91,57 @@ namespace UnitTest1 const fs::path& base) { std::random_device rd; - constexpr auto max_depth = std::uint64_t(3); - const auto width = depth ? uid{0, (max_depth - depth) * 3 / 2}(urbg) : 5; + constexpr std::uint64_t max_depth = 5; + constexpr std::uint64_t width = 5; + const auto type = depth < max_depth ? uid{0, 9}(urbg) : uid{7, 9}(urbg); + + // 0 <= type < 7 : directory + // 7 = type : regular + // 8 = type : regular symlink (regular file if !ALLOW_SYMLINKS) + // 9 = type : directory symlink (^^) std::error_code ec; - if (width == 0) + if (type >= 7) { // I don't want to move urbg forward conditionally - const auto type = uid{0, 3}(urbg); - if (type == 0 || !ALLOW_SYMLINKS) + if (type == 7 || !ALLOW_SYMLINKS) { - // 0 is a regular file + // regular file fs.write_contents(base, "", ec); } - else if (type == 1) + else if (type == 8) { - // 1 is a regular symlink + // regular symlink fs.write_contents(base, "", ec); Assert::IsFalse(bool(ec)); - fs::path base_link = base; - base_link.append("-link"); - fs::stdfs::create_symlink(base, base_link, ec); + const std::filesystem::path basep = base.native(); + auto basep_link = basep; + basep_link.replace_filename(basep.filename().native() + L"-link"); + std::filesystem::create_symlink(basep, basep_link, ec); } else { - // 2 is a directory symlink - fs::stdfs::create_directory_symlink(".", base, ec); + // directory symlink + std::filesystem::path basep = base.native(); + std::filesystem::create_directory_symlink(basep / "..", basep, ec); } Assert::IsFalse(bool(ec)); - return; } - - fs.create_directory(base, ec); - Assert::IsFalse(bool(ec)); - - for (int i = 0; i < width; ++i) + else { - create_directory_tree(urbg, fs, depth + 1, base / get_random_filename(urbg)); + // directory + fs.create_directory(base, ec); + Assert::IsFalse(bool(ec)); + + for (int i = 0; i < width; ++i) + { + create_directory_tree(urbg, fs, depth + 1, base / get_random_filename(urbg)); + } + } + } }; } diff --git a/toolsrc/src/tests.strings.cpp b/toolsrc/src/tests.strings.cpp index 14b449e861..6301ea05d7 100644 --- a/toolsrc/src/tests.strings.cpp +++ b/toolsrc/src/tests.strings.cpp @@ -16,21 +16,21 @@ namespace UnitTest1 { std::vector> map; - map.emplace_back(0, "AAAAAAAAAAA"); - map.emplace_back(1, "BAAAAAAAAAA"); + map.emplace_back(0, "AAAAAAAAAAAAA"); + map.emplace_back(1, "BAAAAAAAAAAAA"); - map.emplace_back(u64(1) << 32, "AAAAAEAAAAA"); - map.emplace_back((u64(1) << 32) + 1, "BAAAAEAAAAA"); + map.emplace_back(u64(1) << 32, "AAAAAAEAAAAAA"); + map.emplace_back((u64(1) << 32) + 1, "BAAAAAEAAAAAA"); - map.emplace_back(0xE4D0'1065'D11E'0229, "pIgHRXGEQTO"); - map.emplace_back(0xA626'FE45'B135'07FF, "_fQNxWk_mYK"); - map.emplace_back(0xEE36'D228'0C31'D405, "FQdMMgi024O"); - map.emplace_back(0x1405'64E7'FE7E'A88C, "Miqf-fOZFQB"); - map.emplace_back(0xFFFF'FFFF'FFFF'FFFF, "__________P"); + map.emplace_back(0xE4D0'1065'D11E'0229, "JRA4RIXMQAUJO"); + map.emplace_back(0xA626'FE45'B135'07FF, "77BKTYWI6XJMK"); + map.emplace_back(0xEE36'D228'0C31'D405, "FAVDDGAFSWN4O"); + map.emplace_back(0x1405'64E7'FE7E'A88C, "MEK5H774ELBIB"); + map.emplace_back(0xFFFF'FFFF'FFFF'FFFF, "777777777777P"); std::string result; for (const auto& pr : map) { - result = vcpkg::Strings::b64url_encode(pr.first); + result = vcpkg::Strings::b32_encode(pr.first); Assert::AreEqual(result, pr.second); } } diff --git a/toolsrc/src/vcpkg/base/files.cpp b/toolsrc/src/vcpkg/base/files.cpp index e40822705a..6c6945e44a 100644 --- a/toolsrc/src/vcpkg/base/files.cpp +++ b/toolsrc/src/vcpkg/base/files.cpp @@ -423,7 +423,7 @@ namespace vcpkg::Files { std::error_code ec; - const auto tmp_name = Strings::b64url_encode(info.index++); + const auto tmp_name = Strings::b32_encode(info.index++); const auto tmp_path = info.tmp_directory / tmp_name; fs::stdfs::rename(current_path, tmp_path, ec); diff --git a/toolsrc/src/vcpkg/base/strings.cpp b/toolsrc/src/vcpkg/base/strings.cpp index ade4384a97..7970e1b46c 100644 --- a/toolsrc/src/vcpkg/base/strings.cpp +++ b/toolsrc/src/vcpkg/base/strings.cpp @@ -294,26 +294,21 @@ namespace vcpkg::Strings namespace { template - std::string b64url_encode_implementation(Integral x) + std::string b32_encode_implementation(Integral x) { static_assert(std::is_integral::value, "b64url_encode must take an integer type"); using Unsigned = std::make_unsigned_t; auto value = static_cast(x); - // 64 values, plus the implicit \0 - constexpr static char map[65] = - /* 0123456789ABCDEF */ - /*0*/ "ABCDEFGHIJKLMNOP" - /*1*/ "QRSTUVWXYZabcdef" - /*2*/ "ghijklmnopqrstuv" - /*3*/ "wxyz0123456789-_"; + // 32 values, plus the implicit \0 + constexpr static char map[33] = "ABCDEFGHIJKLMNOP" "QRSTUVWXYZ234567"; - // log2(64) - constexpr static int shift = 6; - // 64 - 1 - constexpr static auto mask = 63; + // log2(32) + constexpr static int shift = 5; + // 32 - 1 + constexpr static auto mask = 31; - // ceiling(bitsize(Integral) / log2(64)) + // ceiling(bitsize(Integral) / log2(32)) constexpr static auto result_size = (sizeof(value) * 8 + shift - 1) / shift; std::string result; @@ -329,6 +324,6 @@ namespace vcpkg::Strings } } - std::string b64url_encode(std::uint64_t x) noexcept { return b64url_encode_implementation(x); } + std::string b32_encode(std::uint64_t x) noexcept { return b32_encode_implementation(x); } } From f599f19bad8fd97b60f41063537be2e4ecef3ca7 Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Wed, 17 Jul 2019 18:58:23 -0700 Subject: [PATCH 13/17] tests.files.cpp:create_directory_tree -- change magic numbers to names --- toolsrc/src/tests.files.cpp | 79 ++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 36 deletions(-) diff --git a/toolsrc/src/tests.files.cpp b/toolsrc/src/tests.files.cpp index 56b0ceac6e..e60662fd98 100644 --- a/toolsrc/src/tests.files.cpp +++ b/toolsrc/src/tests.files.cpp @@ -93,45 +93,31 @@ namespace UnitTest1 std::random_device rd; constexpr std::uint64_t max_depth = 5; constexpr std::uint64_t width = 5; - const auto type = depth < max_depth ? uid{0, 9}(urbg) : uid{7, 9}(urbg); - // 0 <= type < 7 : directory - // 7 = type : regular - // 8 = type : regular symlink (regular file if !ALLOW_SYMLINKS) - // 9 = type : directory symlink (^^) + // we want ~70% of our "files" to be directories, and then a third + // each of the remaining ~30% to be regular files, directory symlinks, + // and regular symlinks + constexpr std::uint64_t directory_min_tag = 0; + constexpr std::uint64_t directory_max_tag = 6; + constexpr std::uint64_t regular_file_tag = 7; + constexpr std::uint64_t regular_symlink_tag = 8; + constexpr std::uint64_t directory_symlink_tag = 9; + + // if we're at the max depth, we only want to build non-directories + std::uint64_t file_type; + if (depth < max_depth) { + file_type = uid{directory_min_tag, regular_symlink_tag}(urbg); + } else { + file_type = uid{regular_file_tag, regular_symlink_tag}(urbg); + } + + if (!ALLOW_SYMLINKS && file_type > regular_file_tag) { + file_type = regular_file_tag; + } std::error_code ec; - if (type >= 7) + if (type <= directory_max_tag) { - // I don't want to move urbg forward conditionally - if (type == 7 || !ALLOW_SYMLINKS) - { - // regular file - fs.write_contents(base, "", ec); - } - else if (type == 8) - { - // regular symlink - fs.write_contents(base, "", ec); - Assert::IsFalse(bool(ec)); - const std::filesystem::path basep = base.native(); - auto basep_link = basep; - basep_link.replace_filename(basep.filename().native() + L"-link"); - std::filesystem::create_symlink(basep, basep_link, ec); - } - else - { - // directory symlink - std::filesystem::path basep = base.native(); - std::filesystem::create_directory_symlink(basep / "..", basep, ec); - } - - Assert::IsFalse(bool(ec)); - - } - else - { - // directory fs.create_directory(base, ec); Assert::IsFalse(bool(ec)); @@ -139,9 +125,30 @@ namespace UnitTest1 { create_directory_tree(urbg, fs, depth + 1, base / get_random_filename(urbg)); } - + } + else if (type == regular_file_tag) + { + // regular file + fs.write_contents(base, "", ec); + } + else if (type == regular_symlink_tag) + { + // regular symlink + fs.write_contents(base, "", ec); + Assert::IsFalse(bool(ec)); + const std::filesystem::path basep = base.native(); + auto basep_link = basep; + basep_link.replace_filename(basep.filename().native() + L"-link"); + std::filesystem::create_symlink(basep, basep_link, ec); + } + else // type == directory_symlink_tag + { + // directory symlink + std::filesystem::path basep = base.native(); + std::filesystem::create_directory_symlink(basep / "..", basep, ec); } + Assert::IsFalse(bool(ec)); } }; } From fddebb75da034752fb267ba121497ba58157bb79 Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Thu, 18 Jul 2019 16:40:52 -0700 Subject: [PATCH 14/17] clang-format all the things --- toolsrc/include/vcpkg/base/strings.h | 4 ++-- toolsrc/include/vcpkg/base/work_queue.h | 3 +-- toolsrc/src/tests.files.cpp | 16 ++++++++++------ toolsrc/src/tests.strings.cpp | 9 ++++++--- toolsrc/src/vcpkg/base/strings.cpp | 3 ++- toolsrc/src/vcpkg/build.cpp | 15 ++++++--------- 6 files changed, 27 insertions(+), 23 deletions(-) diff --git a/toolsrc/include/vcpkg/base/strings.h b/toolsrc/include/vcpkg/base/strings.h index aa4c4d6900..d7de9b0b23 100644 --- a/toolsrc/include/vcpkg/base/strings.h +++ b/toolsrc/include/vcpkg/base/strings.h @@ -186,7 +186,7 @@ namespace vcpkg::Strings bool contains(StringView haystack, StringView needle); // base 32 encoding, since base64 encoding requires lowercase letters, - // which are not distinct from uppercase letters on macOS or Windows filesystems. - // follows RFC 4648 + // which are not distinct from uppercase letters on macOS or Windows filesystems. + // follows RFC 4648 std::string b32_encode(std::uint64_t x) noexcept; } diff --git a/toolsrc/include/vcpkg/base/work_queue.h b/toolsrc/include/vcpkg/base/work_queue.h index 69ca387f3a..70142e110c 100644 --- a/toolsrc/include/vcpkg/base/work_queue.h +++ b/toolsrc/include/vcpkg/base/work_queue.h @@ -98,8 +98,7 @@ namespace vcpkg if (!running_workers()) { m_cv.notify_one(); - - } + } } // wait for all threads to join diff --git a/toolsrc/src/tests.files.cpp b/toolsrc/src/tests.files.cpp index e60662fd98..482675c344 100644 --- a/toolsrc/src/tests.files.cpp +++ b/toolsrc/src/tests.files.cpp @@ -3,8 +3,8 @@ #include #include -#include #include // required for filesystem::create_{directory_}symlink +#include #include #include @@ -31,11 +31,11 @@ namespace UnitTest1 } else { - // if we get a permissions error, we still know that we're in developer mode + // if we get a permissions error, we still know that we're in developer mode ALLOW_SYMLINKS = true; } - if (status == ERROR_SUCCESS) RegCloseKey(key); + if (status == ERROR_SUCCESS) RegCloseKey(key); } private: @@ -105,13 +105,17 @@ namespace UnitTest1 // if we're at the max depth, we only want to build non-directories std::uint64_t file_type; - if (depth < max_depth) { + if (depth < max_depth) + { file_type = uid{directory_min_tag, regular_symlink_tag}(urbg); - } else { + } + else + { file_type = uid{regular_file_tag, regular_symlink_tag}(urbg); } - if (!ALLOW_SYMLINKS && file_type > regular_file_tag) { + if (!ALLOW_SYMLINKS && file_type > regular_file_tag) + { file_type = regular_file_tag; } diff --git a/toolsrc/src/tests.strings.cpp b/toolsrc/src/tests.strings.cpp index 6301ea05d7..f541a203d9 100644 --- a/toolsrc/src/tests.strings.cpp +++ b/toolsrc/src/tests.strings.cpp @@ -8,8 +8,10 @@ using namespace Microsoft::VisualStudio::CppUnitTestFramework; -namespace UnitTest1 { - class StringsTest : public TestClass { +namespace UnitTest1 +{ + class StringsTest : public TestClass + { TEST_METHOD(b64url_encode) { using u64 = std::uint64_t; @@ -29,7 +31,8 @@ namespace UnitTest1 { map.emplace_back(0xFFFF'FFFF'FFFF'FFFF, "777777777777P"); std::string result; - for (const auto& pr : map) { + for (const auto& pr : map) + { result = vcpkg::Strings::b32_encode(pr.first); Assert::AreEqual(result, pr.second); } diff --git a/toolsrc/src/vcpkg/base/strings.cpp b/toolsrc/src/vcpkg/base/strings.cpp index 7970e1b46c..46e78a3634 100644 --- a/toolsrc/src/vcpkg/base/strings.cpp +++ b/toolsrc/src/vcpkg/base/strings.cpp @@ -301,7 +301,8 @@ namespace vcpkg::Strings auto value = static_cast(x); // 32 values, plus the implicit \0 - constexpr static char map[33] = "ABCDEFGHIJKLMNOP" "QRSTUVWXYZ234567"; + constexpr static char map[33] = "ABCDEFGHIJKLMNOP" + "QRSTUVWXYZ234567"; // log2(32) constexpr static int shift = 5; diff --git a/toolsrc/src/vcpkg/build.cpp b/toolsrc/src/vcpkg/build.cpp index c50acce7e3..66d8dae2c2 100644 --- a/toolsrc/src/vcpkg/build.cpp +++ b/toolsrc/src/vcpkg/build.cpp @@ -363,8 +363,7 @@ namespace vcpkg::Build const Triplet& triplet = spec.triplet(); const auto& triplet_file_path = paths.get_triplet_file_path(spec.triplet()).u8string(); - if (!Strings::case_insensitive_ascii_starts_with(triplet_file_path, - paths.triplets.u8string())) + if (!Strings::case_insensitive_ascii_starts_with(triplet_file_path, paths.triplets.u8string())) { System::printf("-- Loading triplet configuration from: %s\n", triplet_file_path); } @@ -431,12 +430,11 @@ namespace vcpkg::Build } command.append(cmd_launch_cmake); const auto timer = Chrono::ElapsedTimer::create_started(); - const int return_code = System::cmd_execute_clean( - command, - {} + const int return_code = System::cmd_execute_clean(command, + {} #ifdef _WIN32 - , - powershell_exe_path.parent_path().u8string() + ";" + , + powershell_exe_path.parent_path().u8string() + ";" #endif ); const auto buildtimeus = timer.microseconds(); @@ -1020,8 +1018,7 @@ namespace vcpkg::Build hash += "-"; hash += Hash::get_file_hash(fs, *p, "SHA1"); } - else if (pre_build_info.cmake_system_name.empty() || - pre_build_info.cmake_system_name == "WindowsStore") + else if (pre_build_info.cmake_system_name.empty() || pre_build_info.cmake_system_name == "WindowsStore") { hash += "-"; hash += Hash::get_file_hash(fs, paths.scripts / "toolchains" / "windows.cmake", "SHA1"); From c55ea0a0d5229b9dd79aa8ea888f6c0408f9e5dd Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Fri, 19 Jul 2019 12:56:24 -0700 Subject: [PATCH 15/17] switch to new test framework --- toolsrc/CMakeLists.txt | 6 +- toolsrc/include/vcpkg-tests/util.h | 5 + toolsrc/src/tests.files.cpp | 158 ---------------------------- toolsrc/src/tests.strings.cpp | 41 -------- toolsrc/src/vcpkg-tests/files.cpp | 124 ++++++++++++++++++++++ toolsrc/src/vcpkg-tests/strings.cpp | 33 ++++++ toolsrc/src/vcpkg-tests/util.cpp | 54 ++++++++++ toolsrc/src/vcpkg/build.cpp | 48 --------- toolsrc/vcpkgtest/vcpkgtest.vcxproj | 2 + 9 files changed, 223 insertions(+), 248 deletions(-) delete mode 100644 toolsrc/src/tests.files.cpp delete mode 100644 toolsrc/src/tests.strings.cpp create mode 100644 toolsrc/src/vcpkg-tests/files.cpp create mode 100644 toolsrc/src/vcpkg-tests/strings.cpp diff --git a/toolsrc/CMakeLists.txt b/toolsrc/CMakeLists.txt index ca23db4131..6697a919e5 100644 --- a/toolsrc/CMakeLists.txt +++ b/toolsrc/CMakeLists.txt @@ -52,7 +52,11 @@ add_executable(vcpkg-test EXCLUDE_FROM_ALL ${VCPKGTEST_SOURCES} ${VCPKGLIB_SOURC target_compile_definitions(vcpkg-test PRIVATE -DDISABLE_METRICS=${DISABLE_METRICS_VALUE}) target_include_directories(vcpkg-test PRIVATE include) -foreach(TEST_NAME arguments chrono dependencies paragraph plan specifier supports) +foreach(TEST_NAME + arguments chrono dependencies files + paragraph plan specifier statusparagraphs + strings supports update +) add_test(${TEST_NAME} vcpkg-test [${TEST_NAME}]) endforeach() diff --git a/toolsrc/include/vcpkg-tests/util.h b/toolsrc/include/vcpkg-tests/util.h index fe4ee0eec9..8a0486285f 100644 --- a/toolsrc/include/vcpkg-tests/util.h +++ b/toolsrc/include/vcpkg-tests/util.h @@ -1,3 +1,4 @@ +#include #include #include @@ -30,4 +31,8 @@ T&& unwrap(vcpkg::Optional&& opt) return std::move(*opt.get()); } +extern const bool SYMLINKS_ALLOWED; + +extern const fs::path TEMPORARY_DIRECTORY; + } diff --git a/toolsrc/src/tests.files.cpp b/toolsrc/src/tests.files.cpp deleted file mode 100644 index 482675c344..0000000000 --- a/toolsrc/src/tests.files.cpp +++ /dev/null @@ -1,158 +0,0 @@ -#include "tests.pch.h" - -#include -#include - -#include // required for filesystem::create_{directory_}symlink -#include -#include - -#include - -using namespace Microsoft::VisualStudio::CppUnitTestFramework; - -namespace UnitTest1 -{ - class FilesTest : public TestClass - { - using uid = std::uniform_int_distribution; - - public: - FilesTest() - { - HKEY key; - const auto status = RegOpenKeyExW( - HKEY_LOCAL_MACHINE, LR"(SOFTWARE\Microsoft\Windows\CurrentVersion\AppModelUnlock)", 0, 0, &key); - - if (status == ERROR_FILE_NOT_FOUND) - { - ALLOW_SYMLINKS = false; - std::clog << "Symlinks are not allowed on this system\n"; - } - else - { - // if we get a permissions error, we still know that we're in developer mode - ALLOW_SYMLINKS = true; - } - - if (status == ERROR_SUCCESS) RegCloseKey(key); - } - - private: - TEST_METHOD(remove_all) - { - auto urbg = get_urbg(0); - - fs::path temp_dir; - - { - wchar_t* tmp = static_cast(calloc(32'767, 2)); - - if (!GetEnvironmentVariableW(L"TEMP", tmp, 32'767)) - { - Assert::Fail(L"GetEnvironmentVariable(\"TEMP\") failed"); - } - - temp_dir = tmp; - - std::string dir_name = "vcpkg-tmp-dir-"; - dir_name += get_random_filename(urbg); - - temp_dir /= dir_name; - } - - auto& fs = vcpkg::Files::get_real_filesystem(); - - std::clog << "temp dir is: " << temp_dir << '\n'; - - create_directory_tree(urbg, fs, 0, temp_dir); - - std::error_code ec; - fs::path fp; - fs.remove_all(temp_dir, ec, fp); - Assert::IsFalse(bool(ec)); - - Assert::IsFalse(fs.exists(temp_dir)); - } - - bool ALLOW_SYMLINKS; - - std::mt19937_64 get_urbg(std::uint64_t index) - { - // smallest prime > 2**63 - 1 - return std::mt19937_64{index + 9223372036854775837}; - } - - std::string get_random_filename(std::mt19937_64& urbg) { return vcpkg::Strings::b32_encode(uid{}(urbg)); } - - void create_directory_tree(std::mt19937_64& urbg, - vcpkg::Files::Filesystem& fs, - std::uint64_t depth, - const fs::path& base) - { - std::random_device rd; - constexpr std::uint64_t max_depth = 5; - constexpr std::uint64_t width = 5; - - // we want ~70% of our "files" to be directories, and then a third - // each of the remaining ~30% to be regular files, directory symlinks, - // and regular symlinks - constexpr std::uint64_t directory_min_tag = 0; - constexpr std::uint64_t directory_max_tag = 6; - constexpr std::uint64_t regular_file_tag = 7; - constexpr std::uint64_t regular_symlink_tag = 8; - constexpr std::uint64_t directory_symlink_tag = 9; - - // if we're at the max depth, we only want to build non-directories - std::uint64_t file_type; - if (depth < max_depth) - { - file_type = uid{directory_min_tag, regular_symlink_tag}(urbg); - } - else - { - file_type = uid{regular_file_tag, regular_symlink_tag}(urbg); - } - - if (!ALLOW_SYMLINKS && file_type > regular_file_tag) - { - file_type = regular_file_tag; - } - - std::error_code ec; - if (type <= directory_max_tag) - { - fs.create_directory(base, ec); - Assert::IsFalse(bool(ec)); - - for (int i = 0; i < width; ++i) - { - create_directory_tree(urbg, fs, depth + 1, base / get_random_filename(urbg)); - } - } - else if (type == regular_file_tag) - { - // regular file - fs.write_contents(base, "", ec); - } - else if (type == regular_symlink_tag) - { - // regular symlink - fs.write_contents(base, "", ec); - Assert::IsFalse(bool(ec)); - const std::filesystem::path basep = base.native(); - auto basep_link = basep; - basep_link.replace_filename(basep.filename().native() + L"-link"); - std::filesystem::create_symlink(basep, basep_link, ec); - } - else // type == directory_symlink_tag - { - // directory symlink - std::filesystem::path basep = base.native(); - std::filesystem::create_directory_symlink(basep / "..", basep, ec); - } - - Assert::IsFalse(bool(ec)); - } - }; -} diff --git a/toolsrc/src/tests.strings.cpp b/toolsrc/src/tests.strings.cpp deleted file mode 100644 index f541a203d9..0000000000 --- a/toolsrc/src/tests.strings.cpp +++ /dev/null @@ -1,41 +0,0 @@ -#include "tests.pch.h" - -#include - -#include -#include -#include - -using namespace Microsoft::VisualStudio::CppUnitTestFramework; - -namespace UnitTest1 -{ - class StringsTest : public TestClass - { - TEST_METHOD(b64url_encode) - { - using u64 = std::uint64_t; - - std::vector> map; - - map.emplace_back(0, "AAAAAAAAAAAAA"); - map.emplace_back(1, "BAAAAAAAAAAAA"); - - map.emplace_back(u64(1) << 32, "AAAAAAEAAAAAA"); - map.emplace_back((u64(1) << 32) + 1, "BAAAAAEAAAAAA"); - - map.emplace_back(0xE4D0'1065'D11E'0229, "JRA4RIXMQAUJO"); - map.emplace_back(0xA626'FE45'B135'07FF, "77BKTYWI6XJMK"); - map.emplace_back(0xEE36'D228'0C31'D405, "FAVDDGAFSWN4O"); - map.emplace_back(0x1405'64E7'FE7E'A88C, "MEK5H774ELBIB"); - map.emplace_back(0xFFFF'FFFF'FFFF'FFFF, "777777777777P"); - - std::string result; - for (const auto& pr : map) - { - result = vcpkg::Strings::b32_encode(pr.first); - Assert::AreEqual(result, pr.second); - } - } - }; -} diff --git a/toolsrc/src/vcpkg-tests/files.cpp b/toolsrc/src/vcpkg-tests/files.cpp new file mode 100644 index 0000000000..d2edffcfff --- /dev/null +++ b/toolsrc/src/vcpkg-tests/files.cpp @@ -0,0 +1,124 @@ +#include +#include + +#include +#include + +#include // required for filesystem::create_{directory_}symlink +#include +#include + +#include + +using vcpkg::Test::SYMLINKS_ALLOWED; +using vcpkg::Test::TEMPORARY_DIRECTORY; + +namespace +{ + using uid = std::uniform_int_distribution; + + std::mt19937_64 get_urbg(std::uint64_t index) + { + // smallest prime > 2**63 - 1 + return std::mt19937_64{index + 9223372036854775837}; + } + + std::string get_random_filename(std::mt19937_64& urbg) { return vcpkg::Strings::b32_encode(uid{}(urbg)); } + + void create_directory_tree(std::mt19937_64& urbg, + vcpkg::Files::Filesystem& fs, + std::uint64_t depth, + const fs::path& base) + { + std::random_device rd; + constexpr std::uint64_t max_depth = 5; + constexpr std::uint64_t width = 5; + + // we want ~70% of our "files" to be directories, and then a third + // each of the remaining ~30% to be regular files, directory symlinks, + // and regular symlinks + constexpr std::uint64_t directory_min_tag = 0; + constexpr std::uint64_t directory_max_tag = 6; + constexpr std::uint64_t regular_file_tag = 7; + constexpr std::uint64_t regular_symlink_tag = 8; + constexpr std::uint64_t directory_symlink_tag = 9; + + // if we're at the max depth, we only want to build non-directories + std::uint64_t file_type; + if (depth < max_depth) + { + file_type = uid{directory_min_tag, regular_symlink_tag}(urbg); + } + else + { + file_type = uid{regular_file_tag, regular_symlink_tag}(urbg); + } + + if (!SYMLINKS_ALLOWED && file_type > regular_file_tag) + { + file_type = regular_file_tag; + } + + std::error_code ec; + if (file_type <= directory_max_tag) + { + fs.create_directory(base, ec); + if (ec) { + INFO("File that failed: " << base); + REQUIRE_FALSE(ec); + } + + for (int i = 0; i < width; ++i) + { + create_directory_tree(urbg, fs, depth + 1, base / get_random_filename(urbg)); + } + } + else if (file_type == regular_file_tag) + { + // regular file + fs.write_contents(base, "", ec); + } + else if (file_type == regular_symlink_tag) + { + // regular symlink + fs.write_contents(base, "", ec); + REQUIRE_FALSE(ec); + const std::filesystem::path basep = base.native(); + auto basep_link = basep; + basep_link.replace_filename(basep.filename().native() + L"-link"); + std::filesystem::create_symlink(basep, basep_link, ec); + } + else // type == directory_symlink_tag + { + // directory symlink + std::filesystem::path basep = base.native(); + std::filesystem::create_directory_symlink(basep / "..", basep, ec); + } + + REQUIRE_FALSE(ec); + } +} + +TEST_CASE ("remove all", "[files]") +{ + auto urbg = get_urbg(0); + + fs::path temp_dir = TEMPORARY_DIRECTORY / get_random_filename(urbg); + + auto& fs = vcpkg::Files::get_real_filesystem(); + + std::error_code ec; + fs.create_directory(TEMPORARY_DIRECTORY, ec); + + REQUIRE_FALSE(ec); + + INFO("temp dir is: " << temp_dir); + + create_directory_tree(urbg, fs, 0, temp_dir); + + fs::path fp; + fs.remove_all(temp_dir, ec, fp); + REQUIRE_FALSE(ec); + + REQUIRE_FALSE(fs.exists(temp_dir)); +} diff --git a/toolsrc/src/vcpkg-tests/strings.cpp b/toolsrc/src/vcpkg-tests/strings.cpp new file mode 100644 index 0000000000..3168a7c95f --- /dev/null +++ b/toolsrc/src/vcpkg-tests/strings.cpp @@ -0,0 +1,33 @@ +#include + +#include + +#include +#include +#include + +TEST_CASE ("b32 encoding", "[strings]") +{ + using u64 = std::uint64_t; + + std::vector> map; + + map.emplace_back(0, "AAAAAAAAAAAAA"); + map.emplace_back(1, "BAAAAAAAAAAAA"); + + map.emplace_back(u64(1) << 32, "AAAAAAEAAAAAA"); + map.emplace_back((u64(1) << 32) + 1, "BAAAAAEAAAAAA"); + + map.emplace_back(0xE4D0'1065'D11E'0229, "JRA4RIXMQAUJO"); + map.emplace_back(0xA626'FE45'B135'07FF, "77BKTYWI6XJMK"); + map.emplace_back(0xEE36'D228'0C31'D405, "FAVDDGAFSWN4O"); + map.emplace_back(0x1405'64E7'FE7E'A88C, "MEK5H774ELBIB"); + map.emplace_back(0xFFFF'FFFF'FFFF'FFFF, "777777777777P"); + + std::string result; + for (const auto& pr : map) + { + result = vcpkg::Strings::b32_encode(pr.first); + REQUIRE(vcpkg::Strings::b32_encode(pr.first) == pr.second); + } +} diff --git a/toolsrc/src/vcpkg-tests/util.cpp b/toolsrc/src/vcpkg-tests/util.cpp index 54102f1aaa..f146283790 100644 --- a/toolsrc/src/vcpkg-tests/util.cpp +++ b/toolsrc/src/vcpkg-tests/util.cpp @@ -1,10 +1,16 @@ #include #include +#include #include +#include #include +#if defined(_WIN32) +#include +#endif + namespace vcpkg::Test { std::unique_ptr make_status_pgh(const char* name, @@ -44,4 +50,52 @@ namespace vcpkg::Test return m_ret.value_or_exit(VCPKG_LINE_INFO); } + + #if defined(_WIN32) + + static bool system_allows_symlinks() { + HKEY key; + bool allow_symlinks = true; + + const auto status = RegOpenKeyExW( + HKEY_LOCAL_MACHINE, LR"(SOFTWARE\Microsoft\Windows\CurrentVersion\AppModelUnlock)", 0, 0, &key); + + if (status == ERROR_FILE_NOT_FOUND) + { + allow_symlinks = false; + std::clog << "Symlinks are not allowed on this system\n"; + } + + if (status == ERROR_SUCCESS) RegCloseKey(key); + + return allow_symlinks; + } + + static fs::path internal_temporary_directory() { + wchar_t* tmp = static_cast(std::calloc(32'767, 2)); + + if (!GetEnvironmentVariableW(L"TEMP", tmp, 32'767)) { + std::cerr << "No temporary directory found.\n"; + std::abort(); + } + + fs::path result = tmp; + std::free(tmp); + + return result / L"vcpkg-test"; + } + + #else + + constexpr static bool system_allows_symlinks() { + return true; + } + static fs::path internal_temporary_directory() { + return "/tmp/vcpkg-test"; + } + + #endif + + const bool SYMLINKS_ALLOWED = system_allows_symlinks(); + const fs::path TEMPORARY_DIRECTORY = internal_temporary_directory(); } diff --git a/toolsrc/src/vcpkg/build.cpp b/toolsrc/src/vcpkg/build.cpp index d9305b271d..235adb819c 100644 --- a/toolsrc/src/vcpkg/build.cpp +++ b/toolsrc/src/vcpkg/build.cpp @@ -1097,56 +1097,8 @@ namespace vcpkg::Build } } -<<<<<<< HEAD - pre_build_info.triplet_abi_tag = [&]() { - const auto& fs = paths.get_filesystem(); - static std::map s_hash_cache; - - auto it_hash = s_hash_cache.find(triplet_file_path); - if (it_hash != s_hash_cache.end()) - { - return it_hash->second; - } - auto hash = Hash::get_file_hash(fs, triplet_file_path, "SHA1"); - - if (auto p = pre_build_info.external_toolchain_file.get()) - { - hash += "-"; - hash += Hash::get_file_hash(fs, *p, "SHA1"); - } - else if (pre_build_info.cmake_system_name.empty() || pre_build_info.cmake_system_name == "WindowsStore") - { - hash += "-"; - hash += Hash::get_file_hash(fs, paths.scripts / "toolchains" / "windows.cmake", "SHA1"); - } - else if (pre_build_info.cmake_system_name == "Linux") - { - hash += "-"; - hash += Hash::get_file_hash(fs, paths.scripts / "toolchains" / "linux.cmake", "SHA1"); - } - else if (pre_build_info.cmake_system_name == "Darwin") - { - hash += "-"; - hash += Hash::get_file_hash(fs, paths.scripts / "toolchains" / "osx.cmake", "SHA1"); - } - else if (pre_build_info.cmake_system_name == "FreeBSD") - { - hash += "-"; - hash += Hash::get_file_hash(fs, paths.scripts / "toolchains" / "freebsd.cmake", "SHA1"); - } - else if (pre_build_info.cmake_system_name == "Android") - { - hash += "-"; - hash += Hash::get_file_hash(fs, paths.scripts / "toolchains" / "android.cmake", "SHA1"); - } - - s_hash_cache.emplace(triplet_file_path, hash); - return hash; - }(); -======= pre_build_info.triplet_abi_tag = get_triplet_abi(paths, pre_build_info, triplet); ->>>>>>> trunk return pre_build_info; } diff --git a/toolsrc/vcpkgtest/vcpkgtest.vcxproj b/toolsrc/vcpkgtest/vcpkgtest.vcxproj index 4e385f376a..cf411d5918 100644 --- a/toolsrc/vcpkgtest/vcpkgtest.vcxproj +++ b/toolsrc/vcpkgtest/vcpkgtest.vcxproj @@ -23,10 +23,12 @@ + + From 0d8bba52e4c0a861b25f9d32006bfde9b749e09f Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Fri, 19 Jul 2019 23:20:28 -0700 Subject: [PATCH 16/17] allow tests to run on older standard libraries --- toolsrc/CMakeLists.txt | 3 +- .../{vcpkg-tests => vcpkg-test}/catch.h | 0 .../{vcpkg-tests => vcpkg-test}/util.h | 4 + .../{vcpkg-tests => vcpkg-test}/arguments.cpp | 2 +- .../src/{vcpkg-tests => vcpkg-test}/catch.cpp | 2 +- .../{vcpkg-tests => vcpkg-test}/chrono.cpp | 2 +- .../dependencies.cpp | 2 +- .../src/{vcpkg-tests => vcpkg-test}/files.cpp | 17 ++-- .../{vcpkg-tests => vcpkg-test}/paragraph.cpp | 4 +- .../src/{vcpkg-tests => vcpkg-test}/plan.cpp | 4 +- .../{vcpkg-tests => vcpkg-test}/specifier.cpp | 2 +- .../statusparagraphs.cpp | 4 +- .../{vcpkg-tests => vcpkg-test}/strings.cpp | 2 +- .../{vcpkg-tests => vcpkg-test}/supports.cpp | 2 +- .../{vcpkg-tests => vcpkg-test}/update.cpp | 4 +- .../src/{vcpkg-tests => vcpkg-test}/util.cpp | 83 ++++++++++++++++--- toolsrc/vcpkg/vcpkg.vcxproj | 6 +- toolsrc/vcpkglib/vcpkglib.vcxproj | 6 +- .../vcpkgmetricsuploader.vcxproj | 6 +- toolsrc/vcpkgtest/vcpkgtest.vcxproj | 4 +- 20 files changed, 109 insertions(+), 50 deletions(-) rename toolsrc/include/{vcpkg-tests => vcpkg-test}/catch.h (100%) rename toolsrc/include/{vcpkg-tests => vcpkg-test}/util.h (86%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/arguments.cpp (99%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/catch.cpp (85%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/chrono.cpp (96%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/dependencies.cpp (96%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/files.cpp (84%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/paragraph.cpp (99%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/plan.cpp (99%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/specifier.cpp (99%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/statusparagraphs.cpp (97%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/strings.cpp (96%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/supports.cpp (98%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/update.cpp (98%) rename toolsrc/src/{vcpkg-tests => vcpkg-test}/util.cpp (66%) diff --git a/toolsrc/CMakeLists.txt b/toolsrc/CMakeLists.txt index 6697a919e5..1db8bc36a4 100644 --- a/toolsrc/CMakeLists.txt +++ b/toolsrc/CMakeLists.txt @@ -35,7 +35,7 @@ if(GCC OR CLANG) endif() file(GLOB_RECURSE VCPKGLIB_SOURCES src/vcpkg/*.cpp) -file(GLOB_RECURSE VCPKGTEST_SOURCES src/vcpkg-tests/*.cpp) +file(GLOB_RECURSE VCPKGTEST_SOURCES src/vcpkg-test/*.cpp) if (DEFINE_DISABLE_METRICS) set(DISABLE_METRICS_VALUE "1") @@ -95,3 +95,4 @@ endif() set(THREADS_PREFER_PTHREAD_FLAG ON) find_package(Threads REQUIRED) target_link_libraries(vcpkg PRIVATE Threads::Threads) +target_link_libraries(vcpkg-test PRIVATE Threads::Threads) diff --git a/toolsrc/include/vcpkg-tests/catch.h b/toolsrc/include/vcpkg-test/catch.h similarity index 100% rename from toolsrc/include/vcpkg-tests/catch.h rename to toolsrc/include/vcpkg-test/catch.h diff --git a/toolsrc/include/vcpkg-tests/util.h b/toolsrc/include/vcpkg-test/util.h similarity index 86% rename from toolsrc/include/vcpkg-tests/util.h rename to toolsrc/include/vcpkg-test/util.h index 8a0486285f..fa650abc85 100644 --- a/toolsrc/include/vcpkg-tests/util.h +++ b/toolsrc/include/vcpkg-test/util.h @@ -35,4 +35,8 @@ extern const bool SYMLINKS_ALLOWED; extern const fs::path TEMPORARY_DIRECTORY; +void create_symlink(const fs::path& file, const fs::path& target, std::error_code& ec); + +void create_directory_symlink(const fs::path& file, const fs::path& target, std::error_code& ec); + } diff --git a/toolsrc/src/vcpkg-tests/arguments.cpp b/toolsrc/src/vcpkg-test/arguments.cpp similarity index 99% rename from toolsrc/src/vcpkg-tests/arguments.cpp rename to toolsrc/src/vcpkg-test/arguments.cpp index 8c625be0f8..3fe5fa420a 100644 --- a/toolsrc/src/vcpkg-tests/arguments.cpp +++ b/toolsrc/src/vcpkg-test/arguments.cpp @@ -1,4 +1,4 @@ -#include +#include #include diff --git a/toolsrc/src/vcpkg-tests/catch.cpp b/toolsrc/src/vcpkg-test/catch.cpp similarity index 85% rename from toolsrc/src/vcpkg-tests/catch.cpp rename to toolsrc/src/vcpkg-test/catch.cpp index 701dcb39ae..8b5d1aa15b 100644 --- a/toolsrc/src/vcpkg-tests/catch.cpp +++ b/toolsrc/src/vcpkg-test/catch.cpp @@ -1,5 +1,5 @@ #define CATCH_CONFIG_RUNNER -#include +#include #include diff --git a/toolsrc/src/vcpkg-tests/chrono.cpp b/toolsrc/src/vcpkg-test/chrono.cpp similarity index 96% rename from toolsrc/src/vcpkg-tests/chrono.cpp rename to toolsrc/src/vcpkg-test/chrono.cpp index c164753f94..306217ad07 100644 --- a/toolsrc/src/vcpkg-tests/chrono.cpp +++ b/toolsrc/src/vcpkg-test/chrono.cpp @@ -1,4 +1,4 @@ -#include +#include #include diff --git a/toolsrc/src/vcpkg-tests/dependencies.cpp b/toolsrc/src/vcpkg-test/dependencies.cpp similarity index 96% rename from toolsrc/src/vcpkg-tests/dependencies.cpp rename to toolsrc/src/vcpkg-test/dependencies.cpp index 0dee6f296e..5ed05cc078 100644 --- a/toolsrc/src/vcpkg-tests/dependencies.cpp +++ b/toolsrc/src/vcpkg-test/dependencies.cpp @@ -1,4 +1,4 @@ -#include +#include #include diff --git a/toolsrc/src/vcpkg-tests/files.cpp b/toolsrc/src/vcpkg-test/files.cpp similarity index 84% rename from toolsrc/src/vcpkg-tests/files.cpp rename to toolsrc/src/vcpkg-test/files.cpp index d2edffcfff..9e14cec0c3 100644 --- a/toolsrc/src/vcpkg-tests/files.cpp +++ b/toolsrc/src/vcpkg-test/files.cpp @@ -1,10 +1,9 @@ -#include -#include +#include +#include #include #include -#include // required for filesystem::create_{directory_}symlink #include #include @@ -20,7 +19,7 @@ namespace std::mt19937_64 get_urbg(std::uint64_t index) { // smallest prime > 2**63 - 1 - return std::mt19937_64{index + 9223372036854775837}; + return std::mt19937_64{index + 9223372036854775837ULL}; } std::string get_random_filename(std::mt19937_64& urbg) { return vcpkg::Strings::b32_encode(uid{}(urbg)); } @@ -83,16 +82,14 @@ namespace // regular symlink fs.write_contents(base, "", ec); REQUIRE_FALSE(ec); - const std::filesystem::path basep = base.native(); - auto basep_link = basep; - basep_link.replace_filename(basep.filename().native() + L"-link"); - std::filesystem::create_symlink(basep, basep_link, ec); + auto base_link = base; + base_link.replace_filename(base.filename().u8string() + "-link"); + vcpkg::Test::create_symlink(base, base_link, ec); } else // type == directory_symlink_tag { // directory symlink - std::filesystem::path basep = base.native(); - std::filesystem::create_directory_symlink(basep / "..", basep, ec); + vcpkg::Test::create_directory_symlink(base / "..", base, ec); } REQUIRE_FALSE(ec); diff --git a/toolsrc/src/vcpkg-tests/paragraph.cpp b/toolsrc/src/vcpkg-test/paragraph.cpp similarity index 99% rename from toolsrc/src/vcpkg-tests/paragraph.cpp rename to toolsrc/src/vcpkg-test/paragraph.cpp index 0fb85ec693..a95879cfa3 100644 --- a/toolsrc/src/vcpkg-tests/paragraph.cpp +++ b/toolsrc/src/vcpkg-test/paragraph.cpp @@ -1,5 +1,5 @@ -#include -#include +#include +#include #include diff --git a/toolsrc/src/vcpkg-tests/plan.cpp b/toolsrc/src/vcpkg-test/plan.cpp similarity index 99% rename from toolsrc/src/vcpkg-tests/plan.cpp rename to toolsrc/src/vcpkg-test/plan.cpp index 7ecab460b3..049ef20662 100644 --- a/toolsrc/src/vcpkg-tests/plan.cpp +++ b/toolsrc/src/vcpkg-test/plan.cpp @@ -1,5 +1,5 @@ -#include -#include +#include +#include #include #include diff --git a/toolsrc/src/vcpkg-tests/specifier.cpp b/toolsrc/src/vcpkg-test/specifier.cpp similarity index 99% rename from toolsrc/src/vcpkg-tests/specifier.cpp rename to toolsrc/src/vcpkg-test/specifier.cpp index 52ef044e79..330a96d78a 100644 --- a/toolsrc/src/vcpkg-tests/specifier.cpp +++ b/toolsrc/src/vcpkg-test/specifier.cpp @@ -1,4 +1,4 @@ -#include +#include #include #include diff --git a/toolsrc/src/vcpkg-tests/statusparagraphs.cpp b/toolsrc/src/vcpkg-test/statusparagraphs.cpp similarity index 97% rename from toolsrc/src/vcpkg-tests/statusparagraphs.cpp rename to toolsrc/src/vcpkg-test/statusparagraphs.cpp index df52ccb875..c0833e8ba0 100644 --- a/toolsrc/src/vcpkg-tests/statusparagraphs.cpp +++ b/toolsrc/src/vcpkg-test/statusparagraphs.cpp @@ -1,5 +1,5 @@ -#include -#include +#include +#include #include #include diff --git a/toolsrc/src/vcpkg-tests/strings.cpp b/toolsrc/src/vcpkg-test/strings.cpp similarity index 96% rename from toolsrc/src/vcpkg-tests/strings.cpp rename to toolsrc/src/vcpkg-test/strings.cpp index 3168a7c95f..6b744eee65 100644 --- a/toolsrc/src/vcpkg-tests/strings.cpp +++ b/toolsrc/src/vcpkg-test/strings.cpp @@ -1,4 +1,4 @@ -#include +#include #include diff --git a/toolsrc/src/vcpkg-tests/supports.cpp b/toolsrc/src/vcpkg-test/supports.cpp similarity index 98% rename from toolsrc/src/vcpkg-tests/supports.cpp rename to toolsrc/src/vcpkg-test/supports.cpp index c6c88bdbca..8bd386da05 100644 --- a/toolsrc/src/vcpkg-tests/supports.cpp +++ b/toolsrc/src/vcpkg-test/supports.cpp @@ -1,4 +1,4 @@ -#include +#include #include diff --git a/toolsrc/src/vcpkg-tests/update.cpp b/toolsrc/src/vcpkg-test/update.cpp similarity index 98% rename from toolsrc/src/vcpkg-tests/update.cpp rename to toolsrc/src/vcpkg-test/update.cpp index 93a8f74a98..70b2f04c1b 100644 --- a/toolsrc/src/vcpkg-tests/update.cpp +++ b/toolsrc/src/vcpkg-test/update.cpp @@ -1,5 +1,5 @@ -#include -#include +#include +#include #include diff --git a/toolsrc/src/vcpkg-tests/util.cpp b/toolsrc/src/vcpkg-test/util.cpp similarity index 66% rename from toolsrc/src/vcpkg-tests/util.cpp rename to toolsrc/src/vcpkg-test/util.cpp index f146283790..19f8f355e1 100644 --- a/toolsrc/src/vcpkg-tests/util.cpp +++ b/toolsrc/src/vcpkg-test/util.cpp @@ -1,6 +1,7 @@ -#include -#include +#include +#include +#include #include #include @@ -11,6 +12,18 @@ #include #endif +#if defined(_MSC_VER) && _MSC_VER >= 1914 + +#define USE_STD_FILESYSTEM + +#include // required for filesystem::create_{directory_}symlink + +#elif !defined(_MSC_VER) + +#include + +#endif + namespace vcpkg::Test { std::unique_ptr make_status_pgh(const char* name, @@ -51,9 +64,14 @@ namespace vcpkg::Test } - #if defined(_WIN32) + + // I am so sorry for this awful mix of macros static bool system_allows_symlinks() { +#if defined(_WIN32) + #if !defined(USE_STD_FILESYSTEM) + return false; + #else HKEY key; bool allow_symlinks = true; @@ -69,9 +87,14 @@ namespace vcpkg::Test if (status == ERROR_SUCCESS) RegCloseKey(key); return allow_symlinks; + #endif +#else + return true; +#endif } static fs::path internal_temporary_directory() { +#if defined(_WIN32) wchar_t* tmp = static_cast(std::calloc(32'767, 2)); if (!GetEnvironmentVariableW(L"TEMP", tmp, 32'767)) { @@ -83,19 +106,53 @@ namespace vcpkg::Test std::free(tmp); return result / L"vcpkg-test"; - } - - #else - - constexpr static bool system_allows_symlinks() { - return true; - } - static fs::path internal_temporary_directory() { +#else return "/tmp/vcpkg-test"; +#endif } - #endif - const bool SYMLINKS_ALLOWED = system_allows_symlinks(); const fs::path TEMPORARY_DIRECTORY = internal_temporary_directory(); + + void create_symlink(const fs::path& target, const fs::path& file, std::error_code& ec) { +#if defined(_MSC_VER) + #if defined(USE_STD_FILESYSTEM) + if (SYMLINKS_ALLOWED) + { + std::filesystem::path targetp = target.native(); + std::filesystem::path filep = file.native(); + + std::filesystem::create_symlink(targetp, filep); + } + else + #endif + { + vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "Symlinks are not allowed on this system"); + } +#else + if(symlink(target.c_str(), file.c_str()) != 0) { + ec.assign(errno, std::system_category()); + } +#endif + } + + void create_directory_symlink(const fs::path& target, const fs::path& file, std::error_code& ec) { +#if defined(_MSC_VER) + #if defined(USE_STD_FILESYSTEM) + if (SYMLINKS_ALLOWED) + { + std::filesystem::path targetp = target.native(); + std::filesystem::path filep = file.native(); + + std::filesystem::create_symlink(targetp, filep); + } + else + #endif + { + vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "Symlinks are not allowed on this system"); + } +#else + ::vcpkg::Test::create_symlink(target, file, ec); +#endif + } } diff --git a/toolsrc/vcpkg/vcpkg.vcxproj b/toolsrc/vcpkg/vcpkg.vcxproj index 06d849a740..917cd3b9c8 100644 --- a/toolsrc/vcpkg/vcpkg.vcxproj +++ b/toolsrc/vcpkg/vcpkg.vcxproj @@ -21,8 +21,8 @@ {34671B80-54F9-46F5-8310-AC429C11D4FB} vcpkg - 10.0 - v142 + 8.1 + v140 @@ -147,4 +147,4 @@ - \ No newline at end of file + diff --git a/toolsrc/vcpkglib/vcpkglib.vcxproj b/toolsrc/vcpkglib/vcpkglib.vcxproj index 9312dd627d..d28bae8ec5 100644 --- a/toolsrc/vcpkglib/vcpkglib.vcxproj +++ b/toolsrc/vcpkglib/vcpkglib.vcxproj @@ -21,8 +21,8 @@ {B98C92B7-2874-4537-9D46-D14E5C237F04} vcpkglib - 10.0 - v142 + 8.1 + v140 @@ -274,4 +274,4 @@ - \ No newline at end of file + diff --git a/toolsrc/vcpkgmetricsuploader/vcpkgmetricsuploader.vcxproj b/toolsrc/vcpkgmetricsuploader/vcpkgmetricsuploader.vcxproj index 2e689c7fca..14ec1e105d 100644 --- a/toolsrc/vcpkgmetricsuploader/vcpkgmetricsuploader.vcxproj +++ b/toolsrc/vcpkgmetricsuploader/vcpkgmetricsuploader.vcxproj @@ -21,8 +21,8 @@ {7D6FDEEB-B299-4A23-85EE-F67C4DED47BE} vcpkgmetricsuploader - 10.0 - v142 + 8.1 + v140 @@ -144,4 +144,4 @@ - \ No newline at end of file + diff --git a/toolsrc/vcpkgtest/vcpkgtest.vcxproj b/toolsrc/vcpkgtest/vcpkgtest.vcxproj index cf411d5918..530dfbc5dc 100644 --- a/toolsrc/vcpkgtest/vcpkgtest.vcxproj +++ b/toolsrc/vcpkgtest/vcpkgtest.vcxproj @@ -46,8 +46,8 @@ {F27B8DB0-1279-4AF8-A2E3-1D49C4F0220D} Win32Proj vcpkgtest - 10.0 - v142 + 8.1 + v140 From 2c20a9d98186e029ff443932295d7cdcad96980e Mon Sep 17 00:00:00 2001 From: Nicole Mazzuca Date: Mon, 22 Jul 2019 11:16:07 -0700 Subject: [PATCH 17/17] fix some of the awful mix of macros --- toolsrc/src/vcpkg-test/util.cpp | 67 +++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 24 deletions(-) diff --git a/toolsrc/src/vcpkg-test/util.cpp b/toolsrc/src/vcpkg-test/util.cpp index 19f8f355e1..a80ab36a0f 100644 --- a/toolsrc/src/vcpkg-test/util.cpp +++ b/toolsrc/src/vcpkg-test/util.cpp @@ -5,6 +5,9 @@ #include #include +// used to get the implementation specific compiler flags (i.e., __cpp_lib_filesystem) +#include + #include #include @@ -12,16 +15,24 @@ #include #endif -#if defined(_MSC_VER) && _MSC_VER >= 1914 +#define FILESYSTEM_SYMLINK_STD 0 +#define FILESYSTEM_SYMLINK_UNIX 1 +#define FILESYSTEM_SYMLINK_NONE 2 -#define USE_STD_FILESYSTEM +#if defined(__cpp_lib_filesystem) +#define FILESYSTEM_SYMLINK FILESYSTEM_SYMLINK_STD #include // required for filesystem::create_{directory_}symlink #elif !defined(_MSC_VER) +#define FILESYSTEM_SYMLINK FILESYSTEM_SYMLINK_UNIX #include +#else + +#define FILESYSTEM_SYMLINK FILESYSTEM_SYMLINK_NONE + #endif namespace vcpkg::Test @@ -63,15 +74,14 @@ namespace vcpkg::Test return m_ret.value_or_exit(VCPKG_LINE_INFO); } - - - // I am so sorry for this awful mix of macros - - static bool system_allows_symlinks() { + static bool system_allows_symlinks() + { #if defined(_WIN32) - #if !defined(USE_STD_FILESYSTEM) - return false; - #else + if (!__cpp_lib_filesystem) + { + return false; + } + HKEY key; bool allow_symlinks = true; @@ -87,17 +97,18 @@ namespace vcpkg::Test if (status == ERROR_SUCCESS) RegCloseKey(key); return allow_symlinks; - #endif #else return true; #endif } - static fs::path internal_temporary_directory() { + static fs::path internal_temporary_directory() + { #if defined(_WIN32) wchar_t* tmp = static_cast(std::calloc(32'767, 2)); - if (!GetEnvironmentVariableW(L"TEMP", tmp, 32'767)) { + if (!GetEnvironmentVariableW(L"TEMP", tmp, 32'767)) + { std::cerr << "No temporary directory found.\n"; std::abort(); } @@ -114,9 +125,14 @@ namespace vcpkg::Test const bool SYMLINKS_ALLOWED = system_allows_symlinks(); const fs::path TEMPORARY_DIRECTORY = internal_temporary_directory(); - void create_symlink(const fs::path& target, const fs::path& file, std::error_code& ec) { -#if defined(_MSC_VER) - #if defined(USE_STD_FILESYSTEM) +#if FILESYSTEM_SYMLINK == FILSYSTEM_SYMLINK_NONE + constexpr inline char no_filesystem_message[] = + " doesn't exist; on windows, we don't attempt to use the win32 calls to create symlinks"; +#endif + + void create_symlink(const fs::path& target, const fs::path& file, std::error_code& ec) + { +#if FILESYSTEM_SYMLINK == FILESYSTEM_SYMLINK_STD if (SYMLINKS_ALLOWED) { std::filesystem::path targetp = target.native(); @@ -125,20 +141,22 @@ namespace vcpkg::Test std::filesystem::create_symlink(targetp, filep); } else - #endif { vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "Symlinks are not allowed on this system"); } -#else - if(symlink(target.c_str(), file.c_str()) != 0) { +#elif FILESYSTEM_SYMLINK == FILESYSTEM_SYMLINK_UNIX + if (symlink(target.c_str(), file.c_str()) != 0) + { ec.assign(errno, std::system_category()); } +#else + vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, no_filesystem_message); #endif } - void create_directory_symlink(const fs::path& target, const fs::path& file, std::error_code& ec) { -#if defined(_MSC_VER) - #if defined(USE_STD_FILESYSTEM) + void create_directory_symlink(const fs::path& target, const fs::path& file, std::error_code& ec) + { +#if FILESYSTEM_SYMLINK == FILESYSTEM_SYMLINK_STD if (SYMLINKS_ALLOWED) { std::filesystem::path targetp = target.native(); @@ -147,12 +165,13 @@ namespace vcpkg::Test std::filesystem::create_symlink(targetp, filep); } else - #endif { vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "Symlinks are not allowed on this system"); } -#else +#elif FILESYSTEM_SYMLINK == FILESYSTEM_SYMLINK_UNIX ::vcpkg::Test::create_symlink(target, file, ec); +#else + vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, no_filesystem_message); #endif } }