Merge pull request #13181 from obsidiansystems/derivation-building-goal

Split `DerivationGoal` in two

maintainers/flake-module.nix

@@ -276,6 +276,8 @@
               ''^src/libstore/store-api\.cc$''
               ''^src/libstore/include/nix/store/store-api\.hh$''
               ''^src/libstore/include/nix/store/store-dir-config\.hh$''
+              ''^src/libstore/build/derivation-building-goal\.cc$''
+              ''^src/libstore/include/nix/store/build/derivation-building-goal\.hh$''
               ''^src/libstore/build/derivation-goal\.cc$''
               ''^src/libstore/include/nix/store/build/derivation-goal\.hh$''
               ''^src/libstore/build/drv-output-substitution-goal\.cc$''

src/libstore/build/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/worker.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
@@ -87,6 +88,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)
 {
     return initGoalIfNeeded(substitutionGoals[path], path, *this, repair, ca);
@@ -134,8 +149,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);
@@ -198,6 +214,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();
         }
@@ -221,6 +240,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/include/nix/store/build/derivation-building-goal.hh

@@ -0,0 +1,194 @@
+#pragma once
+///@file
+
+#include "nix/store/parsed-derivations.hh"
+#include "nix/store/derivations.hh"
+#include "nix/store/derivation-options.hh"
+#include "nix/store/build/derivation-building-misc.hh"
+#include "nix/store/outputs-spec.hh"
+#include "nix/store/store-api.hh"
+#include "nix/store/pathlocks.hh"
+#include "nix/store/build/goal.hh"
+
+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,
+    Logger & logger,
+    const StorePath & drvPath,
+    const StorePathSet & outputPaths);
+
+/**
+ * A goal for building some or all of the outputs of a derivation.
+ */
+struct DerivationBuildingGoal : public Goal
+{
+    /** The path of the derivation. */
+    StorePath drvPath;
+
+    /**
+     * The derivation stored at drvPath.
+     */
+    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>> 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;
+
+    DerivationBuildingGoal(const StorePath & drvPath, const Derivation & drv,
+        Worker & worker,
+        BuildMode buildMode = bmNormal);
+    ~DerivationBuildingGoal();
+
+    void timedOut(Error && ex) override;
+
+    std::string key() override;
+
+    /**
+     * The states.
+     */
+    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
+     * derivation.  This is currently needed because when there is no drv file
+     * there also is no DB entry.
+     */
+    std::map<std::string, std::optional<StorePath>> queryPartialDerivationOutputMap();
+
+    /**
+     * Update 'initialOutputs' to determine the current status of the
+     * outputs of the derivation. Also returns a Boolean denoting
+     * whether all outputs are valid and non-corrupt, and a
+     * 'SingleDrvOutputs' structure containing the valid outputs.
+     */
+    std::pair<bool, SingleDrvOutputs> checkPathValidity();
+
+    /**
+     * Aborts if any output is not valid or corrupt, and otherwise
+     * returns a 'SingleDrvOutputs' structure containing all outputs.
+     */
+    SingleDrvOutputs assertPathValidity();
+
+    /**
+     * Forcibly kill the child process, if any.
+     */
+    void killChild();
+
+    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;
+    };
+};
+
+}

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

@@ -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,73 +66,15 @@ 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,
@@ -147,9 +82,9 @@ struct DerivationGoal : public Goal
     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/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.
      */

src/libstore/include/nix/store/build/worker.hh

@@ -14,6 +14,7 @@ namespace nix {
 
 /* Forward definition. */
 struct DerivationGoal;
+struct DerivationBuildingGoal;
 struct PathSubstitutionGoal;
 class DrvOutputSubstitutionGoal;
 
@@ -104,6 +105,7 @@ private:
      * same derivation / path.
      */
     std::map<StorePath, std::weak_ptr<DerivationGoal>> derivationGoals;
+    std::map<StorePath, std::weak_ptr<DerivationBuildingGoal>> derivationBuildingGoals;
     std::map<StorePath, std::weak_ptr<PathSubstitutionGoal>> substitutionGoals;
     std::map<DrvOutput, std::weak_ptr<DrvOutputSubstitutionGoal>> drvOutputSubstitutionGoals;
 
@@ -210,6 +212,13 @@ public:
         const StorePath & drvPath, const BasicDerivation & drv,
         const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal);
 
+    /**
+     * @ref DerivationBuildingGoal "derivation goal"
+     */
+    std::shared_ptr<DerivationBuildingGoal> makeDerivationBuildingGoal(
+        const StorePath & drvPath, const Derivation & drv,
+        BuildMode buildMode = bmNormal);
+
     /**
      * @ref PathSubstitutionGoal "substitution goal"
      */

src/libstore/include/nix/store/meson.build

@@ -13,6 +13,7 @@ headers = [config_pub_h] + files(
   'binary-cache-store.hh',
   'build-result.hh',
   'build/derivation-goal.hh',
+  'build/derivation-building-goal.hh',
   'build/derivation-building-misc.hh',
   'build/drv-output-substitution-goal.hh',
   'build/goal.hh',

src/libstore/meson.build

@@ -254,6 +254,7 @@ sources = files(
   'binary-cache-store.cc',
   'build-result.cc',
   'build/derivation-goal.cc',
+  'build/derivation-building-goal.cc',
   'build/drv-output-substitution-goal.cc',
   'build/entry-points.cc',
   'build/goal.cc',