From 37fca662b0acef3c104a159709a394832e297dda Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 25 Mar 2022 01:26:07 +0000 Subject: [PATCH] Make `KeyedBuildResult`, `BuildResult` like before, and fix bug another way In https://github.com/NixOS/nix/pull/6311#discussion_r834863823, 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` 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. ----- Unfortunately, we need to avoid C++20 strictness on designated initializers. (BTW https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2287r1.html this offers some new syntax for this use-case. Hopefully this will be adopted and we can eventually use it.) No having that yet, maybe it would be better to not make `KeyedBuildResult` a subclass to just avoid this. Co-authored-by: Robert Hensing --- src/libstore/build-result.hh | 16 ++++--- src/libstore/build/derivation-goal.cc | 30 ++++++++++--- src/libstore/build/entry-points.cc | 29 +++++++----- src/libstore/build/goal.cc | 23 ++++++++++ src/libstore/build/goal.hh | 16 ++++++- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/legacy-ssh-store.cc | 9 +--- src/libstore/remote-store.cc | 50 +++++++++++++-------- src/libstore/remote-store.hh | 2 +- src/libstore/store-api.hh | 3 +- src/libstore/worker-protocol.hh | 1 + 11 files changed, 130 insertions(+), 51 deletions(-) diff --git a/src/libstore/build-result.hh b/src/libstore/build-result.hh index 27d1a1b6c..e07296eab 100644 --- a/src/libstore/build-result.hh +++ b/src/libstore/build-result.hh @@ -83,11 +83,6 @@ struct BuildResult */ 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. @@ -116,4 +111,15 @@ struct BuildResult } }; +/** + * A `BuildResult` together with its "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 index 26faf8c8e..606f9fd48 100644 --- 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() @@ -1452,12 +1454,28 @@ void DerivationGoal::waiteeDone(GoalPtr waitee, ExitCode result) { Goal::waiteeDone(waitee, result); - if (waitee->buildResult.success()) - if (auto bfd = std::get_if(&waitee->buildResult.path)) - for (auto & [output, realisation] : waitee->buildResult.builtOutputs) + if (!useDerivation) return; + auto & fullDrv = *dynamic_cast(drv.get()); + + auto * dg = dynamic_cast(&*waitee); + if (!dg) return; + + auto outputs = fullDrv.inputDrvs.find(dg->drvPath); + if (outputs == fullDrv.inputDrvs.end()) return; + + for (auto & outputName : outputs->second) { + auto buildResult = dg->getBuildResult(DerivedPath::Built { + .drvPath = dg->drvPath, + .outputs = OutputsSpec::Names { outputName }, + }); + if (buildResult.success()) { + for (auto & [output, realisation] : 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 index 7d725b881..74eae0692 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -39,7 +39,7 @@ void Store::buildPaths(const std::vector & reqs, BuildMode buildMod } } -std::vector Store::buildPathsWithResults( +std::vector Store::buildPathsWithResults( const std::vector & reqs, BuildMode buildMode, std::shared_ptr evalStore) @@ -47,15 +47,23 @@ std::vector Store::buildPathsWithResults( Worker worker(*this, evalStore ? *evalStore : *this); Goals goals; - for (const auto & br : reqs) - goals.insert(worker.makeGoal(br, buildMode)); + std::vector> state; + + for (const auto & req : reqs) { + auto goal = worker.makeGoal(req, buildMode); + goals.insert(goal); + state.push_back({req, goal}); + } worker.run(goals); - std::vector results; + std::vector results; - for (auto & i : goals) - results.push_back(i->buildResult); + for (auto & [req, goalPtr] : state) + results.emplace_back(KeyedBuildResult { + goalPtr->getBuildResult(req), + /* .path = */ req, + }); return results; } @@ -68,15 +76,14 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat try { worker.run(Goals{goal}); - return goal->buildResult; + return goal->getBuildResult(DerivedPath::Built { + .drvPath = drvPath, + .outputs = OutputsSpec::All {}, + }); } catch (Error & e) { return BuildResult { .status = BuildResult::MiscFailure, .errorMsg = e.msg(), - .path = DerivedPath::Built { - .drvPath = drvPath, - .outputs = OutputsSpec::All { }, - }, }; }; } diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index d59b94797..13b2e509a 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -11,6 +11,29 @@ bool CompareGoalPtrs::operator() (const GoalPtr & a, const GoalPtr & b) const { } +BuildResult Goal::getBuildResult(const DerivedPath & req) { + BuildResult res { buildResult }; + + if (auto pbp = std::get_if(&req)) { + 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. + */ + + for (auto it = res.builtOutputs.begin(); it != res.builtOutputs.end();) { + if (bp.outputs.contains(it->first.outputName)) + ++it; + else + it = res.builtOutputs.erase(it); + } + } + + return res; +} + + void addToWeakGoals(WeakGoals & goals, GoalPtr p) { if (goals.find(p) != goals.end()) diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index f4bf6f38b..c0e12a2ed 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -81,11 +81,26 @@ struct Goal : public std::enable_shared_from_this */ ExitCode exitCode = ecBusy; +protected: /** * Build result. */ BuildResult buildResult; +public: + + /** + * Project a `BuildResult` with just the information that pertains + * to the given request. + * + * In general, goals may be aliased between multiple requests, and + * the stored `BuildResult` has information for the union of all + * requests. We don't want to leak what the other request are for + * sake of both privacy and determinism, and this "safe accessor" + * ensures we don't. + */ + BuildResult getBuildResult(const DerivedPath &); + /** * Exception containing an error message, if any. */ @@ -93,7 +108,6 @@ struct Goal : public std::enable_shared_from_this 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 index 58d6901d3..af937f6b1 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1335,7 +1335,7 @@ struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual Lo result.rethrow(); } - std::vector buildPathsWithResults( + std::vector buildPathsWithResults( const std::vector & paths, BuildMode buildMode = bmNormal, std::shared_ptr evalStore = nullptr) override diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index d2ddbbe5f..7b40b27e0 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -287,12 +287,7 @@ public: 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; @@ -330,7 +325,7 @@ public: 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 index b862902d1..734e6f27f 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -125,10 +125,26 @@ void write(const Store & store, Sink & out, const DrvOutput & drvOutput) } -BuildResult read(const Store & store, Source & from, Phantom _) +KeyedBuildResult read(const Store & store, Source & from, Phantom _) { auto path = worker_proto::read(store, from, Phantom {}); - BuildResult res { .path = path }; + auto br = worker_proto::read(store, from, Phantom {}); + 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(res)); +} + + +BuildResult read(const Store & store, Source & from, Phantom _) +{ + BuildResult res; res.status = (BuildResult::Status) readInt(from); from >> res.errorMsg @@ -142,7 +158,6 @@ BuildResult read(const Store & store, Source & from, Phantom _) void write(const Store & store, Sink & to, const BuildResult & res) { - worker_proto::write(store, to, res.path); to << res.status << res.errorMsg @@ -865,7 +880,7 @@ void RemoteStore::buildPaths(const std::vector & drvPaths, BuildMod readInt(conn->from); } -std::vector RemoteStore::buildPathsWithResults( +std::vector RemoteStore::buildPathsWithResults( const std::vector & paths, BuildMode buildMode, std::shared_ptr evalStore) @@ -880,7 +895,7 @@ std::vector RemoteStore::buildPathsWithResults( writeDerivedPaths(*this, conn, paths); conn->to << buildMode; conn.processStderr(); - return worker_proto::read(*this, conn->from, Phantom> {}); + return worker_proto::read(*this, conn->from, Phantom> {}); } else { // Avoid deadlock. conn_.reset(); @@ -889,21 +904,25 @@ std::vector RemoteStore::buildPathsWithResults( // fails, but meh. buildPaths(paths, buildMode, evalStore); - std::vector results; + std::vector results; for (auto & path : paths) { std::visit( overloaded { [&](const DerivedPath::Opaque & bo) { - results.push_back(BuildResult { - .status = BuildResult::Substituted, - .path = bo, + results.push_back(KeyedBuildResult { + { + .status = BuildResult::Substituted, + }, + /* .path = */ bo, }); }, [&](const DerivedPath::Built & bfd) { - BuildResult res { - .status = BuildResult::Built, - .path = bfd, + KeyedBuildResult res { + { + .status = BuildResult::Built + }, + /* .path = */ bfd, }; OutputPathMap outputs; @@ -952,12 +971,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 index 1c45f543e..a30466647 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -114,7 +114,7 @@ public: void buildPaths(const std::vector & paths, BuildMode buildMode, std::shared_ptr evalStore) override; - std::vector buildPathsWithResults( + std::vector buildPathsWithResults( const std::vector & paths, BuildMode buildMode, std::shared_ptr evalStore) override; diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 5bee272bf..9d6880de9 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -92,6 +92,7 @@ enum BuildMode { bmNormal, bmRepair, bmCheck }; enum TrustedFlag : bool { NotTrusted = false, Trusted = true }; struct BuildResult; +struct KeyedBuildResult; typedef std::map> StorePathCAMap; @@ -575,7 +576,7 @@ public: * case of a build/substitution error, this function won't throw an * exception, but return a BuildResult containing an error message. */ - virtual std::vector buildPathsWithResults( + virtual std::vector buildPathsWithResults( const std::vector & paths, BuildMode buildMode = bmNormal, std::shared_ptr evalStore = nullptr); diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index c7a6f8688..34b2fc17b 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -103,6 +103,7 @@ MAKE_WORKER_PROTO(, DerivedPath); MAKE_WORKER_PROTO(, Realisation); MAKE_WORKER_PROTO(, DrvOutput); MAKE_WORKER_PROTO(, BuildResult); +MAKE_WORKER_PROTO(, KeyedBuildResult); MAKE_WORKER_PROTO(, std::optional); MAKE_WORKER_PROTO(template, std::vector);