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<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.

-----

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 <roberth@users.noreply.github.com>
This commit is contained in:
John Ericson 2022-03-25 01:26:07 +00:00
parent 9df7f3f537
commit 37fca662b0
11 changed files with 130 additions and 51 deletions

View file

@ -83,11 +83,6 @@ struct BuildResult
*/ */
bool isNonDeterministic = false; 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 * For derivations, a mapping from the names of the wanted outputs
* to actual paths. * 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;
};
} }

View file

@ -330,6 +330,10 @@ void DerivationGoal::outputsSubstitutionTried()
produced using a substitute. So we have to build instead. */ produced using a substitute. So we have to build instead. */
void DerivationGoal::gaveUpOnSubstitution() 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. */ /* The inputs must be built before we can build this goal. */
inputDrvOutputs.clear(); inputDrvOutputs.clear();
if (useDerivation) if (useDerivation)
@ -570,8 +574,6 @@ void DerivationGoal::inputsRealised()
build hook. */ build hook. */
state = &DerivationGoal::tryToBuild; state = &DerivationGoal::tryToBuild;
worker.wakeUp(shared_from_this()); worker.wakeUp(shared_from_this());
buildResult = BuildResult { .path = buildResult.path };
} }
void DerivationGoal::started() void DerivationGoal::started()
@ -1452,12 +1454,28 @@ void DerivationGoal::waiteeDone(GoalPtr waitee, ExitCode result)
{ {
Goal::waiteeDone(waitee, result); Goal::waiteeDone(waitee, result);
if (waitee->buildResult.success()) if (!useDerivation) return;
if (auto bfd = std::get_if<DerivedPath::Built>(&waitee->buildResult.path)) auto & fullDrv = *dynamic_cast<Derivation *>(drv.get());
for (auto & [output, realisation] : waitee->buildResult.builtOutputs)
auto * dg = dynamic_cast<DerivationGoal *>(&*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( inputDrvOutputs.insert_or_assign(
{ bfd->drvPath, output.outputName }, { dg->drvPath, output.outputName },
realisation.outPath); realisation.outPath);
}
}
}
} }
} }

View file

@ -39,7 +39,7 @@ 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, const std::vector<DerivedPath> & reqs,
BuildMode buildMode, BuildMode buildMode,
std::shared_ptr<Store> evalStore) std::shared_ptr<Store> evalStore)
@ -47,15 +47,23 @@ std::vector<BuildResult> Store::buildPathsWithResults(
Worker worker(*this, evalStore ? *evalStore : *this); Worker worker(*this, evalStore ? *evalStore : *this);
Goals goals; Goals goals;
for (const auto & br : reqs) std::vector<std::pair<const DerivedPath &, GoalPtr>> state;
goals.insert(worker.makeGoal(br, buildMode));
for (const auto & req : reqs) {
auto goal = worker.makeGoal(req, buildMode);
goals.insert(goal);
state.push_back({req, goal});
}
worker.run(goals); worker.run(goals);
std::vector<BuildResult> results; std::vector<KeyedBuildResult> results;
for (auto & i : goals) for (auto & [req, goalPtr] : state)
results.push_back(i->buildResult); results.emplace_back(KeyedBuildResult {
goalPtr->getBuildResult(req),
/* .path = */ req,
});
return results; return results;
} }
@ -68,15 +76,14 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat
try { try {
worker.run(Goals{goal}); worker.run(Goals{goal});
return goal->buildResult; return goal->getBuildResult(DerivedPath::Built {
.drvPath = drvPath,
.outputs = OutputsSpec::All {},
});
} catch (Error & e) { } catch (Error & e) {
return BuildResult { return BuildResult {
.status = BuildResult::MiscFailure, .status = BuildResult::MiscFailure,
.errorMsg = e.msg(), .errorMsg = e.msg(),
.path = DerivedPath::Built {
.drvPath = drvPath,
.outputs = OutputsSpec::All { },
},
}; };
}; };
} }

View file

