lix-project/lix
20
58
Fork 31
Code Issues 359 Code Review (Gerrit) Projects 3 Releases Wiki Activity

Introduce SingleDrvOutputs

Browse source 
In many cases we are dealing with a collection of realisations, they are
all outputs of the same derivation. In that case, we don't need
"derivation hashes modulos" to be part of our map key, because the
output names alone will be unique. Those hashes are still part of the
realisation proper, so we aren't loosing any information, we're just
"normalizing our schema" by narrowing the "primary key".

Besides making our data model a bit "tighter" this allows us to avoid a
double `for` loop in `DerivationGoal::waiteeDone`. The inner `for` loop
was previously just to select the output we cared about without knowing
its hash. Now we can just select the output by name directly.

Note that neither protocol is changed as part of this: we are still
transferring `DrvOutputs` over the wire for `BuildResult`s. I would only
consider revising this once #6223 is merged, and we can mention protocol
versions inside factored-out serialization logic. Until then it is
better not change anything because it would come a the cost of code
reuse.
This commit is contained in:
John Ericson 2023-04-14 18:18:32 -04:00
parent 0f2b5146c7
commit 24866b71c4
13 changed files with 90 additions and 38 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
src/build-remote/build-remote.cc
View file

@ -311,8 +311,9 @@ connected:
auto thisOutputId = DrvOutput{ thisOutputHash, outputName };
if (!store->queryRealisation(thisOutputId)) {
debug("missing output %s", outputName);
assert(result.builtOutputs.count(thisOutputId));
auto newRealisation = result.builtOutputs.at(thisOutputId);
auto i = result.builtOutputs.find(outputName);
assert(i != result.builtOutputs.end());
auto & newRealisation = i->second;
missingRealisations.insert(newRealisation);
missingPaths.insert(newRealisation.outPath);
}

4
src/libcmd/installables.cc
View file

