Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare for CA derivation support with lower impact changes #1316

Merged
merged 14 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/hydra-eval-jobs/hydra-eval-jobs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,11 @@ static void worker(

if (auto drv = getDerivation(state, *v, false)) {

DrvInfo::Outputs outputs = drv->queryOutputs();
// CA derivations do not have static output paths, so we
// have to defensively not query output paths in case we
// encounter one.
DrvInfo::Outputs outputs = drv->queryOutputs(
!experimentalFeatureSettings.isEnabled(Xp::CaDerivations));

if (drv->querySystem() == "unknown")
throw EvalError("derivation must have a 'system' attribute");
Expand Down Expand Up @@ -239,12 +243,12 @@ static void worker(
}

nlohmann::json out;
for (auto & j : outputs)
// FIXME: handle CA/impure builds.
if (j.second)
out[j.first] = state.store->printStorePath(*j.second);
for (auto & [outputName, optOutputPath] : outputs)
if (optOutputPath)
out[outputName] = state.store->printStorePath(*optOutputPath);
else
out[outputName] = "";
Copy link
Member Author

@Ericson2314 Ericson2314 Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new case is dead code so long as ca-derivations is not an enabled experimental feature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we warn or errror for this atm (at least if ca-derivations isn't enabled), just to make sure that all these are indeed dead code and we're not accidentally triggering something unexpected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, good call!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

job["outputs"] = std::move(out);

reply["job"] = std::move(job);
}

Expand Down
104 changes: 85 additions & 19 deletions src/hydra-queue-runner/build-remote.cc
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,40 @@ static StorePaths reverseTopoSortPaths(const std::map<StorePath, ValidPathInfo>
return sorted;
}

/**
* Replace the input derivations by their output paths to send a minimal closure
* to the builder.
*
* If we can afford it, resolve it, so that the newly generated derivation still
* has some sensible output paths.
*/
BasicDerivation inlineInputDerivations(Store & store, Derivation & drv, const StorePath & drvPath)
{
BasicDerivation ret;
if (!drv.type().hasKnownOutputPaths()) {
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
auto maybeBasicDrv = drv.tryResolve(store);
if (!maybeBasicDrv)
throw Error(
"the derivation '%s' can’t be resolved. It’s probably "
"missing some outputs",
store.printStorePath(drvPath));
ret = *maybeBasicDrv;
} else {
// If the derivation is a real `InputAddressed` derivation, we must
// resolve it manually to keep the original output paths
ret = BasicDerivation(drv);
for (auto & [drvPath, node] : drv.inputDrvs.map) {
auto drv2 = store.readDerivation(drvPath);
auto drv2Outputs = drv2.outputsAndOptPaths(store);
for (auto & name : node.value) {
auto inputPath = drv2Outputs.at(name);
ret.inputSrcs.insert(*inputPath.second);
}
}
}
return ret;
}

static std::pair<Path, AutoCloseFD> openLogFile(const std::string & logDir, const StorePath & drvPath)
{
std::string base(drvPath.to_string());
Expand Down Expand Up @@ -228,17 +262,7 @@ static BasicDerivation sendInputs(
counter & nrStepsCopyingTo
)
{
BasicDerivation basicDrv(*step.drv);

for (const auto & [drvPath, node] : step.drv->inputDrvs.map) {
auto drv2 = localStore.readDerivation(drvPath);
for (auto & name : node.value) {
if (auto i = get(drv2.outputs, name)) {
auto outPath = i->path(localStore, drv2.name, name);
basicDrv.inputSrcs.insert(*outPath);
}
}
}
BasicDerivation basicDrv = inlineInputDerivations(localStore, *step.drv, step.drvPath);

/* Ensure that the inputs exist in the destination store. This is
a no-op for regular stores, but for the binary cache store,
Expand Down Expand Up @@ -323,8 +347,33 @@ static BuildResult performBuild(
result.stopTime = stop;
}
}

// Get the newly built outputs, either from the remote if it
// supports it, or by introspecting the derivation if the remote is
// too old.
if (GET_PROTOCOL_MINOR(conn.remoteVersion) >= 6) {
ServeProto::Serialise<DrvOutputs>::read(localStore, conn);
auto builtOutputs = ServeProto::Serialise<DrvOutputs>::read(localStore, conn);
for (auto && [output, realisation] : builtOutputs)
result.builtOutputs.insert_or_assign(
std::move(output.outputName),
std::move(realisation));
} else {
// If the remote is too old to handle CA derivations, we can’t get this
// far anyways
assert(drv.type().hasKnownOutputPaths());
DerivationOutputsAndOptPaths drvOutputs = drv.outputsAndOptPaths(localStore);
auto outputHashes = staticOutputHashes(localStore, drv);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These output hashes are not really used in the input-addressing case, but they may be expensive to calculate because the underlying hashDerivationModulo loads the derivation closure (but cached).

for (auto & [outputName, output] : drvOutputs) {
auto outputPath = output.second;
// We’ve just asserted that the output paths of the derivation
// were known
assert(outputPath);
auto outputHash = outputHashes.at(outputName);
auto drvOutput = DrvOutput { outputHash, outputName };
result.builtOutputs.insert_or_assign(
std::move(outputName),
Realisation { drvOutput, *outputPath });
}
}

return result;
Expand Down Expand Up @@ -376,6 +425,7 @@ static void copyPathFromRemote(
const ValidPathInfo & info
)
{
for (auto * store : {&destStore, &localStore}) {
/* Receive the NAR from the remote and add it to the
destination store. Meanwhile, extract all the info from the
NAR that getBuildOutput() needs. */
Expand All @@ -395,7 +445,8 @@ static void copyPathFromRemote(
extractNarData(tee, localStore.printStorePath(info.path), narMembers);
});

destStore.addToStore(info, *source2, NoRepair, NoCheckSigs);
store->addToStore(info, *source2, NoRepair, NoCheckSigs);
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
}
}

static void copyPathsFromRemote(
Expand Down Expand Up @@ -592,6 +643,10 @@ void State::buildRemote(ref<Store> destStore,
result.logFile = "";
}

StorePathSet outputs;
for (auto & [_, realisation] : buildResult.builtOutputs)
outputs.insert(realisation.outPath);

/* Copy the output paths. */
if (!machine->isLocalhost() || localStore != std::shared_ptr<Store>(destStore)) {
updateStep(ssReceivingOutputs);
Expand All @@ -600,12 +655,6 @@ void State::buildRemote(ref<Store> destStore,

auto now1 = std::chrono::steady_clock::now();

StorePathSet outputs;
for (auto & i : step->drv->outputsAndOptPaths(*localStore)) {
if (i.second.second)
outputs.insert(*i.second.second);
}

size_t totalNarSize = 0;
auto infos = build_remote::queryPathInfos(conn, *localStore, outputs, totalNarSize);

Expand All @@ -624,6 +673,23 @@ void State::buildRemote(ref<Store> destStore,
result.overhead += std::chrono::duration_cast<std::chrono::milliseconds>(now2 - now1).count();
}

/* Register the outputs of the newly built drv */
if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipped in non-CA case

auto outputHashes = staticOutputHashes(*localStore, *step->drv);
for (auto & [outputName, realisation] : buildResult.builtOutputs) {
// Register the resolved drv output
localStore->registerDrvOutput(realisation);
destStore->registerDrvOutput(realisation);

// Also register the unresolved one
auto unresolvedRealisation = realisation;
unresolvedRealisation.signatures.clear();
unresolvedRealisation.id.drvHash = outputHashes.at(outputName);
localStore->registerDrvOutput(unresolvedRealisation);
destStore->registerDrvOutput(unresolvedRealisation);
}
}

/* Shut down the connection. */
child.to = -1;
child.pid.wait();
Expand Down
19 changes: 9 additions & 10 deletions src/hydra-queue-runner/build-result.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,18 @@ using namespace nix;
BuildOutput getBuildOutput(
nix::ref<Store> store,
NarMemberDatas & narMembers,
const Derivation & drv)
const OutputPathMap derivationOutputs)
{
BuildOutput res;

/* Compute the closure size. */
StorePathSet outputs;
StorePathSet closure;
for (auto & i : drv.outputsAndOptPaths(*store))
if (i.second.second) {
store->computeFSClosure(*i.second.second, closure);
outputs.insert(*i.second.second);
}
for (auto& [outputName, outputPath] : derivationOutputs) {
store->computeFSClosure(outputPath, closure);
outputs.insert(outputPath);
res.outputs.insert({outputName, outputPath});
}
for (auto & path : closure) {
auto info = store->queryPathInfo(path);
res.closureSize += info->narSize;
Expand Down Expand Up @@ -107,13 +107,12 @@ BuildOutput getBuildOutput(
/* If no build products were explicitly declared, then add all
outputs as a product of type "nix-build". */
if (!explicitProducts) {
for (auto & [name, output] : drv.outputs) {
for (auto & [name, output] : derivationOutputs) {
BuildProduct product;
auto outPath = output.path(*store, drv.name, name);
product.path = store->printStorePath(*outPath);
product.path = store->printStorePath(output);
product.type = "nix-build";
product.subtype = name == "out" ? "" : name;
product.name = outPath->name();
product.name = output.name();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole file is the same logic as before. In the CA case more work has to be done to get the paths so that was moved to the caller. In the input-addressed case it is easy and moving the tiny bit of work back and forth should not be noticeable.


auto file = narMembers.find(product.path);
assert(file != narMembers.end());
Expand Down
8 changes: 4 additions & 4 deletions src/hydra-queue-runner/builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ State::StepResult State::doBuildStep(nix::ref<Store> destStore,

if (result.stepStatus == bsSuccess) {
updateStep(ssPostProcessing);
res = getBuildOutput(destStore, narMembers, *step->drv);
res = getBuildOutput(destStore, narMembers, localStore->queryDerivationOutputMap(step->drvPath));
}
}

Expand Down Expand Up @@ -277,9 +277,9 @@ State::StepResult State::doBuildStep(nix::ref<Store> destStore,

assert(stepNr);

for (auto & i : step->drv->outputsAndOptPaths(*localStore)) {
if (i.second.second)
addRoot(*i.second.second);
for (auto & i : localStore->queryPartialDerivationOutputMap(step->drvPath)) {
if (i.second)
addRoot(*i.second);
}

/* Register success in the database for all Build objects that
Expand Down
4 changes: 3 additions & 1 deletion src/hydra-queue-runner/hydra-build-result.hh
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ struct BuildOutput

std::list<BuildProduct> products;

std::map<std::string, nix::StorePath> outputs;

std::map<std::string, BuildMetric> metrics;
};

BuildOutput getBuildOutput(
nix::ref<nix::Store> store,
NarMemberDatas & narMembers,
const nix::Derivation & drv);
const nix::OutputPathMap derivationOutputs);
27 changes: 24 additions & 3 deletions src/hydra-queue-runner/hydra-queue-runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -312,10 +312,10 @@ unsigned int State::createBuildStep(pqxx::work & txn, time_t startTime, BuildID

if (r.affected_rows() == 0) goto restart;

for (auto & [name, output] : step->drv->outputs)
for (auto & [name, output] : localStore->queryPartialDerivationOutputMap(step->drvPath))
txn.exec_params0
("insert into BuildStepOutputs (build, stepnr, name, path) values ($1, $2, $3, $4)",
buildId, stepNr, name, localStore->printStorePath(*output.path(*localStore, step->drv->name, name)));
buildId, stepNr, name, output ? localStore->printStorePath(*output) : "");

if (status == bsBusy)
txn.exec(fmt("notify step_started, '%d\t%d'", buildId, stepNr));
Expand Down Expand Up @@ -352,11 +352,23 @@ void State::finishBuildStep(pqxx::work & txn, const RemoteResult & result,
assert(result.logFile.find('\t') == std::string::npos);
txn.exec(fmt("notify step_finished, '%d\t%d\t%s'",
buildId, stepNr, result.logFile));

if (result.stepStatus == bsSuccess) {
// Update the corresponding `BuildStepOutputs` row to add the output path
auto res = txn.exec_params1("select drvPath from BuildSteps where build = $1 and stepnr = $2", buildId, stepNr);
assert(res.size());
StorePath drvPath = localStore->parseStorePath(res[0].as<std::string>());
// If we've finished building, all the paths should be known
for (auto & [name, output] : localStore->queryDerivationOutputMap(drvPath))
txn.exec_params0
("update BuildStepOutputs set path = $4 where build = $1 and stepnr = $2 and name = $3",
buildId, stepNr, name, localStore->printStorePath(output));
}
Copy link
Member Author

@Ericson2314 Ericson2314 Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an idempotent / no changed output extra step in the input-addressed case.

}


int State::createSubstitutionStep(pqxx::work & txn, time_t startTime, time_t stopTime,
Build::ptr build, const StorePath & drvPath, const std::string & outputName, const StorePath & storePath)
Build::ptr build, const StorePath & drvPath, const nix::Derivation drv, const std::string & outputName, const StorePath & storePath)
{
restart:
auto stepNr = allocBuildStep(txn, build->id);
Expand Down Expand Up @@ -457,6 +469,15 @@ void State::markSucceededBuild(pqxx::work & txn, Build::ptr build,
res.releaseName != "" ? std::make_optional(res.releaseName) : std::nullopt,
isCachedBuild ? 1 : 0);

for (auto & [outputName, outputPath] : res.outputs) {
txn.exec_params0
("update BuildOutputs set path = $3 where build = $1 and name = $2",
build->id,
outputName,
localStore->printStorePath(outputPath)
);
}

Comment on lines +472 to +480
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an idempotent / no changed output extra step in the input-addressed case.

txn.exec_params0("delete from BuildProducts where build = $1", build->id);

unsigned int productNr = 1;
Expand Down
Loading
Loading