From 9a23629ff179c16309c7cfdb898926be03e8b105 Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Wed, 28 Aug 2024 13:25:31 +0200 Subject: just-mr maps: Properly check for missing values in map chain The root async map in a chain of calls should always be checked for missing value, which can happen if, e.g., a cycle happens or a thread gets killed by the system. Properly handle this by checking explicitly if a value has been posted. If not, check for cycles where it makes sense (for example, in the resolving of symlinks), otherwise report any pending map keys not yet processed. This is done for all just-mr commands working with async maps. --- src/other_tools/just_mr/TARGETS | 3 +++ src/other_tools/just_mr/fetch.cpp | 18 ++++++++++++++++-- src/other_tools/just_mr/setup.cpp | 17 +++++++++++++++++ src/other_tools/just_mr/update.cpp | 9 +++++++++ src/other_tools/root_maps/TARGETS | 2 -- src/other_tools/root_maps/content_git_map.cpp | 20 -------------------- src/other_tools/root_maps/fpath_git_map.cpp | 20 -------------------- 7 files changed, 45 insertions(+), 44 deletions(-) diff --git a/src/other_tools/just_mr/TARGETS b/src/other_tools/just_mr/TARGETS index 15478898..5d03675a 100644 --- a/src/other_tools/just_mr/TARGETS +++ b/src/other_tools/just_mr/TARGETS @@ -122,6 +122,7 @@ , ["src/buildtool/common/remote", "retry_config"] , ["src/buildtool/logging", "logging"] , ["src/buildtool/main", "retry"] + , ["src/buildtool/multithreading", "async_map_utils"] , ["src/buildtool/multithreading", "task_system"] , "exit_codes" , ["src/other_tools/just_mr/progress_reporting", "progress"] @@ -159,6 +160,7 @@ [ ["@", "fmt", "", "fmt"] , ["@", "json", "", "json"] , ["src/buildtool/logging", "logging"] + , ["src/buildtool/multithreading", "async_map_utils"] , ["src/buildtool/multithreading", "task_system"] , ["src/buildtool/storage", "config"] , ["src/other_tools/git_operations", "git_repo_remote"] @@ -189,6 +191,7 @@ , ["src/buildtool/common/remote", "retry_config"] , ["src/buildtool/logging", "logging"] , ["src/buildtool/main", "retry"] + , ["src/buildtool/multithreading", "async_map_utils"] , ["src/buildtool/multithreading", "task_system"] , ["src/buildtool/storage", "fs_utils"] , "exit_codes" diff --git a/src/other_tools/just_mr/fetch.cpp b/src/other_tools/just_mr/fetch.cpp index d236b7e8..c1d65010 100644 --- a/src/other_tools/just_mr/fetch.cpp +++ b/src/other_tools/just_mr/fetch.cpp @@ -31,6 +31,7 @@ #include "src/buildtool/logging/log_level.hpp" #include "src/buildtool/logging/logger.hpp" #include "src/buildtool/main/retry.hpp" +#include "src/buildtool/multithreading/async_map_utils.hpp" #include "src/buildtool/multithreading/task_system.hpp" #include "src/buildtool/serve_api/remote/config.hpp" #include "src/buildtool/serve_api/remote/serve_api.hpp" @@ -542,12 +543,15 @@ auto MultiRepoFetch(std::shared_ptr const& config, // do the fetch bool failed_archives{false}; + bool has_value_archives{false}; { TaskSystem ts{common_args.jobs}; archive_fetch_map.ConsumeAfterKeysReady( &ts, archives_to_fetch, - []([[maybe_unused]] auto const& values) {}, + [&has_value_archives]([[maybe_unused]] auto const& values) { + has_value_archives = true; + }, [&failed_archives, &multi_repository_tool_name](auto const& msg, bool fatal) { Logger::Log(fatal ? LogLevel::Error : LogLevel::Warning, @@ -558,12 +562,15 @@ auto MultiRepoFetch(std::shared_ptr const& config, }); } bool failed_git_trees{false}; + bool has_value_trees{false}; { TaskSystem ts{common_args.jobs}; git_tree_fetch_map.ConsumeAfterKeysReady( &ts, git_trees_to_fetch, - []([[maybe_unused]] auto const& values) {}, + [&has_value_trees]([[maybe_unused]] auto const& values) { + has_value_trees = true; + }, [&failed_git_trees, &multi_repository_tool_name](auto const& msg, bool fatal) { Logger::Log(fatal ? LogLevel::Error : LogLevel::Warning, @@ -582,6 +589,13 @@ auto MultiRepoFetch(std::shared_ptr const& config, if (failed_archives or failed_git_trees) { return kExitFetchError; } + if (not has_value_archives or not has_value_trees) { + DetectAndReportPending( + "fetch archives", archive_fetch_map, kArchiveContentPrinter); + DetectAndReportPending( + "fetch trees", git_tree_fetch_map, kGitTreeInfoPrinter); + return kExitFetchError; + } // report success Logger::Log(LogLevel::Info, "Fetch completed"); return kExitSuccess; diff --git a/src/other_tools/just_mr/setup.cpp b/src/other_tools/just_mr/setup.cpp index ba5e39b4..ba22936d 100644 --- a/src/other_tools/just_mr/setup.cpp +++ b/src/other_tools/just_mr/setup.cpp @@ -34,6 +34,7 @@ #include "src/buildtool/logging/log_level.hpp" #include "src/buildtool/logging/logger.hpp" #include "src/buildtool/main/retry.hpp" +#include "src/buildtool/multithreading/async_map_utils.hpp" #include "src/buildtool/multithreading/task_system.hpp" #include "src/buildtool/serve_api/remote/config.hpp" #include "src/buildtool/serve_api/remote/serve_api.hpp" @@ -344,12 +345,15 @@ auto MultiRepoSetup(std::shared_ptr const& config, // Populate workspace_root and TAKE_OVER fields bool failed{false}; + bool has_value{false}; + { TaskSystem ts{common_args.jobs}; repos_to_setup_map.ConsumeAfterKeysReady( &ts, setup_repos->to_setup, [&failed, + &has_value, &mr_config, repos, setup_repos, @@ -357,6 +361,7 @@ auto MultiRepoSetup(std::shared_ptr const& config, interactive, multi_repo_tool_name](auto const& values) noexcept { try { + has_value = true; // set the initial setup repositories nlohmann::json mr_repos{}; for (auto const& repo : setup_repos->to_setup) { @@ -438,6 +443,18 @@ auto MultiRepoSetup(std::shared_ptr const& config, if (failed) { return std::nullopt; } + if (not has_value) { + // check for cycles in maps where cycles can occur and have meaning + if (auto error = DetectAndReportCycle("resolving symlinks", + resolve_symlinks_map, + kGitObjectToResolvePrinter)) { + Logger::Log(LogLevel::Error, *error); + return std::nullopt; + } + DetectAndReportPending( + "setup", repos_to_setup_map, kReposToSetupPrinter); + return std::nullopt; + } // if successful, return the output config return StorageUtils::AddToCAS(storage, mr_config.dump(2)); } diff --git a/src/other_tools/just_mr/update.cpp b/src/other_tools/just_mr/update.cpp index df9de506..1c1626eb 100644 --- a/src/other_tools/just_mr/update.cpp +++ b/src/other_tools/just_mr/update.cpp @@ -25,6 +25,7 @@ #include "nlohmann/json.hpp" #include "src/buildtool/logging/log_level.hpp" #include "src/buildtool/logging/logger.hpp" +#include "src/buildtool/multithreading/async_map_utils.hpp" #include "src/buildtool/multithreading/task_system.hpp" #include "src/other_tools/git_operations/git_repo_remote.hpp" #include "src/other_tools/just_mr/exit_codes.hpp" @@ -240,16 +241,19 @@ auto MultiRepoUpdate(std::shared_ptr const& config, // do the update bool failed{false}; + bool has_value{false}; { TaskSystem ts{common_args.jobs}; git_update_map.ConsumeAfterKeysReady( &ts, repos_to_update, [&failed, + &has_value, &mr_config, repos_to_update_names = update_args.repos_to_update, multi_repo_tool_name](auto const& values) noexcept { try { + has_value = true; for (auto const& repo_name : repos_to_update_names) { auto i = static_cast( &repo_name - @@ -287,6 +291,11 @@ auto MultiRepoUpdate(std::shared_ptr const& config, if (failed) { return kExitUpdateError; } + if (not has_value) { + DetectAndReportPending( + "update", git_update_map, kRepoDescriptionPrinter); + return kExitUpdateError; + } // report success Logger::Log(LogLevel::Info, "Update completed"); // print mr_config to stdout diff --git a/src/other_tools/root_maps/TARGETS b/src/other_tools/root_maps/TARGETS index d4d1e4ba..49e1ee60 100644 --- a/src/other_tools/root_maps/TARGETS +++ b/src/other_tools/root_maps/TARGETS @@ -88,7 +88,6 @@ , ["src/buildtool/execution_api/local", "config"] , ["src/buildtool/file_system", "file_root"] , ["src/buildtool/file_system", "git_repo"] - , ["src/buildtool/multithreading", "async_map_utils"] , ["src/buildtool/multithreading", "task_system"] , ["src/buildtool/storage", "fs_utils"] , ["src/other_tools/git_operations", "git_repo_remote"] @@ -122,7 +121,6 @@ , ["src/buildtool/file_system", "file_storage"] , ["src/buildtool/file_system", "git_repo"] , ["src/buildtool/file_system/symlinks_map", "pragma_special"] - , ["src/buildtool/multithreading", "async_map_utils"] , ["src/buildtool/multithreading", "task_system"] , ["src/buildtool/storage", "fs_utils"] , ["src/other_tools/git_operations", "git_repo_remote"] diff --git a/src/other_tools/root_maps/content_git_map.cpp b/src/other_tools/root_maps/content_git_map.cpp index f46e3940..d39a70e5 100644 --- a/src/other_tools/root_maps/content_git_map.cpp +++ b/src/other_tools/root_maps/content_git_map.cpp @@ -18,7 +18,6 @@ #include "src/buildtool/file_system/file_root.hpp" #include "src/buildtool/file_system/file_storage.hpp" #include "src/buildtool/file_system/symlinks_map/pragma_special.hpp" -#include "src/buildtool/multithreading/async_map_utils.hpp" #include "src/buildtool/multithreading/task_system.hpp" #include "src/buildtool/storage/fs_utils.hpp" #include "src/other_tools/root_maps/root_utils.hpp" @@ -234,25 +233,6 @@ void ResolveContentTree( ts, ws_setter, logger](auto const& hashes) { - if (not hashes[0]) { - // check for cycles - if (auto error = DetectAndReportCycle( - fmt::format("resolving Git tree {}", tree_hash), - *resolve_symlinks_map, - kGitObjectToResolvePrinter)) { - (*logger)(fmt::format("Failed to resolve symlinks " - "in tree {}:\n{}", - tree_hash, - *error), - /*fatal=*/true); - return; - } - (*logger)(fmt::format("Unknown error in resolving " - "symlinks in tree {}", - tree_hash), - /*fatal=*/true); - return; - } auto const& resolved_tree_id = hashes[0]->id; // keep tree alive in Git cache via a tagged commit GitOpKey op_key = { diff --git a/src/other_tools/root_maps/fpath_git_map.cpp b/src/other_tools/root_maps/fpath_git_map.cpp index 37b0c543..dbda05ce 100644 --- a/src/other_tools/root_maps/fpath_git_map.cpp +++ b/src/other_tools/root_maps/fpath_git_map.cpp @@ -20,7 +20,6 @@ #include "src/buildtool/execution_api/local/config.hpp" #include "src/buildtool/file_system/file_root.hpp" #include "src/buildtool/file_system/git_repo.hpp" -#include "src/buildtool/multithreading/async_map_utils.hpp" #include "src/buildtool/multithreading/task_system.hpp" #include "src/buildtool/storage/fs_utils.hpp" #include "src/other_tools/git_operations/git_repo_remote.hpp" @@ -151,25 +150,6 @@ void ResolveFilePathTree( ts, ws_setter, logger](auto const& hashes) { - if (not hashes[0]) { - // check for cycles - if (auto error = DetectAndReportCycle( - fmt::format("resolving Git tree {}", tree_hash), - *resolve_symlinks_map, - kGitObjectToResolvePrinter)) { - (*logger)(fmt::format("Failed to resolve symlinks " - "in tree {}:\n{}", - tree_hash, - *error), - /*fatal=*/true); - return; - } - (*logger)(fmt::format("Unknown error in resolving " - "symlinks in tree {}", - tree_hash), - /*fatal=*/true); - return; - } auto const& resolved_tree_id = hashes[0]->id; // keep tree alive in Git cache via a tagged commit GitOpKey op_key = { -- cgit v1.2.3