Use std::variant to enforce BuildResult invariants

There is now a clean separation between successful and failing build
results.

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

@@ -90,7 +90,7 @@ void DerivationBuildingGoal::timedOut(Error && ex)
     killChild();
     // We're not inside a coroutine, hence we can't use co_return here.
     // Thus we ignore the return value.
-    [[maybe_unused]] Done _ = doneFailure({BuildResult::TimedOut, std::move(ex)});
+    [[maybe_unused]] Done _ = doneFailure({BuildResult::Failure::TimedOut, std::move(ex)});
 }
 
 /**
@@ -205,7 +205,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
                 nrFailed,
                 nrFailed == 1 ? "dependency" : "dependencies");
         msg += showKnownOutputs(worker.store, *drv);
-        co_return doneFailure(BuildError(BuildResult::DependencyFailed, msg));
+        co_return doneFailure(BuildError(BuildResult::Failure::DependencyFailed, msg));
     }
 
     /* Gather information necessary for computing the closure and/or
@@ -256,14 +256,18 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
                         return std::nullopt;
 
                     auto & buildResult = (*mEntry)->buildResult;
-                    if (!buildResult.success())
-                        return std::nullopt;
+                    return std::visit(
+                        overloaded{
+                            [](const BuildResult::Failure &) -> std::optional<StorePath> { return std::nullopt; },
+                            [&](const BuildResult::Success & success) -> std::optional<StorePath> {
+                                auto i = get(success.builtOutputs, outputName);
+                                if (!i)
+                                    return std::nullopt;
 
-                    auto i = get(buildResult.builtOutputs, outputName);
-                    if (!i)
-                        return std::nullopt;
-
-                    return i->outPath;
+                                return i->outPath;
+                            },
+                        },
+                        buildResult.inner);
                 });
             if (!attempt) {
                 /* TODO (impure derivations-induced tech debt) (see below):
@@ -306,7 +310,9 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
 
             auto resolvedResult = resolvedDrvGoal->buildResult;
 
-            if (resolvedResult.success()) {
+            // No `std::visit` for coroutines yet
+            if (auto * successP = resolvedResult.tryGetSuccess()) {
+                auto & success = *successP;
                 SingleDrvOutputs builtOutputs;
 
                 auto outputHashes = staticOutputHashes(worker.evalStore, *drv);
@@ -324,7 +330,7 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
                             outputName);
 
                     auto realisation = [&] {
-                        auto take1 = get(resolvedResult.builtOutputs, outputName);
+                        auto take1 = get(success.builtOutputs, outputName);
                         if (take1)
                             return *take1;
 
@@ -360,18 +366,19 @@ Goal::Co DerivationBuildingGoal::gaveUpOnSubstitution()
 
                 runPostBuildHook(worker.store, *logger, drvPath, outputPaths);
 
-                auto status = resolvedResult.status;
-                if (status == BuildResult::AlreadyValid)
-                    status = BuildResult::ResolvesToAlreadyValid;
+                auto status = success.status;
+                if (status == BuildResult::Success::AlreadyValid)
+                    status = BuildResult::Success::ResolvesToAlreadyValid;
 
-                co_return doneSuccess(status, std::move(builtOutputs));
-            } else {
+                co_return doneSuccess(success.status, std::move(builtOutputs));
+            } else if (resolvedResult.tryGetFailure()) {
                 co_return doneFailure({
-                    BuildResult::DependencyFailed,
+                    BuildResult::Failure::DependencyFailed,
                     "build of resolved derivation '%s' failed",
                     worker.store.printStorePath(pathResolved),
                 });
-            }
+            } else
+                assert(false);
         }
 
         /* If we get this far, we know no dynamic drvs inputs */
@@ -536,7 +543,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
             debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath));
             outputLocks.setDeletion(true);
             outputLocks.unlock();
