chore(tvix): Thread a std::ostream through Store::buildPaths r/1689

This part of the store API needs to carry a handle to the log sink
from now on, so that it can be passed in as appropriate from the gRPC
handlers.

In all places where there is no such handler available at the moment,
the discarding log sink has been inserted. This can be used as a
convenient grep target in the future.

Change-Id: I26628e30b4c6437dccdf8f722ca2e8ed827dfc19
Reviewed-on: https://cl.tvl.fyi/c/depot/+/1797
Tested-by: BuildkiteCI
Reviewed-by: kanepyork <rikingcoding@gmail.com>
Reviewed-by: glittershark <grfn@gws.fyi>

15 files changed, 63 insertions, 43 deletions

diff --git a/third_party/nix/src/libexpr/primops.cc b/third_party/nix/src/libexpr/primops.cc
index 7819dd4b13..1684c20413 100644
--- a/third_party/nix/src/libexpr/primops.cc
+++ b/third_party/nix/src/libexpr/primops.cc
@@ -91,7 +91,12 @@ void EvalState::realiseContext(const PathSet& context) {
   unsigned long long narSize;
   store->queryMissing(drvs, willBuild, willSubstitute, unknown, downloadSize,
                       narSize);
-  nix::util::OkOrThrow(store->buildPaths(drvs));
+
+  // TODO(tazjin): Figure out where these logs are supposed to go ...
+  // unless we keep a per-store stream open persistently there's no
+  // "generic" way to send logs anywhere for cases like this (IFD).
+  auto discard_logs = DiscardLogsSink();
+  nix::util::OkOrThrow(store->buildPaths(discard_logs, 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 91f80e51e0..4b9232b1a2 100644
--- a/third_party/nix/src/libstore/build.cc
+++ b/third_party/nix/src/libstore/build.cc
@@ -7,6 +7,7 @@
 #include <iostream>
 #include <map>
 #include <memory>
+#include <ostream>
 #include <queue>
 #include <regex>
 #include <sstream>
@@ -4687,8 +4688,9 @@ static void primeCache(Store& store, const PathSet& paths) {
   }
 }
 
-absl::Status LocalStore::buildPaths(const PathSet& drvPaths,
-                                    BuildMode buildMode) {
+absl::Status LocalStore::buildPaths(std::ostream& /* log_sink */,
+                                    const PathSet& drvPaths,
+                                    BuildMode build_mode) {
   Worker worker(*this);
 
   primeCache(*this, drvPaths);
@@ -4697,10 +4699,10 @@ absl::Status LocalStore::buildPaths(const PathSet& drvPaths,
   for (auto& i : drvPaths) {
     DrvPathWithOutputs i2 = parseDrvPathWithOutputs(i);
     if (isDerivation(i2.first)) {
-      goals.insert(worker.makeDerivationGoal(i2.first, i2.second, buildMode));
+      goals.insert(worker.makeDerivationGoal(i2.first, i2.second, build_mode));
     } else {
       goals.insert(worker.makeSubstitutionGoal(
-          i, buildMode == bmRepair ? Repair : NoRepair));
+          i, build_mode == bmRepair ? Repair : NoRepair));
     }
   }
 
diff --git a/third_party/nix/src/libstore/local-store.hh b/third_party/nix/src/libstore/local-store.hh
index f9e08e5e6b..669b878b66 100644
--- a/third_party/nix/src/libstore/local-store.hh
+++ b/third_party/nix/src/libstore/local-store.hh
@@ -156,7 +156,8 @@ class LocalStore : public LocalFSStore {
   Path addTextToStore(const std::string& name, const std::string& s,
                       const PathSet& references, RepairFlag repair) override;
 
-  absl::Status buildPaths(const PathSet& paths, BuildMode buildMode) override;
+  absl::Status buildPaths(std::ostream& log_sink, const PathSet& paths,
+                          BuildMode build_mode) 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 6948d4e61d..e99c442c3d 100644
--- a/third_party/nix/src/libstore/remote-store.cc
+++ b/third_party/nix/src/libstore/remote-store.cc
@@ -460,18 +460,19 @@ Path RemoteStore::addTextToStore(const std::string& name, const std::string& s,
   return readStorePath(*this, conn->from);
 }
 
-absl::Status RemoteStore::buildPaths(const PathSet& drvPaths,
-                                     BuildMode buildMode) {
+absl::Status RemoteStore::buildPaths(std::ostream& /* log_sink */,
+                                     const PathSet& drvPaths,
+                                     BuildMode build_mode) {
   auto conn(getConnection());
   conn->to << wopBuildPaths;
   if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 13) {
     conn->to << drvPaths;
     if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 15) {
-      conn->to << buildMode;
+      conn->to << build_mode;
     } else
         /* Old daemons did not take a 'buildMode' parameter, so we
            need to validate it here on the client side.  */
-        if (buildMode != bmNormal) {
+        if (build_mode != bmNormal) {
       return absl::Status(
           absl::StatusCode::kInvalidArgument,
           "repairing or checking is not supported when building through the "
diff --git a/third_party/nix/src/libstore/remote-store.hh b/third_party/nix/src/libstore/remote-store.hh
index 4aad5a5d2f..60da6045a8 100644
--- a/third_party/nix/src/libstore/remote-store.hh
+++ b/third_party/nix/src/libstore/remote-store.hh
@@ -71,7 +71,8 @@ class RemoteStore : public virtual Store {
   Path addTextToStore(const std::string& name, const std::string& s,
                       const PathSet& references, RepairFlag repair) override;
 
-  absl::Status buildPaths(const PathSet& paths, BuildMode buildMode) override;
+  absl::Status buildPaths(std::ostream& log_sink, const PathSet& paths,
+                          BuildMode build_mode) 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 f3fc5582f3..92ad4e762b 100644
--- a/third_party/nix/src/libstore/rpc-store.cc
+++ b/third_party/nix/src/libstore/rpc-store.cc
@@ -4,6 +4,7 @@
 #include <filesystem>
 #include <memory>
 #include <optional>
+#include <ostream>
 
 #include <absl/status/status.h>
 #include <absl/strings/str_cat.h>
@@ -318,7 +319,8 @@ Path RpcStore::addTextToStore(const std::string& name,
   return result.path();
 }
 
-absl::Status RpcStore::buildPaths(const PathSet& paths, BuildMode buildMode) {
+absl::Status RpcStore::buildPaths(std::ostream& log_sink, const PathSet& paths,
+                                  BuildMode build_mode) {
   ClientContext ctx;
   proto::BuildPathsRequest request;
   for (const auto& path : paths) {
@@ -326,14 +328,7 @@ absl::Status RpcStore::buildPaths(const PathSet& paths, BuildMode buildMode) {
   }
 
   google::protobuf::Empty response;
-  request.set_mode(nix::BuildModeToProto(buildMode));
-
-  // TODO(tazjin): Temporary no-op sink used to discard build output,
-  // but satisfy the compiler. A real one is needed.
-  //
-  // Maybe this should just be stderr, considering that this is the
-  // *client*, but I'm not sure.
-  std::ostream discard_logs = DiscardLogsSink();
+  request.set_mode(nix::BuildModeToProto(build_mode));
 
   std::unique_ptr<grpc::ClientReader<proto::BuildEvent>> reader =
       stub_->BuildPaths(&ctx, request);
@@ -342,11 +337,11 @@ absl::Status RpcStore::buildPaths(const PathSet& paths, BuildMode buildMode) {
   while (reader->Read(&event)) {
     if (event.has_build_log()) {
       // TODO(tazjin): Include .path()?
-      discard_logs << event.build_log().line();
+      log_sink << event.build_log().line();
     } else {
-      discard_logs << std::endl
-                   << "Building path: " << event.building_path().path()
-                   << std::endl;
+      log_sink << std::endl
+               << "Building path: " << event.building_path().path()
+               << std::endl;
     }
 
     // has_result() is not in use in this call (for now)
diff --git a/third_party/nix/src/libstore/rpc-store.hh b/third_party/nix/src/libstore/rpc-store.hh
index b1c2945323..3a156e9e1a 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 absl::Status buildPaths(const PathSet& paths,
-                                  BuildMode buildMode) override;
+  absl::Status buildPaths(std::ostream& log_sink, const PathSet& paths,
+                          BuildMode build_mode) 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 7972f50836..0ea6b1d62c 100644
--- a/third_party/nix/src/libstore/store-api.cc
+++ b/third_party/nix/src/libstore/store-api.cc
@@ -714,7 +714,8 @@ const Store::Stats& Store::getStats() {
   return stats;
 }
 
-absl::Status Store::buildPaths(const PathSet& paths, BuildMode) {
+absl::Status Store::buildPaths(std::ostream& /* log_sink */,
+                               const PathSet& paths, BuildMode) {
   for (auto& path : paths) {
     if (isDerivation(path)) {
       return absl::Status(absl::StatusCode::kUnimplemented,
diff --git a/third_party/nix/src/libstore/store-api.hh b/third_party/nix/src/libstore/store-api.hh
index 6c125ad276..91dd54cfe5 100644
--- a/third_party/nix/src/libstore/store-api.hh
+++ b/third_party/nix/src/libstore/store-api.hh
@@ -455,11 +455,13 @@ 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. */
-  [[nodiscard]] virtual absl::Status buildPaths(const PathSet& paths,
-                                                BuildMode buildMode);
+  [[nodiscard]] virtual absl::Status buildPaths(std::ostream& log_sink,
+                                                const PathSet& paths,
+                                                BuildMode build_mode);
 
-  [[nodiscard]] absl::Status buildPaths(const PathSet& paths) {
-    return buildPaths(paths, bmNormal);
+  [[nodiscard]] absl::Status buildPaths(std::ostream& log_sink,
+                                        const PathSet& paths) {
+    return buildPaths(log_sink, paths, bmNormal);
   }
 
   /* Build a single non-materialized derivation (i.e. not from an
diff --git a/third_party/nix/src/nix-build/nix-build.cc b/third_party/nix/src/nix-build/nix-build.cc
index 1fb8a2f3ad..cc2d8bc2fb 100644
--- a/third_party/nix/src/nix-build/nix-build.cc
+++ b/third_party/nix/src/nix-build/nix-build.cc
@@ -359,7 +359,8 @@ static void _main(int argc, char** argv) {
     }
 
     if (!dryRun) {
-      util::OkOrThrow(store->buildPaths(paths, buildMode));
+      auto discard_logs = DiscardLogsSink();
+      util::OkOrThrow(store->buildPaths(discard_logs, 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 cbc5452a71..6d18fc4096 100644
--- a/third_party/nix/src/nix-daemon/nix-daemon-proto.cc
+++ b/third_party/nix/src/nix-daemon/nix-daemon-proto.cc
@@ -290,7 +290,7 @@ class WorkerServiceImpl final : public WorkerService::Service {
 
   Status BuildPaths(
       grpc::ServerContext*, const nix::proto::BuildPathsRequest* request,
-      grpc::ServerWriter<nix::proto::BuildEvent>* /* writer */) override {
+      grpc::ServerWriter<nix::proto::BuildEvent>* writer) override {
     return HandleExceptions(
         [&]() -> Status {
           PathSet drvs;
@@ -303,11 +303,14 @@ class WorkerServiceImpl final : public WorkerService::Service {
             return Status(grpc::StatusCode::INTERNAL, "Invalid build mode");
           }
 
+          BuildLogStreambuf log_buffer(writer);
+          std::ostream log_sink(&log_buffer);
+
           // 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)
           return nix::util::proto::AbslToGRPCStatus(
-              store_->buildPaths(drvs, mode.value()));
+              store_->buildPaths(log_sink, 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 7502a648d8..e42fc29c22 100644
--- a/third_party/nix/src/nix-env/nix-env.cc
+++ b/third_party/nix/src/nix-env/nix-env.cc
@@ -722,8 +722,10 @@ static void opSet(Globals& globals, Strings opFlags, Strings opArgs) {
     if (globals.dryRun) {
       return;
     }
+    auto discard_logs = DiscardLogsSink();
     nix::util::OkOrThrow(globals.state->store->buildPaths(
-        paths, globals.state->repair != 0u ? bmRepair : bmNormal));
+        discard_logs, 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 dac0c52a81..e3124a60e3 100644
--- a/third_party/nix/src/nix-env/user-env.cc
+++ b/third_party/nix/src/nix-env/user-env.cc
@@ -38,8 +38,9 @@ bool createUserEnv(EvalState& state, DrvInfos& elems, const Path& profile,
   }
 
   DLOG(INFO) << "building user environment dependencies";
+  auto discard_logs = DiscardLogsSink();
   util::OkOrThrow(state.store->buildPaths(
-      drvsToBuild, state.repair != 0u ? bmRepair : bmNormal));
+      discard_logs, drvsToBuild, state.repair != 0u ? bmRepair : bmNormal));
 
   /* Construct the whole top level derivation. */
   PathSet references;
@@ -139,7 +140,7 @@ bool createUserEnv(EvalState& state, DrvInfos& elems, const Path& profile,
   /* Realise the resulting store expression. */
   DLOG(INFO) << "building user environment";
   util::OkOrThrow(state.store->buildPaths(
-      {topLevelDrv}, state.repair != 0u ? bmRepair : bmNormal));
+      discard_logs, {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 a036fcec8b..101ab01ef9 100644
--- a/third_party/nix/src/nix-store/nix-store.cc
+++ b/third_party/nix/src/nix-store/nix-store.cc
@@ -69,7 +69,8 @@ static PathSet realisePath(Path path, bool build = true) {
 
   if (isDerivation(p.first)) {
     if (build) {
-      util::OkOrThrow(store->buildPaths({path}));
+      auto discard_logs = DiscardLogsSink();
+      util::OkOrThrow(store->buildPaths(discard_logs, {path}));
     }
     Derivation drv = store->derivationFromPath(p.first);
     rootNr++;
@@ -185,8 +186,9 @@ static void opRealise(Strings opFlags, Strings opArgs) {
   }
 
   /* Build all paths at the same time to exploit parallelism. */
-  util::OkOrThrow(
-      store->buildPaths(PathSet(paths.begin(), paths.end()), buildMode));
+  auto discard_logs = DiscardLogsSink();
+  util::OkOrThrow(store->buildPaths(
+      discard_logs, PathSet(paths.begin(), paths.end()), buildMode));
 
   if (!ignoreUnknown) {
     for (auto& i : paths) {
@@ -1004,7 +1006,8 @@ static void opServe(Strings opFlags, Strings opArgs) {
              does one path at a time. */
           if (!willSubstitute.empty()) {
             try {
-              util::OkOrThrow(store->buildPaths(willSubstitute));
+              auto discard_logs = DiscardLogsSink();
+              util::OkOrThrow(store->buildPaths(discard_logs, willSubstitute));
             } catch (Error& e) {
               LOG(WARNING) << e.msg();
             }
@@ -1066,7 +1069,8 @@ static void opServe(Strings opFlags, Strings opArgs) {
 
         try {
           MonitorFdHup monitor(in.fd);
-          util::OkOrThrow(store->buildPaths(paths));
+          auto discard_logs = DiscardLogsSink();
+          util::OkOrThrow(store->buildPaths(discard_logs, 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 dd1202586a..b5980ce476 100644
--- a/third_party/nix/src/nix/installables.cc
+++ b/third_party/nix/src/nix/installables.cc
@@ -274,7 +274,8 @@ Buildables build(
   if (mode == DryRun) {
     printMissing(store, pathsToBuild);
   } else if (mode == Build) {
-    util::OkOrThrow(store->buildPaths(pathsToBuild));
+    auto discard_logs = DiscardLogsSink();
+    util::OkOrThrow(store->buildPaths(discard_logs, pathsToBuild));
   }
 
   return buildables;