Separate derivation building from the scheduler by Ericson2314 · Pull Request #12663 · NixOS/nix

Ericson2314 · 2025-03-17T04:00:37Z

Motivation

Part 1 of #12628. The logic is finally separate!

The interface between LocalDerivationGoal and the DerivationBuilder can almost certainly be improved, but such qualitative changes are best kept separate from this sort of mechanical refactoring.

(We probably can just get rid of LocalDerivationGoal too, with just a DerivationGoal and DerivationGoalImpl with the latter handling both cases.

Context

It's probably good to look through each commit one by one.

~~Depends on #12658~~
~~Depends on #12692~~
~~Depends on #12697~~

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Ericson2314 · 2025-03-17T04:09:05Z

CC @rickynils and @elaforge, since you were both interested in this sort of modularity work.

L-as · 2025-03-17T23:04:20Z

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

+    virtual void killSandbox(bool getStats) = 0;
+};
+
+std::unique_ptr<DerivationBuilder> makeDerivationBuilder(


Because DerivationBuilder only has one implementation (I assume),
you should perhaps consider making this not a virtual class, but just exposing the symbols.

You could do it as such:

namespace derivation_builder { struct t; std::unique_ptr<t> make(); bool prepareBuild(t &); // etc. }

There is a minor concrete performance benefit I think, in that from my experience
clang and gcc will not inline virtual calls even if the callee is statically known,
but it could inline these with LTO I think, but we don't use that anyway.

Maybe not worth it.

L-as · 2025-03-17T23:04:42Z

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

+ * rather than incoming call edges that either should be removed, or
+ * become (higher order) function parameters.
+ */
+struct DerivationBuilder : RestrictionContext


Why not make this a class, if it morally is one?

I'm not a fan of having RestrictionContext as a superclass. Why not just add it as a field member?

L-as · 2025-03-17T23:05:07Z

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

+
+    const BuildMode & buildMode;
+
+    DerivationBuilderParams(


Couldn't this be a simple struct without a constructor?

Not if it has references, unfortunately :(

You could just use pointers honestly

We have nonnull or something right?

I don't know of a non-null, I've generally been using references to get non-null.

I think I hallucinated it or had a commit where I added it. Might be worth adding, maybe not. References are in general IMO not well designed though, especially since you can't tell that a function is taking something by reference at the caller site without looking at the type.

You could have

template <typename T> class ptr { T& t; explicit ptr(T& t) : t(t) {} [..] }

Then we could use this everywhere instead of references I think and it would Just Work.
In function calls you would do f(ptr(x)), which would make it clear what's happening too, unless I'm wrong and this doesn't type check.

L-as · 2025-03-18T00:12:22Z

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

+/**
+ * Callbacks that `DerivationBuilder` needs.
+ */
+struct DerivationBuilderCallbacks


Couldn't you merge params and callbacks?

Well maybe you shouldn't so params can be a simple struct

L-as · 2025-03-18T00:14:12Z

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

+     */
+    Pid pid;
+
+    DerivationBuilder() = default;


What happens again if you omit this?

I am pretty sure then Impl cannot construct this abstract class. But yes it is funny to need that.

I feel like you're supposed to mark this as virtual or protected or both.

There are no virtual constructors, but yes it should be protected.

L-as · 2025-03-18T00:15:38Z

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

+ * become (higher order) function parameters.
+ */
+class DerivationBuilderImpl : public DerivationBuilder, DerivationBuilderParams
+{


There are a lot of fields remaining, many of which aren't used much of the time I imagine.

Couldn't we try to fix this properly by turning it into a sum type of all the possible states?

Might be better for a future PR though.

Yeah in this one I wanted to mainly figure out what things are used elsewhere, so we can then clean up the internals more easily.

L-as · 2025-03-18T18:23:04Z

src/libstore/build/derivation-goal.cc

 }


-void appendLogTailErrorMsg(


Is this really a good change?

See the commit message --- the other parameters I think don't belong in the interface but rather in the "closure environment" of the implementation.

Really longer term, this UI-focused thing is a layer violation and should not be in here at all.

dpulls · 2025-03-19T10:00:36Z

🎉 All dependencies have been resolved !

roberth · 2025-04-09T21:16:33Z

First commit LGTM

roberth · 2025-04-14T15:02:38Z

Second (fe915ad) LGTM

The other parameters it took were somewhat implementation-specific.

roberth

Third commit

src/libstore/unix/build/local-derivation-goal.cc

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

roberth · 2025-04-16T21:21:11Z

src/libstore/unix/build/local-derivation-goal.cc

+            DerivationBuilderParams {
+                DerivationGoal::drvPath,
+                DerivationGoal::buildMode,
+                DerivationGoal::buildResult,
+                DerivationGoal::drv,
+                DerivationGoal::parsedDrv,
+                DerivationGoal::drvOptions,
+                DerivationGoal::inputPaths,
+                DerivationGoal::initialOutputs,
+            })}


Use { .drvPath = DerivationGoal::drvPath, etc syntax?

Same in the other constructor

Doesn't work with references :(

This is unreleated to the other commits in this PR.

Now, most of it is in two new functions: `LocalDerivationGoal::{,un}repareBuild`. This might seems like a step backwards from coroutines --- now we have more functions, and are stuck with class vars --- but I don't think it needs to be. There's a few options here: - (Re)introduce coroutines for the isolated building logic. We could use the same coroutines types, or simpler ones specialized to this use-case. The `tryLocalBuild` caller can still use `Goal::Co`, and just will manually "pump" this inner coroutine. - Return closures from each step. This is sort of like coroutines by hand, but it still allows us to stop writing down the local variables in each type. Being able to fully-use RAII again would be very nice! - Keep top-level first-order functions like now, but make more functional. Instead of having one state object (`DerivationBuilder`) for all steps (setup, run, teardown), we can have separate structs for the live variables at each point we consume and return. This at least avoids "are these variables active at this time?" questions, but doesn't give us the full benefit of RAII as we must manually ensure FIFO create/destroy orders still. One thing to note is that by keeping the `outputLock` unlocking in `tryLocalBuild`, we are arguably uncovering a rebuild scheduling vs building distinction, as the output locks are pretty squarely a scheduling concern. It's nice that the builder doesn't need to know about them at all.

Ericson2314 · 2025-04-16T21:49:30Z

@mergify queue

mergify · 2025-04-16T21:49:47Z

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at b509502

Ericson2314 · 2025-04-16T21:49:50Z

With @roberth's approval, I'll just do the commits he didn't get to yet in a separate PR!

We have a new `DerivationBuilder` struct, and `DerivationBuilderParams` `DerivationBuilderCallbacks` supporting it. `LocalDerivationGoal` doesn't subclass any of these, so we are ready to now move them out to a new file!

Ericson2314 requested a review from edolstra as a code owner March 17, 2025 04:00

github-actions bot added the store Issues and pull requests concerning the Nix store label Mar 17, 2025

Ericson2314 force-pushed the local-derivation-goal-encapsulation branch from ad62a08 to 1e01ec5 Compare March 17, 2025 04:05

Ericson2314 force-pushed the local-derivation-goal-encapsulation branch from 1e01ec5 to 2497082 Compare March 17, 2025 15:10

L-as reviewed Mar 17, 2025

View reviewed changes

L-as mentioned this pull request Mar 17, 2025

Remove multiple entrypoints into Goal code #12668

Closed

L-as reviewed Mar 18, 2025

View reviewed changes

This was referenced Mar 19, 2025

Small derivation goal cleanups #12692

Merged

Make Goal code use abstractions over interations with Worker #12697

Merged

Ericson2314 force-pushed the local-derivation-goal-encapsulation branch 5 times, most recently from 99f9331 to 380369d Compare March 26, 2025 19:06

Ericson2314 force-pushed the local-derivation-goal-encapsulation branch from 380369d to 681947c Compare April 9, 2025 20:04

Make appendLogTailErrorMsg as class method after all

e83ef7a

The other parameters it took were somewhat implementation-specific.

Ericson2314 force-pushed the local-derivation-goal-encapsulation branch from 681947c to e21057a Compare April 16, 2025 21:01

roberth reviewed Apr 16, 2025

View reviewed changes

Ericson2314 added 2 commits April 16, 2025 17:39

Remove some unused includes

ae7f411

This is unreleated to the other commits in this PR.

Ericson2314 force-pushed the local-derivation-goal-encapsulation branch from e21057a to 5fbd6cb Compare April 16, 2025 21:41

Ericson2314 force-pushed the local-derivation-goal-encapsulation branch from 5fbd6cb to d31e6c4 Compare April 16, 2025 21:49

Ericson2314 added this to the ca-derivations stabilisation milestone Apr 16, 2025

Ericson2314 mentioned this pull request Apr 16, 2025

Move DerivationBuilder to its own files #13039

Merged

Start separating scheduling from building

d98c0db

We have a new `DerivationBuilder` struct, and `DerivationBuilderParams` `DerivationBuilderCallbacks` supporting it. `LocalDerivationGoal` doesn't subclass any of these, so we are ready to now move them out to a new file!

Ericson2314 force-pushed the local-derivation-goal-encapsulation branch from d31e6c4 to d98c0db Compare April 16, 2025 21:55

mergify bot merged commit b509502 into NixOS:master Apr 16, 2025
13 checks passed

Ericson2314 deleted the local-derivation-goal-encapsulation branch April 16, 2025 22:58

Ericson2314 mentioned this pull request Apr 18, 2025

Remove double indirection from DerivationBuilder params #13047

Merged

Uh oh!

Conversation

Ericson2314 commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Uh oh!

Ericson2314 commented Mar 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

L-as Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

L-as Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpulls bot commented Mar 19, 2025

Uh oh!

roberth commented Apr 9, 2025

Uh oh!

roberth commented Apr 14, 2025

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Apr 16, 2025

Uh oh!

mergify bot commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ The pull request has been merged automatically

Uh oh!

Ericson2314 commented Apr 16, 2025

Uh oh!

Uh oh!

Reviewers

Ericson2314 commented Mar 17, 2025 •

edited

Loading

L-as Mar 18, 2025 •

edited

Loading

L-as Mar 18, 2025 •

edited

Loading

mergify bot commented Apr 16, 2025 •

edited

Loading