From 1caf2afb1d1fffe0ff54244d335c168f4c32cbdf Mon Sep 17 00:00:00 2001
From: eldritch horrors <pennae@lix.systems>
Date: Sat, 5 Oct 2024 00:38:35 +0200
Subject: [PATCH] libstore: move Goal::buildResult to WorkResult

derivation goals still hold a BuildResult member variable since parts of
these results of accumulated in different places, but the Goal class now
no longer has such a field. substitution goals don't need it at all, and
derivation goals should also be refactored to not drop their buildResult

Change-Id: Ic6d3d471cdbe790a6e09a43445e25bedec6ed446
---
 src/libstore/build/derivation-goal.hh              | 5 +++++
 src/libstore/build/drv-output-substitution-goal.cc | 7 +++----
 src/libstore/build/entry-points.cc                 | 4 ++--
 src/libstore/build/goal.hh                         | 5 -----
 src/libstore/build/substitution-goal.cc            | 2 +-
 5 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh
index b461b7d0d..3885afe4a 100644
--- a/src/libstore/build/derivation-goal.hh
+++ b/src/libstore/build/derivation-goal.hh
@@ -184,6 +184,11 @@ struct DerivationGoal : public Goal
 
     std::map<std::string, InitialOutput> initialOutputs;
 
+    /**
+     * Build result.
+     */
+    BuildResult buildResult;
+
     /**
      * File descriptor for the log file.
      */
diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc
index adfed5845..f04beb884 100644
--- a/src/libstore/build/drv-output-substitution-goal.cc
+++ b/src/libstore/build/drv-output-substitution-goal.cc
@@ -30,7 +30,7 @@ try {
 
     /* If the derivation already exists, we’re done */
     if (worker.store.queryRealisation(id)) {
-        co_return WorkResult{ecSuccess, std::move(buildResult)};
+        co_return WorkResult{ecSuccess};
     }
 
     subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>();
@@ -61,7 +61,7 @@ try {
         /* Hack: don't indicate failure if there were no substituters.
            In that case the calling derivation should just do a
            build. */
-        co_return WorkResult{substituterFailed ? ecFailed : ecNoSubstituters, std::move(buildResult)};
+        co_return WorkResult{substituterFailed ? ecFailed : ecNoSubstituters};
     }
 
     sub = subs.front();
@@ -142,7 +142,6 @@ try {
         debug("The output path of the derivation output '%s' could not be substituted", id.to_string());
         return {WorkResult{
             nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed,
-            std::move(buildResult),
         }};
     }
 
@@ -155,7 +154,7 @@ try {
 kj::Promise<Result<Goal::WorkResult>> DrvOutputSubstitutionGoal::finished() noexcept
 try {
     trace("finished");
-    return {WorkResult{ecSuccess, std::move(buildResult)}};
+    return {WorkResult{ecSuccess}};
 } catch (...) {
     return {std::current_exception()};
 }
diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc
index 3efd31682..edfad3cbb 100644
--- a/src/libstore/build/entry-points.cc
+++ b/src/libstore/build/entry-points.cc
@@ -72,7 +72,7 @@ std::vector<KeyedBuildResult> Store::buildPathsWithResults(
     std::vector<KeyedBuildResult> results;
 
     for (auto & [req, goalPtr] : state)
-        results.emplace_back(goalPtr->buildResult.restrictTo(req));
+        results.emplace_back(goals[goalPtr].result.restrictTo(req));
 
     return results;
 }
@@ -90,7 +90,7 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat
             return goals;
         });
         auto [goal, result] = *goals.begin();
-        return goal->buildResult.restrictTo(DerivedPath::Built {
+        return result.result.restrictTo(DerivedPath::Built {
             .drvPath = makeConstantStorePathRef(drvPath),
             .outputs = OutputsSpec::All {},
         });
diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh
index 904b7564b..bceea3c54 100644
--- a/src/libstore/build/goal.hh
+++ b/src/libstore/build/goal.hh
@@ -82,11 +82,6 @@ struct Goal
      */
     std::string name;
 
-    /**
-     * Build result.
-     */
-    BuildResult buildResult;
-
     struct WorkResult;
 
     // for use by Worker and Goal only. will go away once work() is a promise.
diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc
index 25a5eb5e4..e0ca23a86 100644
--- a/src/libstore/build/substitution-goal.cc
+++ b/src/libstore/build/substitution-goal.cc
@@ -37,7 +37,7 @@ Goal::WorkResult PathSubstitutionGoal::done(
     BuildResult::Status status,
     std::optional<std::string> errorMsg)
 {
-    buildResult.status = status;
+    BuildResult buildResult{.status = status};
     if (errorMsg) {
         debug(*errorMsg);
         buildResult.errorMsg = *errorMsg;