Skip to content

Commit a55806a

Browse files
Ericson2314xokdvium
andcommitted
Get rid of addWantedOutputs
This is just a code cleanup; it should not be behavior change. `addWantedOutputs` is removed by introducing `DerivationTrampolineGoal`. `DerivationGoal` now only tracks a single output, and is back to tracking a plain store path `drvPath`, not a deriving path one. Its `addWantedOutputs` method is gone. These changes will allow subsequent PRs to simplify it greatly. Because the purpose of each goal is back to being immutable, we can also once again make `Goal::buildResult` a public field, and get rid of the `getBuildResult` method. This simplifies things also. `DerivationTrampolineGoal` is, as the nane is supposed to indicate, a cheap "trampoline" goal. It takes immutable sets of wanted outputs, and just kicks of `DerivationGoal`s for them. Since now "actual work" is done in these goals, it is not wasteful to have separate ones for separate sets of outputs, even if those outputs (and the derivations they are from) overlap. This design is described in more detail in the doc comments on the goal types, which I've now greatly expanded. --- This separation of concerns will make it possible for future work on issues like #11928, and to continue the path of having more goal types, but each goal type does fewer things (issue #12628). --- This commit in some sense reverts f4f28cd, but that one kept around `addWantedOutputs`. I am quite sure it was having two layers of goals with `addWantedOutputs` that caused the issues --- restarting logic like `addWantedOutputs` has is very tempermental! In this version of the change, we have *zero* layers of `addWantedOutputs` --- no goal type needs it, or otherwise has a mutable objective --- and so I think this change is safe. Co-authored-by: Sergei Zimmerman <[email protected]>
1 parent 47b7d91 commit a55806a

16 files changed

+477
-357
lines changed

src/libstore/build/derivation-building-goal.cc

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#include "nix/store/build/derivation-building-goal.hh"
2-
#include "nix/store/build/derivation-goal.hh"
2+
#include "nix/store/build/derivation-trampoline-goal.hh"
33
#ifndef _WIN32 // TODO enable build hook on Windows
44
# include "nix/store/build/hook-instance.hh"
55
# include "nix/store/build/derivation-builder.hh"
@@ -72,7 +72,7 @@ std::string DerivationBuildingGoal::key()
7272
/* Ensure that derivations get built in order of their name,
7373
i.e. a derivation named "aardvark" always comes before
7474
"baboon". And substitution goals always happen before
75-
derivation goals (due to "b$"). */
75+
derivation goals (due to "bd$"). */
7676
return "bd$" + std::string(drvPath.name()) + "$" + worker.store.printStorePath(drvPath);
7777
}
7878

@@ -266,7 +266,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
266266
auto mEntry = get(inputGoals, drvPath);
267267
if (!mEntry) return std::nullopt;
268268

269-
auto buildResult = (*mEntry)->getBuildResult(DerivedPath::Built{drvPath, OutputsSpec::Names{outputName}});
269+
auto & buildResult = (*mEntry)->buildResult;
270270
if (!buildResult.success()) return std::nullopt;
271271

272272
auto i = get(buildResult.builtOutputs, outputName);
@@ -296,30 +296,28 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
296296
worker.store.printStorePath(pathResolved),
297297
});
298298

299-
// FIXME wanted outputs
300-
auto resolvedDrvGoal = worker.makeDerivationGoal(
301-
makeConstantStorePathRef(pathResolved), OutputsSpec::All{}, buildMode);
299+
/* TODO https://github.com/NixOS/nix/issues/13247 we should
300+
let the calling goal do this, so it has a change to pass
301+
just the output(s) it cares about. */
302+
auto resolvedDrvGoal = worker.makeDerivationTrampolineGoal(
303+
pathResolved, OutputsSpec::All{}, drvResolved, buildMode);
302304
{
303305
Goals waitees{resolvedDrvGoal};
304306
co_await await(std::move(waitees));
305307
}
306308

307309
trace("resolved derivation finished");
308310

309-
auto resolvedDrv = *resolvedDrvGoal->drv;
310-
auto resolvedResult = resolvedDrvGoal->getBuildResult(DerivedPath::Built{
311-
.drvPath = makeConstantStorePathRef(pathResolved),
312-
.outputs = OutputsSpec::All{},
313-
});
311+
auto resolvedResult = resolvedDrvGoal->buildResult;
314312

315313
SingleDrvOutputs builtOutputs;
316314

317315
if (resolvedResult.success()) {
318-
auto resolvedHashes = staticOutputHashes(worker.store, resolvedDrv);
316+
auto resolvedHashes = staticOutputHashes(worker.store, drvResolved);
319317

320318
StorePathSet outputPaths;
321319

322-
for (auto & outputName : resolvedDrv.outputNames()) {
320+
for (auto & outputName : drvResolved.outputNames()) {
323321
auto initialOutput = get(initialOutputs, outputName);
324322
auto resolvedHash = get(resolvedHashes, outputName);
325323
if ((!initialOutput) || (!resolvedHash))
@@ -340,7 +338,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
340338

341339
throw Error(
342340
"derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/realisation)",
343-
resolvedDrvGoal->drvReq->to_string(worker.store), outputName);
341+
worker.store.printStorePath(pathResolved), outputName);
344342
}();
345343

346344
if (!drv->type().isImpure()) {

0 commit comments

Comments
 (0)