Revert "Revert "Adapt scheduler to work with dynamic derivations

[Ericson2314]

Motivation

This fixes dynamic derivations, reverting #9081.

Context

I believe that this time around, #9052 is fixed. When I first rebased this, tests were failing (which wasn't the case before). The cause of those test failures were due to the crude job in which the outer goal tried to exit with the inner goal's status.

Now, that error handling has been reworked to be more faithful. The exit exit status and exception of the inner goal is returned by the outer goal. The exception was what was causing the test failures, but I believe it was not having the right error code (there is more than one for failure) that caused #9081.

The only cost of doing things the "right way" was that I had to introduce a hacky preserveException boolean. I don't like this, but, then again, none of us like anything about how the scheduler works. Issue #11927 is still there to clean everything up, subsuming the need for any preserveException because I doubt we will be fishing information out of state machines like this at all.

This reverts commit 8440afb.

Depends on #12392

Priorities

Add 👍 to pull requests you find important.

[roberth]

This might be a tall order, but could this change be split?
That'd be one way to make progress and narrow down where the problem really is.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/pre-rfc-implement-dependency-retrieval-primitive/43418/9

[roberth]

I believe the changes after #11005 will either uncover or silently fix the bug.

[L-as]

I wouldn't be so sure yet; I haven't rewritten DerivationGoal.

But the subsequent PRs will.

[roberth]

@L-as yeah, I meant after, and it is only a belief.
I didn't mean to set a bar for your work; I just wanted to highlight it because I think it's good, and helpful for delivering dynamic derivations.

[Ericson2314]

Yeah I think it will help too: there is no more need for outer derivation goal with that change, because we don't need separate types just to have different local variables persisted across yield points.

[ConnorBaker]

Hi all — as a quick status update, did #11005 resolve the issue blocking this from proceeding? If not, any ideas on how to proceed?

[L-as]

Pretty sure it hasn't. I'm still doing work in this direction that should fix it naturally.

[Ericson2314]

Yeah looking forward to closing this once @L-as's work obviates it :)

[dpulls]

🎉 All dependencies have been resolved !

[Ericson2314]

Diff in the main from the last, merged version.

