From d1c38d9597e110bef92a548c86a651174bd385dc Mon Sep 17 00:00:00 2001 From: Griffin Smith Date: Thu, 13 Aug 2020 22:06:23 -0400 Subject: refactor(tvix): Make Store::buildPaths return a Status 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 --- third_party/nix/src/libexpr/primops.cc | 3 +- third_party/nix/src/libstore/build.cc | 10 +++- third_party/nix/src/libstore/local-store.hh | 3 +- third_party/nix/src/libstore/remote-store.cc | 9 ++- third_party/nix/src/libstore/remote-store.hh | 2 +- third_party/nix/src/libstore/rpc-store.cc | 6 +- third_party/nix/src/libstore/rpc-store.hh | 4 +- third_party/nix/src/libstore/store-api.cc | 11 +++- third_party/nix/src/libstore/store-api.hh | 3 +- third_party/nix/src/libutil/CMakeLists.txt | 1 + third_party/nix/src/libutil/proto.hh | 64 +++++++++++++++++++++- third_party/nix/src/libutil/status.hh | 17 ++++++ third_party/nix/src/nix-build/nix-build.cc | 3 +- third_party/nix/src/nix-daemon/nix-daemon-proto.cc | 6 +- third_party/nix/src/nix-env/nix-env.cc | 5 +- third_party/nix/src/nix-env/user-env.cc | 9 +-- third_party/nix/src/nix-store/nix-store.cc | 10 ++-- third_party/nix/src/nix/installables.cc | 3 +- 18 files changed, 137 insertions(+), 32 deletions(-) create mode 100644 third_party/nix/src/libutil/status.hh (limited to 'third_party/nix') 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 #include +#include #include #include #include @@ -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 #include +#include #include #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 #include +#include #include #include #include @@ -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 #include +#include #include #include #include @@ -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 srcStore, const ref& 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, 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 #include +#include #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 +#include +#include + +#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(); 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; -- cgit 1.4.1