-            co_return doneSuccess(BuildResult::AlreadyValid, std::move(validOutputs));
+            co_return doneSuccess(BuildResult::Success::AlreadyValid, std::move(validOutputs));
         }
 
         /* If any of the outputs already exist but are not valid, delete
@@ -628,7 +635,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
 
         /* Check the exit status. */
         if (!statusOk(status)) {
-            auto e = fixupBuilderFailureErrorMessage({BuildResult::MiscFailure, status, ""});
+            auto e = fixupBuilderFailureErrorMessage({BuildResult::Failure::MiscFailure, status, ""});
 
             outputLocks.unlock();
 
@@ -669,7 +676,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
         outputLocks.setDeletion(true);
         outputLocks.unlock();
 
-        co_return doneSuccess(BuildResult::Built, std::move(builtOutputs));
+        co_return doneSuccess(BuildResult::Success::Built, std::move(builtOutputs));
     }
 
     co_await yield();
@@ -832,15 +839,15 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
 #  pragma GCC diagnostic push
 #  pragma GCC diagnostic ignored "-Wswitch-enum"
         switch (e.status) {
-        case BuildResult::HashMismatch:
+        case BuildResult::Failure::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). */
-            e.status = BuildResult::OutputRejected;
+            e.status = BuildResult::Failure::OutputRejected;
             break;
-        case BuildResult::NotDeterministic:
+        case BuildResult::Failure::NotDeterministic:
             worker.checkMismatch = true;
             break;
         default:
@@ -866,7 +873,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
            (unlinked) lock files. */
         outputLocks.setDeletion(true);
         outputLocks.unlock();
-        co_return doneSuccess(BuildResult::Built, std::move(builtOutputs));
+        co_return doneSuccess(BuildResult::Success::Built, std::move(builtOutputs));
     }
 #endif
 }
