Make KeyedBuildResult, BuildResult like before, and fix bug anoth…

…er way In NixOS#6311 (comment), I realized since derivation goals' wanted outputs can "grow" due to overlapping dependencies (See `DerivationGoal::addWantedOutputs`, called by `Worker::makeDerivationGoalCommon`), the previous bug fix had an unfortunate side effect of causing more pointless rebuilds. In paticular, we have this situation: 1. Goal made from `DerivedPath::Built { foo, {a} }`. 2. Goal gives on on substituting, starts building. 3. Goal made from `DerivedPath::Built { foo, {b} }`, in fact is just modified original goal. 4. Though the goal had gotten as far as building, so all outputs were going to be produced, `addWantedOutputs` no longer knows that and so the goal is flagged to be restarted. This might sound far-fetched with input-addressed drvs, where we usually basically have all our goals "planned out" before we start doing anything, but with CA derivation goals and especially RFC 92, where *drv resolution* means goals are created after some building is completed, it is more likely to happen. So the first thing to do was restore the clearing of `wantedOutputs` we used to do, and then filter the outputs in `buildPathsWithResults` to only get the ones we care about. But fix also has its own side effect in that the `DerivedPath` in the `BuildResult` in `DerivationGoal` cannot be trusted; it is merely the *first* `DerivedPath` for which this goal was originally created. To remedy this, I made `BuildResult` be like it was before, and instead made `KeyedBuildResult` be a subclass wit the path. Only `buildPathsWithResults` returns `KeyedBuildResult`s, everything else just becomes like it was before, where the "key" is unambiguous from context. I think separating the "primary key" field(s) from the other fields is good practical in general anyways. (I would like to do the same thing for `ValidPathInfo`.) Among other things, it allows constructions like `std::map<Key, ThingWithKey>` where doesn't contain duplicate keys and just precludes the possibility of those duplicate keys being out of sync. We might leverage the above someday to overload `buildPathsWithResults` to take a *set* of return a *map* per the above.
obsidiansystems · Feb 24, 2023 · 20b9278 · 20b9278
1 parent 92611e6
commit 20b9278
Show file tree

Hide file tree

Showing 10 changed files with 87 additions and 49 deletions.
diff --git a/src/libstore/build-result.hh b/src/libstore/build-result.hh
@@ -67,9 +67,6 @@ struct BuildResult
        non-determinism.) */
     bool isNonDeterministic = false;
 
-    /* The derivation we built or the store path we substituted. */
-    DerivedPath path;
-
     /* For derivations, a mapping from the names of the wanted outputs
        to actual paths. */
     DrvOutputs builtOutputs;
@@ -92,4 +89,11 @@ struct BuildResult
     }
 };
 
+/* A build result together with it's "primary key" */
+struct KeyedBuildResult : BuildResult
+{
+    /* The derivation we built or the store path we substituted. */
+    DerivedPath path;
+};
+
 }
diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc
@@ -330,6 +330,10 @@ void DerivationGoal::outputsSubstitutionTried()
    produced using a substitute.  So we have to build instead. */
 void DerivationGoal::gaveUpOnSubstitution()
 {
+    /* Make sure checkPathValidity() from now on checks all
+       outputs. */
+    wantedOutputs = OutputsSpec::All { };
+
     /* The inputs must be built before we can build this goal. */
     inputDrvOutputs.clear();
     if (useDerivation)
@@ -570,8 +574,6 @@ void DerivationGoal::inputsRealised()
        build hook. */
     state = &DerivationGoal::tryToBuild;
     worker.wakeUp(shared_from_this());
-
-    buildResult = BuildResult { .path = buildResult.path };
 }
 
 void DerivationGoal::started()
@@ -1449,10 +1451,10 @@ void DerivationGoal::waiteeDone(GoalPtr waitee, ExitCode result)
     Goal::waiteeDone(waitee, result);
 
     if (waitee->buildResult.success())
-        if (auto bfd = std::get_if<DerivedPath::Built>(&waitee->buildResult.path))
+        if (auto * dg = dynamic_cast<DerivationGoal *>(&*waitee))
             for (auto & [output, realisation] : waitee->buildResult.builtOutputs)
                 inputDrvOutputs.insert_or_assign(
-                    { bfd->drvPath, output.outputName },
+                    { dg->drvPath, output.outputName },
                     realisation.outPath);
 }
 

diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc
@@ -47,31 +47,57 @@ void Store::buildPaths(const std::vector<DerivedPath> & reqs, BuildMode buildMod
     }
 }
 
