From 9ef2ffa9acdc19453c81696aa2ed4da7cb84078c Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Thu, 21 Sep 2023 18:19:58 +0200 Subject: RemoteServeConfig: Remove problematic inheritance This was causing the remote serve address to overwrite the one set for remote execution. Also, to keep things clean, some common remote server-related methods and definitions were moved into their own library. --- src/buildtool/common/remote/TARGETS | 7 +++ src/buildtool/common/remote/remote_common.hpp | 63 ++++++++++++++++++++++ src/buildtool/execution_api/remote/TARGETS | 6 +-- src/buildtool/execution_api/remote/config.hpp | 45 +--------------- src/buildtool/graph_traverser/TARGETS | 1 + src/buildtool/graph_traverser/graph_traverser.hpp | 3 +- src/buildtool/serve_api/remote/TARGETS | 2 +- src/buildtool/serve_api/remote/config.hpp | 20 ++++++- src/buildtool/serve_api/serve_service/TARGETS | 1 + .../serve_api/serve_service/source_tree.cpp | 2 +- .../serve_api/serve_service/source_tree.hpp | 3 +- 11 files changed, 98 insertions(+), 55 deletions(-) create mode 100644 src/buildtool/common/remote/remote_common.hpp (limited to 'src') diff --git a/src/buildtool/common/remote/TARGETS b/src/buildtool/common/remote/TARGETS index daf7afc2..97966431 100644 --- a/src/buildtool/common/remote/TARGETS +++ b/src/buildtool/common/remote/TARGETS @@ -22,4 +22,11 @@ ] , "stage": ["src", "buildtool", "common", "remote"] } +, "remote_common": + { "type": ["@", "rules", "CC", "library"] + , "name": ["remote_common"] + , "hdrs": ["remote_common.hpp"] + , "deps": [["@", "fmt", "", "fmt"], ["@", "json", "", "json"], "port"] + , "stage": ["src", "buildtool", "common", "remote"] + } } diff --git a/src/buildtool/common/remote/remote_common.hpp b/src/buildtool/common/remote/remote_common.hpp new file mode 100644 index 00000000..e8ce2ed7 --- /dev/null +++ b/src/buildtool/common/remote/remote_common.hpp @@ -0,0 +1,63 @@ +// Copyright 2023 Huawei Cloud Computing Technology Co., Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef INCLUDED_SRC_BUILDTOOL_COMMON_REMOTE_ADDRESS_HPP +#define INCLUDED_SRC_BUILDTOOL_COMMON_REMOTE_ADDRESS_HPP + +#include +#include +#include + +#include +#include + +#include "src/buildtool/common/remote/port.hpp" + +struct ServerAddress { + std::string host{}; + Port port{}; + + [[nodiscard]] auto ToJson() const noexcept -> nlohmann::json { + return nlohmann::json{ + fmt::format("{}:{}", host, static_cast(port))}; + } +}; + +[[nodiscard]] static auto ParseAddress(std::string const& address) noexcept + -> std::optional { + std::istringstream iss(address); + std::string host; + std::string port; + if (not std::getline(iss, host, ':') or not std::getline(iss, port, ':')) { + return std::nullopt; + } + auto port_num = ParsePort(port); + if (not host.empty() and port_num) { + return ServerAddress{std::move(host), *port_num}; + } + return std::nullopt; +} + +[[nodiscard]] static auto ParseProperty(std::string const& property) noexcept + -> std::optional> { + std::istringstream pss(property); + std::string key; + std::string val; + if (not std::getline(pss, key, ':') or not std::getline(pss, val, ':')) { + return std::nullopt; + } + return std::make_pair(key, val); +} + +#endif // INCLUDED_SRC_BUILDTOOL_COMMON_REMOTE_ADDRESS_HPP diff --git a/src/buildtool/execution_api/remote/TARGETS b/src/buildtool/execution_api/remote/TARGETS index 234050ce..8543db0d 100644 --- a/src/buildtool/execution_api/remote/TARGETS +++ b/src/buildtool/execution_api/remote/TARGETS @@ -72,11 +72,7 @@ , "name": ["config"] , "hdrs": ["config.hpp"] , "srcs": ["config.cpp"] - , "deps": - [ ["@", "fmt", "", "fmt"] - , ["@", "json", "", "json"] - , ["src/buildtool/common/remote", "port"] - ] + , "deps": [["src/buildtool/common/remote", "remote_common"]] , "stage": ["src", "buildtool", "execution_api", "remote"] } } diff --git a/src/buildtool/execution_api/remote/config.hpp b/src/buildtool/execution_api/remote/config.hpp index e7ad3a57..bad729ab 100644 --- a/src/buildtool/execution_api/remote/config.hpp +++ b/src/buildtool/execution_api/remote/config.hpp @@ -19,29 +19,15 @@ #include #include #include -#include #include #include #include #include -#include -#include - -#include "src/buildtool/common/remote/port.hpp" +#include "src/buildtool/common/remote/remote_common.hpp" class RemoteExecutionConfig { public: - struct ServerAddress { - std::string host{}; - Port port{}; - - [[nodiscard]] auto ToJson() const noexcept -> nlohmann::json { - return nlohmann::json{ - fmt::format("{}:{}", host, static_cast(port))}; - } - }; - // Obtain global instance [[nodiscard]] static auto Instance() noexcept -> RemoteExecutionConfig& { static RemoteExecutionConfig config; @@ -114,35 +100,6 @@ class RemoteExecutionConfig { // Platform properties for execution. std::map platform_properties_{}; - - [[nodiscard]] static auto ParseAddress(std::string const& address) noexcept - -> std::optional { - std::istringstream iss(address); - std::string host; - std::string port; - if (not std::getline(iss, host, ':') or - not std::getline(iss, port, ':')) { - return std::nullopt; - } - auto port_num = ParsePort(port); - if (not host.empty() and port_num) { - return ServerAddress{std::move(host), *port_num}; - } - return std::nullopt; - } - - [[nodiscard]] static auto ParseProperty( - std::string const& property) noexcept - -> std::optional> { - std::istringstream pss(property); - std::string key; - std::string val; - if (not std::getline(pss, key, ':') or - not std::getline(pss, val, ':')) { - return std::nullopt; - } - return std::make_pair(key, val); - } }; #endif // INCLUDED_SRC_BUILDTOOL_EXECUTION_API_REMOTE_CONFIG_HPP diff --git a/src/buildtool/graph_traverser/TARGETS b/src/buildtool/graph_traverser/TARGETS index b9911633..51b0a653 100644 --- a/src/buildtool/graph_traverser/TARGETS +++ b/src/buildtool/graph_traverser/TARGETS @@ -5,6 +5,7 @@ , "deps": [ ["src/buildtool/common", "cli"] , ["src/buildtool/common", "tree"] + , ["src/buildtool/common/remote", "remote_common"] , ["src/buildtool/execution_engine/dag", "dag"] , ["src/buildtool/execution_engine/executor", "executor"] , ["src/buildtool/execution_engine/traverser", "traverser"] diff --git a/src/buildtool/graph_traverser/graph_traverser.hpp b/src/buildtool/graph_traverser/graph_traverser.hpp index 9f99e6f2..8567395e 100644 --- a/src/buildtool/graph_traverser/graph_traverser.hpp +++ b/src/buildtool/graph_traverser/graph_traverser.hpp @@ -30,6 +30,7 @@ #include "fmt/core.h" #include "gsl/gsl" #include "src/buildtool/common/cli.hpp" +#include "src/buildtool/common/remote/remote_common.hpp" #include "src/buildtool/common/statistics.hpp" #include "src/buildtool/common/tree.hpp" #include "src/buildtool/execution_api/bazel_msg/bazel_blob_container.hpp" @@ -270,7 +271,7 @@ class GraphTraverser { } [[nodiscard]] static auto CreateExecutionApi( - std::optional const& address) + std::optional const& address) -> gsl::not_null { if (address) { ExecutionConfiguration config; diff --git a/src/buildtool/serve_api/remote/TARGETS b/src/buildtool/serve_api/remote/TARGETS index 71f82b82..45114f57 100644 --- a/src/buildtool/serve_api/remote/TARGETS +++ b/src/buildtool/serve_api/remote/TARGETS @@ -2,7 +2,7 @@ { "type": ["@", "rules", "CC", "library"] , "name": ["config"] , "hdrs": ["config.hpp"] - , "deps": [["src/buildtool/execution_api/remote", "config"]] + , "deps": [["src/buildtool/common/remote", "remote_common"]] , "stage": ["src", "buildtool", "serve_api", "remote"] } , "serve_target_level_cache_client": diff --git a/src/buildtool/serve_api/remote/config.hpp b/src/buildtool/serve_api/remote/config.hpp index dac7fabf..6ea927cb 100644 --- a/src/buildtool/serve_api/remote/config.hpp +++ b/src/buildtool/serve_api/remote/config.hpp @@ -16,9 +16,9 @@ #include #include -#include "src/buildtool/execution_api/remote/config.hpp" +#include "src/buildtool/common/remote/remote_common.hpp" -class RemoteServeConfig : public RemoteExecutionConfig { +class RemoteServeConfig { public: // Obtain global instance [[nodiscard]] static auto Instance() noexcept -> RemoteServeConfig& { @@ -26,6 +26,13 @@ class RemoteServeConfig : public RemoteExecutionConfig { return config; } + // Set remote execution and cache address, unsets if parsing `address` fails + [[nodiscard]] static auto SetRemoteAddress( + std::string const& address) noexcept -> bool { + auto& inst = Instance(); + return static_cast(inst.remote_address_ = ParseAddress(address)); + } + // Set the list of known repositories [[nodiscard]] static auto SetKnownRepositories( std::vector const& repos) noexcept -> bool { @@ -36,6 +43,12 @@ class RemoteServeConfig : public RemoteExecutionConfig { return repos.size() == inst.repositories_.size(); } + // Remote execution address, if set + [[nodiscard]] static auto RemoteAddress() noexcept + -> std::optional { + return Instance().remote_address_; + } + // Repositories known to 'just serve' [[nodiscard]] static auto KnownRepositories() noexcept -> const std::vector& { @@ -43,6 +56,9 @@ class RemoteServeConfig : public RemoteExecutionConfig { } private: + // Server address of remote execution. + std::optional remote_address_{}; + // Known Git repositories to serve server. std::vector repositories_{}; }; diff --git a/src/buildtool/serve_api/serve_service/TARGETS b/src/buildtool/serve_api/serve_service/TARGETS index ec94ee76..dae08d35 100644 --- a/src/buildtool/serve_api/serve_service/TARGETS +++ b/src/buildtool/serve_api/serve_service/TARGETS @@ -14,6 +14,7 @@ , "proto": ["just_serve_proto"] , "deps": [ ["src/buildtool/logging", "logging"] + , ["src/buildtool/common/remote", "remote_common"] , ["src/buildtool/execution_api/common", "common"] , ["src/buildtool/execution_api/remote", "config"] ] diff --git a/src/buildtool/serve_api/serve_service/source_tree.cpp b/src/buildtool/serve_api/serve_service/source_tree.cpp index 8d91c2f5..69a7a607 100644 --- a/src/buildtool/serve_api/serve_service/source_tree.cpp +++ b/src/buildtool/serve_api/serve_service/source_tree.cpp @@ -25,7 +25,7 @@ #include "src/buildtool/storage/config.hpp" auto SourceTreeService::CreateExecutionApi( - std::optional const& address) + std::optional const& address) -> gsl::not_null { if (address) { ExecutionConfiguration config; diff --git a/src/buildtool/serve_api/serve_service/source_tree.hpp b/src/buildtool/serve_api/serve_service/source_tree.hpp index a1599508..0bdf48b8 100644 --- a/src/buildtool/serve_api/serve_service/source_tree.hpp +++ b/src/buildtool/serve_api/serve_service/source_tree.hpp @@ -23,6 +23,7 @@ #include "gsl/gsl" #include "justbuild/just_serve/just_serve.grpc.pb.h" +#include "src/buildtool/common/remote/remote_common.hpp" #include "src/buildtool/execution_api/common/execution_api.hpp" #include "src/buildtool/execution_api/remote/config.hpp" #include "src/buildtool/logging/logger.hpp" @@ -57,7 +58,7 @@ class SourceTreeService final CreateExecutionApi(std::nullopt)}; [[nodiscard]] static auto CreateExecutionApi( - std::optional const& address) + std::optional const& address) -> gsl::not_null; [[nodiscard]] static auto GetTreeFromCommit( -- cgit v1.2.3