Skip to content

Commit 39f6fd9

Browse files
Ericson2314Ericson2314
committed
Fix #13247
Resolve the derivation before creating a building goal, in a context where we know what output(s) we want. That way we have a chance just to download the outputs we want. Fix #13247
1 parent 8f4a739 commit 39f6fd9

File tree

3 files changed

+92
-107
lines changed

3 files changed

+92
-107
lines changed

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

Lines changed: 0 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
#include "nix/store/build/derivation-building-goal.hh"
2-
#include "nix/store/build/derivation-resolution-goal.hh"
32
#include "nix/store/build/derivation-env-desugar.hh"
4-
#include "nix/store/build/derivation-trampoline-goal.hh"
53
#ifndef _WIN32 // TODO enable build hook on Windows
64
# include "nix/store/build/hook-instance.hh"
75
# include "nix/store/build/derivation-builder.hh"
@@ -175,107 +173,6 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
175173
/* Determine the full set of input paths. */
176174

177175
{
178-
auto resolutionGoal = worker.makeDerivationResolutionGoal(drvPath, *drv, buildMode);
179-
{
180-
Goals waitees{resolutionGoal};
181-
co_await await(std::move(waitees));
182-
}
183-
if (nrFailed != 0) {
184-
co_return doneFailure({BuildResult::Failure::DependencyFailed, "resolution failed"});
185-
}
186-
187-
if (resolutionGoal->resolvedDrv) {
188-
auto & [pathResolved, drvResolved] = *resolutionGoal->resolvedDrv;
189-
190-
/* Store the resolved derivation, as part of the record of
191-
what we're actually building */
192-
writeDerivation(worker.store, drvResolved);
193-
194-
/* TODO https://github.com/NixOS/nix/issues/13247 we should
195-
let the calling goal do this, so it has a change to pass
196-
just the output(s) it cares about. */
197-
auto resolvedDrvGoal =
198-
worker.makeDerivationTrampolineGoal(pathResolved, OutputsSpec::All{}, drvResolved, buildMode);
199-
{
200-
Goals waitees{resolvedDrvGoal};
201-
co_await await(std::move(waitees));
202-
}
203-
204-
trace("resolved derivation finished");
205-
206-
auto resolvedResult = resolvedDrvGoal->buildResult;
207-
208-
// No `std::visit` for coroutines yet
209-
if (auto * successP = resolvedResult.tryGetSuccess()) {
210-
auto & success = *successP;
211-
SingleDrvOutputs builtOutputs;
212-
213-
auto outputHashes = staticOutputHashes(worker.evalStore, *drv);
214-
auto resolvedHashes = staticOutputHashes(worker.store, drvResolved);
215-
216-
StorePathSet outputPaths;
217-
218-
for (auto & outputName : drvResolved.outputNames()) {
219-
auto outputHash = get(outputHashes, outputName);
220-
auto resolvedHash = get(resolvedHashes, outputName);
221-
if ((!outputHash) || (!resolvedHash))
222-
throw Error(
223-
"derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/resolve)",
224-
worker.store.printStorePath(drvPath),
225-
outputName);
226-
227-
auto realisation = [&] {
228-
auto take1 = get(success.builtOutputs, outputName);
229-
if (take1)
230-
return *take1;
231-
232-
/* The above `get` should work. But stateful tracking of
233-
outputs in resolvedResult, this can get out of sync with the
234-
store, which is our actual source of truth. For now we just
235-
check the store directly if it fails. */
236-
auto take2 = worker.evalStore.queryRealisation(DrvOutput{*resolvedHash, outputName});
237-
if (take2)
238-
return *take2;
239-
240-
throw Error(
241-
"derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/realisation)",
242-
worker.store.printStorePath(pathResolved),
243-
outputName);
244-
}();
245-
246-
if (!drv->type().isImpure()) {
247-
auto newRealisation = realisation;
248-
newRealisation.id = DrvOutput{*outputHash, outputName};
249-
newRealisation.signatures.clear();
250-
if (!drv->type().isFixed()) {
251-
auto & drvStore = worker.evalStore.isValidPath(drvPath) ? worker.evalStore : worker.store;
252-
newRealisation.dependentRealisations =
253-
drvOutputReferences(worker.store, *drv, realisation.outPath, &drvStore);
254-
}
255-
worker.store.signRealisation(newRealisation);
256-
worker.store.registerDrvOutput(newRealisation);
257-
}
258-
outputPaths.insert(realisation.outPath);
259-
builtOutputs.emplace(outputName, realisation);
260-
}
261-
262-
runPostBuildHook(worker.store, *logger, drvPath, outputPaths);
263-
264-
auto status = success.status;
265-
if (status == BuildResult::Success::AlreadyValid)
266-
status = BuildResult::Success::ResolvesToAlreadyValid;
267-
268-
co_return doneSuccess(success.status, std::move(builtOutputs));
269-
} else if (resolvedResult.tryGetFailure()) {
270-
co_return doneFailure({
271-
BuildResult::Failure::DependencyFailed,
272-
"build of resolved derivation '%s' failed",
273-
worker.store.printStorePath(pathResolved),
274-
});
275-
} else
276-
assert(false);
277-
}
278-
279176
/* If we get this far, we know no dynamic drvs inputs */
280177

281178
for (auto & [depDrvPath, depNode] : drv->inputDrvs.map) {

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

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include "nix/store/build/derivation-goal.hh"
22
#include "nix/store/build/derivation-building-goal.hh"
3+
#include "nix/store/build/derivation-resolution-goal.hh"
34
#ifndef _WIN32 // TODO enable build hook on Windows
45
# include "nix/store/build/hook-instance.hh"
56
# include "nix/store/build/derivation-builder.hh"
@@ -146,6 +147,96 @@ Goal::Co DerivationGoal::haveDerivation()
146147
worker.store.printStorePath(drvPath));
147148
}
148149