-std::vector<BuildResult> Store::buildPathsWithResults(
+std::vector<KeyedBuildResult> Store::buildPathsWithResults(
     const std::vector<DerivedPath> & reqs,
     BuildMode buildMode,
     std::shared_ptr<Store> evalStore)
 {
     Worker worker(*this, evalStore ? *evalStore : *this);
 
     Goals goals;
-    for (const auto & br : reqs) {
-        std::visit(overloaded {
-            [&](const DerivedPath::Built & bfd) {
-                goals.insert(worker.makeDerivationGoal(bfd.drvPath, bfd.outputs, buildMode));
+    std::vector<std::pair<GoalPtr, const DerivedPath &>> reqs2;
+
+    for (const auto & req : reqs) {
+        auto g = std::visit(overloaded {
+            [&](const DerivedPath::Built & bfd) -> GoalPtr {
+                return worker.makeDerivationGoal(bfd.drvPath, bfd.outputs, buildMode);
             },
-            [&](const DerivedPath::Opaque & bo) {
-                goals.insert(worker.makePathSubstitutionGoal(bo.path, buildMode == bmRepair ? Repair : NoRepair));
+            [&](const DerivedPath::Opaque & bo) -> GoalPtr {
+                return worker.makePathSubstitutionGoal(bo.path, buildMode == bmRepair ? Repair : NoRepair);
             },
-        }, br.raw());
+        }, req.raw());
+        goals.insert(g);
+        reqs2.push_back({g, req});
     }
 
     worker.run(goals);
 
-    std::vector<BuildResult> results;
+    std::vector<KeyedBuildResult> results;
 
-    for (auto & i : goals)
-        results.push_back(i->buildResult);
+    for (auto & [gp, req] : reqs2) {
+        auto & br = results.emplace_back(KeyedBuildResult {
+            gp->buildResult,
+            .path = req
+        });
+
+        auto pbp = std::get_if<DerivedPath::Built>(&req);
+        if (!pbp) continue;
+        auto & bp = *pbp;
+
+        /* Because goals are in general shared between derived paths
+           that share the same derivation, we need to filter their
+           results to get back just the results we care about.
+         */
+
+        auto & bos = br.builtOutputs;
+
+        for (auto it = bos.begin(); it != bos.end();) {
+            if (bp.outputs.contains(it->first.outputName))
+                ++it;
+            else
+                it = bos.erase(it);
+        }
+    }
 
     return results;
 }
@@ -89,10 +115,6 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat
         return BuildResult {
             .status = BuildResult::MiscFailure,
             .errorMsg = e.msg(),
-            .path = DerivedPath::Built {
-                .drvPath = drvPath,
-                .outputs = OutputsSpec::All { },
-            },
         };
     };
 }

diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh
@@ -64,7 +64,6 @@ struct Goal : public std::enable_shared_from_this<Goal>
 
     Goal(Worker & worker, DerivedPath path)
         : worker(worker)
-        , buildResult { .path = std::move(path) }
     { }
 
     virtual ~Goal()

diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
@@ -1329,7 +1329,7 @@ struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual Lo
                 result.rethrow();
     }
 
-    std::vector<BuildResult> buildPathsWithResults(
+    std::vector<KeyedBuildResult> buildPathsWithResults(
         const std::vector<DerivedPath> & paths,
         BuildMode buildMode = bmNormal,
         std::shared_ptr<Store> evalStore = nullptr) override

diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc
@@ -278,12 +278,7 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
 
         conn->to.flush();
 
-        BuildResult status {
-            .path = DerivedPath::Built {
-                .drvPath = drvPath,
-                .outputs = OutputsSpec::All { },
-            },
-        };
+        BuildResult status;
         status.status = (BuildResult::Status) readInt(conn->from);
         conn->from >> status.errorMsg;
 
@@ -321,7 +316,7 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
 
         conn->to.flush();
 
-        BuildResult result { .path = DerivedPath::Opaque { StorePath::dummy } };
+        BuildResult result;
         result.status = (BuildResult::Status) readInt(conn->from);
 
         if (!result.success()) {

diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
@@ -91,10 +91,26 @@ void write(const Store & store, Sink & out, const DrvOutput & drvOutput)
 }
 
 
-BuildResult read(const Store & store, Source & from, Phantom<BuildResult> _)
+KeyedBuildResult read(const Store & store, Source & from, Phantom<KeyedBuildResult> _)
 {
     auto path = worker_proto::read(store, from, Phantom<DerivedPath> {});
-    BuildResult res { .path = path };
+    auto br = worker_proto::read(store, from, Phantom<BuildResult> {});
+    return KeyedBuildResult {
+        std::move(br),
+        .path = std::move(path),
+    };
+}
+
+void write(const Store & store, Sink & to, const KeyedBuildResult & res)
+{
+    worker_proto::write(store, to, res.path);
+    worker_proto::write(store, to, static_cast<const BuildResult &>(res));
+}
+
+
+BuildResult read(const Store & store, Source & from, Phantom<BuildResult> _)
+{
+    BuildResult res;
     res.status = (BuildResult::Status) readInt(from);
     from
         >> res.errorMsg
@@ -108,7 +124,6 @@ BuildResult read(const Store & store, Source & from, Phantom<BuildResult> _)
 
 void write(const Store & store, Sink & to, const BuildResult & res)
 {
-    worker_proto::write(store, to, res.path);
     to
         << res.status
         << res.errorMsg
@@ -824,7 +839,7 @@ void RemoteStore::buildPaths(const std::vector<DerivedPath> & drvPaths, BuildMod
     readInt(conn->from);
 }
 
-std::vector<BuildResult> RemoteStore::buildPathsWithResults(
+std::vector<KeyedBuildResult> RemoteStore::buildPathsWithResults(
     const std::vector<DerivedPath> & paths,
     BuildMode buildMode,
     std::shared_ptr<Store> evalStore)
@@ -839,7 +854,7 @@ std::vector<BuildResult> RemoteStore::buildPathsWithResults(
         writeDerivedPaths(*this, conn, paths);
         conn->to << buildMode;
         conn.processStderr();
-        return worker_proto::read(*this, conn->from, Phantom<std::vector<BuildResult>> {});
+        return worker_proto::read(*this, conn->from, Phantom<std::vector<KeyedBuildResult>> {});
     } else {
         // Avoid deadlock.
         conn_.reset();
@@ -848,20 +863,24 @@ std::vector<BuildResult> RemoteStore::buildPathsWithResults(
         // fails, but meh.
         buildPaths(paths, buildMode, evalStore);
 
-        std::vector<BuildResult> results;
+        std::vector<KeyedBuildResult> results;
 
         for (auto & path : paths) {
             std::visit(
                 overloaded {
                     [&](const DerivedPath::Opaque & bo) {
-                        results.push_back(BuildResult {
-                            .status = BuildResult::Substituted,
+                        results.push_back(KeyedBuildResult {
+                            {
+                                .status = BuildResult::Substituted,
+                            },
                             .path = bo,
                         });
                     },
                     [&](const DerivedPath::Built & bfd) {
-                        BuildResult res {
-                            .status = BuildResult::Built,
+                        KeyedBuildResult res {
+                            {
+                                .status = BuildResult::Built
+                            },
                             .path = bfd,
                         };
 
@@ -911,12 +930,7 @@ BuildResult RemoteStore::buildDerivation(const StorePath & drvPath, const BasicD
     writeDerivation(conn->to, *this, drv);
     conn->to << buildMode;
     conn.processStderr();
-    BuildResult res {
-        .path = DerivedPath::Built {
-            .drvPath = drvPath,
-            .outputs = OutputsSpec::All { },
-        },
-    };
+    BuildResult res;
     res.status = (BuildResult::Status) readInt(conn->from);
     conn->from >> res.errorMsg;
     if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 29) {

diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh
@@ -107,7 +107,7 @@ public:
 
     void buildPaths(const std::vector<DerivedPath> & paths, BuildMode buildMode, std::shared_ptr<Store> evalStore) override;
 
-    std::vector<BuildResult> buildPathsWithResults(
+    std::vector<KeyedBuildResult> buildPathsWithResults(
         const std::vector<DerivedPath> & paths,
         BuildMode buildMode,
         std::shared_ptr<Store> evalStore) override;

diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
@@ -85,6 +85,7 @@ const uint32_t exportMagic = 0x4558494e;
 enum BuildMode { bmNormal, bmRepair, bmCheck };
 
 struct BuildResult;
+struct KeyedBuildResult;
 
 
 typedef std::map<StorePath, std::optional<ContentAddress>> StorePathCAMap;
@@ -457,7 +458,7 @@ public:
        a build/substitution error, this function won't throw an
        exception, but return a `BuildResult` containing an error
        message. */
-    virtual std::vector<BuildResult> buildPathsWithResults(
+    virtual std::vector<KeyedBuildResult> buildPathsWithResults(
         const std::vector<DerivedPath> & paths,
         BuildMode buildMode = bmNormal,
         std::shared_ptr<Store> evalStore = nullptr);

diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh
@@ -93,6 +93,7 @@ MAKE_WORKER_PROTO(, DerivedPath);
 MAKE_WORKER_PROTO(, Realisation);
 MAKE_WORKER_PROTO(, DrvOutput);
 MAKE_WORKER_PROTO(, BuildResult);
+MAKE_WORKER_PROTO(, KeyedBuildResult);
 
 MAKE_WORKER_PROTO(template<typename T>, std::vector<T>);
 MAKE_WORKER_PROTO(template<typename T>, std::set<T>);