@@ -1149,7 +1156,7 @@ void DerivationBuildingGoal::handleChildOutput(Descriptor fd, std::string_view d
             // We're not inside a coroutine, hence we can't use co_return here.
             // Thus we ignore the return value.
             [[maybe_unused]] Done _ = doneFailure(BuildError(
-                BuildResult::LogLimitExceeded,
+                BuildResult::Failure::LogLimitExceeded,
                 "%s killed after writing more than %d bytes of log output",
                 getName(),
                 settings.maxLogSize));
@@ -1306,16 +1313,16 @@ DerivationBuildingGoal::checkPathValidity(std::map<std::string, InitialOutput> &
     return {allValid, validOutputs};
 }
 
-Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Status status, SingleDrvOutputs builtOutputs)
+Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Success::Status status, SingleDrvOutputs builtOutputs)
 {
-    buildResult.status = status;
-
-    assert(buildResult.success());
+    buildResult.inner = BuildResult::Success{
+        .status = status,
+        .builtOutputs = std::move(builtOutputs),
+    };
 
     mcRunningBuilds.reset();
 
-    buildResult.builtOutputs = std::move(builtOutputs);
-    if (status == BuildResult::Built)
+    if (status == BuildResult::Success::Built)
         worker.doneBuilds++;
 
     worker.updateProgress();
@@ -1325,16 +1332,18 @@ Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Status status, Singl
 
 Goal::Done DerivationBuildingGoal::doneFailure(BuildError ex)
 {
-    buildResult.status = ex.status;
-    buildResult.errorMsg = fmt("%s", Uncolored(ex.info().msg));
-    if (buildResult.status == BuildResult::TimedOut)
-        worker.timedOut = true;
-    if (buildResult.status == BuildResult::PermanentFailure)
-        worker.permanentFailure = true;
+    buildResult.inner = BuildResult::Failure{
+        .status = ex.status,
+        .errorMsg = fmt("%s", Uncolored(ex.info().msg)),
+    };
 
     mcRunningBuilds.reset();
 
-    if (ex.status != BuildResult::DependencyFailed)
+    if (ex.status == BuildResult::Failure::TimedOut)
+        worker.timedOut = true;
+    if (ex.status == BuildResult::Failure::PermanentFailure)
+        worker.permanentFailure = true;
+    if (ex.status != BuildResult::Failure::DependencyFailed)
         worker.failedBuilds++;
 
     worker.updateProgress();

src/libstore/build/derivation-check.cc

@@ -33,7 +33,7 @@ void checkOutputs(
                 /* Throw an error after registering the path as
                    valid. */
                 throw BuildError(
-                    BuildResult::HashMismatch,
+                    BuildResult::Failure::HashMismatch,
                     "hash mismatch in fixed-output derivation '%s':\n  specified: %s\n     got:    %s",
                     store.printStorePath(drvPath),
                     wanted.to_string(HashFormat::SRI, true),
@@ -42,7 +42,7 @@ void checkOutputs(
             if (!info.references.empty()) {
                 auto numViolations = info.references.size();
                 throw BuildError(
-                    BuildResult::HashMismatch,
+                    BuildResult::Failure::HashMismatch,
                     "fixed-output derivations must not reference store paths: '%s' references %d distinct paths, e.g. '%s'",
                     store.printStorePath(drvPath),
                     numViolations,
@@ -84,7 +84,7 @@ void checkOutputs(
         auto applyChecks = [&](const DerivationOptions::OutputChecks & checks) {
             if (checks.maxSize && info.narSize > *checks.maxSize)
                 throw BuildError(
-                    BuildResult::OutputRejected,
+                    BuildResult::Failure::OutputRejected,
                     "path '%s' is too large at %d bytes; limit is %d bytes",
                     store.printStorePath(info.path),
                     info.narSize,
@@ -94,7 +94,7 @@ void checkOutputs(
                 uint64_t closureSize = getClosure(info.path).second;
                 if (closureSize > *checks.maxClosureSize)
                     throw BuildError(
-                        BuildResult::OutputRejected,
+                        BuildResult::Failure::OutputRejected,
                         "closure of path '%s' is too large at %d bytes; limit is %d bytes",
                         store.printStorePath(info.path),
                         closureSize,
@@ -115,7 +115,7 @@ void checkOutputs(
                         std::string outputsListing =
                             concatMapStringsSep(", ", outputs, [](auto & o) { return o.first; });
                         throw BuildError(
-                            BuildResult::OutputRejected,
+                            BuildResult::Failure::OutputRejected,
                             "derivation '%s' output check for '%s' contains an illegal reference specifier '%s',"
                             " expected store path or output name (one of [%s])",
                             store.printStorePath(drvPath),
@@ -148,7 +148,7 @@ void checkOutputs(
                         badPathsStr += store.printStorePath(i);
                     }
                     throw BuildError(
-                        BuildResult::OutputRejected,
+                        BuildResult::Failure::OutputRejected,
                         "output '%s' is not allowed to refer to the following paths:%s",
                         store.printStorePath(info.path),
                         badPathsStr);

src/libstore/build/derivation-goal.cc

@@ -94,7 +94,7 @@ Goal::Co DerivationGoal::haveDerivation()
 
         /* If they are all valid, then we're done. */
         if (checkResult && checkResult->second == PathStatus::Valid && buildMode == bmNormal) {
-            co_return doneSuccess(BuildResult::AlreadyValid, checkResult->first);
+            co_return doneSuccess(BuildResult::Success::AlreadyValid, checkResult->first);
         }
 
         Goals waitees;
@@ -123,7 +123,7 @@ Goal::Co DerivationGoal::haveDerivation()
 
         if (nrFailed > 0 && nrFailed > nrNoSubstituters && !settings.tryFallback) {
             co_return doneFailure(BuildError(
-                BuildResult::TransientFailure,
+                BuildResult::Failure::TransientFailure,
                 "some substitutes for the outputs of derivation '%s' failed (usually happens due to networking issues); try '--fallback' to build derivation from source ",
                 worker.store.printStorePath(drvPath)));
         }
@@ -135,7 +135,7 @@ Goal::Co DerivationGoal::haveDerivation()
         bool allValid = checkResult && checkResult->second == PathStatus::Valid;
 
         if (buildMode == bmNormal && allValid) {
-            co_return doneSuccess(BuildResult::Substituted, checkResult->first);
+            co_return doneSuccess(BuildResult::Success::Substituted, checkResult->first);
         }
         if (buildMode == bmRepair && allValid) {
             co_return repairClosure();
@@ -163,25 +163,27 @@ Goal::Co DerivationGoal::haveDerivation()
 
     buildResult = g->buildResult;
 
-    if (buildMode == bmCheck) {
-        /* In checking mode, the builder will not register any outputs.
-           So we want to make sure the ones that we wanted to check are
-           properly there. */
-        buildResult.builtOutputs = {{wantedOutput, assertPathValidity()}};
-    } else {
-        /* Otherwise the builder will give us info for out output, but
-           also for other outputs. Filter down to just our output so as
-           not to leak info on unrelated things. */
-        for (auto it = buildResult.builtOutputs.begin(); it != buildResult.builtOutputs.end();) {
-            if (it->first != wantedOutput) {
-                it = buildResult.builtOutputs.erase(it);
-            } else {
-                ++it;
+    if (auto * successP = buildResult.tryGetSuccess()) {
+        auto & success = *successP;
+        if (buildMode == bmCheck) {
+            /* In checking mode, the builder will not register any outputs.
+               So we want to make sure the ones that we wanted to check are
+               properly there. */
+            success.builtOutputs = {{wantedOutput, assertPathValidity()}};
+        } else {
+            /* Otherwise the builder will give us info for out output, but
+               also for other outputs. Filter down to just our output so as
+               not to leak info on unrelated things. */
+            for (auto it = success.builtOutputs.begin(); it != success.builtOutputs.end();) {
+                if (it->first != wantedOutput) {
+                    it = success.builtOutputs.erase(it);
+                } else {
+                    ++it;
+                }
             }
-        }
 
-        if (buildResult.success())
-            assert(buildResult.builtOutputs.count(wantedOutput) > 0);
+            assert(success.builtOutputs.count(wantedOutput) > 0);
+        }
     }
 
     co_return amDone(g->exitCode, g->ex);
@@ -279,7 +281,7 @@ Goal::Co DerivationGoal::repairClosure()
                 "some paths in the output closure of derivation '%s' could not be repaired",
                 worker.store.printStorePath(drvPath));
     }
-    co_return doneSuccess(BuildResult::AlreadyValid, assertPathValidity());
+    co_return doneSuccess(BuildResult::Success::AlreadyValid, assertPathValidity());
 }
 
 std::optional<std::pair<Realisation, PathStatus>> DerivationGoal::checkPathValidity()
@@ -337,16 +339,16 @@ Realisation DerivationGoal::assertPathValidity()
     return checkResult->first;
 }
 
-Goal::Done DerivationGoal::doneSuccess(BuildResult::Status status, Realisation builtOutput)
+Goal::Done DerivationGoal::doneSuccess(BuildResult::Success::Status status, Realisation builtOutput)
 {
-    buildResult.status = status;
-
-    assert(buildResult.success());
+    buildResult.inner = BuildResult::Success{
+        .status = status,
+        .builtOutputs = {{wantedOutput, std::move(builtOutput)}},
+    };
 
     mcExpectedBuilds.reset();
 
-    buildResult.builtOutputs = {{wantedOutput, std::move(builtOutput)}};
-    if (status == BuildResult::Built)
+    if (status == BuildResult::Success::Built)
         worker.doneBuilds++;
 
     worker.updateProgress();
@@ -356,16 +358,18 @@ Goal::Done DerivationGoal::doneSuccess(BuildResult::Status status, Realisation b
 
 Goal::Done DerivationGoal::doneFailure(BuildError ex)
 {
-    buildResult.status = ex.status;
-    buildResult.errorMsg = fmt("%s", Uncolored(ex.info().msg));
-    if (buildResult.status == BuildResult::TimedOut)
-        worker.timedOut = true;
-    if (buildResult.status == BuildResult::PermanentFailure)
-        worker.permanentFailure = true;
+    buildResult.inner = BuildResult::Failure{
+        .status = ex.status,
+        .errorMsg = fmt("%s", Uncolored(ex.info().msg)),
+    };
 
     mcExpectedBuilds.reset();
 
-    if (ex.status != BuildResult::DependencyFailed)
+    if (ex.status == BuildResult::Failure::TimedOut)
+        worker.timedOut = true;
+    if (ex.status == BuildResult::Failure::PermanentFailure)
+        worker.permanentFailure = true;
+    if (ex.status != BuildResult::Failure::DependencyFailed)
         worker.failedBuilds++;
 
     worker.updateProgress();

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

@@ -164,10 +164,11 @@ Goal::Co DerivationTrampolineGoal::haveDerivation(StorePath drvPath, Derivation
 
     auto & g = *concreteDrvGoals.begin();
     buildResult = g->buildResult;
-    for (auto & g2 : concreteDrvGoals) {
-        for (auto && [x, y] : g2->buildResult.builtOutputs)
-            buildResult.builtOutputs.insert_or_assign(x, y);
-    }
+    if (auto * successP = buildResult.tryGetSuccess())
+        for (auto & g2 : concreteDrvGoals)
+            if (auto * successP2 = g2->buildResult.tryGetSuccess())
+                for (auto && [x, y] : successP2->builtOutputs)
+                    successP->builtOutputs.insert_or_assign(x, y);
 
     co_return amDone(g->exitCode, g->ex);
 }

src/libstore/build/entry-points.cc

@@ -82,10 +82,10 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat
         worker.run(Goals{goal});
         return goal->buildResult;
     } catch (Error & e) {
-        return BuildResult{
-            .status = BuildResult::MiscFailure,
+        return BuildResult{.inner{BuildResult::Failure{
+            .status = BuildResult::Failure::MiscFailure,
             .errorMsg = e.msg(),
-        };
+        }}};
     };
 }
 

src/libstore/build/substitution-goal.cc

@@ -27,13 +27,21 @@ PathSubstitutionGoal::~PathSubstitutionGoal()
     cleanup();
 }
 
-Goal::Done PathSubstitutionGoal::done(ExitCode result, BuildResult::Status status, std::optional<std::string> errorMsg)
+Goal::Done PathSubstitutionGoal::doneSuccess(BuildResult::Success::Status status)
 {
-    buildResult.status = status;
-    if (errorMsg) {
-        debug(*errorMsg);
-        buildResult.errorMsg = *errorMsg;
-    }
+    buildResult.inner = BuildResult::Success{
+        .status = status,
+    };
+    return amDone(ecSuccess);
+}
+
+Goal::Done PathSubstitutionGoal::doneFailure(ExitCode result, BuildResult::Failure::Status status, std::string errorMsg)
+{
+    debug(errorMsg);
+    buildResult.inner = BuildResult::Failure{
+        .status = status,
+        .errorMsg = std::move(errorMsg),
+    };
     return amDone(result);
 }
 
@@ -45,7 +53,7 @@ Goal::Co PathSubstitutionGoal::init()
 
     /* If the path already exists we're done. */
     if (!repair && worker.store.isValidPath(storePath)) {
-        co_return done(ecSuccess, BuildResult::AlreadyValid);
+        co_return doneSuccess(BuildResult::Success::AlreadyValid);
     }
 
     if (settings.readOnlyMode)
@@ -165,9 +173,9 @@ Goal::Co PathSubstitutionGoal::init()
     /* Hack: don't indicate failure if there were no substituters.
        In that case the calling derivation should just do a
        build. */
-    co_return done(
+    co_return doneFailure(
         substituterFailed ? ecFailed : ecNoSubstituters,
-        BuildResult::NoSubstituters,
+        BuildResult::Failure::NoSubstituters,
         fmt("path '%s' is required, but there is no substituter that can build it",
             worker.store.printStorePath(storePath)));
 }
@@ -178,9 +186,9 @@ Goal::Co PathSubstitutionGoal::tryToRun(
     trace("all references realised");
 
     if (nrFailed > 0) {
-        co_return done(
+        co_return doneFailure(
             nrNoSubstituters > 0 ? ecNoSubstituters : ecFailed,
-            BuildResult::DependencyFailed,
+            BuildResult::Failure::DependencyFailed,
             fmt("some references of path '%s' could not be realised", worker.store.printStorePath(storePath)));
     }
 
@@ -297,7 +305,7 @@ Goal::Co PathSubstitutionGoal::tryToRun(
 
     worker.updateProgress();
 
-    co_return done(ecSuccess, BuildResult::Substituted);
+    co_return doneSuccess(BuildResult::Success::Substituted);
 }
 
 void PathSubstitutionGoal::handleEOF(Descriptor fd)