Separate derivation building from the scheduler #12663

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.

We might want to also rename local-derivation-goal.{cc,hh}, that way the git diffs stay with the larger file (derivation-builder.{cc,hh}). Or, we can do the copy-then-filter that we did for the original split so long ago.

Depends on #12658

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

@@ -868,11 +868,7 @@ void runPostBuildHook(
 }


-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 !

The other parameters it took were somewhat implementation-specific.

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.

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!

This is done to prior to splitting, just like 05cc5a8 for 68f4c72.

The building logic is now free of the scheduling logic! (The interface between them is just what is in the new header. This makes it much easier to audit, and shrink over time.)

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

github-actions bot added the 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

Open

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 added 5 commits March 21, 2025 11:49

Make appendLogTailErrorMsg as class method after all

c436d38

The other parameters it took were somewhat implementation-specific.

Start separating scheduling from building

6e138af

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!

Copy local-derivation-goal.cc to derivation-builder.{cc,hh}

1bf3b90

This is done to prior to splitting, just like 05cc5a8 for 68f4c72.

Move DerivationBuilder to its own file/header

30b2ae3

The building logic is now free of the scheduling logic! (The interface between them is just what is in the new header. This makes it much easier to audit, and shrink over time.)

Ericson2314 force-pushed the local-derivation-goal-encapsulation branch from 2497082 to 30b2ae3 Compare March 21, 2025 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate derivation building from the scheduler #12663

Separate derivation building from the scheduler #12663

Ericson2314 commented Mar 17, 2025 •

edited

Loading

Ericson2314 commented Mar 17, 2025

L-as Mar 17, 2025

L-as Mar 17, 2025

L-as Mar 18, 2025

L-as Mar 17, 2025

Ericson2314 Mar 18, 2025

L-as Mar 18, 2025 •

edited

Loading

L-as Mar 18, 2025

Ericson2314 Mar 18, 2025

L-as Mar 18, 2025

L-as Mar 18, 2025

L-as Mar 18, 2025

L-as Mar 18, 2025

Ericson2314 Mar 18, 2025

L-as Mar 18, 2025 •

edited

Loading

Ericson2314 Mar 18, 2025

L-as Mar 18, 2025

L-as Mar 18, 2025

Ericson2314 Mar 18, 2025

L-as Mar 18, 2025

Ericson2314 Mar 18, 2025

dpulls bot commented Mar 19, 2025

		@@ -868,11 +868,7 @@ void runPostBuildHook(
		}


		void appendLogTailErrorMsg(

Separate derivation building from the scheduler #12663

Are you sure you want to change the base?

Separate derivation building from the scheduler #12663

Conversation

Ericson2314 commented Mar 17, 2025 • edited Loading

Motivation

Context

Ericson2314 commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L-as Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L-as Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpulls bot commented Mar 19, 2025

Ericson2314 commented Mar 17, 2025 •

edited

Loading

L-as Mar 18, 2025 •

edited

Loading

L-as Mar 18, 2025 •

edited

Loading