Split DerivationGoal in two

This separation of concerns is generally good, but in particular sets up
for removing `addWantedOutputs` next.

Author: Ericson2314

src/libstore/build/derivation-building-goal.cc

@@ -155,7 +155,7 @@ Goal::Done Goal::amDone(ExitCode result, std::optional<Error> ex)
     exitCode = result;
 
     if (ex) {
-        if (!waiters.empty())
+        if (!preserveException && !waiters.empty())
             logError(ex->info());
         else
             this->ex = std::move(*ex);

src/libstore/build/derivation-goal.cc

@@ -4,6 +4,7 @@
 #include "nix/store/build/substitution-goal.hh"
 #include "nix/store/build/drv-output-substitution-goal.hh"
 #include "nix/store/build/derivation-goal.hh"
+#include "nix/store/build/derivation-building-goal.hh"
 #ifndef _WIN32 // TODO Enable building on Windows
 #  include "nix/store/build/hook-instance.hh"
 #endif
@@ -77,6 +78,20 @@ std::shared_ptr<DerivationGoal> Worker::makeBasicDerivationGoal(const StorePath
 }
 
 
+std::shared_ptr<DerivationBuildingGoal> Worker::makeDerivationBuildingGoal(const StorePath & drvPath,
+    const Derivation & drv, BuildMode buildMode)
+{
+    std::weak_ptr<DerivationBuildingGoal> & goal_weak = derivationBuildingGoals[drvPath];
+    auto goal = goal_weak.lock(); // FIXME
+    if (!goal) {
+        goal = std::make_shared<DerivationBuildingGoal>(drvPath, drv, *this, buildMode);
+        goal_weak = goal;
+        wakeUp(goal);
+    }
+    return goal;
+}
+
+
 std::shared_ptr<PathSubstitutionGoal> Worker::makePathSubstitutionGoal(const StorePath & path, RepairFlag repair, std::optional<ContentAddress> ca)
 {
     std::weak_ptr<PathSubstitutionGoal> & goal_weak = substitutionGoals[path];
@@ -138,8 +153,9 @@ void Worker::removeGoal(GoalPtr goal)
 {
     if (auto drvGoal = std::dynamic_pointer_cast<DerivationGoal>(goal))
         nix::removeGoal(drvGoal, derivationGoals);
-    else
-    if (auto subGoal = std::dynamic_pointer_cast<PathSubstitutionGoal>(goal))
+    else if (auto drvBuildingGoal = std::dynamic_pointer_cast<DerivationBuildingGoal>(goal))
+        nix::removeGoal(drvBuildingGoal, derivationBuildingGoals);
+    else if (auto subGoal = std::dynamic_pointer_cast<PathSubstitutionGoal>(goal))
         nix::removeGoal(subGoal, substitutionGoals);
     else if (auto subGoal = std::dynamic_pointer_cast<DrvOutputSubstitutionGoal>(goal))
         nix::removeGoal(subGoal, drvOutputSubstitutionGoals);
@@ -202,6 +218,9 @@ void Worker::childStarted(GoalPtr goal, const std::set<MuxablePipePollState::Com
         case JobCategory::Build:
             nrLocalBuilds++;
             break;
+        case JobCategory::Administration:
+            /* Intentionally not limited, see docs */
+            break;
         default:
             unreachable();
         }
@@ -225,6 +244,9 @@ void Worker::childTerminated(Goal * goal, bool wakeSleepers)
             assert(nrLocalBuilds > 0);
             nrLocalBuilds--;
             break;
+        case JobCategory::Administration:
+            /* Intentionally not limited, see docs */
+            break;
         default:
             unreachable();
         }

src/libstore/build/goal.cc

@@ -31,43 +31,11 @@ void runPostBuildHook(
 /**
  * A goal for building some or all of the outputs of a derivation.
  */
-struct DerivationGoal : public Goal
+struct DerivationBuildingGoal : public Goal
 {
     /** The path of the derivation. */
     StorePath drvPath;
 
-    /**
-     * The specific outputs that we need to build.
-     */
-    OutputsSpec wantedOutputs;
-
-    /**
-     * See `needRestart`; just for that field.
-     */
-    enum struct NeedRestartForMoreOutputs {
-        /**
-         * The goal state machine is progressing based on the current value of
-         * `wantedOutputs. No actions are needed.
-         */
-        OutputsUnmodifedDontNeed,
-        /**
-         * `wantedOutputs` has been extended, but the state machine is
-         * proceeding according to its old value, so we need to restart.
-         */
-        OutputsAddedDoNeed,
-        /**
-         * The goal state machine has progressed to the point of doing a build,
-         * in which case all outputs will be produced, so extensions to
-         * `wantedOutputs` no longer require a restart.
-         */
-        BuildInProgressWillNotNeed,
-    };
-
-    /**
-     * Whether additional wanted outputs have been added.
-     */
-    NeedRestartForMoreOutputs needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed;
-
     /**
      * The derivation stored at drvPath.
      */
@@ -125,7 +93,7 @@ struct DerivationGoal : public Goal
 
     BuildMode buildMode;
 
-    std::unique_ptr<MaintainCount<uint64_t>> mcExpectedBuilds, mcRunningBuilds;
+    std::unique_ptr<MaintainCount<uint64_t>> mcRunningBuilds;
 
     std::unique_ptr<Activity> act;
 
@@ -141,28 +109,18 @@ struct DerivationGoal : public Goal
      */
     std::string machineName;
 
-    DerivationGoal(const StorePath & drvPath,
-        const OutputsSpec & wantedOutputs, Worker & worker,
+    DerivationBuildingGoal(const StorePath & drvPath, const Derivation & drv,
+        Worker & worker,
         BuildMode buildMode = bmNormal);
-    DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv,
-        const OutputsSpec & wantedOutputs, Worker & worker,
-        BuildMode buildMode = bmNormal);
-    ~DerivationGoal();
+    ~DerivationBuildingGoal();
 
     void timedOut(Error && ex) override;
 
     std::string key() override;
 
-    /**
-     * Add wanted outputs to an already existing derivation goal.
-     */
-    void addWantedOutputs(const OutputsSpec & outputs);
-
     /**
      * The states.
      */
-    Co loadDerivation();
-    Co haveDerivation();
     Co gaveUpOnSubstitution();
     Co tryToBuild();
     Co hookDone();
@@ -197,7 +155,6 @@ struct DerivationGoal : public Goal
      * there also is no DB entry.
      */
     std::map<std::string, std::optional<StorePath>> queryPartialDerivationOutputMap();
-    OutputPathMap queryDerivationOutputMap();
 
     /**
      * Update 'initialOutputs' to determine the current status of the
@@ -218,8 +175,6 @@ struct DerivationGoal : public Goal
      */
     void killChild();
 
-    Co repairClosure();
-
     void started();
 
     Done done(

src/libstore/build/worker.cc

@@ -14,13 +14,6 @@ namespace nix {
 
 using std::map;
 
-#ifndef _WIN32 // TODO enable build hook on Windows
-struct HookInstance;
-struct DerivationBuilder;
-#endif
-
-typedef enum {rpAccept, rpDecline, rpPostpone} HookReply;
-
 /** Used internally */
 void runPostBuildHook(
     Store & store,
@@ -73,83 +66,25 @@ struct DerivationGoal : public Goal
      */
     std::unique_ptr<Derivation> drv;
 
-    std::unique_ptr<StructuredAttrs> parsedDrv;
-    std::unique_ptr<DerivationOptions> drvOptions;
-
     /**
      * The remainder is state held during the build.
      */
 
-    /**
-     * Locks on (fixed) output paths.
-     */
-    PathLocks outputLocks;
-
-    /**
-     * All input paths (that is, the union of FS closures of the
-     * immediate input paths).
-     */
-    StorePathSet inputPaths;
-
     std::map<std::string, InitialOutput> initialOutputs;
 
-    /**
-     * File descriptor for the log file.
-     */
-    AutoCloseFD fdLogFile;
-    std::shared_ptr<BufferedSink> logFileSink, logSink;
-
-    /**
-     * Number of bytes received from the builder's stdout/stderr.
-     */
-    unsigned long logSize;
-
-    /**
-     * The most recent log lines.
-     */
-    std::list<std::string> logTail;
-
-    std::string currentLogLine;
-    size_t currentLogLinePos = 0; // to handle carriage return
-
-    std::string currentHookLine;
-
-#ifndef _WIN32 // TODO enable build hook on Windows
-    /**
-     * The build hook.
-     */
-    std::unique_ptr<HookInstance> hook;
-
-    std::unique_ptr<DerivationBuilder> builder;
-#endif
-
     BuildMode buildMode;
 
-    std::unique_ptr<MaintainCount<uint64_t>> mcExpectedBuilds, mcRunningBuilds;
-
-    std::unique_ptr<Activity> act;
-
-    /**
-     * Activity that denotes waiting for a lock.
-     */
-    std::unique_ptr<Activity> actLock;
-
-    std::map<ActivityId, Activity> builderActivities;
-
-    /**
-     * The remote machine on which we're building.
-     */
-    std::string machineName;
+    std::unique_ptr<MaintainCount<uint64_t>> mcExpectedBuilds;
 
     DerivationGoal(const StorePath & drvPath,
         const OutputsSpec & wantedOutputs, Worker & worker,
         BuildMode buildMode = bmNormal);
     DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv,
         const OutputsSpec & wantedOutputs, Worker & worker,
         BuildMode buildMode = bmNormal);
-    ~DerivationGoal();
+    ~DerivationGoal() = default;
 
-    void timedOut(Error && ex) override;
+    void timedOut(Error && ex) override { unreachable(); };
 
     std::string key() override;
 
@@ -163,33 +98,6 @@ struct DerivationGoal : public Goal
      */
     Co loadDerivation();
     Co haveDerivation();
-    Co gaveUpOnSubstitution();
-    Co tryToBuild();
-    Co hookDone();
-
-    /**
-     * Is the build hook willing to perform the build?
-     */
-    HookReply tryBuildHook();
-
-    /**
-     * Open a log file and a pipe to it.
-     */
-    Path openLogFile();
-
-    /**
-     * Close the log file.
-     */
-    void closeLogFile();
-
-    bool isReadDesc(Descriptor fd);
-
-    /**
-     * Callback used by the worker to write to the log.
-     */
-    void handleChildOutput(Descriptor fd, std::string_view data) override;
-    void handleEOF(Descriptor fd) override;
-    void flushLine();
 
     /**
      * Wrappers around the corresponding Store methods that first consult the
@@ -213,26 +121,15 @@ struct DerivationGoal : public Goal
      */
     SingleDrvOutputs assertPathValidity();
 
-    /**
-     * Forcibly kill the child process, if any.
-     */
-    void killChild();
-
     Co repairClosure();
 
-    void started();
-
     Done done(
         BuildResult::Status status,
         SingleDrvOutputs builtOutputs = {},
         std::optional<Error> ex = {});
 
-    void appendLogTailErrorMsg(std::string & msg);
-
-    StorePathSet exportReferences(const StorePathSet & storePaths);
-
     JobCategory jobCategory() const override {
-        return JobCategory::Build;
+        return JobCategory::Administration;
     };
 };
 

src/libstore/include/nix/store/build/derivation-building-goal.hh

@@ -50,6 +50,16 @@ enum struct JobCategory {
      * A substitution an arbitrary store object; it will use network resources.
      */
     Substitution,
+    /**
+     * A goal that does no "real" work by itself, and just exists to depend on
+     * other goals which *do* do real work. These goals therefore are not
+     * limited.
+     *
+     * These goals cannot infinitely create themselves, so there is no risk of
+     * a "fork bomb" type situation (which would be a problem even though the
+     * goal do no real work) either.
+     */
+    Administration,
 };
 
 struct Goal : public std::enable_shared_from_this<Goal>
@@ -360,6 +370,17 @@ public:
      */
     BuildResult getBuildResult(const DerivedPath &) const;
 
+    /**
+     * Hack to say that this goal should not log `ex`, but instead keep
+     * it around. Set by a waitee which sees itself as the designated
+     * continuation of this goal, responsible for reporting its
+     * successes or failures.
+     *
+     * @todo this is yet another not-nice hack in the goal system that
+     * we ought to get rid of. See #11927
+     */
+    bool preserveException = false;
+
     /**
      * Exception containing an error message, if any.
      */