diff --git a/src/libstore/build/create-derivation-and-realise-goal.cc b/src/libstore/build/create-derivation-and-realise-goal.cc
index 60f67956d..27a6e74e0 100644
--- a/src/libstore/build/create-derivation-and-realise-goal.cc
+++ b/src/libstore/build/create-derivation-and-realise-goal.cc
@@ -3,49 +3,39 @@
 
 namespace nix {
 
-CreateDerivationAndRealiseGoal::CreateDerivationAndRealiseGoal(ref<SingleDerivedPath> drvReq,
-    const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode)
-    : Goal(worker, DerivedPath::Built { .drvPath = drvReq, .outputs = wantedOutputs })
+CreateDerivationAndRealiseGoal::CreateDerivationAndRealiseGoal(
+    ref<SingleDerivedPath> drvReq, const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode)
+    : Goal(worker, DerivedPath::Built{.drvPath = drvReq, .outputs = wantedOutputs})
     , drvReq(drvReq)
     , wantedOutputs(wantedOutputs)
     , buildMode(buildMode)
 {
-    state = &CreateDerivationAndRealiseGoal::getDerivation;
-    name = fmt(
-        "outer obtaining drv from '%s' and then building outputs %s",
-        drvReq->to_string(worker.store),
-        std::visit(overloaded {
-            [&](const OutputsSpec::All) -> std::string {
-                return "* (all of them)";
-            },
-            [&](const OutputsSpec::Names os) {
-                return concatStringsSep(", ", quoteStrings(os));
-            },
-        }, wantedOutputs.raw));
+    name =
+        fmt("outer obtaining drv from '%s' and then building outputs %s",
+            drvReq->to_string(worker.store),
+            std::visit(
+                overloaded{
+                    [&](const OutputsSpec::All) -> std::string { return "* (all of them)"; },
+                    [&](const OutputsSpec::Names os) { return concatStringsSep(", ", quoteStrings(os)); },
+                },
+                wantedOutputs.raw));
     trace("created outer");
 
     worker.updateProgress();
 }
 
-
-CreateDerivationAndRealiseGoal::~CreateDerivationAndRealiseGoal()
-{
-}
-
+CreateDerivationAndRealiseGoal::~CreateDerivationAndRealiseGoal() {}
 
 static StorePath pathPartOfReq(const SingleDerivedPath & req)
 {
-    return std::visit(overloaded {
-        [&](const SingleDerivedPath::Opaque & bo) {
-            return bo.path;
+    return std::visit(
+        overloaded{
+            [&](const SingleDerivedPath::Opaque & bo) { return bo.path; },
+            [&](const SingleDerivedPath::Built & bfd) { return pathPartOfReq(*bfd.drvPath); },
         },
-        [&](const SingleDerivedPath::Built & bfd) {
-            return pathPartOfReq(*bfd.drvPath);
-        },
-    }, req.raw());
+        req.raw());
 }
 
-
 std::string CreateDerivationAndRealiseGoal::key()
 {
     /* Ensure that derivations get built in order of their name,
@@ -55,17 +45,7 @@ std::string CreateDerivationAndRealiseGoal::key()
     return "c$" + std::string(pathPartOfReq(*drvReq).name()) + "$" + drvReq->to_string(worker.store);
 }
 
-
-void CreateDerivationAndRealiseGoal::timedOut(Error && ex)
-{
-}
-
-
-void CreateDerivationAndRealiseGoal::work()
-{
-    (this->*state)();
-}
-
+void CreateDerivationAndRealiseGoal::timedOut(Error && ex) {}
 
 void CreateDerivationAndRealiseGoal::addWantedOutputs(const OutputsSpec & outputs)
 {
@@ -74,7 +54,8 @@ void CreateDerivationAndRealiseGoal::addWantedOutputs(const OutputsSpec & output
     bool needRestart = !newWanted.isSubsetOf(wantedOutputs);
     wantedOutputs = newWanted;
 
-    if (!needRestart) return;
+    if (!needRestart)
+        return;
 
     if (!optDrvPath)
         // haven't started steps where the outputs matter yet
@@ -82,8 +63,7 @@ void CreateDerivationAndRealiseGoal::addWantedOutputs(const OutputsSpec & output
     worker.makeDerivationGoal(*optDrvPath, outputs, buildMode);
 }
 
-
-void CreateDerivationAndRealiseGoal::getDerivation()
+Goal::Co CreateDerivationAndRealiseGoal::init()
 {
     trace("outer init");
 
@@ -91,67 +71,56 @@ void CreateDerivationAndRealiseGoal::getDerivation()
        exists.  If it doesn't, it may be created through a
        substitute. */
     if (auto optDrvPath = [this]() -> std::optional<StorePath> {
-        if (buildMode != bmNormal) return std::nullopt;
-
-        auto drvPath = StorePath::dummy;
-        try {
-            drvPath = resolveDerivedPath(worker.store, *drvReq);
-        } catch (MissingRealisation &) {
-            return std::nullopt;
-        }
-        return worker.evalStore.isValidPath(drvPath) || worker.store.isValidPath(drvPath)
-            ? std::optional { drvPath }
-            : std::nullopt;
-    }()) {
-        trace(fmt("already have drv '%s' for '%s', can go straight to building",
-            worker.store.printStorePath(*optDrvPath),
-            drvReq->to_string(worker.store)));
-
-        loadAndBuildDerivation();
+            if (buildMode != bmNormal)
+                return std::nullopt;
+
+            auto drvPath = StorePath::dummy;
+            try {
+                drvPath = resolveDerivedPath(worker.store, *drvReq);
+            } catch (MissingRealisation &) {
+                return std::nullopt;
+            }
+            auto cond = worker.evalStore.isValidPath(drvPath) || worker.store.isValidPath(drvPath);
+            return cond ? std::optional{drvPath} : std::nullopt;
+        }()) {
+        trace(
+            fmt("already have drv '%s' for '%s', can go straight to building",
+                worker.store.printStorePath(*optDrvPath),
+                drvReq->to_string(worker.store)));
     } else {
         trace("need to obtain drv we want to build");
-
         addWaitee(worker.makeGoal(DerivedPath::fromSingle(*drvReq)));
-
-        state = &CreateDerivationAndRealiseGoal::loadAndBuildDerivation;
-        if (waitees.empty()) work();
+        co_await Suspend{};
     }
-}
 
-
-void CreateDerivationAndRealiseGoal::loadAndBuildDerivation()
-{
     trace("outer load and build derivation");
 
     if (nrFailed != 0) {
-        amDone(ecFailed, Error("cannot build missing derivation '%s'", drvReq->to_string(worker.store)));
-        return;
+        co_return amDone(ecFailed, Error("cannot build missing derivation '%s'", drvReq->to_string(worker.store)));
     }
 
     StorePath drvPath = resolveDerivedPath(worker.store, *drvReq);
     /* Build this step! */
     concreteDrvGoal = worker.makeDerivationGoal(drvPath, wantedOutputs, buildMode);
-    addWaitee(upcast_goal(concreteDrvGoal));
-    state = &CreateDerivationAndRealiseGoal::buildDone;
+    {
+        auto g = upcast_goal(concreteDrvGoal);
+        /* We will finish with it ourselves, as if we were the derivational goal. */
+        g->preserveException = true;
+    }
     optDrvPath = std::move(drvPath);
-    if (waitees.empty()) work();
-}
-
+    addWaitee(upcast_goal(concreteDrvGoal));
+    co_await Suspend{};
 
-void CreateDerivationAndRealiseGoal::buildDone()
-{
     trace("outer build done");
 
-    buildResult = upcast_goal(concreteDrvGoal)->getBuildResult(DerivedPath::Built {
-        .drvPath = drvReq,
-        .outputs = wantedOutputs,
-    });
+    buildResult = upcast_goal(concreteDrvGoal)
+                      ->getBuildResult(DerivedPath::Built{
+                          .drvPath = drvReq,
+                          .outputs = wantedOutputs,
+                      });
 
-    if (buildResult.success())
-        amDone(ecSuccess);
-    else
-        amDone(ecFailed, Error("building '%s' failed", drvReq->to_string(worker.store)));
+    auto g = upcast_goal(concreteDrvGoal);
+    co_return amDone(g->exitCode, g->ex);
 }
 
-
 }

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-12-nix-team-meeting-minutes-212-5/60216/1