From 1d9d40b2a663464f1e6800d6de8df61433507423 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 20 Oct 2024 22:55:00 +0200 Subject: [PATCH] libstore: move failingExitStatus into worker results we already have a results type for the entire worker call, we may as well put this bit of info in there. we do keep the assumption of the old code that the field will only be read if some goals have failed; fixing that is a very different mess, and not immediately necessary. Change-Id: If3fc32649dcd88e1987cdd1758c6c5743e3b35ac --- src/libstore/build/entry-points.cc | 28 ++++++++--------- src/libstore/build/worker.cc | 42 ++++++++++++------------- src/libstore/build/worker.hh | 50 ++++++++++++++++-------------- 3 files changed, 60 insertions(+), 60 deletions(-) diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index 808179a4d..2e91b1778 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -16,7 +16,7 @@ void Store::buildPaths(const std::vector & reqs, BuildMode buildMod auto aio = kj::setupAsyncIo(); Worker worker(*this, evalStore ? *evalStore : *this, aio); - auto goals = runWorker(worker, [&](GoalFactory & gf) { + auto results = runWorker(worker, [&](GoalFactory & gf) { Worker::Targets goals; for (auto & br : reqs) goals.emplace(gf.makeGoal(br, buildMode)); @@ -25,7 +25,7 @@ void Store::buildPaths(const std::vector & reqs, BuildMode buildMod StringSet failed; std::shared_ptr ex; - for (auto & [i, result] : goals) { + for (auto & [i, result] : results.goals) { if (result.ex) { if (ex) logError(result.ex->info()); @@ -41,11 +41,11 @@ void Store::buildPaths(const std::vector & reqs, BuildMode buildMod } if (failed.size() == 1 && ex) { - ex->withExitStatus(worker.failingExitStatus()); + ex->withExitStatus(results.failingExitStatus); throw std::move(*ex); } else if (!failed.empty()) { if (ex) logError(ex->info()); - throw Error(worker.failingExitStatus(), "build of %s failed", concatStringsSep(", ", quoteStrings(failed))); + throw Error(results.failingExitStatus, "build of %s failed", concatStringsSep(", ", quoteStrings(failed))); } } @@ -67,7 +67,7 @@ std::vector Store::buildPathsWithResults( goals.emplace(std::move(goal)); } return goals; - }); + }).goals; std::vector results; @@ -84,12 +84,12 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat Worker worker(*this, *this, aio); try { - auto goals = runWorker(worker, [&](GoalFactory & gf) { + auto results = runWorker(worker, [&](GoalFactory & gf) { Worker::Targets goals; goals.emplace(gf.makeBasicDerivationGoal(drvPath, drv, OutputsSpec::All{}, buildMode)); return goals; }); - auto [goal, result] = *goals.begin(); + auto [goal, result] = *results.goals.begin(); return result.result.restrictTo(DerivedPath::Built { .drvPath = makeConstantStorePathRef(drvPath), .outputs = OutputsSpec::All {}, @@ -111,19 +111,19 @@ void Store::ensurePath(const StorePath & path) auto aio = kj::setupAsyncIo(); Worker worker(*this, *this, aio); - auto goals = runWorker(worker, [&](GoalFactory & gf) { + auto results = runWorker(worker, [&](GoalFactory & gf) { Worker::Targets goals; goals.emplace(gf.makePathSubstitutionGoal(path)); return goals; }); - auto [goal, result] = *goals.begin(); + auto [goal, result] = *results.goals.begin(); if (result.exitCode != Goal::ecSuccess) { if (result.ex) { - result.ex->withExitStatus(worker.failingExitStatus()); + result.ex->withExitStatus(results.failingExitStatus); throw std::move(*result.ex); } else - throw Error(worker.failingExitStatus(), "path '%s' does not exist and cannot be created", printStorePath(path)); + throw Error(results.failingExitStatus, "path '%s' does not exist and cannot be created", printStorePath(path)); } } @@ -133,12 +133,12 @@ void Store::repairPath(const StorePath & path) auto aio = kj::setupAsyncIo(); Worker worker(*this, *this, aio); - auto goals = runWorker(worker, [&](GoalFactory & gf) { + auto results = runWorker(worker, [&](GoalFactory & gf) { Worker::Targets goals; goals.emplace(gf.makePathSubstitutionGoal(path, Repair)); return goals; }); - auto [goal, result] = *goals.begin(); + auto [goal, result] = *results.goals.begin(); if (result.exitCode != Goal::ecSuccess) { /* Since substituting the path didn't work, if we have a valid @@ -158,7 +158,7 @@ void Store::repairPath(const StorePath & path) return goals; }); } else - throw Error(worker.failingExitStatus(), "cannot repair path '%s'", printStorePath(path)); + throw Error(results.failingExitStatus, "cannot repair path '%s'", printStorePath(path)); } } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 10f58f5d3..33bee1019 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -270,7 +270,7 @@ try { while (auto done = co_await collect.next()) { // propagate goal exceptions outward BOOST_OUTCOME_CO_TRY(auto result, done->second); - results.emplace(done->first, result); + results.goals.emplace(done->first, result); /* If a top-level goal failed, then kill all other goals (unless keepGoing was set). */ @@ -285,6 +285,25 @@ try { --keep-going *is* set, then they must all be finished now. */ assert(!settings.keepGoing || children.isEmpty()); + results.failingExitStatus = [&] { + // See API docs in header for explanation + unsigned int mask = 0; + bool buildFailure = permanentFailure || timedOut || hashMismatch; + if (buildFailure) + mask |= 0x04; // 100 + if (timedOut) + mask |= 0x01; // 101 + if (hashMismatch) + mask |= 0x02; // 102 + if (checkMismatch) { + mask |= 0x08; // 104 + } + + if (mask) + mask |= 0x60; + return mask ? mask : 1; + }(); + co_return std::move(results); } catch (...) { co_return result::failure(std::current_exception()); @@ -301,27 +320,6 @@ try { } -unsigned int Worker::failingExitStatus() -{ - // See API docs in header for explanation - unsigned int mask = 0; - bool buildFailure = permanentFailure || timedOut || hashMismatch; - if (buildFailure) - mask |= 0x04; // 100 - if (timedOut) - mask |= 0x01; // 101 - if (hashMismatch) - mask |= 0x02; // 102 - if (checkMismatch) { - mask |= 0x08; // 104 - } - - if (mask) - mask |= 0x60; - return mask ? mask : 1; -} - - bool Worker::pathContentsGood(const StorePath & path) { auto i = pathContentsGoodCache.find(path); diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index 1a913ca16..e80981876 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -86,7 +86,32 @@ class Worker : public WorkerBase { public: using Targets = std::map>>; - using Results = std::map; + struct Results { + std::map goals; + + /** + * The exit status in case of failure. + * + * In the case of a build failure, returned value follows this + * bitmask: + * + * ``` + * 0b1100100 + * ^^^^ + * |||`- timeout + * ||`-- output hash mismatch + * |`--- build failure + * `---- not deterministic + * ``` + * + * In other words, the failure code is at least 100 (0b1100100), but + * might also be greater. + * + * Otherwise (no build failure, but some other sort of failure by + * assumption), this returned value is 1. + */ + unsigned int failingExitStatus; + }; private: @@ -248,29 +273,6 @@ public: */ Results run(std::function req); - /*** - * The exit status in case of failure. - * - * In the case of a build failure, returned value follows this - * bitmask: - * - * ``` - * 0b1100100 - * ^^^^ - * |||`- timeout - * ||`-- output hash mismatch - * |`--- build failure - * `---- not deterministic - * ``` - * - * In other words, the failure code is at least 100 (0b1100100), but - * might also be greater. - * - * Otherwise (no build failure, but some other sort of failure by - * assumption), this returned value is 1. - */ - unsigned int failingExitStatus(); - /** * Check whether the given valid path exists and has the right * contents.