Merge pull request #4143 from obsidiansystems/typed-goal-maps

Properly type the derivation and substitution goal maps

src/libstore/build/derivation-goal.cc

@@ -231,7 +231,7 @@ void DerivationGoal::getDerivation()
         return;
     }
 
-    addWaitee(worker.makeSubstitutionGoal(drvPath));
+    addWaitee(upcast_goal(worker.makeSubstitutionGoal(drvPath)));
 
     state = &DerivationGoal::loadDerivation;
 }
@@ -304,10 +304,10 @@ void DerivationGoal::haveDerivation()
                 /* Nothing to wait for; tail call */
                 return DerivationGoal::gaveUpOnSubstitution();
             }
-            addWaitee(worker.makeSubstitutionGoal(
+            addWaitee(upcast_goal(worker.makeSubstitutionGoal(
                 status.known->path,
                 buildMode == bmRepair ? Repair : NoRepair,
-                getDerivationCA(*drv)));
+                getDerivationCA(*drv))));
         }
 
     if (waitees.empty()) /* to prevent hang (no wake-up event) */
@@ -388,7 +388,7 @@ void DerivationGoal::gaveUpOnSubstitution()
         if (!settings.useSubstitutes)
             throw Error("dependency '%s' of '%s' does not exist, and substitution is disabled",
                 worker.store.printStorePath(i), worker.store.printStorePath(drvPath));
-        addWaitee(worker.makeSubstitutionGoal(i));
+        addWaitee(upcast_goal(worker.makeSubstitutionGoal(i)));
     }
 
     if (waitees.empty()) /* to prevent hang (no wake-up event) */
@@ -442,7 +442,7 @@ void DerivationGoal::repairClosure()
         });
         auto drvPath2 = outputsToDrv.find(i);
         if (drvPath2 == outputsToDrv.end())
-            addWaitee(worker.makeSubstitutionGoal(i, Repair));
+            addWaitee(upcast_goal(worker.makeSubstitutionGoal(i, Repair)));
         else
             addWaitee(worker.makeDerivationGoal(drvPath2->second, StringSet(), bmRepair));
     }

src/libstore/build/worker.cc

@@ -43,16 +43,13 @@ std::shared_ptr<DerivationGoal> Worker::makeDerivationGoalCommon(
     const StringSet & wantedOutputs,
     std::function<std::shared_ptr<DerivationGoal>()> mkDrvGoal)
 {
-    WeakGoalPtr & abstract_goal_weak = derivationGoals[drvPath];
-    GoalPtr abstract_goal = abstract_goal_weak.lock(); // FIXME
-    std::shared_ptr<DerivationGoal> goal;
-    if (!abstract_goal) {
+    std::weak_ptr<DerivationGoal> & goal_weak = derivationGoals[drvPath];
+    std::shared_ptr<DerivationGoal> goal = goal_weak.lock();
+    if (!goal) {
         goal = mkDrvGoal();
-        abstract_goal_weak = goal;
+        goal_weak = goal;
         wakeUp(goal);
     } else {
-        goal = std::dynamic_pointer_cast<DerivationGoal>(abstract_goal);
-        assert(goal);
         goal->addWantedOutputs(wantedOutputs);
     }
     return goal;
@@ -77,10 +74,10 @@ std::shared_ptr<DerivationGoal> Worker::makeBasicDerivationGoal(const StorePath
 }
 
 
-GoalPtr Worker::makeSubstitutionGoal(const StorePath & path, RepairFlag repair, std::optional<ContentAddress> ca)
+std::shared_ptr<SubstitutionGoal> Worker::makeSubstitutionGoal(const StorePath & path, RepairFlag repair, std::optional<ContentAddress> ca)
 {
-    WeakGoalPtr & goal_weak = substitutionGoals[path];
-    GoalPtr goal = goal_weak.lock(); // FIXME
+    std::weak_ptr<SubstitutionGoal> & goal_weak = substitutionGoals[path];
+    auto goal = goal_weak.lock(); // FIXME
     if (!goal) {
         goal = std::make_shared<SubstitutionGoal>(path, *this, repair, ca);
         goal_weak = goal;
@@ -89,14 +86,14 @@ GoalPtr Worker::makeSubstitutionGoal(const StorePath & path, RepairFlag repair,
     return goal;
 }
 
-
-static void removeGoal(GoalPtr goal, WeakGoalMap & goalMap)
+template<typename G>
+static void removeGoal(std::shared_ptr<G> goal, std::map<StorePath, std::weak_ptr<G>> & goalMap)
 {
     /* !!! inefficient */
-    for (WeakGoalMap::iterator i = goalMap.begin();
+    for (auto i = goalMap.begin();
          i != goalMap.end(); )
         if (i->second.lock() == goal) {
-            WeakGoalMap::iterator j = i; ++j;
+            auto j = i; ++j;
             goalMap.erase(i);
             i = j;
         }
@@ -106,8 +103,12 @@ static void removeGoal(GoalPtr goal, WeakGoalMap & goalMap)
 
 void Worker::removeGoal(GoalPtr goal)
 {
-    nix::removeGoal(goal, derivationGoals);
-    nix::removeGoal(goal, substitutionGoals);
+    if (auto drvGoal = std::dynamic_pointer_cast<DerivationGoal>(goal))
+        nix::removeGoal(drvGoal, derivationGoals);
+    else if (auto subGoal = std::dynamic_pointer_cast<SubstitutionGoal>(goal))
+        nix::removeGoal(subGoal, substitutionGoals);
+    else
+        assert(false);
     if (topGoals.find(goal) != topGoals.end()) {
         topGoals.erase(goal);
         /* If a top-level goal failed, then kill all other goals
@@ -452,4 +453,9 @@ void Worker::markContentsGood(const StorePath & path)
     pathContentsGoodCache.insert_or_assign(path, true);
 }
 
+
+GoalPtr upcast_goal(std::shared_ptr<SubstitutionGoal> subGoal) {
+    return subGoal;
+}
+
 }

src/libstore/build/worker.hh

@@ -9,6 +9,18 @@ namespace nix {
 
 /* Forward definition. */
 class DerivationGoal;
+class SubstitutionGoal;
+
+/* Workaround for not being able to declare a something like
+
+     class SubstitutionGoal : public Goal;
+
+   even when Goal is a complete type.
+
+   This is still a static cast. The purpose of exporting it is to define it in
+   a place where `SubstitutionGoal` is concrete, and use it in a place where it
+   is opaque. */
+GoalPtr upcast_goal(std::shared_ptr<SubstitutionGoal> subGoal);
 
 typedef std::chrono::time_point<std::chrono::steady_clock> steady_time_point;
 
@@ -56,8 +68,8 @@ private:
 
     /* Maps used to prevent multiple instantiations of a goal for the
        same derivation / path. */
-    WeakGoalMap derivationGoals;
-    WeakGoalMap substitutionGoals;
+    std::map<StorePath, std::weak_ptr<DerivationGoal>> derivationGoals;
+    std::map<StorePath, std::weak_ptr<SubstitutionGoal>> substitutionGoals;
 
     /* Goals waiting for busy paths to be unlocked. */
     WeakGoals waitingForAnyGoal;
@@ -131,7 +143,7 @@ public:
         const StringSet & wantedOutputs, BuildMode buildMode = bmNormal);
 
     /* substitution goal */
-    GoalPtr makeSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt);
+    std::shared_ptr<SubstitutionGoal> makeSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt);
 
     /* Remove a dead goal. */
     void removeGoal(GoalPtr goal);