@ -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<DerivedPath::Built>(&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) void addToWeakGoals(WeakGoals & goals, GoalPtr p)
{ {
if (goals.find(p) != goals.end()) if (goals.find(p) != goals.end())

View file

@ -81,11 +81,26 @@ struct Goal : public std::enable_shared_from_this<Goal>
*/ */
ExitCode exitCode = ecBusy; ExitCode exitCode = ecBusy;
protected:
/** /**
* Build result. * Build result.
*/ */
BuildResult buildResult; 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. * Exception containing an error message, if any.
*/ */
@ -93,7 +108,6 @@ struct Goal : public std::enable_shared_from_this<Goal>
Goal(Worker & worker, DerivedPath path) Goal(Worker & worker, DerivedPath path)
: worker(worker) : worker(worker)
, buildResult { .path = std::move(path) }
{ } { }
virtual ~Goal() virtual ~Goal()

View file

@ -1335,7 +1335,7 @@ struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual Lo
result.rethrow(); result.rethrow();
} }
std::vector<BuildResult> buildPathsWithResults( std::vector<KeyedBuildResult> buildPathsWithResults(
const std::vector<DerivedPath> & paths, const std::vector<DerivedPath> & paths,
BuildMode buildMode = bmNormal, BuildMode buildMode = bmNormal,
std::shared_ptr<Store> evalStore = nullptr) override std::shared_ptr<Store> evalStore = nullptr) override

View file

