From 19e874a9854c0d7f49d79fa98177a84b6997ce9a Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Thu, 20 Aug 2020 02:32:51 +0100 Subject: feat(tvix): Introduce build event streams in worker protocol Introduces a new `BuildEvent` proto type which is streamed in response to calls that trigger builds of derivations. This type can currently supply build statuses, log lines and information about builds starting. This is in preparation for threading build logs through the processes. Since we have nowhere to send the logs (yet), a null sink is used instead. Co-authored-by: Griffin Smith Change-Id: If7332337b89506c7e404cd20174acdaa1a3be4e8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1793 Tested-by: BuildkiteCI Reviewed-by: glittershark Reviewed-by: kanepyork --- third_party/nix/src/libstore/rpc-store.cc | 51 +++++++++++++++++++--- third_party/nix/src/libstore/store-api.cc | 4 +- third_party/nix/src/libstore/store-api.hh | 4 +- third_party/nix/src/nix-daemon/nix-daemon-proto.cc | 23 ++++++---- third_party/nix/src/proto/worker.proto | 35 ++++++++++++--- 5 files changed, 92 insertions(+), 25 deletions(-) (limited to 'third_party') diff --git a/third_party/nix/src/libstore/rpc-store.cc b/third_party/nix/src/libstore/rpc-store.cc index 2d1278f40c..f3fc5582f3 100644 --- a/third_party/nix/src/libstore/rpc-store.cc +++ b/third_party/nix/src/libstore/rpc-store.cc @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -326,8 +327,32 @@ absl::Status RpcStore::buildPaths(const PathSet& paths, BuildMode buildMode) { google::protobuf::Empty response; request.set_mode(nix::BuildModeToProto(buildMode)); - return nix::util::proto::GRPCStatusToAbsl( - stub_->BuildPaths(&ctx, request, &response)); + + // 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(); + + std::unique_ptr> reader = + stub_->BuildPaths(&ctx, request); + + proto::BuildEvent event; + while (reader->Read(&event)) { + if (event.has_build_log()) { + // TODO(tazjin): Include .path()? + discard_logs << event.build_log().line(); + } else { + discard_logs << std::endl + << "Building path: " << event.building_path().path() + << std::endl; + } + + // has_result() is not in use in this call (for now) + } + + return nix::util::proto::GRPCStatusToAbsl(reader->Finish()); } BuildResult RpcStore::buildDerivation(const Path& drvPath, @@ -339,11 +364,25 @@ BuildResult RpcStore::buildDerivation(const Path& drvPath, auto proto_drv = drv.to_proto(); request.set_allocated_derivation(&proto_drv); request.set_build_mode(BuildModeToProto(buildMode)); - proto::BuildDerivationResponse response; - SuccessOrThrow(stub_->BuildDerivation(&ctx, request, &response), - __FUNCTION__); - const auto result = BuildResult::FromProto(response); + // Same note as in ::buildPaths ... + std::ostream discard_logs = DiscardLogsSink(); + + std::unique_ptr> reader = + stub_->BuildDerivation(&ctx, request); + + std::optional result; + + proto::BuildEvent event; + while (reader->Read(&event)) { + if (event.has_build_log()) { + discard_logs << event.build_log().line(); + } else if (event.has_result()) { + result = BuildResult::FromProto(event.result()); + } + } + SuccessOrThrow(reader->Finish(), __FUNCTION__); + if (!result.has_value()) { throw Error("Invalid response from daemon for buildDerivation"); } diff --git a/third_party/nix/src/libstore/store-api.cc b/third_party/nix/src/libstore/store-api.cc index 3ada63532c..7972f50836 100644 --- a/third_party/nix/src/libstore/store-api.cc +++ b/third_party/nix/src/libstore/store-api.cc @@ -92,7 +92,7 @@ nix::proto::BuildStatus BuildResult::status_to_proto() { } std::optional BuildResult::FromProto( - const nix::proto::BuildDerivationResponse& resp) { + const nix::proto::BuildResult& resp) { BuildResult result; switch (resp.status()) { case proto::BuildStatus::Built: @@ -125,7 +125,7 @@ std::optional BuildResult::FromProto( return {}; } - result.errorMsg = resp.error_message(); + result.errorMsg = resp.msg(); return result; } diff --git a/third_party/nix/src/libstore/store-api.hh b/third_party/nix/src/libstore/store-api.hh index 39769dfb3c..1d81b228f2 100644 --- a/third_party/nix/src/libstore/store-api.hh +++ b/third_party/nix/src/libstore/store-api.hh @@ -189,7 +189,7 @@ struct ValidPathInfo { virtual ~ValidPathInfo() {} }; -typedef std::list ValidPathInfos; +using ValidPathInfos = std::list; enum BuildMode { bmNormal, bmRepair, bmCheck }; @@ -243,7 +243,7 @@ struct BuildResult { nix::proto::BuildStatus status_to_proto(); static std::optional FromProto( - const nix::proto::BuildDerivationResponse& resp); + const nix::proto::BuildResult& resp); }; class Store : public std::enable_shared_from_this, public Config { 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 ec8e0ff42c..b7d5836091 100644 --- a/third_party/nix/src/nix-daemon/nix-daemon-proto.cc +++ b/third_party/nix/src/nix-daemon/nix-daemon-proto.cc @@ -29,7 +29,6 @@ namespace nix::daemon { using ::google::protobuf::util::TimeUtil; using ::grpc::Status; -using ::nix::proto::BuildStatus; using ::nix::proto::PathInfo; using ::nix::proto::StorePath; using ::nix::proto::StorePaths; @@ -271,9 +270,9 @@ class WorkerServiceImpl final : public WorkerService::Service { __FUNCTION__); } - Status BuildPaths(grpc::ServerContext*, - const nix::proto::BuildPathsRequest* request, - google::protobuf::Empty*) override { + Status BuildPaths( + grpc::ServerContext*, const nix::proto::BuildPathsRequest* request, + grpc::ServerWriter* /* writer */) override { return HandleExceptions( [&]() -> Status { PathSet drvs; @@ -612,7 +611,7 @@ class WorkerServiceImpl final : public WorkerService::Service { Status BuildDerivation( grpc::ServerContext* context, const nix::proto::BuildDerivationRequest* request, - nix::proto::BuildDerivationResponse* response) override { + grpc::ServerWriter* writer) override { return HandleExceptions( [&]() -> Status { auto drv_path = request->drv_path().path(); @@ -625,11 +624,19 @@ class WorkerServiceImpl final : public WorkerService::Service { return Status(grpc::StatusCode::INTERNAL, "Invalid build mode"); } - auto res = store_->buildDerivation(drv_path, drv, *build_mode); + BuildResult res = store_->buildDerivation(drv_path, drv, *build_mode); + + proto::BuildResult proto_res{}; + proto_res.set_status(res.status_to_proto()); + + if (!res.errorMsg.empty()) { + proto_res.set_msg(res.errorMsg); + } - response->set_status(res.status_to_proto()); - response->set_error_message(res.errorMsg); + proto::BuildEvent event{}; + *event.mutable_result() = proto_res; + writer->Write(event); return Status::OK; }, __FUNCTION__); diff --git a/third_party/nix/src/proto/worker.proto b/third_party/nix/src/proto/worker.proto index fe89b292a2..6a80b3e32b 100644 --- a/third_party/nix/src/proto/worker.proto +++ b/third_party/nix/src/proto/worker.proto @@ -27,7 +27,7 @@ service WorkerService { // Build the specified derivations in one of the specified build // modes, defaulting to a normal build. - rpc BuildPaths(BuildPathsRequest) returns (google.protobuf.Empty); + rpc BuildPaths(BuildPathsRequest) returns (stream BuildEvent); // TODO: What does this do? rpc EnsurePath(StorePath) returns (google.protobuf.Empty); @@ -90,7 +90,7 @@ service WorkerService { // Build a single non-materialized derivation (i.e. not from an // on-disk .drv file). - rpc BuildDerivation(BuildDerivationRequest) returns (BuildDerivationResponse); + rpc BuildDerivation(BuildDerivationRequest) returns (stream BuildEvent); // Add signatures to the specified store path. The signatures are not // verified. @@ -174,6 +174,32 @@ message Signatures { repeated string sigs = 1; } +// Represents the outcome of a build for a single store path. +message BuildResult { + StorePath path = 1; + BuildStatus status = 2; + string msg = 3; +} + +// Represents an event occuring during a build. +message BuildEvent { + message LogLine { + string line = 1; + StorePath path = 2; + } + + oneof result_type { + // Build for a store path has finished + BuildResult result = 1; + + // A line of build log output was produced + LogLine build_log = 2; + + // Build for a store path has started + StorePath building_path = 3; + } +} + message IsValidPathResponse { bool is_valid = 1; } @@ -316,11 +342,6 @@ message BuildDerivationRequest { BuildMode build_mode = 3; } -message BuildDerivationResponse { - BuildStatus status = 1; - string error_message = 2; -} - message AddSignaturesRequest { StorePath path = 1; Signatures sigs = 2; -- cgit 1.4.1