ExecutionServer: use one method for conversion to bazel ExecuteResponse

2 files changed, 38 insertions, 62 deletions

diff --git a/src/buildtool/execution_api/execution_service/execution_server.cpp b/src/buildtool/execution_api/execution_service/execution_server.cpp
index daa91b9e..87b1b56d 100644
--- a/src/buildtool/execution_api/execution_service/execution_server.cpp
+++ b/src/buildtool/execution_api/execution_service/execution_server.cpp
@@ -151,68 +151,50 @@ auto ExecutionServiceImpl::GetIExecutionAction(
     return std::move(i_execution_action);
 }
 
-auto ExecutionServiceImpl::AddResult(
-    ::bazel_re::ExecuteResponse* response,
-    IExecutionResponse::Ptr const& i_execution_response,
-    std::string const& action_hash) const noexcept
-    -> expected<std::monostate, std::string> {
-    auto action_result =
-        ToBazelActionResult(i_execution_response->Artifacts(),
-                            i_execution_response->DirectorySymlinks(),
-                            storage_);
-    if (not action_result) {
-        logger_.Emit(LogLevel::Error, "{}", action_result.error());
-        return unexpected{std::move(action_result).error()};
+auto ExecutionServiceImpl::ToBazelExecuteResponse(
+    IExecutionResponse::Ptr const& i_execution_response) const noexcept
+    -> expected<::bazel_re::ExecuteResponse, std::string> {
+    auto result = ToBazelActionResult(i_execution_response->Artifacts(),
+                                      i_execution_response->DirectorySymlinks(),
+                                      storage_);
+    if (not result) {
+        return unexpected{std::move(result).error()};
     }
-    (*response->mutable_result()) = *std::move(action_result);
 
-    auto* result = response->mutable_result();
-    result->set_exit_code(i_execution_response->ExitCode());
+    auto action_result = *std::move(result);
+
+    action_result.set_exit_code(i_execution_response->ExitCode());
     if (i_execution_response->HasStdErr()) {
-        auto dgst = storage_.CAS().StoreBlob(i_execution_response->StdErr(),
-                                             /*is_executable=*/false);
-        if (not dgst) {
-            auto str =
-                fmt::format("Could not store stderr of action {}", action_hash);
-            logger_.Emit(LogLevel::Error, "{}", str);
-            return unexpected{std::move(str)};
+        auto cas_digest =
+            storage_.CAS().StoreBlob(i_execution_response->StdErr(),
+                                     /*is_executable=*/false);
+        if (not cas_digest) {
+            return unexpected{
+                fmt::format("Could not store stderr of action {}",
+                            i_execution_response->ActionDigest())};
         }
-        result->mutable_stderr_digest()->CopyFrom(*dgst);
+        action_result.mutable_stderr_digest()->CopyFrom(*cas_digest);
     }
+
     if (i_execution_response->HasStdOut()) {
-        auto dgst = storage_.CAS().StoreBlob(i_execution_response->StdOut(),
-                                             /*is_executable=*/false);
-        if (not dgst) {
-            auto str =
-                fmt::format("Could not store stdout of action {}", action_hash);
-            logger_.Emit(LogLevel::Error, "{}", str);
-            return unexpected{std::move(str)};
+        auto cas_digest =
+            storage_.CAS().StoreBlob(i_execution_response->StdOut(),
+                                     /*is_executable=*/false);
+        if (not cas_digest) {
+            return unexpected{
+                fmt::format("Could not store stdout of action {}",
+                            i_execution_response->ActionDigest())};
         }
-        result->mutable_stdout_digest()->CopyFrom(*dgst);
+        action_result.mutable_stdout_digest()->CopyFrom(*cas_digest);
     }
-    return std::monostate{};
-}
 
-static void AddStatus(::bazel_re::ExecuteResponse* response) noexcept {
-    ::google::rpc::Status status{};
-    // we run the action locally, so no communication issues should happen
-    status.set_code(grpc::StatusCode::OK);
-    *(response->mutable_status()) = status;
-}
+    ::bazel_re::ExecuteResponse bazel_response{};
+    (*bazel_response.mutable_result()) = std::move(action_result);
+    bazel_response.set_cached_result(i_execution_response->IsCached());
 
-auto ExecutionServiceImpl::GetResponse(
-    ::bazel_re::ExecuteRequest const* request,
-    IExecutionResponse::Ptr const& i_execution_response) const noexcept
-    -> expected<::bazel_re::ExecuteResponse, std::string> {
-    ::bazel_re::ExecuteResponse response{};
-    AddStatus(&response);
-    auto result = AddResult(
-        &response, i_execution_response, request->action_digest().hash());
-    if (not result) {
-        return unexpected{std::move(result).error()};
-    }
-    response.set_cached_result(i_execution_response->IsCached());
-    return std::move(response);
+    // we run the action locally, so no communication issues should happen
+    bazel_response.mutable_status()->set_code(grpc::StatusCode::OK);
+    return std::move(bazel_response);
 }
 
 void ExecutionServiceImpl::WriteResponse(
@@ -270,12 +252,14 @@ auto ExecutionServiceImpl::Execute(
         request->action_digest().hash(),
         std::chrono::duration_cast<std::chrono::seconds>(t1 - t0).count());
 
-    auto execute_response = GetResponse(request, i_execution_response);
+    auto execute_response = ToBazelExecuteResponse(i_execution_response);
     if (not execute_response) {
+        logger_.Emit(LogLevel::Error, execute_response.error());
         return ::grpc::Status{grpc::StatusCode::INTERNAL,
                               std::move(execute_response).error()};
     }
 
+    // Store the result in action cache
     if (i_execution_response->ExitCode() == 0 and not action->do_not_cache()) {
         if (not storage_.ActionCache().StoreResult(
                 request->action_digest(), execute_response->result())) {
diff --git a/src/buildtool/execution_api/execution_service/execution_server.hpp b/src/buildtool/execution_api/execution_service/execution_server.hpp
index f1337604..bd7eacb7 100644
--- a/src/buildtool/execution_api/execution_service/execution_server.hpp
+++ b/src/buildtool/execution_api/execution_service/execution_server.hpp
@@ -18,7 +18,6 @@
 #include <cstdint>
 #include <optional>
 #include <string>
-#include <variant>  //std::monostate
 
 #include "build/bazel/remote/execution/v2/remote_execution.grpc.pb.h"
 #include "gsl/gsl"
@@ -149,8 +148,7 @@ class ExecutionServiceImpl final : public bazel_re::Execution::Service {
         ::bazel_re::Action const& action) const
         -> expected<IExecutionAction::Ptr, std::string>;
 
-    [[nodiscard]] auto GetResponse(
-        ::bazel_re::ExecuteRequest const* request,
+    [[nodiscard]] auto ToBazelExecuteResponse(
         IExecutionResponse::Ptr const& i_execution_response) const noexcept
         -> expected<::bazel_re::ExecuteResponse, std::string>;
 
@@ -158,12 +156,6 @@ class ExecutionServiceImpl final : public bazel_re::Execution::Service {
         ::bazel_re::ExecuteResponse const& execute_response,
         ::grpc::ServerWriter<::google::longrunning::Operation>* writer,
         ::google::longrunning::Operation* op) noexcept;
-
-    [[nodiscard]] auto AddResult(
-        ::bazel_re::ExecuteResponse* response,
-        IExecutionResponse::Ptr const& i_execution_response,
-        std::string const& hash) const noexcept
-        -> expected<std::monostate, std::string>;
 };
 
 #endif