@ -593,8 +593,8 @@ std::vector<std::pair<ref<Installable>, BuiltPathWithResult>> Installable::build
std::visit(overloaded {
[&](const DerivedPath::Built & bfd) {
std::map<std::string, StorePath> outputs;
for (auto & path : buildResult.builtOutputs)
outputs.emplace(path.first.outputName, path.second.outPath);
for (auto & [outputName, realisation] : buildResult.builtOutputs)
outputs.emplace(outputName, realisation.outPath);
res.push_back({aux.installable, {
.path = BuiltPath::Built { bfd.drvPath, outputs },
.info = aux.info,

2
src/libstore/build-result.hh
View file

@ -87,7 +87,7 @@ struct BuildResult
* For derivations, a mapping from the names of the wanted outputs
* to actual paths.
*/
DrvOutputs builtOutputs;
SingleDrvOutputs builtOutputs;
/**
* The start/stop times of the build (or one of the rounds, if it

26
src/libstore/build/derivation-goal.cc
View file

@ -1013,7 +1013,7 @@ void DerivationGoal::resolvedFinished()
auto resolvedDrv = *resolvedDrvGoal->drv;
auto & resolvedResult = resolvedDrvGoal->buildResult;
DrvOutputs builtOutputs;
SingleDrvOutputs builtOutputs;
if (resolvedResult.success()) {
auto resolvedHashes = staticOutputHashes(worker.store, resolvedDrv);
@ -1039,7 +1039,7 @@ void DerivationGoal::resolvedFinished()
worker.store.printStorePath(drvPath), wantedOutput);
auto realisation = [&]{
auto take1 = get(resolvedResult.builtOutputs, DrvOutput { *resolvedHash, wantedOutput });
auto take1 = get(resolvedResult.builtOutputs, wantedOutput);
if (take1) return *take1;
/* The above `get` should work. But sateful tracking of
@ -1064,7 +1064,7 @@ void DerivationGoal::resolvedFinished()
worker.store.registerDrvOutput(newRealisation);
}
outputPaths.insert(realisation.outPath);
builtOutputs.emplace(realisation.id, realisation);
builtOutputs.emplace(wantedOutput, realisation);
}
runPostBuildHook(
@ -1189,7 +1189,7 @@ HookReply DerivationGoal::tryBuildHook()
}
DrvOutputs DerivationGoal::registerOutputs()
SingleDrvOutputs DerivationGoal::registerOutputs()
{
/* When using a build hook, the build hook can register the output
as valid (by doing `nix-store --import'). If so we don't have
@ -1351,7 +1351,7 @@ OutputPathMap DerivationGoal::queryDerivationOutputMap()
}
std::pair<bool, DrvOutputs> DerivationGoal::checkPathValidity()
std::pair<bool, SingleDrvOutputs> DerivationGoal::checkPathValidity()
{
if (!drv->type().isPure()) return { false, {} };
@ -1364,7 +1364,7 @@ std::pair<bool, DrvOutputs> DerivationGoal::checkPathValidity()
return static_cast<StringSet>(names);
},
}, wantedOutputs.raw());
DrvOutputs validOutputs;
SingleDrvOutputs validOutputs;
for (auto & i : queryPartialDerivationOutputMap()) {
auto initialOutput = get(initialOutputs, i.first);
@ -1407,7 +1407,7 @@ std::pair<bool, DrvOutputs> DerivationGoal::checkPathValidity()
}
}
if (info.wanted && info.known && info.known->isValid())
validOutputs.emplace(drvOutput, Realisation { drvOutput, info.known->path });
validOutputs.emplace(i.first, Realisation { drvOutput, info.known->path });
}
// If we requested all the outputs, we are always fine.
@ -1431,7 +1431,7 @@ std::pair<bool, DrvOutputs> DerivationGoal::checkPathValidity()
}
DrvOutputs DerivationGoal::assertPathValidity()
SingleDrvOutputs DerivationGoal::assertPathValidity()
{
auto [allValid, validOutputs] = checkPathValidity();
if (!allValid)
@ -1442,7 +1442,7 @@ DrvOutputs DerivationGoal::assertPathValidity()
void DerivationGoal::done(
BuildResult::Status status,
DrvOutputs builtOutputs,
SingleDrvOutputs builtOutputs,
std::optional<Error> ex)
{
buildResult.status = status;
@ -1498,11 +1498,11 @@ void DerivationGoal::waiteeDone(GoalPtr waitee, ExitCode result)
.outputs = OutputsSpec::Names { outputName },
});
if (buildResult.success()) {
for (auto & [output, realisation] : buildResult.builtOutputs) {
auto i = buildResult.builtOutputs.find(outputName);
if (i != buildResult.builtOutputs.end())
inputDrvOutputs.insert_or_assign(
{ dg->drvPath, output.outputName },
realisation.outPath);
}
{ dg->drvPath, outputName },
i->second.outPath);
}
}
}

12
src/libstore/build/derivation-goal.hh
View file

@ -253,7 +253,7 @@ struct DerivationGoal : public Goal
* Check that the derivation outputs all exist and register them
* as valid.
*/
virtual DrvOutputs registerOutputs();
virtual SingleDrvOutputs registerOutputs();
/**
* Open a log file and a pipe to it.
@ -306,17 +306,17 @@ struct DerivationGoal : public Goal
* 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
* 'DrvOutputs' structure containing the valid and wanted
* 'SingleDrvOutputs' structure containing the valid and wanted
* outputs.
*/
std::pair<bool, DrvOutputs> checkPathValidity();
std::pair<bool, SingleDrvOutputs> checkPathValidity();
/**
* Aborts if any output is not valid or corrupt, and otherwise
* returns a 'DrvOutputs' structure containing the wanted
* returns a 'SingleDrvOutputs' structure containing the wanted
* outputs.
*/
DrvOutputs assertPathValidity();
SingleDrvOutputs assertPathValidity();
/**
* Forcibly kill the child process, if any.
@ -329,7 +329,7 @@ struct DerivationGoal : public Goal
void done(
BuildResult::Status status,
DrvOutputs builtOutputs = {},
SingleDrvOutputs builtOutputs = {},
std::optional<Error> ex = {});
void waiteeDone(GoalPtr waitee, ExitCode result) override;

2
src/libstore/build/goal.cc
View file

@ -23,7 +23,7 @@ BuildResult Goal::getBuildResult(const DerivedPath & req) {
*/
for (auto it = res.builtOutputs.begin(); it != res.builtOutputs.end();) {
if (bp.outputs.contains(it->first.outputName))
if (bp.outputs.contains(it->first))
++it;
else
it = res.builtOutputs.erase(it);

6
src/libstore/build/local-derivation-goal.cc
View file

@ -2174,7 +2174,7 @@ void LocalDerivationGoal::runChild()
}
DrvOutputs LocalDerivationGoal::registerOutputs()
SingleDrvOutputs LocalDerivationGoal::registerOutputs()
{
/* When using a build hook, the build hook can register the output
as valid (by doing `nix-store --import'). If so we don't have
@ -2691,7 +2691,7 @@ DrvOutputs LocalDerivationGoal::registerOutputs()
means it's safe to link the derivation to the output hash. We must do
that for floating CA derivations, which otherwise couldn't be cached,
but it's fine to do in all cases. */
DrvOutputs builtOutputs;
SingleDrvOutputs builtOutputs;
for (auto & [outputName, newInfo] : infos) {
auto oldinfo = get(initialOutputs, outputName);
@ -2710,7 +2710,7 @@ DrvOutputs LocalDerivationGoal::registerOutputs()
worker.store.registerDrvOutput(thisRealisation);
}
if (wantedOutputs.contains(outputName))
builtOutputs.emplace(thisRealisation.id, thisRealisation);
builtOutputs.emplace(outputName, thisRealisation);
}
return builtOutputs;

2
src/libstore/build/local-derivation-goal.hh
View file

@ -237,7 +237,7 @@ struct LocalDerivationGoal : public DerivationGoal
* Check that the derivation outputs all exist and register them
* as valid.
*/
DrvOutputs registerOutputs() override;
SingleDrvOutputs registerOutputs() override;
void signRealisation(Realisation &) override;

5
src/libstore/daemon.cc
View file

@ -637,7 +637,10 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
to << res.timesBuilt << res.isNonDeterministic << res.startTime << res.stopTime;
}
if (GET_PROTOCOL_MINOR(clientVersion) >= 28) {
worker_proto::write(*store, to, res.builtOutputs);
DrvOutputs builtOutputs;
for (auto & [output, realisation] : res.builtOutputs)
builtOutputs.insert_or_assign(realisation.id, realisation);
worker_proto::write(*store, to, builtOutputs);
}
break;
}

6
src/libstore/legacy-ssh-store.cc
View file

@ -294,7 +294,11 @@ public:
if (GET_PROTOCOL_MINOR(conn->remoteVersion) >= 3)
conn->from >> status.timesBuilt >> status.isNonDeterministic >> status.startTime >> status.stopTime;
if (GET_PROTOCOL_MINOR(conn->remoteVersion) >= 6) {
status.builtOutputs = worker_proto::read(*this, conn->from, Phantom<DrvOutputs> {});
auto builtOutputs = worker_proto::read(*this, conn->from, Phantom<DrvOutputs> {});
for (auto && [output, realisation] : builtOutputs)
status.builtOutputs.insert_or_assign(
std::move(output.outputName),
std::move(realisation));
}
return status;
}

33
src/libstore/realisation.hh
View file

@ -13,9 +13,25 @@ namespace nix {
class Store;
/**
* A general `Realisation` key.
*
* This is similar to a `DerivedPath::Opaque`, but the derivation is
* identified by its "hash modulo" instead of by its store path.
*/
struct DrvOutput {
// The hash modulo of the derivation
/**
* The hash modulo of the derivation.
*
* Computed from the derivation itself for most types of
* derivations, but computed from the (fixed) content address of the
* output for fixed-output derivations.
*/
Hash drvHash;
/**
* The name of the output.
*/
std::string outputName;
std::string to_string() const;
@ -60,6 +76,21 @@ struct Realisation {
GENERATE_CMP(Realisation, me->id, me->outPath);
};
/**
* Collection type for a single derivation's outputs' `Realisation`s.
*
* Since these are the outputs of a single derivation, we know the
* output names are unique so we can use them as the map key.
*/
typedef std::map<std::string, Realisation> SingleDrvOutputs;
/**
* Collection type for multiple derivations' outputs' `Realisation`s.
*
* `DrvOutput` is used because in general the derivations are not all
* the same, so we need to identify firstly which derivation, and
* secondly which output of that derivation.
*/
typedef std::map<DrvOutput, Realisation> DrvOutputs;
struct OpaquePath {

20
src/libstore/remote-store.cc
View file

@ -152,7 +152,11 @@ BuildResult read(const Store & store, Source & from, Phantom<BuildResult> _)
>> res.isNonDeterministic
>> res.startTime
>> res.stopTime;
res.builtOutputs = worker_proto::read(store, from, Phantom<DrvOutputs> {});
auto builtOutputs = worker_proto::read(store, from, Phantom<DrvOutputs> {});
for (auto && [output, realisation] : builtOutputs)
res.builtOutputs.insert_or_assign(
std::move(output.outputName),
std::move(realisation));
return res;
}
@ -165,7 +169,10 @@ void write(const Store & store, Sink & to, const BuildResult & res)
<< res.isNonDeterministic
<< res.startTime
<< res.stopTime;
worker_proto::write(store, to, res.builtOutputs);
DrvOutputs builtOutputs;
for (auto & [output, realisation] : res.builtOutputs)
builtOutputs.insert_or_assign(realisation.id, realisation);
worker_proto::write(store, to, builtOutputs);
}
@ -941,10 +948,10 @@ std::vector<KeyedBuildResult> RemoteStore::buildPathsWithResults(
queryRealisation(outputId);
if (!realisation)
throw MissingRealisation(outputId);
res.builtOutputs.emplace(realisation->id, *realisation);
res.builtOutputs.emplace(output, *realisation);
} else {
res.builtOutputs.emplace(
outputId,
output,
Realisation {
.id = outputId,
.outPath = outputPath,
@ -979,7 +986,10 @@ BuildResult RemoteStore::buildDerivation(const StorePath & drvPath, const BasicD
}
if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 28) {
auto builtOutputs = worker_proto::read(*this, conn->from, Phantom<DrvOutputs> {});
res.builtOutputs = builtOutputs;
for (auto && [output, realisation] : builtOutputs)
res.builtOutputs.insert_or_assign(
std::move(output.outputName),
std::move(realisation));
}
return res;
}

5
src/nix-store/nix-store.cc
View file

@ -935,7 +935,10 @@ static void opServe(Strings opFlags, Strings opArgs)
if (GET_PROTOCOL_MINOR(clientVersion) >= 3)
out << status.timesBuilt << status.isNonDeterministic << status.startTime << status.stopTime;
if (GET_PROTOCOL_MINOR(clientVersion) >= 6) {
worker_proto::write(*store, out, status.builtOutputs);
DrvOutputs builtOutputs;
for (auto & [output, realisation] : status.builtOutputs)
builtOutputs.insert_or_assign(realisation.id, realisation);
worker_proto::write(*store, out, builtOutputs);
}
break;