From 0b85b023d8ee3ab75e4c2511a0f391eb7361d569 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 28 Aug 2025 10:51:05 -0400 Subject: [PATCH] Get rid of `delayedException` in `DerivationBuilder` 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. --- src/libstore/build/derivation-check.cc | 35 +++- src/libstore/build/derivation-check.hh | 1 + src/libstore/unix/build/derivation-builder.cc | 177 ++++++++---------- 3 files changed, 109 insertions(+), 104 deletions(-) diff --git a/src/libstore/build/derivation-check.cc b/src/libstore/build/derivation-check.cc index c5b489b23..82e92e1f3 100644 --- a/src/libstore/build/derivation-check.cc +++ b/src/libstore/build/derivation-check.cc @@ -10,6 +10,7 @@ namespace nix { void checkOutputs( Store & store, const StorePath & drvPath, + const decltype(Derivation::outputs) & drvOutputs, const decltype(DerivationOptions::outputChecks) & outputChecks, const std::map & outputs) { @@ -17,9 +18,37 @@ void checkOutputs( for (auto & output : outputs) outputsByPath.emplace(store.printStorePath(output.second.path), output.second); - for (auto & output : outputs) { - auto & outputName = output.first; - auto & info = output.second; + for (auto & [outputName, info] : outputs) { + + auto * outputSpec = get(drvOutputs, outputName); + assert(outputSpec); + + if (const auto * dof = std::get_if(&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 is slightly tricky because some of its references (namely diff --git a/src/libstore/build/derivation-check.hh b/src/libstore/build/derivation-check.hh index 249e176c5..8f6b2b6b5 100644 --- a/src/libstore/build/derivation-check.hh +++ b/src/libstore/build/derivation-check.hh @@ -17,6 +17,7 @@ namespace nix { void checkOutputs( Store & store, const StorePath & drvPath, + const decltype(Derivation::outputs) & drvOutputs, const decltype(DerivationOptions::outputChecks) & drvOptions, const std::map & outputs); diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 710e5a2b2..c9b603db9 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -1327,8 +1327,6 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() outputs to allow hard links between outputs. */ InodesSeen inodesSeen; - std::exception_ptr delayedException; - /* The paths that can be referenced are the input closures, the output paths, and any paths that have been built via recursive Nix calls. */ @@ -1647,36 +1645,11 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() std::filesystem::rename(tmpOutput, actualPath); - auto newInfo0 = newInfoFromCA( + return newInfoFromCA( DerivationOutput::CAFloating{ .method = dof.ca.method, .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); }, @@ -1740,84 +1713,91 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() } if (buildMode == bmCheck) { + /* Check against already registered outputs */ - if (!store.isValidPath(newInfo.path)) - continue; - ValidPathInfo oldInfo(*store.queryPathInfo(newInfo.path)); - if (newInfo.narHash != oldInfo.narHash) { - if (settings.runDiffHook || settings.keepFailed) { - auto dst = store.toRealPath(finalDestPath + ".check"); - deletePath(dst); - movePath(actualPath, dst); + if (store.isValidPath(newInfo.path)) { + ValidPathInfo oldInfo(*store.queryPathInfo(newInfo.path)); + if (newInfo.narHash != oldInfo.narHash) { + if (settings.runDiffHook || settings.keepFailed) { + auto dst = store.toRealPath(finalDestPath + ".check"); + deletePath(dst); + movePath(actualPath, dst); - handleDiffHook( - buildUser ? buildUser->getUID() : getuid(), - buildUser ? buildUser->getGID() : getgid(), - finalDestPath, - dst, - store.printStorePath(drvPath), - tmpDir); + handleDiffHook( + buildUser ? buildUser->getUID() : getuid(), + buildUser ? buildUser->getGID() : getgid(), + finalDestPath, + dst, + store.printStorePath(drvPath), + tmpDir); - throw NotDeterministic( - "derivation '%s' may not be deterministic: output '%s' differs from '%s'", - store.printStorePath(drvPath), - store.toRealPath(finalDestPath), - dst); - } else - throw NotDeterministic( - "derivation '%s' may not be deterministic: output '%s' differs", - store.printStorePath(drvPath), - store.toRealPath(finalDestPath)); + throw NotDeterministic( + "derivation '%s' may not be deterministic: output '%s' differs from '%s'", + store.printStorePath(drvPath), + store.toRealPath(finalDestPath), + dst); + } else + throw NotDeterministic( + "derivation '%s' may not be deterministic: output '%s' differs", + store.printStorePath(drvPath), + 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. */ - if (!oldInfo.ultimate) { - oldInfo.ultimate = true; - store.signPathInfo(oldInfo); - store.registerValidPaths({{oldInfo.path, oldInfo}}); - } + store.optimisePath(actualPath, NoRepair); // FIXME: combine with scanForReferences() + miscMethods->markContentsGood(newInfo.path); - 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. */ - for (auto & i : inputPaths) { - if (references.count(i)) - 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}}); - + /* Do this in both the check and non-check cases, because we + want `checkOutputs` below to work, which needs these path + infos. */ 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) { - /* 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 {}; } - /* Apply output checks. */ - checkOutputs(store, drvPath, drvOptions.outputChecks, infos); - /* Register each output path as valid, and register the sets of paths referenced by each of them. If there are cycles in the outputs, this will fail. */ @@ -1829,16 +1809,11 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs() store.registerValidPaths(infos2); } - /* In case of a fixed-output derivation hash mismatch, throw an - exception now that we have registered the output as valid. */ - if (delayedException) - std::rethrow_exception(delayedException); - - /* 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. */ + /* If we made it this far, we are sure the output matches the + derivation 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; for (auto & [outputName, newInfo] : infos) {