From 0252d51cc065e6366aa19b73a8a102d33bda90f2 Mon Sep 17 00:00:00 2001 From: Lutz Justen Date: Mon, 21 Nov 2022 23:22:09 +0100 Subject: [PATCH] Add actions for building and testing (#8) * Add a Github action for building and testing On Windows, -- -//third_party/... doesn't seem to work, so add all test directories manually. Also run the tests_*. We run only fastbuild tests here, since the opt tests will be run in the release workflow. Also fix a number of compilation and test issues found along the way. --- .github/workflows/build_and_test.yml | 65 ++++++++++++++++++++++++++++ .gitignore | 1 + cdc_fuse_fs/cdc_fuse_fs.cc | 2 +- common/path_test.cc | 58 +++++++++++++++++++++---- common/process_test.cc | 2 +- common/process_win.cc | 2 +- common/semaphore_test.cc | 6 +-- common/threadpool.cc | 2 +- common/threadpool_test.cc | 6 +-- data_store/data_provider.cc | 19 +++++--- data_store/data_provider.h | 12 +++-- fastcdc/fastcdc.h | 2 + manifest/BUILD | 2 + 13 files changed, 151 insertions(+), 28 deletions(-) create mode 100644 .github/workflows/build_and_test.yml diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml new file mode 100644 index 0000000..d8e31ed --- /dev/null +++ b/.github/workflows/build_and_test.yml @@ -0,0 +1,65 @@ +name: Build & Test + +# Run when something is pushed to main or when there's action on a pull request. +on: + push: + branches: + - main + pull_request: + +# Cancel running workflow if a pull request is modified. Note that head_ref is +# undefined for pushes to main. Use run_id as fallback. This is unique for each +# run, so runs for pushes to main are never cancelled. +concurrency: + group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + Build-And-Test-Linux: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: Initialize submodules + run: git submodule update --init --recursive + + - name: Build (fastbuild) + run: | + bazel build --config=linux -- //... -//third_party/... + + # Skip file_finder_test: The test works when file_finder_test is run + # directly, but not through bazel test. The reason is, bazel test + # creates symlinks of test files, but the finder ignores symlinks. + # Also run tests sequentially since some tests write to a common tmp dir. + - name: Test (fastbuild) + run: | + bazel test --config=linux --test_output=errors --local_test_jobs=1 -- //... -//third_party/... -//cdc_rsync_server:file_finder_test + + Build-And-Test-Windows: + runs-on: windows-2019 + steps: + - uses: actions/checkout@v3 + + - name: Initialize submodules + run: git submodule update --init --recursive + + - name: Build + run: | + bazel build --config=windows //cdc_rsync //cdc_stream //asset_stream_manager //tests_common //tests_asset_streaming_30 //tests_cdc_rsync + + - name: Test + run: | + bazel-bin\tests_common\tests_common.exe + bazel-bin\tests_asset_streaming_30\tests_asset_streaming_30.exe + bazel-bin\tests_cdc_rsync\tests_cdc_rsync.exe + bazel test --config=windows --test_output=errors --local_test_jobs=1 ` + //asset_stream_manager/... ` + //cdc_fuse_fs/... ` + //cdc_rsync/... ` + //cdc_rsync/base/... ` + //cdc_rsync_server/... ` + //common/... ` + //data_store/... ` + //fastcdc/... ` + //manifest/... ` + //metrics/... diff --git a/.gitignore b/.gitignore index afa1b26..59bcc04 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,4 @@ dependencies *.includes .qtc_clangd bazel-* +user.bazelrc diff --git a/cdc_fuse_fs/cdc_fuse_fs.cc b/cdc_fuse_fs/cdc_fuse_fs.cc index 7386477..674666c 100644 --- a/cdc_fuse_fs/cdc_fuse_fs.cc +++ b/cdc_fuse_fs/cdc_fuse_fs.cc @@ -1648,7 +1648,7 @@ absl::Status Run(DataStoreReader* data_store_reader, bool consistency_check) { ctx->data_store_reader = data_store_reader; InitializeRootManifest(); #ifndef USE_MOCK_LIBFUSE - RETURN_IF_ERROR(ctx->config_stream_client_->StartListeningToManifestUpdates( + RETURN_IF_ERROR(ctx->config_stream_client->StartListeningToManifestUpdates( [](const ContentIdProto& id) { return SetManifest(id); }), "Failed to listen to manifest updates"); diff --git a/common/path_test.cc b/common/path_test.cc index e0a08e0..8928c60 100644 --- a/common/path_test.cc +++ b/common/path_test.cc @@ -29,6 +29,14 @@ #include "common/util.h" #include "gtest/gtest.h" +#if PLATFORM_WINDOWS +#ifndef WIN32_LEAN_AND_MEAN +#define WIN32_LEAN_AND_MEAN 1 +#endif +#include // GetLongPathName +#undef ReplaceFile +#endif + namespace cdc_ft { namespace { @@ -53,6 +61,22 @@ constexpr char kTestFileName[] = "test.txt"; constexpr char kUnicodeTestFileName[] = u8"unicode test \U0001F964.txt"; constexpr char kSubDirFileName[] = "subdir_file.txt"; +// Converts e.g. C:\Progra~1 to C:\Program Files on Windows. +std::string GetLongPath(std::string path) { +#if PLATFORM_WINDOWS + std::wstring wpath = Util::Utf8ToWideStr(path); + std::wstring long_wpath(4096, 0); + size_t buf_len = + GetLongPathName(wpath.c_str(), const_cast(long_wpath.c_str()), + long_wpath.size() * sizeof(wchar_t)); + EXPECT_GT(buf_len, 0); + long_wpath.resize(buf_len); + return Util::WideToUtf8Str(long_wpath); +#else + return path; +#endif +} + class PathTest : public ::testing::Test { public: PathTest() { @@ -159,7 +183,10 @@ class PathTest : public ::testing::Test { std::vector SearchFiles(const std::string& pattern, bool recursive); - std::string base_dir_ = path::Join(path::GetTempDir(), kBaseDirName); + // Some tests get confused if the temp dir is a short filename + // (e.g. RUNNER~1 instead of runneradmin - I'm looking at you, Github!). + const std::string tmp_dir_ = GetLongPath(path::GetTempDir()); + std::string base_dir_ = path::Join(tmp_dir_, kBaseDirName); std::string search_dir_ = path::Join(base_dir_, kSearchDirName); std::string subdir_ = path::Join(search_dir_, kSubDirName); const std::string subdir_fulldirpath_ = subdir_; @@ -303,9 +330,11 @@ TEST_F(PathTest, GetFullPath) { path::Join(cwd, u8"\U0001F964", "foo")); #if PLATFORM_WINDOWS - // These test cases assume that C: is the current drive. - EXPECT_EQ(path::GetFullPath("C:"), cwd); - EXPECT_EQ(path::GetFullPath("C:foo"), path::Join(cwd, "foo")); + std::string current_drive = path::GetDrivePrefix(cwd); + ASSERT_NE(current_drive, std::string()); + ASSERT_EQ(current_drive.size(), 2) << current_drive; + EXPECT_EQ(path::GetFullPath(current_drive), cwd); + EXPECT_EQ(path::GetFullPath(current_drive + "foo"), path::Join(cwd, "foo")); EXPECT_EQ(path::GetFullPath("V:foo"), "V:\\foo"); EXPECT_EQ(path::GetFullPath("C:\\"), "C:\\"); EXPECT_EQ(path::GetFullPath("C:\\foo"), "C:\\foo"); @@ -322,9 +351,10 @@ TEST_F(PathTest, GetFullPath) { // other Windows APIs are trimming such spaces, most notably GetFullPathNameW. EXPECT_EQ(path::GetFullPath("trailing space "), path::Join(cwd, "trailing space ")); - EXPECT_EQ(path::GetFullPath("C:trailing space "), + EXPECT_EQ(path::GetFullPath(current_drive + "trailing space "), path::Join(cwd, "trailing space ")); - EXPECT_EQ(path::GetFullPath("C:\\trailing space "), "C:\\trailing space "); + EXPECT_EQ(path::GetFullPath(current_drive + "\\trailing space "), + current_drive + "\\trailing space "); EXPECT_EQ(path::GetFullPath("V:trailing space "), "V:\\trailing space "); #else EXPECT_EQ(path::GetFullPath("/"), "/"); @@ -753,6 +783,12 @@ std::vector PathTest::SearchFiles(const std::string& pattern, }; EXPECT_OK(path::SearchFiles(pattern, recursive, handler)); + + // Linux and Windows yield a slightly different order, so sort first. + std::sort(files.begin(), files.end(), [](const File& a, const File& b) { + return a.filename_ < b.filename_; + }); + return files; } @@ -771,9 +807,11 @@ TEST_F(PathTest, SearchFiles_NonRecursiveTrailingSeparator) { EXPECT_EQ(files[1].dir_, search_dir_); EXPECT_EQ(files[1].Path(), test_filepath_); + EXPECT_FALSE(files[1].is_directory_); EXPECT_EQ(files[2].dir_, search_dir_); EXPECT_EQ(files[2].Path(), unicode_test_filepath_); + EXPECT_FALSE(files[2].is_directory_); #endif } @@ -793,9 +831,11 @@ TEST_F(PathTest, SearchFiles_NonRecursiveNoTrailingSeparator) { EXPECT_EQ(files[1].dir_, search_dir_); EXPECT_EQ(files[1].Path(), test_filepath_); + EXPECT_FALSE(files[1].is_directory_); EXPECT_EQ(files[2].dir_, search_dir_); EXPECT_EQ(files[2].Path(), unicode_test_filepath_); + EXPECT_FALSE(files[2].is_directory_); #endif } @@ -814,9 +854,11 @@ TEST_F(PathTest, SearchFiles_RecursiveTrailingSeparator) { EXPECT_EQ(files[2].dir_, search_dir_); EXPECT_EQ(files[2].Path(), test_filepath_); + EXPECT_FALSE(files[2].is_directory_); EXPECT_EQ(files[3].dir_, search_dir_); EXPECT_EQ(files[3].Path(), unicode_test_filepath_); + EXPECT_FALSE(files[3].is_directory_); } #if PLATFORM_WINDOWS @@ -1215,7 +1257,7 @@ TEST_F(PathTest, Exists) { } TEST_F(PathTest, CreateDir) { - std::string dir = path::Join(path::GetTempDir(), "createdir_test"); + std::string dir = path::Join(tmp_dir_, "createdir_test"); std::string unicode_dir = path::Join(dir, u8"\U0001F964\U0001F964\U0001F964"); path::RemoveDirRec(dir).IgnoreError(); EXPECT_NOT_OK(path::CreateDir(path::Join(dir, "subdir"))); @@ -1232,7 +1274,7 @@ TEST_F(PathTest, CreateDir) { } TEST_F(PathTest, CreateDirRec) { - std::string dir = path::Join(path::GetTempDir(), "createdir_test", "subdir"); + std::string dir = path::Join(tmp_dir_, "createdir_test", "subdir"); std::string unicode_dir = path::Join(dir, u8"\U0001F964\U0001F964\U0001F964"); path::RemoveDirRec(dir).IgnoreError(); EXPECT_OK(path::CreateDirRec(dir)); diff --git a/common/process_test.cc b/common/process_test.cc index fd856e1..9d1c960 100644 --- a/common/process_test.cc +++ b/common/process_test.cc @@ -230,7 +230,7 @@ TEST_F(ProcessTest, RunUntil) { ProcessStartInfo start_info; start_info.command = "findstr stop"; start_info.redirect_stdin = true; - std::atomic_bool stop(false); + std::atomic_bool stop{false}; start_info.stdout_handler = [&std_out, &stop](const char* data, size_t) { // Check whether someone sent the "stop" command. // Note: This runs in a background thread. diff --git a/common/process_win.cc b/common/process_win.cc index 42eb62b..bacc4f2 100644 --- a/common/process_win.cc +++ b/common/process_win.cc @@ -49,7 +49,7 @@ void SetThreadName(const std::string& name) { } } -std::atomic_int g_pipe_serial_number(0); +std::atomic_int g_pipe_serial_number{0}; // Creates a pipe suitable for overlapped IO. Regular anonymous pipes in Windows // don't support overlapped IO. This method creates a named pipe with a unique diff --git a/common/semaphore_test.cc b/common/semaphore_test.cc index c2183e7..5594dbb 100644 --- a/common/semaphore_test.cc +++ b/common/semaphore_test.cc @@ -79,9 +79,9 @@ TEST_F(SemaphoreTest, DoesNotBlockIfInitialCountIsOne) { TEST_F(SemaphoreTest, SignalManyThreads) { Semaphore semaphore(0); - std::atomic_int a(0); - std::atomic_int b(0); - std::atomic_int c(0); + std::atomic_int a{0}; + std::atomic_int b{0}; + std::atomic_int c{0}; const int N = 16; std::vector threads; diff --git a/common/threadpool.cc b/common/threadpool.cc index 1135ccf..5c1acc8 100644 --- a/common/threadpool.cc +++ b/common/threadpool.cc @@ -16,7 +16,7 @@ namespace cdc_ft { -Threadpool::Threadpool(size_t num_threads) : shutdown_(false) { +Threadpool::Threadpool(size_t num_threads) : shutdown_{false} { workers_.reserve(num_threads); for (size_t n = 0; n < num_threads; ++n) { workers_.emplace_back([this]() { ThreadWorkerMain(); }); diff --git a/common/threadpool_test.cc b/common/threadpool_test.cc index 1e9404f..f0ce99a 100644 --- a/common/threadpool_test.cc +++ b/common/threadpool_test.cc @@ -49,7 +49,7 @@ TEST_F(ThreadpoolTest, WaitShutdownWorkWithoutTasks) { } TEST_F(ThreadpoolTest, SingleThreadedRunsToCompletion) { - std::atomic_bool task_finished(false); + std::atomic_bool task_finished{false}; auto task_func = [&task_finished](Task::IsCancelledPredicate) { task_finished = true; }; @@ -69,7 +69,7 @@ TEST_F(ThreadpoolTest, SingleThreadedRunsToCompletion) { TEST_F(ThreadpoolTest, MultiThreadedRunsToCompletion) { const int num_tasks = 19; const int num_threads = 7; - std::atomic_int num_completed(0); + std::atomic_int num_completed{0}; Threadpool pool(num_threads); std::unordered_set tasks; @@ -95,7 +95,7 @@ TEST_F(ThreadpoolTest, MultiThreadedRunsToCompletion) { TEST_F(ThreadpoolTest, TaskIsCancelledOnShutdown) { Semaphore task_started(0); - std::atomic_bool task_finished(false); + std::atomic_bool task_finished{false}; auto task_func = [&task_started, &task_finished](Task::IsCancelledPredicate is_cancelled) { task_started.Signal(); diff --git a/data_store/data_provider.cc b/data_store/data_provider.cc index 49a8f9c..76fbc1f 100644 --- a/data_store/data_provider.cc +++ b/data_store/data_provider.cc @@ -40,7 +40,7 @@ DataProvider::DataProvider( : prefetch_size_(prefetch_size), writer_(std::move(writer)), readers_(std::move(readers)), - chunks_updated_(true), + chunks_updated_{true}, cleanup_timeout_sec_(cleanup_timeout_sec), access_idle_timeout_sec_(access_idle_timeout_sec) { if (writer_) { @@ -74,7 +74,7 @@ size_t DataProvider::PrefetchSize(size_t read_size) const { absl::StatusOr DataProvider::Get(const ContentIdProto& content_id, void* data, size_t offset, size_t size) { - last_access_ts_ = steady_clock_->Now(); + last_access_sec_ = GetSteadyNowSec(); absl::Mutex* content_mutex = GetContentMutex(content_id); absl::StatusOr read_bytes; if (writer_) { @@ -127,7 +127,7 @@ absl::StatusOr DataProvider::Get(const ContentIdProto& content_id, } absl::Status DataProvider::Get(ChunkTransferList* chunks) { - last_access_ts_ = steady_clock_->Now(); + last_access_sec_ = GetSteadyNowSec(); // Try to fetch chunks from the cache first. RETURN_IF_ERROR(GetFromWriter(chunks, /*lock_required=*/true)); if (chunks->ReadDone()) return absl::OkStatus(); @@ -175,7 +175,7 @@ absl::Status DataProvider::Get(ChunkTransferList* chunks) { } absl::Status DataProvider::Get(const ContentIdProto& content_id, Buffer* data) { - last_access_ts_ = steady_clock_->Now(); + last_access_sec_ = GetSteadyNowSec(); absl::Mutex* content_mutex = GetContentMutex(content_id); absl::Status status = absl::OkStatus(); if (writer_) { @@ -324,9 +324,7 @@ void DataProvider::CleanupThreadMain() { next_cleanup_time - steady_clock_->Now()) .count()))); int64_t time_sec_since_last_access = - std::chrono::duration_cast(steady_clock_->Now() - - last_access_ts_.load()) - .count(); + GetSteadyNowSec() - last_access_sec_.load(); if (chunks_updated_ && time_sec_since_last_access > access_idle_timeout_sec_) { WriterMutexLockList locks; @@ -361,4 +359,11 @@ bool DataProvider::WaitForCleanupAndResetForTesting(absl::Duration timeout) { is_cleaned_ = false; return is_cleaned; } + +int64_t DataProvider::GetSteadyNowSec() { + return std::chrono::duration_cast(steady_clock_->Now() - + first_now_) + .count(); +} + } // namespace cdc_ft diff --git a/data_store/data_provider.h b/data_store/data_provider.h index 30abf51..fc3bccb 100644 --- a/data_store/data_provider.h +++ b/data_store/data_provider.h @@ -105,6 +105,9 @@ class DataProvider : public DataStoreReader { // Periodically cleans up data in |writer_|. void CleanupThreadMain() ABSL_LOCKS_EXCLUDED(shutdown_mutex_, cleaned_mutex_); + // Returns the current time of |steady_clock_| in seconds. + int64_t GetSteadyNowSec(); + static constexpr unsigned int kNumberOfMutexes = 256; // How much additional data to prefetch when a max. FUSE read is encountered. @@ -124,15 +127,18 @@ class DataProvider : public DataStoreReader { // Indicates whether the shutdown was triggered. bool shutdown_ ABSL_GUARDED_BY(shutdown_mutex_) = false; - // The last access time. - std::atomic> - last_access_ts_; + // The last access time in seconds since construction. Note that for some + // compilers we can't use std::chrono::time_point with atomics, so keep the + // time in seconds. + std::atomic last_access_sec_; // Identifies if new data was added to the cache since the last cleanup. std::atomic chunks_updated_; // Clock to track the last access time. SteadyClock* steady_clock_ = DefaultSteadyClock::GetInstance(); + const std::chrono::time_point first_now_ = + steady_clock_->Now(); // Cleanup interval. uint32_t cleanup_timeout_sec_ = kCleanupTimeoutSec; diff --git a/fastcdc/fastcdc.h b/fastcdc/fastcdc.h index 8d6ac0a..c4c1b0e 100644 --- a/fastcdc/fastcdc.h +++ b/fastcdc/fastcdc.h @@ -17,6 +17,8 @@ #ifndef FASTCDC_FASTCDC_H_ #define FASTCDC_FASTCDC_H_ +#include + #include #include #include diff --git a/manifest/BUILD b/manifest/BUILD index 1ff8fe8..d107ded 100644 --- a/manifest/BUILD +++ b/manifest/BUILD @@ -119,6 +119,8 @@ cc_library( "manifest_updater.h", "pending_assets_queue.h", ], + # Tests don't work under Linux, but we only need it on Windows, anyway. + target_compatible_with = ["@platforms//os:windows"], deps = [ ":file_chunk_map", ":manifest_builder",