@ -287,12 +287,7 @@ public:
conn->to.flush(); conn->to.flush();
BuildResult status { BuildResult status;
.path = DerivedPath::Built {
.drvPath = drvPath,
.outputs = OutputsSpec::All { },
},
};
status.status = (BuildResult::Status) readInt(conn->from); status.status = (BuildResult::Status) readInt(conn->from);
conn->from >> status.errorMsg; conn->from >> status.errorMsg;
@ -330,7 +325,7 @@ public:
conn->to.flush(); conn->to.flush();
BuildResult result { .path = DerivedPath::Opaque { StorePath::dummy } }; BuildResult result;
result.status = (BuildResult::Status) readInt(conn->from); result.status = (BuildResult::Status) readInt(conn->from);
if (!result.success()) { if (!result.success()) {

View file

@ -125,10 +125,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> {}); 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); res.status = (BuildResult::Status) readInt(from);
from from
>> res.errorMsg >> res.errorMsg
@ -142,7 +158,6 @@ BuildResult read(const Store & store, Source & from, Phantom<BuildResult> _)
void write(const Store & store, Sink & to, const BuildResult & res) void write(const Store & store, Sink & to, const BuildResult & res)
{ {
worker_proto::write(store, to, res.path);
to to
<< res.status << res.status
<< res.errorMsg << res.errorMsg
@ -865,7 +880,7 @@ void RemoteStore::buildPaths(const std::vector<DerivedPath> & drvPaths, BuildMod
readInt(conn->from); readInt(conn->from);
} }
std::vector<BuildResult> RemoteStore::buildPathsWithResults( std::vector<KeyedBuildResult> RemoteStore::buildPathsWithResults(
const std::vector<DerivedPath> & paths, const std::vector<DerivedPath> & paths,
BuildMode buildMode, BuildMode buildMode,
std::shared_ptr<Store> evalStore) std::shared_ptr<Store> evalStore)
@ -880,7 +895,7 @@ std::vector<BuildResult> RemoteStore::buildPathsWithResults(
writeDerivedPaths(*this, conn, paths); writeDerivedPaths(*this, conn, paths);
conn->to << buildMode; conn->to << buildMode;
conn.processStderr(); 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 { } else {
// Avoid deadlock. // Avoid deadlock.
conn_.reset(); conn_.reset();
@ -889,21 +904,25 @@ std::vector<BuildResult> RemoteStore::buildPathsWithResults(
// fails, but meh. // fails, but meh.
buildPaths(paths, buildMode, evalStore); buildPaths(paths, buildMode, evalStore);
std::vector<BuildResult> results; std::vector<KeyedBuildResult> results;
for (auto & path : paths) { for (auto & path : paths) {
std::visit( std::visit(
overloaded { overloaded {
[&](const DerivedPath::Opaque & bo) { [&](const DerivedPath::Opaque & bo) {
results.push_back(BuildResult { results.push_back(KeyedBuildResult {
.status = BuildResult::Substituted, {
.path = bo, .status = BuildResult::Substituted,
},
/* .path = */ bo,
}); });
}, },
[&](const DerivedPath::Built & bfd) { [&](const DerivedPath::Built & bfd) {
BuildResult res { KeyedBuildResult res {
.status = BuildResult::Built, {
.path = bfd, .status = BuildResult::Built
},
/* .path = */ bfd,
}; };
OutputPathMap outputs; OutputPathMap outputs;
@ -952,12 +971,7 @@ BuildResult RemoteStore::buildDerivation(const StorePath & drvPath, const BasicD
writeDerivation(conn->to, *this, drv); writeDerivation(conn->to, *this, drv);
conn->to << buildMode; conn->to << buildMode;
conn.processStderr(); conn.processStderr();
BuildResult res { BuildResult res;
.path = DerivedPath::Built {
.drvPath = drvPath,
.outputs = OutputsSpec::All { },
},
};
res.status = (BuildResult::Status) readInt(conn->from); res.status = (BuildResult::Status) readInt(conn->from);
conn->from >> res.errorMsg; conn->from >> res.errorMsg;
if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 29) { if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 29) {

View file

@ -114,7 +114,7 @@ public:
void buildPaths(const std::vector<DerivedPath> & paths, BuildMode buildMode, std::shared_ptr<Store> evalStore) override; 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, const std::vector<DerivedPath> & paths,
BuildMode buildMode, BuildMode buildMode,
std::shared_ptr<Store> evalStore) override; std::shared_ptr<Store> evalStore) override;

View file

@ -92,6 +92,7 @@ enum BuildMode { bmNormal, bmRepair, bmCheck };
enum TrustedFlag : bool { NotTrusted = false, Trusted = true }; enum TrustedFlag : bool { NotTrusted = false, Trusted = true };
struct BuildResult; struct BuildResult;
struct KeyedBuildResult;
typedef std::map<StorePath, std::optional<ContentAddress>> StorePathCAMap; typedef std::map<StorePath, std::optional<ContentAddress>> StorePathCAMap;
@ -575,7 +576,7 @@ public:
* case of a build/substitution error, this function won't throw an * case of a build/substitution error, this function won't throw an
* exception, but return a BuildResult containing an error message. * exception, but return a BuildResult containing an error message.
*/ */
virtual std::vector<BuildResult> buildPathsWithResults( virtual std::vector<KeyedBuildResult> buildPathsWithResults(
const std::vector<DerivedPath> & paths, const std::vector<DerivedPath> & paths,
BuildMode buildMode = bmNormal, BuildMode buildMode = bmNormal,
std::shared_ptr<Store> evalStore = nullptr); std::shared_ptr<Store> evalStore = nullptr);

View file

@ -103,6 +103,7 @@ MAKE_WORKER_PROTO(, DerivedPath);
MAKE_WORKER_PROTO(, Realisation); MAKE_WORKER_PROTO(, Realisation);
MAKE_WORKER_PROTO(, DrvOutput); MAKE_WORKER_PROTO(, DrvOutput);
MAKE_WORKER_PROTO(, BuildResult); MAKE_WORKER_PROTO(, BuildResult);
MAKE_WORKER_PROTO(, KeyedBuildResult);
MAKE_WORKER_PROTO(, std::optional<TrustedFlag>); MAKE_WORKER_PROTO(, std::optional<TrustedFlag>);
MAKE_WORKER_PROTO(template<typename T>, std::vector<T>); MAKE_WORKER_PROTO(template<typename T>, std::vector<T>);