From a5c1e73fa8e004a93e37254a3582ba91048c4550 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 25 Aug 2024 13:41:56 +0200 Subject: [PATCH] libstore: add "is dependency" info to goal whether goal errors are reported via the `ex` member or just printed to the log depends on whether the goal is a toplevel goal or a dependency. if goals are aware of this themselves we can move error printing out of the worker loop, and since a running worker can only be used by running goals it's totally sufficient to keep a `Worker::running` flag for this Change-Id: I6b5cbe6eccee1afa5fde80653c4b968554ddd16f --- src/libstore/build/derivation-goal.cc | 8 ++-- src/libstore/build/derivation-goal.hh | 4 +- .../build/drv-output-substitution-goal.cc | 3 +- .../build/drv-output-substitution-goal.hh | 8 +++- src/libstore/build/goal.hh | 10 ++++- src/libstore/build/local-derivation-goal.hh | 2 + src/libstore/build/substitution-goal.cc | 10 ++++- src/libstore/build/substitution-goal.hh | 8 +++- src/libstore/build/worker.cc | 45 ++++++++++++++----- src/libstore/build/worker.hh | 2 + src/libstore/platform.cc | 18 ++++---- 11 files changed, 86 insertions(+), 32 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 3ed3cb6bd..462c381b8 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -58,8 +58,8 @@ namespace nix { DerivationGoal::DerivationGoal(const StorePath & drvPath, - const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode) - : Goal(worker) + const OutputsSpec & wantedOutputs, Worker & worker, bool isDependency, BuildMode buildMode) + : Goal(worker, isDependency) , useDerivation(true) , drvPath(drvPath) , wantedOutputs(wantedOutputs) @@ -76,8 +76,8 @@ DerivationGoal::DerivationGoal(const StorePath & drvPath, DerivationGoal::DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv, - const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode) - : Goal(worker) + const OutputsSpec & wantedOutputs, Worker & worker, bool isDependency, BuildMode buildMode) + : Goal(worker, isDependency) , useDerivation(false) , drvPath(drvPath) , wantedOutputs(wantedOutputs) diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index c9638c2b6..77f9fef4b 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -234,10 +234,10 @@ struct DerivationGoal : public Goal std::string machineName; DerivationGoal(const StorePath & drvPath, - const OutputsSpec & wantedOutputs, Worker & worker, + const OutputsSpec & wantedOutputs, Worker & worker, bool isDependency, BuildMode buildMode = bmNormal); DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv, - const OutputsSpec & wantedOutputs, Worker & worker, + const OutputsSpec & wantedOutputs, Worker & worker, bool isDependency, BuildMode buildMode = bmNormal); virtual ~DerivationGoal() noexcept(false); diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index e6c7524e9..54d81d7a0 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -9,9 +9,10 @@ namespace nix { DrvOutputSubstitutionGoal::DrvOutputSubstitutionGoal( const DrvOutput & id, Worker & worker, + bool isDependency, RepairFlag repair, std::optional ca) - : Goal(worker) + : Goal(worker, isDependency) , id(id) { state = &DrvOutputSubstitutionGoal::init; diff --git a/src/libstore/build/drv-output-substitution-goal.hh b/src/libstore/build/drv-output-substitution-goal.hh index a660a4f3e..b48c4670b 100644 --- a/src/libstore/build/drv-output-substitution-goal.hh +++ b/src/libstore/build/drv-output-substitution-goal.hh @@ -56,7 +56,13 @@ class DrvOutputSubstitutionGoal : public Goal { bool substituterFailed = false; public: - DrvOutputSubstitutionGoal(const DrvOutput& id, Worker & worker, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); + DrvOutputSubstitutionGoal( + const DrvOutput & id, + Worker & worker, + bool isDependency, + RepairFlag repair = NoRepair, + std::optional ca = std::nullopt + ); typedef WorkResult (DrvOutputSubstitutionGoal::*GoalState)(bool inBuildSlot); GoalState state; diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index 1bfea8231..1f25fb233 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -60,6 +60,13 @@ struct Goal */ Worker & worker; + /** + * Whether this goal is only a dependency of other goals. Toplevel + * goals that are also dependencies of other toplevel goals do not + * set this, only goals that are exclusively dependencies do this. + */ + const bool isDependency; + /** * Goals that this goal is waiting for. */ @@ -143,8 +150,9 @@ public: */ std::shared_ptr ex; - explicit Goal(Worker & worker) + explicit Goal(Worker & worker, bool isDependency) : worker(worker) + , isDependency(isDependency) { } virtual ~Goal() noexcept(false) diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index f17685af8..37a96b4d1 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -186,6 +186,7 @@ struct LocalDerivationGoal : public DerivationGoal const StorePath & drvPath, const OutputsSpec & wantedOutputs, Worker & worker, + bool isDependency, BuildMode buildMode ); @@ -198,6 +199,7 @@ struct LocalDerivationGoal : public DerivationGoal const BasicDerivation & drv, const OutputsSpec & wantedOutputs, Worker & worker, + bool isDependency, BuildMode buildMode ); diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index d84b65a53..390aabb45 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -6,8 +6,14 @@ namespace nix { -PathSubstitutionGoal::PathSubstitutionGoal(const StorePath & storePath, Worker & worker, RepairFlag repair, std::optional ca) - : Goal(worker) +PathSubstitutionGoal::PathSubstitutionGoal( + const StorePath & storePath, + Worker & worker, + bool isDependency, + RepairFlag repair, + std::optional ca +) + : Goal(worker, isDependency) , storePath(storePath) , repair(repair) , ca(ca) diff --git a/src/libstore/build/substitution-goal.hh b/src/libstore/build/substitution-goal.hh index e546fc06f..5d58b34a0 100644 --- a/src/libstore/build/substitution-goal.hh +++ b/src/libstore/build/substitution-goal.hh @@ -80,7 +80,13 @@ struct PathSubstitutionGoal : public Goal std::optional errorMsg = {}); public: - PathSubstitutionGoal(const StorePath & storePath, Worker & worker, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); + PathSubstitutionGoal( + const StorePath & storePath, + Worker & worker, + bool isDependency, + RepairFlag repair = NoRepair, + std::optional ca = std::nullopt + ); ~PathSubstitutionGoal(); Finished timedOut(Error && ex) override { abort(); }; diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 1b4633e64..9e548cc16 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -1,5 +1,6 @@ #include "charptr-cast.hh" #include "worker.hh" +#include "finally.hh" #include "substitution-goal.hh" #include "drv-output-substitution-goal.hh" #include "local-derivation-goal.hh" @@ -59,22 +60,38 @@ std::shared_ptr Worker::makeDerivationGoalCommon( std::shared_ptr Worker::makeDerivationGoal(const StorePath & drvPath, const OutputsSpec & wantedOutputs, BuildMode buildMode) { - return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr { - return !dynamic_cast(&store) - ? std::make_shared(drvPath, wantedOutputs, *this, buildMode) - : LocalDerivationGoal::makeLocalDerivationGoal(drvPath, wantedOutputs, *this, buildMode); - }); + return makeDerivationGoalCommon( + drvPath, + wantedOutputs, + [&]() -> std::shared_ptr { + return !dynamic_cast(&store) + ? std::make_shared( + drvPath, wantedOutputs, *this, running, buildMode + ) + : LocalDerivationGoal::makeLocalDerivationGoal( + drvPath, wantedOutputs, *this, running, buildMode + ); + } + ); } std::shared_ptr Worker::makeBasicDerivationGoal(const StorePath & drvPath, const BasicDerivation & drv, const OutputsSpec & wantedOutputs, BuildMode buildMode) { - return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() -> std::shared_ptr { - return !dynamic_cast(&store) - ? std::make_shared(drvPath, drv, wantedOutputs, *this, buildMode) - : LocalDerivationGoal::makeLocalDerivationGoal(drvPath, drv, wantedOutputs, *this, buildMode); - }); + return makeDerivationGoalCommon( + drvPath, + wantedOutputs, + [&]() -> std::shared_ptr { + return !dynamic_cast(&store) + ? std::make_shared( + drvPath, drv, wantedOutputs, *this, running, buildMode + ) + : LocalDerivationGoal::makeLocalDerivationGoal( + drvPath, drv, wantedOutputs, *this, running, buildMode + ); + } + ); } @@ -83,7 +100,7 @@ std::shared_ptr Worker::makePathSubstitutionGoal(const Sto std::weak_ptr & goal_weak = substitutionGoals[path]; auto goal = goal_weak.lock(); // FIXME if (!goal) { - goal = std::make_shared(path, *this, repair, ca); + goal = std::make_shared(path, *this, running, repair, ca); goal_weak = goal; wakeUp(goal); } @@ -96,7 +113,7 @@ std::shared_ptr Worker::makeDrvOutputSubstitutionGoal std::weak_ptr & goal_weak = drvOutputSubstitutionGoals[id]; auto goal = goal_weak.lock(); // FIXME if (!goal) { - goal = std::make_shared(id, *this, repair, ca); + goal = std::make_shared(id, *this, running, repair, ca); goal_weak = goal; wakeUp(goal); } @@ -313,6 +330,10 @@ void Worker::run(const Goals & _topGoals) { std::vector topPaths; + assert(!running); + running = true; + Finally const _stop([&] { running = false; }); + for (auto & i : _topGoals) { topGoals.insert(i); if (auto goal = dynamic_cast(i.get())) { diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index 7abb966f9..360366f8d 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -47,6 +47,8 @@ class Worker { private: + bool running = false; + /* Note: the worker should only have strong pointers to the top-level goals. */ diff --git a/src/libstore/platform.cc b/src/libstore/platform.cc index 72757e39b..f2c023c82 100644 --- a/src/libstore/platform.cc +++ b/src/libstore/platform.cc @@ -29,17 +29,18 @@ std::shared_ptr LocalDerivationGoal::makeLocalDerivationGoa const StorePath & drvPath, const OutputsSpec & wantedOutputs, Worker & worker, + bool isDependency, BuildMode buildMode ) { #if __linux__ - return std::make_shared(drvPath, wantedOutputs, worker, buildMode); + return std::make_shared(drvPath, wantedOutputs, worker, isDependency, buildMode); #elif __APPLE__ - return std::make_shared(drvPath, wantedOutputs, worker, buildMode); + return std::make_shared(drvPath, wantedOutputs, worker, isDependency, buildMode); #elif __FreeBSD__ - return std::make_shared(drvPath, wantedOutputs, worker, buildMode); + return std::make_shared(drvPath, wantedOutputs, worker, isDependency, buildMode); #else - return std::make_shared(drvPath, wantedOutputs, worker, buildMode); + return std::make_shared(drvPath, wantedOutputs, worker, isDependency, buildMode); #endif } @@ -48,24 +49,25 @@ std::shared_ptr LocalDerivationGoal::makeLocalDerivationGoa const BasicDerivation & drv, const OutputsSpec & wantedOutputs, Worker & worker, + bool isDependency, BuildMode buildMode ) { #if __linux__ return std::make_shared( - drvPath, drv, wantedOutputs, worker, buildMode + drvPath, drv, wantedOutputs, worker, isDependency, buildMode ); #elif __APPLE__ return std::make_shared( - drvPath, drv, wantedOutputs, worker, buildMode + drvPath, drv, wantedOutputs, worker, isDependency, buildMode ); #elif __FreeBSD__ return std::make_shared( - drvPath, drv, wantedOutputs, worker, buildMode + drvPath, drv, wantedOutputs, worker, isDependency, buildMode ); #else return std::make_shared( - drvPath, drv, wantedOutputs, worker, buildMode + drvPath, drv, wantedOutputs, worker, isDependency, buildMode ); #endif }