150+
auto resolutionGoal = worker.makeDerivationResolutionGoal(drvPath, *drv, buildMode);
151+
{
152+
Goals waitees{resolutionGoal};
153+
co_await await(std::move(waitees));
154+
}
155+
if (nrFailed != 0) {
156+
co_return doneFailure({BuildResult::Failure::DependencyFailed, "resolution failed"});
157+
}
158+
159+
if (resolutionGoal->resolvedDrv) {
160+
auto & [pathResolved, drvResolved] = *resolutionGoal->resolvedDrv;
161+
162+
/* Store the resolved derivation, as part of the record of
163+
what we're actually building */
164+
writeDerivation(worker.store, drvResolved);
165+
166+
auto resolvedDrvGoal = worker.makeDerivationGoal(pathResolved, drvResolved, wantedOutput, buildMode);
167+
{
168+
Goals waitees{resolvedDrvGoal};
169+
co_await await(std::move(waitees));
170+
}
171+
172+
trace("resolved derivation finished");
173+
174+
auto resolvedResult = resolvedDrvGoal->buildResult;
175+
176+
// No `std::visit` for coroutines yet
177+
if (auto * successP = resolvedResult.tryGetSuccess()) {
178+
auto & success = *successP;
179+
auto outputHashes = staticOutputHashes(worker.evalStore, *drv);
180+
auto resolvedHashes = staticOutputHashes(worker.store, drvResolved);
181+
182+
StorePathSet outputPaths;
183+
184+
auto outputHash = get(outputHashes, wantedOutput);
185+
auto resolvedHash = get(resolvedHashes, wantedOutput);
186+
if ((!outputHash) || (!resolvedHash))
187+
throw Error(
188+
"derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/resolve)",
189+
worker.store.printStorePath(drvPath),
190+
wantedOutput);
191+
192+
auto realisation = [&] {
193+
auto take1 = get(success.builtOutputs, wantedOutput);
194+
if (take1)
195+
return *take1;
196+
197+
/* The above `get` should work. But stateful tracking of
198+
outputs in resolvedResult, this can get out of sync with the
199+
store, which is our actual source of truth. For now we just
200+
check the store directly if it fails. */
201+
auto take2 = worker.evalStore.queryRealisation(DrvOutput{*resolvedHash, wantedOutput});
202+
if (take2)
203+
return *take2;
204+
205+
throw Error(
206+
"derivation '%s' doesn't have expected output '%s' (derivation-goal.cc/realisation)",
207+
worker.store.printStorePath(pathResolved),
208+
wantedOutput);
209+
}();
210+
211+
if (!drv->type().isImpure()) {
212+
auto newRealisation = realisation;
213+
newRealisation.id = DrvOutput{*outputHash, wantedOutput};
214+
newRealisation.signatures.clear();
215+
if (!drv->type().isFixed()) {
216+
auto & drvStore = worker.evalStore.isValidPath(drvPath) ? worker.evalStore : worker.store;
217+
newRealisation.dependentRealisations =
218+
drvOutputReferences(worker.store, *drv, realisation.outPath, &drvStore);
219+
}
220+
worker.store.signRealisation(newRealisation);
221+
worker.store.registerDrvOutput(newRealisation);
222+
}
223+
outputPaths.insert(realisation.outPath);
224+
225+
auto status = success.status;
226+
if (status == BuildResult::Success::AlreadyValid)
227+
status = BuildResult::Success::ResolvesToAlreadyValid;
228+
229+
co_return doneSuccess(status, std::move(realisation));
230+
} else if (resolvedResult.tryGetFailure()) {
231+
co_return doneFailure({
232+
BuildResult::Failure::DependencyFailed,
233+
"build of resolved derivation '%s' failed",
234+
worker.store.printStorePath(pathResolved),
235+
});
236+
} else
237+
assert(false);
238+
}
239+
149240
/* Give up on substitution for the output we want, actually build this derivation */
150241

151242
auto g = worker.makeDerivationBuildingGoal(drvPath, *drv, buildMode);

‎tests/functional/ca/issue-13247.sh‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,4 @@ buildViaSubstitute use-a-prime-more-outputs^first
6565
# Should only fetch the output we asked for
6666
[[ -d "$(jq -r <"$TEST_ROOT"/a.json '.[0].outputs.out')" ]]
6767
[[ -f "$(jq -r <"$TEST_ROOT"/a.json '.[2].outputs.first')" ]]
68-
69-
# Output should *not* be here, this is the bug
70-
[[ -e "$(jq -r <"$TEST_ROOT"/a.json '.[2].outputs.second')" ]]
71-
skipTest "bug is not yet fixed"
68+
[[ ! -e "$(jq -r <"$TEST_ROOT"/a.json '.[2].outputs.second')" ]]

0 commit comments

Comments
 (0)