ExecutionAPI: Support output_symlinks in response

4 files changed, 68 insertions, 13 deletions

diff --git a/src/buildtool/execution_api/common/execution_response.hpp b/src/buildtool/execution_api/common/execution_response.hpp
index 552e52a6..a77c40cb 100644
--- a/src/buildtool/execution_api/common/execution_response.hpp
+++ b/src/buildtool/execution_api/common/execution_response.hpp
@@ -20,7 +20,6 @@
 #include <optional>
 #include <string>
 #include <unordered_map>
-#include <unordered_set>
 
 #include "gsl/gsl"
 #include "src/buildtool/common/artifact.hpp"
@@ -35,7 +34,6 @@ class IExecutionResponse {
     using Ptr = std::unique_ptr<IExecutionResponse>;
     using ArtifactInfos = std::unordered_map<std::string, Artifact::ObjectInfo>;
     // set of paths found in output_directory_symlinks list of the action result
-    using DirSymlinks = std::unordered_set<std::string>;
 
     enum class StatusCode : std::uint8_t { Failed, Success };
 
diff --git a/src/buildtool/execution_api/local/local_response.hpp b/src/buildtool/execution_api/local/local_response.hpp
index 7f50b2fc..430bde89 100644
--- a/src/buildtool/execution_api/local/local_response.hpp
+++ b/src/buildtool/execution_api/local/local_response.hpp
@@ -15,12 +15,14 @@
 #ifndef INCLUDED_SRC_BUILDTOOL_EXECUTION_API_LOCAL_LOCAL_RESPONSE_HPP
 #define INCLUDED_SRC_BUILDTOOL_EXECUTION_API_LOCAL_LOCAL_RESPONSE_HPP
 
+#include <algorithm>
 #include <cstddef>
 #include <exception>
 #include <filesystem>
 #include <functional>
 #include <optional>
 #include <string>
+#include <unordered_set>
 #include <utility>
 
 #include "fmt/core.h"
@@ -48,6 +50,8 @@ class LocalResponse final : public IExecutionResponse {
     friend class LocalAction;
 
   public:
+    using DirSymlinks = std::unordered_set<std::string>;
+
     auto Status() const noexcept -> StatusCode final {
         return StatusCode::Success;  // unused
     }
@@ -111,11 +115,25 @@ class LocalResponse final : public IExecutionResponse {
             &artifacts_);  // explicit type needed for expected
     }
 
+    // Note that for newer requests (>=RBEv2.1), the local api does not report
+    // output file and directory symlinks separately. In that case, an error
+    // will be returned.
     auto DirectorySymlinks() noexcept
         -> expected<gsl::not_null<DirSymlinks const*>, std::string> {
         if (auto error_msg = Populate()) {
             return unexpected{*std::move(error_msg)};
         }
+        bool has_links_apinew = not output_.action.output_symlinks().empty();
+        bool has_links_apiold =
+            not output_.action.output_file_symlinks().empty() or
+            not output_.action.output_directory_symlinks().empty();
+        // If the new symlinks field (RBEv2.1) is populated but none of the old
+        // symlinks fields, then the local api did not populate them.
+        if (has_links_apinew and not has_links_apiold) {
+            return unexpected{std::string{
+                "Local api does not populate the field "
+                "output_directory_symlinks for >=RBEv2.1 requests."}};
+        }
         return gsl::not_null<DirSymlinks const*>(
             &dir_symlinks_);  // explicit type needed for expected
     }
@@ -157,9 +175,9 @@ class LocalResponse final : public IExecutionResponse {
         artifacts.reserve(
             static_cast<std::size_t>(action_result.output_files_size()) +
             static_cast<std::size_t>(
-                action_result.output_file_symlinks_size()) +
-            static_cast<std::size_t>(
-                action_result.output_directory_symlinks_size()) +
+                std::max(action_result.output_symlinks_size(),
+                         action_result.output_file_symlinks_size() +
+                             action_result.output_directory_symlinks_size())) +
             static_cast<std::size_t>(action_result.output_directories_size()));
 
         DirSymlinks dir_symlinks{};
@@ -193,7 +211,30 @@ class LocalResponse final : public IExecutionResponse {
         }
 
         // collect all symlinks and store them
+        for (auto const& link : action_result.output_symlinks()) {
+            try {
+                // in compatible mode: track upwards symlinks
+                has_upwards_symlinks_ =
+                    has_upwards_symlinks_ or
+                    (not ProtocolTraits::IsNative(hash_type) and
+                     not PathIsNonUpwards(link.target()));
+                artifacts.emplace(
+                    link.path(),
+                    Artifact::ObjectInfo{
+                        .digest =
+                            ArtifactDigestFactory::HashDataAs<ObjectType::File>(
+                                storage_.GetHashFunction(), link.target()),
+                        .type = ObjectType::Symlink});
+            } catch (std::exception const& ex) {
+                return fmt::format(
+                    "LocalResponse: unexpected failure gathering digest for "
+                    "{}:\n{}",
+                    link.path(),
+                    ex.what());
+            }
+        }
         for (auto const& link : action_result.output_file_symlinks()) {
+            // DEPRECATED as of v2.1
             try {
                 // in compatible mode: track upwards symlinks
                 has_upwards_symlinks_ =
@@ -216,6 +257,7 @@ class LocalResponse final : public IExecutionResponse {
             }
         }
         for (auto const& link : action_result.output_directory_symlinks()) {
+            // DEPRECATED as of v2.1
             try {
                 // in compatible mode: track upwards symlinks
                 has_upwards_symlinks_ =
diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
index 5d826a1b..060239f7 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
@@ -111,10 +111,6 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
             action_result.output_directory_symlinks_size()) +
         static_cast<std::size_t>(action_result.output_directories_size()));
 
-    DirSymlinks dir_symlinks{};
-    dir_symlinks.reserve(static_cast<std::size_t>(
-        action_result.output_directory_symlinks_size()));
-
     auto const hash_type = network_->GetHashFunction().GetType();
     // collect files and store them
     for (auto const& file : action_result.output_files()) {
@@ -142,7 +138,29 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
     }
 
     // collect all symlinks and store them
+    for (auto const& link : action_result.output_symlinks()) {
+        try {
+            // in compatible mode: track upwards symlinks
+            has_upwards_symlinks_ = has_upwards_symlinks_ or
+                                    (not ProtocolTraits::IsNative(hash_type) and
+                                     not PathIsNonUpwards(link.target()));
+            artifacts.emplace(
+                link.path(),
+                Artifact::ObjectInfo{
+                    .digest =
+                        ArtifactDigestFactory::HashDataAs<ObjectType::File>(
+                            network_->GetHashFunction(), link.target()),
+                    .type = ObjectType::Symlink});
+        } catch (std::exception const& ex) {
+            return fmt::format(
+                "BazelResponse: unexpected failure gathering digest for "
+                "{}:\n{}",
+                link.path(),
+                ex.what());
+        }
+    }
     for (auto const& link : action_result.output_file_symlinks()) {
+        // DEPRECATED as of v2.1
         try {
             // in compatible mode: track upwards symlinks
             has_upwards_symlinks_ = has_upwards_symlinks_ or
@@ -164,6 +182,7 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
         }
     }
     for (auto const& link : action_result.output_directory_symlinks()) {
+        // DEPRECATED as of v2.1
         try {
             // in compatible mode: track upwards symlinks
             has_upwards_symlinks_ = has_upwards_symlinks_ or
@@ -176,7 +195,6 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
                         ArtifactDigestFactory::HashDataAs<ObjectType::File>(
                             network_->GetHashFunction(), link.target()),
                     .type = ObjectType::Symlink});
-            dir_symlinks.emplace(link.path());  // add it to set
         } catch (std::exception const& ex) {
             return fmt::format(
                 "BazelResponse: unexpected failure gathering digest for "
@@ -211,7 +229,6 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
             }
         }
         artifacts_ = std::move(artifacts);
-        dir_symlinks_ = std::move(dir_symlinks);
         populated_ = true;
         return std::nullopt;
     }
@@ -273,7 +290,6 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
         ++pos;
     }
     artifacts_ = std::move(artifacts);
-    dir_symlinks_ = std::move(dir_symlinks);
     populated_ = true;
     return std::nullopt;
 }
diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp
index 8504ae59..2b890da6 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp
@@ -96,7 +96,6 @@ class BazelResponse final : public IExecutionResponse {
     std::shared_ptr<BazelNetwork> const network_;
     BazelExecutionClient::ExecutionOutput output_{};
     ArtifactInfos artifacts_;
-    DirSymlinks dir_symlinks_;
     bool has_upwards_symlinks_ = false;  // only tracked in compatible mode
     bool populated_ = false;