Merge pull request #13858 from obsidiansystems/no-more-defered-exception

Get rid of `delayedException` in `DerivationBuilder`

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

@@ -653,16 +653,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
                     goal.worker.childTerminated(&goal);
                 }
 
-                void noteHashMismatch() override
-                {
-                    goal.worker.hashMismatch = true;
-                }
-
-                void noteCheckMismatch() override
-                {
-                    goal.worker.checkMismatch = true;
-                }
-
                 void markContentsGood(const StorePath & path) override
                 {
                     goal.worker.markContentsGood(path);
@@ -770,6 +760,26 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
     // N.B. cannot use `std::visit` with co-routine return
     if (auto * ste = std::get_if<0>(&res)) {
         outputLocks.unlock();
+// Allow selecting a subset of enum values
+#  pragma GCC diagnostic push
+#  pragma GCC diagnostic ignored "-Wswitch-enum"
+        switch (ste->status) {
+        case BuildResult::HashMismatch:
+            worker.hashMismatch = true;
+            /* See header, the protocols don't know about `HashMismatch`
+               yet, so change it to `OutputRejected`, which they expect
+               for this case (hash mismatch is a type of output
+               rejection). */
+            ste->status = BuildResult::OutputRejected;
+            break;
+        case BuildResult::NotDeterministic:
+            worker.checkMismatch = true;
+            break;
+        default:
+            /* Other statuses need no adjusting */
+            break;
+        }
+#  pragma GCC diagnostic pop
         co_return doneFailure(std::move(*ste));
     } else if (auto * builtOutputs = std::get_if<1>(&res)) {
         StorePathSet outputPaths;

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<std::string, ValidPathInfo> & outputs)
 {
@@ -17,9 +18,37 @@ void checkOutputs(
     for (auto & output : outputs)
         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
            is slightly tricky because some of its references (namely

src/libstore/build/derivation-check.hh

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

src/libstore/include/nix/store/build-result.hh

@@ -36,6 +36,10 @@ struct BuildResult
         NotDeterministic,
         ResolvesToAlreadyValid,
         NoSubstituters,
+        /// A certain type of `OutputRejected`. The protocols do not yet
+        /// know about this one, so change it back to `OutputRejected`
+        /// before serialization.
+        HashMismatch,
     } status = MiscFailure;
 
     /**

src/libstore/include/nix/store/build/derivation-builder.hh

@@ -108,9 +108,6 @@ struct DerivationBuilderCallbacks
      */
     virtual void childTerminated() = 0;
 
-    virtual void noteHashMismatch(void) = 0;
-    virtual void noteCheckMismatch(void) = 0;
-
     virtual void markContentsGood(const StorePath & path) = 0;
 };
 

src/libstore/unix/build/derivation-builder.cc

@@ -1322,8 +1322,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. */
@@ -1642,37 +1640,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. */
-                        miscMethods->noteHashMismatch();
-                        delayedException = std::make_exception_ptr(BuildError(
-                            BuildResult::OutputRejected,
-                            "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::OutputRejected,
-                            "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); },
@@ -1736,85 +1708,91 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
         }
 
         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) {
-                miscMethods->noteCheckMismatch();
+                        auto dst = store.toRealPath(finalDestPath + ".check");
-                if (settings.runDiffHook || settings.keepFailed) {
+                        deletePath(dst);
-                    auto dst = store.toRealPath(finalDestPath + ".check");
+                        movePath(actualPath, dst);
-                    deletePath(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));
     }
 
+    /* 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. */
@@ -1826,16 +1804,11 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
         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;
 
     for (auto & [outputName, newInfo] : infos) {