refactor(tvix): Make Store::buildPaths return a Status r/1649

Make Store::buildPaths return a Status with [[nodiscard]] rather than
throwing exceptions to signal failure. This is the beginning of a long
road to refactor the entire store API to be status/statusor based
instead of using exceptions.

Change-Id: I2e32371c95a25b87ad129987c217d49c6d6e0c85
Reviewed-on: https://cl.tvl.fyi/c/depot/+/1745
Tested-by: BuildkiteCI
Reviewed-by: kanepyork <rikingcoding@gmail.com>

18 files changed, 137 insertions, 32 deletions

diff --git a/third_party/nix/src/libexpr/primops.cc b/third_party/nix/src/libexpr/primops.cc
index 54dc315fb9..34d659675b 100644
--- a/third_party/nix/src/libexpr/primops.cc
+++ b/third_party/nix/src/libexpr/primops.cc
@@ -22,6 +22,7 @@
 #include "libstore/store-api.hh"
 #include "libutil/archive.hh"
 #include "libutil/json.hh"
+#include "libutil/status.hh"
 #include "libutil/util.hh"
 
 namespace nix {
@@ -90,7 +91,7 @@ void EvalState::realiseContext(const PathSet& context) {
   unsigned long long narSize;
   store->queryMissing(drvs, willBuild, willSubstitute, unknown, downloadSize,
                       narSize);
-  store->buildPaths(drvs);
+  nix::util::OkOrThrow(store->buildPaths(drvs));
 }
 
 /* Load and evaluate an expression from path specified by the
diff --git a/third_party/nix/src/libstore/build.cc b/third_party/nix/src/libstore/build.cc
index 6aad99b37a..a9d40991d3 100644
--- a/third_party/nix/src/libstore/build.cc
+++ b/third_party/nix/src/libstore/build.cc
@@ -13,6 +13,7 @@
 #include <string>
 #include <thread>
 
+#include <absl/status/status.h>
 #include <absl/strings/ascii.h>
 #include <absl/strings/numbers.h>
 #include <absl/strings/str_cat.h>
@@ -4686,7 +4687,8 @@ static void primeCache(Store& store, const PathSet& paths) {
   }
 }
 
-void LocalStore::buildPaths(const PathSet& drvPaths, BuildMode buildMode) {
+absl::Status LocalStore::buildPaths(const PathSet& drvPaths,
+                                    BuildMode buildMode) {
   Worker worker(*this);
 
   primeCache(*this, drvPaths);
@@ -4717,8 +4719,12 @@ void LocalStore::buildPaths(const PathSet& drvPaths, BuildMode buildMode) {
   }
 
   if (!failed.empty()) {
-    throw Error(worker.exitStatus(), "build of %s failed", showPaths(failed));
+    return absl::Status(
+        absl::StatusCode::kInternal,
+        absl::StrFormat("build of %s failed (exit code %d)", showPaths(failed),
+                        worker.exitStatus()));
   }
+  return absl::OkStatus();
 }
 
 BuildResult LocalStore::buildDerivation(const Path& drvPath,
diff --git a/third_party/nix/src/libstore/local-store.hh b/third_party/nix/src/libstore/local-store.hh
index 731cf1764c..f9e08e5e6b 100644
--- a/third_party/nix/src/libstore/local-store.hh
+++ b/third_party/nix/src/libstore/local-store.hh
@@ -5,6 +5,7 @@
 #include <string>
 #include <unordered_set>
 
+#include <absl/status/status.h>
 #include <absl/strings/str_split.h>
 
 #include "libstore/pathlocks.hh"
@@ -155,7 +156,7 @@ class LocalStore : public LocalFSStore {
   Path addTextToStore(const std::string& name, const std::string& s,
                       const PathSet& references, RepairFlag repair) override;
 
-  void buildPaths(const PathSet& paths, BuildMode buildMode) override;
+  absl::Status buildPaths(const PathSet& paths, BuildMode buildMode) override;
 
   BuildResult buildDerivation(const Path& drvPath, const BasicDerivation& drv,
                               BuildMode buildMode) override;
diff --git a/third_party/nix/src/libstore/remote-store.cc b/third_party/nix/src/libstore/remote-store.cc
index 41881f8ef9..6948d4e61d 100644
--- a/third_party/nix/src/libstore/remote-store.cc
+++ b/third_party/nix/src/libstore/remote-store.cc
@@ -3,6 +3,7 @@
 #include <cerrno>
 #include <cstring>
 
+#include <absl/status/status.h>
 #include <absl/strings/ascii.h>
 #include <fcntl.h>
 #include <glog/logging.h>
@@ -459,7 +460,8 @@ Path RemoteStore::addTextToStore(const std::string& name, const std::string& s,
   return readStorePath(*this, conn->from);
 }
 
-void RemoteStore::buildPaths(const PathSet& drvPaths, BuildMode buildMode) {
+absl::Status RemoteStore::buildPaths(const PathSet& drvPaths,
+                                     BuildMode buildMode) {
   auto conn(getConnection());
   conn->to << wopBuildPaths;
   if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 13) {
@@ -470,7 +472,8 @@ void RemoteStore::buildPaths(const PathSet& drvPaths, BuildMode buildMode) {
         /* Old daemons did not take a 'buildMode' parameter, so we
            need to validate it here on the client side.  */
         if (buildMode != bmNormal) {
-      throw Error(
+      return absl::Status(
+          absl::StatusCode::kInvalidArgument,
           "repairing or checking is not supported when building through the "
           "Nix daemon");
     }
@@ -485,6 +488,8 @@ void RemoteStore::buildPaths(const PathSet& drvPaths, BuildMode buildMode) {
   }
   conn.processStderr();
   readInt(conn->from);
+
+  return absl::OkStatus();
 }
 
 BuildResult RemoteStore::buildDerivation(const Path& drvPath,
diff --git a/third_party/nix/src/libstore/remote-store.hh b/third_party/nix/src/libstore/remote-store.hh
index a6829226ca..4aad5a5d2f 100644
--- a/third_party/nix/src/libstore/remote-store.hh
+++ b/third_party/nix/src/libstore/remote-store.hh
@@ -71,7 +71,7 @@ class RemoteStore : public virtual Store {
   Path addTextToStore(const std::string& name, const std::string& s,
                       const PathSet& references, RepairFlag repair) override;
 
-  void buildPaths(const PathSet& paths, BuildMode buildMode) override;
+  absl::Status buildPaths(const PathSet& paths, BuildMode buildMode) override;
 
   BuildResult buildDerivation(const Path& drvPath, const BasicDerivation& drv,
                               BuildMode buildMode) override;
diff --git a/third_party/nix/src/libstore/rpc-store.cc b/third_party/nix/src/libstore/rpc-store.cc
index a0b1ef9cfa..2d1278f40c 100644
--- a/third_party/nix/src/libstore/rpc-store.cc
+++ b/third_party/nix/src/libstore/rpc-store.cc
@@ -317,15 +317,17 @@ Path RpcStore::addTextToStore(const std::string& name,
   return result.path();
 }
 
-void RpcStore::buildPaths(const PathSet& paths, BuildMode buildMode) {
+absl::Status RpcStore::buildPaths(const PathSet& paths, BuildMode buildMode) {
   ClientContext ctx;
   proto::BuildPathsRequest request;
   for (const auto& path : paths) {
     request.add_drvs(path);
   }
+
   google::protobuf::Empty response;
   request.set_mode(nix::BuildModeToProto(buildMode));
-  SuccessOrThrow(stub_->BuildPaths(&ctx, request, &response), __FUNCTION__);
+  return nix::util::proto::GRPCStatusToAbsl(
+      stub_->BuildPaths(&ctx, request, &response));
 }
 
 BuildResult RpcStore::buildDerivation(const Path& drvPath,
diff --git a/third_party/nix/src/libstore/rpc-store.hh b/third_party/nix/src/libstore/rpc-store.hh
index 7bfa78a496..4bf417ad1e 100644
--- a/third_party/nix/src/libstore/rpc-store.hh
+++ b/third_party/nix/src/libstore/rpc-store.hh
@@ -67,8 +67,8 @@ class RpcStore : public LocalFSStore, public virtual Store {
                               const PathSet& references,
                               RepairFlag repair = NoRepair) override;
 
-  virtual void buildPaths(const PathSet& paths,
-                          BuildMode buildMode = bmNormal) override;
+  virtual absl::Status buildPaths(const PathSet& paths,
+                                  BuildMode buildMode = bmNormal) override;
 
   virtual BuildResult buildDerivation(const Path& drvPath,
                                       const BasicDerivation& drv,
diff --git a/third_party/nix/src/libstore/store-api.cc b/third_party/nix/src/libstore/store-api.cc
index ae403b0be6..fc33c7ce57 100644
--- a/third_party/nix/src/libstore/store-api.cc
+++ b/third_party/nix/src/libstore/store-api.cc
@@ -3,6 +3,7 @@
 #include <future>
 #include <utility>
 
+#include <absl/status/status.h>
 #include <absl/strings/match.h>
 #include <absl/strings/numbers.h>
 #include <absl/strings/str_cat.h>
@@ -700,16 +701,20 @@ const Store::Stats& Store::getStats() {
   return stats;
 }
 
-void Store::buildPaths(const PathSet& paths, BuildMode buildMode) {
+absl::Status Store::buildPaths(const PathSet& paths, BuildMode) {
   for (auto& path : paths) {
     if (isDerivation(path)) {
-      unsupported("buildPaths");
+      return absl::Status(absl::StatusCode::kUnimplemented,
+                          "buildPaths is unsupported");
     }
   }
 
   if (queryValidPaths(paths).size() != paths.size()) {
-    unsupported("buildPaths");
+    return absl::Status(absl::StatusCode::kUnimplemented,
+                        "buildPaths is unsupported");
   }
+
+  return absl::OkStatus();
 }
 
 void copyStorePath(ref<Store> srcStore, const ref<Store>& dstStore,
diff --git a/third_party/nix/src/libstore/store-api.hh b/third_party/nix/src/libstore/store-api.hh
index 8d9c6f0861..b1c0b50de3 100644
--- a/third_party/nix/src/libstore/store-api.hh
+++ b/third_party/nix/src/libstore/store-api.hh
@@ -445,7 +445,8 @@ class Store : public std::enable_shared_from_this<Store>, public Config {
      output paths can be created by running the builder, after
      recursively building any sub-derivations. For inputs that are
      not derivations, substitute them. */
-  virtual void buildPaths(const PathSet& paths, BuildMode buildMode = bmNormal);
+  [[nodiscard]] virtual absl::Status buildPaths(const PathSet& paths,
+                                                BuildMode buildMode = bmNormal);
 
   /* Build a single non-materialized derivation (i.e. not from an
      on-disk .drv file). Note that ‘drvPath’ is only used for
diff --git a/third_party/nix/src/libutil/CMakeLists.txt b/third_party/nix/src/libutil/CMakeLists.txt
index cdf4655c40..0b36929218 100644
--- a/third_party/nix/src/libutil/CMakeLists.txt
+++ b/third_party/nix/src/libutil/CMakeLists.txt
@@ -21,6 +21,7 @@ set(HEADER_FILES
     proto.hh
     ref.hh
     serialise.hh
+    status.hh
     sync.hh
     thread-pool.hh
     types.hh
diff --git a/third_party/nix/src/libutil/proto.hh b/third_party/nix/src/libutil/proto.hh
index 484a10a82a..058cb7b7b4 100644
--- a/third_party/nix/src/libutil/proto.hh
+++ b/third_party/nix/src/libutil/proto.hh
@@ -2,19 +2,20 @@
 
 #include <absl/status/status.h>
 #include <grpcpp/impl/codegen/status.h>
+#include <grpcpp/impl/codegen/status_code_enum.h>
 
 #include "libproto/worker.pb.h"
 #include "libutil/types.hh"
 
 namespace nix::util::proto {
 
-::nix::proto::StorePath StorePath(const Path& path) {
+inline ::nix::proto::StorePath StorePath(const Path& path) {
   ::nix::proto::StorePath store_path;
   store_path.set_path(path);
   return store_path;
 }
 
-::nix::proto::StorePaths StorePaths(const PathSet& paths) {
+inline ::nix::proto::StorePaths StorePaths(const PathSet& paths) {
   ::nix::proto::StorePaths result;
   for (const auto& path : paths) {
     result.add_paths(path);
@@ -70,6 +71,47 @@ constexpr absl::StatusCode GRPCStatusCodeToAbsl(grpc::StatusCode code) {
   }
 }
 
+constexpr grpc::StatusCode AbslStatusCodeToGRPC(absl::StatusCode code) {
+  switch (code) {
+    case absl::StatusCode::kOk:
+      return grpc::StatusCode::OK;
+    case absl::StatusCode::kCancelled:
+      return grpc::StatusCode::CANCELLED;
+    case absl::StatusCode::kUnknown:
+      return grpc::StatusCode::UNKNOWN;
+    case absl::StatusCode::kInvalidArgument:
+      return grpc::StatusCode::INVALID_ARGUMENT;
+    case absl::StatusCode::kDeadlineExceeded:
+      return grpc::StatusCode::DEADLINE_EXCEEDED;
+    case absl::StatusCode::kNotFound:
+      return grpc::StatusCode::NOT_FOUND;
+    case absl::StatusCode::kAlreadyExists:
+      return grpc::StatusCode::ALREADY_EXISTS;
+    case absl::StatusCode::kPermissionDenied:
+      return grpc::StatusCode::PERMISSION_DENIED;
+    case absl::StatusCode::kUnauthenticated:
+      return grpc::StatusCode::UNAUTHENTICATED;
+    case absl::StatusCode::kResourceExhausted:
+      return grpc::StatusCode::RESOURCE_EXHAUSTED;
+    case absl::StatusCode::kFailedPrecondition:
+      return grpc::StatusCode::FAILED_PRECONDITION;
+    case absl::StatusCode::kAborted:
+      return grpc::StatusCode::ABORTED;
+    case absl::StatusCode::kOutOfRange:
+      return grpc::StatusCode::OUT_OF_RANGE;
+    case absl::StatusCode::kUnimplemented:
+      return grpc::StatusCode::UNIMPLEMENTED;
+    case absl::StatusCode::kInternal:
+      return grpc::StatusCode::INTERNAL;
+    case absl::StatusCode::kUnavailable:
+      return grpc::StatusCode::UNAVAILABLE;
+    case absl::StatusCode::kDataLoss:
+      return grpc::StatusCode::DATA_LOSS;
+    default:
+      return grpc::StatusCode::INTERNAL;
+  }
+}
+
 constexpr absl::string_view GRPCStatusCodeDescription(grpc::StatusCode code) {
   switch (code) {
     case grpc::StatusCode::OK:
@@ -111,4 +153,22 @@ constexpr absl::string_view GRPCStatusCodeDescription(grpc::StatusCode code) {
   };
 }
 
+inline absl::Status GRPCStatusToAbsl(grpc::Status status) {
+  if (status.ok()) {
+    return absl::OkStatus();
+  }
+
+  return absl::Status(GRPCStatusCodeToAbsl(status.error_code()),
+                      status.error_message());
+}
+
+inline grpc::Status AbslToGRPCStatus(absl::Status status) {
+  if (status.ok()) {
+    return grpc::Status::OK;
+  }
+
+  return grpc::Status(AbslStatusCodeToGRPC(status.code()),
+                      std::string(status.message()));
+}
+
 }  // namespace nix::util::proto
diff --git a/third_party/nix/src/libutil/status.hh b/third_party/nix/src/libutil/status.hh
new file mode 100644
index 0000000000..aeee0f5022
--- /dev/null
+++ b/third_party/nix/src/libutil/status.hh
@@ -0,0 +1,17 @@
+#pragma once
+
+#include <absl/status/status.h>
+#include <absl/strings/str_format.h>
+#include <absl/strings/string_view.h>
+
+#include "libutil/types.hh"
+
+namespace nix::util {
+
+inline void OkOrThrow(absl::Status status) {
+  if (!status.ok()) {
+    throw Error(absl::StrFormat("Operation failed: %s", status.ToString()));
+  }
+}
+
+}  // namespace nix::util
diff --git a/third_party/nix/src/nix-build/nix-build.cc b/third_party/nix/src/nix-build/nix-build.cc
index 34d551c2d8..85b8a32462 100644
--- a/third_party/nix/src/nix-build/nix-build.cc
+++ b/third_party/nix/src/nix-build/nix-build.cc
@@ -19,6 +19,7 @@
 #include "libstore/globals.hh"
 #include "libstore/store-api.hh"
 #include "libutil/affinity.hh"
+#include "libutil/status.hh"
 #include "libutil/util.hh"
 #include "nix/legacy.hh"
 
@@ -358,7 +359,7 @@ static void _main(int argc, char** argv) {
     }
 
     if (!dryRun) {
-      store->buildPaths(paths, buildMode);
+      util::OkOrThrow(store->buildPaths(paths, buildMode));
     }
   };
 
diff --git a/third_party/nix/src/nix-daemon/nix-daemon-proto.cc b/third_party/nix/src/nix-daemon/nix-daemon-proto.cc
index f87f901be0..ec8e0ff42c 100644
--- a/third_party/nix/src/nix-daemon/nix-daemon-proto.cc
+++ b/third_party/nix/src/nix-daemon/nix-daemon-proto.cc
@@ -21,6 +21,7 @@
 #include "libstore/store-api.hh"
 #include "libutil/archive.hh"
 #include "libutil/hash.hh"
+#include "libutil/proto.hh"
 #include "libutil/serialise.hh"
 #include "libutil/types.hh"
 
@@ -288,9 +289,8 @@ class WorkerServiceImpl final : public WorkerService::Service {
           // TODO(grfn): If mode is repair and not trusted, we need to return an
           // error here (but we can't yet because we don't know anything about
           // trusted users)
-          store_->buildPaths(drvs, mode.value());
-
-          return Status::OK;
+          return nix::util::proto::AbslToGRPCStatus(
+              store_->buildPaths(drvs, mode.value()));
         },
         __FUNCTION__);
   }
diff --git a/third_party/nix/src/nix-env/nix-env.cc b/third_party/nix/src/nix-env/nix-env.cc
index a0fad3f3d1..396f65d0d6 100644
--- a/third_party/nix/src/nix-env/nix-env.cc
+++ b/third_party/nix/src/nix-env/nix-env.cc
@@ -23,6 +23,7 @@
 #include "libstore/profiles.hh"
 #include "libstore/store-api.hh"
 #include "libutil/json.hh"
+#include "libutil/status.hh"
 #include "libutil/util.hh"
 #include "libutil/xml-writer.hh"
 #include "nix-env/user-env.hh"
@@ -720,8 +721,8 @@ static void opSet(Globals& globals, Strings opFlags, Strings opArgs) {
     if (globals.dryRun) {
       return;
     }
-    globals.state->store->buildPaths(
-        paths, globals.state->repair != 0u ? bmRepair : bmNormal);
+    nix::util::OkOrThrow(globals.state->store->buildPaths(
+        paths, globals.state->repair != 0u ? bmRepair : bmNormal));
   } else {
     printMissing(globals.state->store, {drv.queryOutPath()});
     if (globals.dryRun) {
diff --git a/third_party/nix/src/nix-env/user-env.cc b/third_party/nix/src/nix-env/user-env.cc
index 06329e74f3..0cb921c824 100644
--- a/third_party/nix/src/nix-env/user-env.cc
+++ b/third_party/nix/src/nix-env/user-env.cc
@@ -9,6 +9,7 @@
 #include "libstore/globals.hh"
 #include "libstore/profiles.hh"
 #include "libstore/store-api.hh"
+#include "libutil/status.hh"
 #include "libutil/util.hh"
 
 namespace nix {
@@ -37,8 +38,8 @@ bool createUserEnv(EvalState& state, DrvInfos& elems, const Path& profile,
   }
 
   DLOG(INFO) << "building user environment dependencies";
-  state.store->buildPaths(drvsToBuild,
-                          state.repair != 0u ? bmRepair : bmNormal);
+  util::OkOrThrow(state.store->buildPaths(
+      drvsToBuild, state.repair != 0u ? bmRepair : bmNormal));
 
   /* Construct the whole top level derivation. */
   PathSet references;
@@ -137,8 +138,8 @@ bool createUserEnv(EvalState& state, DrvInfos& elems, const Path& profile,
 
   /* Realise the resulting store expression. */
   DLOG(INFO) << "building user environment";
-  state.store->buildPaths({topLevelDrv},
-                          state.repair != 0u ? bmRepair : bmNormal);
+  util::OkOrThrow(state.store->buildPaths(
+      {topLevelDrv}, state.repair != 0u ? bmRepair : bmNormal));
 
   /* Switch the current user environment to the output path. */
   auto store2 = state.store.dynamic_pointer_cast<LocalFSStore>();
diff --git a/third_party/nix/src/nix-store/nix-store.cc b/third_party/nix/src/nix-store/nix-store.cc
index 152d716641..a036fcec8b 100644
--- a/third_party/nix/src/nix-store/nix-store.cc
+++ b/third_party/nix/src/nix-store/nix-store.cc
@@ -16,6 +16,7 @@
 #include "libstore/worker-protocol.hh"
 #include "libutil/archive.hh"
 #include "libutil/monitor-fd.hh"
+#include "libutil/status.hh"
 #include "libutil/util.hh"
 #include "nix-store/dotgraph.hh"
 #include "nix-store/graphml.hh"
@@ -68,7 +69,7 @@ static PathSet realisePath(Path path, bool build = true) {
 
   if (isDerivation(p.first)) {
     if (build) {
-      store->buildPaths({path});
+      util::OkOrThrow(store->buildPaths({path}));
     }
     Derivation drv = store->derivationFromPath(p.first);
     rootNr++;
@@ -184,7 +185,8 @@ static void opRealise(Strings opFlags, Strings opArgs) {
   }
 
   /* Build all paths at the same time to exploit parallelism. */
-  store->buildPaths(PathSet(paths.begin(), paths.end()), buildMode);
+  util::OkOrThrow(
+      store->buildPaths(PathSet(paths.begin(), paths.end()), buildMode));
 
   if (!ignoreUnknown) {
     for (auto& i : paths) {
@@ -1002,7 +1004,7 @@ static void opServe(Strings opFlags, Strings opArgs) {
              does one path at a time. */
           if (!willSubstitute.empty()) {
             try {
-              store->buildPaths(willSubstitute);
+              util::OkOrThrow(store->buildPaths(willSubstitute));
             } catch (Error& e) {
               LOG(WARNING) << e.msg();
             }
@@ -1064,7 +1066,7 @@ static void opServe(Strings opFlags, Strings opArgs) {
 
         try {
           MonitorFdHup monitor(in.fd);
-          store->buildPaths(paths);
+          util::OkOrThrow(store->buildPaths(paths));
           out << 0;
         } catch (Error& e) {
           assert(e.status);
diff --git a/third_party/nix/src/nix/installables.cc b/third_party/nix/src/nix/installables.cc
index 5fe155ba16..b257a26f23 100644
--- a/third_party/nix/src/nix/installables.cc
+++ b/third_party/nix/src/nix/installables.cc
@@ -9,6 +9,7 @@
 #include "libmain/shared.hh"
 #include "libstore/derivations.hh"
 #include "libstore/store-api.hh"
+#include "libutil/status.hh"
 #include "nix/command.hh"
 
 namespace nix {
@@ -273,7 +274,7 @@ Buildables build(
   if (mode == DryRun) {
     printMissing(store, pathsToBuild);
   } else if (mode == Build) {
-    store->buildPaths(pathsToBuild);
+    util::OkOrThrow(store->buildPaths(pathsToBuild));
   }
 
   return buildables;