nixos/nix
1
1
Fork 0
mirror of https://github.com/NixOS/nix.git synced 2025-11-16 07:22:43 +01:00
Code Activity

Get rid of delayedException in DerivationBuilder

Browse source 
Instead of that funny business, the fixed output checks are not put in
`checkOutputs`, with the other (newer) output checks, where they also
better belong. The control flow is reworked (with comments!) so that
`checkOutputs` also runs in the `bmCheck` case.

Not only does this preserve existing behavior of `bmCheck`
double-checking fixed output hashes with less tricky code, it also makes
`bmCheck` better by also double-checking the other output checks, rather
than just assuming they pass if the derivation is deterministic.
This commit is contained in:
John Ericson 2025-08-28 10:51:05 -04:00
parent ff961fd9e2
commit 0b85b023d8
3 changed files with 109 additions and 104 deletions
Download patch file Download diff file Expand all files Collapse all files

35
src/libstore/build/derivation-check.cc
View file

@ -10,6 +10,7 @@ namespace nix {
void checkOutputs( void checkOutputs(
Store & store, Store & store,
const StorePath & drvPath, const StorePath & drvPath,
const decltype(Derivation::outputs) & drvOutputs,
const decltype(DerivationOptions::outputChecks) & outputChecks, const decltype(DerivationOptions::outputChecks) & outputChecks,
const std::map<std::string, ValidPathInfo> & outputs) const std::map<std::string, ValidPathInfo> & outputs)
{ {
@ -17,9 +18,37 @@ void checkOutputs(
for (auto & output : outputs) for (auto & output : outputs)
outputsByPath.emplace(store.printStorePath(output.second.path), output.second); outputsByPath.emplace(store.printStorePath(output.second.path), output.second);
for (auto & output : outputs) { for (auto & [outputName, info] : outputs) {
auto & outputName = output.first;
auto & info = output.second; auto * outputSpec = get(drvOutputs, outputName);
assert(outputSpec);
if (const auto * dof = std::get_if<DerivationOutput::CAFixed>(&outputSpec->raw)) {
auto & wanted = dof->ca.hash;
/* Check wanted hash */
assert(info.ca);
auto & got = info.ca->hash;
if (wanted != got) {
/* Throw an error after registering the path as
valid. */
throw BuildError(
BuildResult::HashMismatch,
"hash mismatch in fixed-output derivation '%s':\n specified: %s\n got: %s",
store.printStorePath(drvPath),
wanted.to_string(HashFormat::SRI, true),
got.to_string(HashFormat::SRI, true));
}
if (!info.references.empty()) {
auto numViolations = info.references.size();
throw BuildError(
BuildResult::HashMismatch,
"fixed-output derivations must not reference store paths: '%s' references %d distinct paths, e.g. '%s'",
store.printStorePath(drvPath),
numViolations,
store.printStorePath(*info.references.begin()));
}
}
/* Compute the closure and closure size of some output. This /* Compute the closure and closure size of some output. This
is slightly tricky because some of its references (namely is slightly tricky because some of its references (namely

1
src/libstore/build/derivation-check.hh
View file

@ -17,6 +17,7 @@ namespace nix {
void checkOutputs( void checkOutputs(
Store & store, Store & store,
const StorePath & drvPath, const StorePath & drvPath,
const decltype(Derivation::outputs) & drvOutputs,
const decltype(DerivationOptions::outputChecks) & drvOptions, const decltype(DerivationOptions::outputChecks) & drvOptions,
const std::map<std::string, ValidPathInfo> & outputs); const std::map<std::string, ValidPathInfo> & outputs);

177
src/libstore/unix/build/derivation-builder.cc
View file

@ -1327,8 +1327,6 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
outputs to allow hard links between outputs. */ outputs to allow hard links between outputs. */
InodesSeen inodesSeen; InodesSeen inodesSeen;
std::exception_ptr delayedException;
/* The paths that can be referenced are the input closures, the /* The paths that can be referenced are the input closures, the
output paths, and any paths that have been built via recursive output paths, and any paths that have been built via recursive
Nix calls. */ Nix calls. */
@ -1647,36 +1645,11 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
std::filesystem::rename(tmpOutput, actualPath); std::filesystem::rename(tmpOutput, actualPath);
auto newInfo0 = newInfoFromCA( return newInfoFromCA(
DerivationOutput::CAFloating{ DerivationOutput::CAFloating{
.method = dof.ca.method, .method = dof.ca.method,
.hashAlgo = wanted.algo, .hashAlgo = wanted.algo,
}); });
/* Check wanted hash */
assert(newInfo0.ca);
auto & got = newInfo0.ca->hash;
if (wanted != got) {
/* Throw an error after registering the path as
valid. */
delayedException = std::make_exception_ptr(BuildError(
BuildResult::HashMismatch,
"hash mismatch in fixed-output derivation '%s':\n specified: %s\n got: %s",
store.printStorePath(drvPath),
wanted.to_string(HashFormat::SRI, true),
got.to_string(HashFormat::SRI, true)));
}
if (!newInfo0.references.empty()) {
auto numViolations = newInfo.references.size();
delayedException = std::make_exception_ptr(BuildError(
BuildResult::HashMismatch,
"fixed-output derivations must not reference store paths: '%s' references %d distinct paths, e.g. '%s'",
store.printStorePath(drvPath),
numViolations,
store.printStorePath(*newInfo.references.begin())));
}
return newInfo0;
}, },
[&](const DerivationOutput::CAFloating & dof) { return newInfoFromCA(dof); }, [&](const DerivationOutput::CAFloating & dof) { return newInfoFromCA(dof); },
@ -1740,84 +1713,91 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
} }
if (buildMode == bmCheck) { if (buildMode == bmCheck) {
/* Check against already registered outputs */
if (!store.isValidPath(newInfo.path)) if (store.isValidPath(newInfo.path)) {
continue; ValidPathInfo oldInfo(*store.queryPathInfo(newInfo.path));
ValidPathInfo oldInfo(*store.queryPathInfo(newInfo.path)); if (newInfo.narHash != oldInfo.narHash) {
if (newInfo.narHash != oldInfo.narHash) { if (settings.runDiffHook || settings.keepFailed) {
if (settings.runDiffHook || settings.keepFailed) { auto dst = store.toRealPath(finalDestPath + ".check");
auto dst = store.toRealPath(finalDestPath + ".check"); deletePath(dst);
deletePath(dst); movePath(actualPath, dst);
movePath(actualPath, dst);
handleDiffHook( handleDiffHook(
buildUser ? buildUser->getUID() : getuid(), buildUser ? buildUser->getUID() : getuid(),
buildUser ? buildUser->getGID() : getgid(), buildUser ? buildUser->getGID() : getgid(),
finalDestPath, finalDestPath,
dst, dst,
store.printStorePath(drvPath), store.printStorePath(drvPath),
tmpDir); tmpDir);
throw NotDeterministic( throw NotDeterministic(
"derivation '%s' may not be deterministic: output '%s' differs from '%s'", "derivation '%s' may not be deterministic: output '%s' differs from '%s'",
store.printStorePath(drvPath), store.printStorePath(drvPath),
store.toRealPath(finalDestPath), store.toRealPath(finalDestPath),
dst); dst);
} else } else
throw NotDeterministic( throw NotDeterministic(
"derivation '%s' may not be deterministic: output '%s' differs", "derivation '%s' may not be deterministic: output '%s' differs",
store.printStorePath(drvPath), store.printStorePath(drvPath),
store.toRealPath(finalDestPath)); store.toRealPath(finalDestPath));
}
/* Since we verified the build, it's now ultimately trusted. */
if (!oldInfo.ultimate) {
oldInfo.ultimate = true;
store.signPathInfo(oldInfo);
store.registerValidPaths({{oldInfo.path, oldInfo}});
}
}
} else {
/* do tasks relating to registering these outputs */
/* For debugging, print out the referenced and unreferenced paths. */
for (auto & i : inputPaths) {
if (references.count(i))
debug("referenced input: '%1%'", store.printStorePath(i));
else
debug("unreferenced input: '%1%'", store.printStorePath(i));
} }
/* Since we verified the build, it's now ultimately trusted. */ store.optimisePath(actualPath, NoRepair); // FIXME: combine with scanForReferences()
if (!oldInfo.ultimate) { miscMethods->markContentsGood(newInfo.path);
oldInfo.ultimate = true;
store.signPathInfo(oldInfo);
store.registerValidPaths({{oldInfo.path, oldInfo}});
}
continue; newInfo.deriver = drvPath;
newInfo.ultimate = true;
store.signPathInfo(newInfo);
finish(newInfo.path);
/* If it's a CA path, register it right away. This is necessary if it
isn't statically known so that we can safely unlock the path before
the next iteration
This is also good so that if a fixed-output produces the
wrong path, we still store the result (just don't consider
the derivation sucessful, so if someone fixes the problem by
just changing the wanted hash, the redownload (or whateer
possibly quite slow thing it was) doesn't have to be done
again. */
if (newInfo.ca)
store.registerValidPaths({{newInfo.path, newInfo}});
} }
/* For debugging, print out the referenced and unreferenced paths. */ /* Do this in both the check and non-check cases, because we
for (auto & i : inputPaths) { want `checkOutputs` below to work, which needs these path
if (references.count(i)) infos. */
debug("referenced input: '%1%'", store.printStorePath(i));
else
debug("unreferenced input: '%1%'", store.printStorePath(i));
}
store.optimisePath(actualPath, NoRepair); // FIXME: combine with scanForReferences()
miscMethods->markContentsGood(newInfo.path);
newInfo.deriver = drvPath;
newInfo.ultimate = true;
store.signPathInfo(newInfo);
finish(newInfo.path);
/* If it's a CA path, register it right away. This is necessary if it
isn't statically known so that we can safely unlock the path before
the next iteration */
if (newInfo.ca)
store.registerValidPaths({{newInfo.path, newInfo}});
infos.emplace(outputName, std::move(newInfo)); infos.emplace(outputName, std::move(newInfo));
} }
/* Apply output checks. This includes checking of the wanted vs got
hash of fixed-outputs. */
checkOutputs(store, drvPath, drv.outputs, drvOptions.outputChecks, infos);
if (buildMode == bmCheck) { if (buildMode == bmCheck) {
/* In case of fixed-output derivations, if there are
mismatches on `--check` an error must be thrown as this is
also a source for non-determinism. */
if (delayedException)
std::rethrow_exception(delayedException);
return {}; return {};
} }
/* Apply output checks. */
checkOutputs(store, drvPath, drvOptions.outputChecks, infos);
/* Register each output path as valid, and register the sets of /* Register each output path as valid, and register the sets of
paths referenced by each of them. If there are cycles in the paths referenced by each of them. If there are cycles in the
outputs, this will fail. */ outputs, this will fail. */
@ -1829,16 +1809,11 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
store.registerValidPaths(infos2); store.registerValidPaths(infos2);
} }
/* In case of a fixed-output derivation hash mismatch, throw an /* If we made it this far, we are sure the output matches the
exception now that we have registered the output as valid. */ derivation That means it's safe to link the derivation to the
if (delayedException) output hash. We must do that for floating CA derivations, which
std::rethrow_exception(delayedException); otherwise couldn't be cached, but it's fine to do in all cases.
*/
/* If we made it this far, we are sure the output matches the derivation
(since the delayedException would be a fixed output CA mismatch). That
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. */
SingleDrvOutputs builtOutputs; SingleDrvOutputs builtOutputs;
for (auto & [outputName, newInfo] : infos) { for (auto & [outputName, newInfo] : infos) {