-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Description
The current spaghetti of the goals code is perhaps most serious source of technical debt, at least in the store layer.
(It blocks a lot of priorities of mine in particular:
- CA derivations Make scheduler work with shallow realisations without fetching unneeded store objects #11928 is hard to implement while the goal code is overloaded
- Dynamics derivations has a persistent bug that is also hard to fix while the worker code is so entangled
- Windows' build local and FreeBSD's sandboxing logic is hard to add while the building/sandboxing code is also rather spaghetti-ish
- If the building/sandboxing code was usable in isolation, we could use along with tvix/hnix-store/whatever to ship alternative daemon implementations way sooner. (Everything but this code is much easier to rewrite.)
)
Bottom-up plan in brief
One way to fix this is a "bottom-up" plan:
-
Better detach the local build code from the scheduling code. Think of this as "
LocalDerivationGoalbecomes a function". The local building code is now well-isolated (no bad class vars). -
Get rid of the build hook indirection: Separate stores for evaluation, building and storing the result #5025. Before doing this, the remote building is quite convoluted, but after step 1 it is at least not also entangled with the local building code. Having it just be its own mess should finally make this tractable to solve.
-
A this point both code paths --- remote build and local building --- should both be cleaned up and well isolated. This should free up local/remote-agnostic scheduling code (remaining
DerivationGoal, the other goal types). To the extent we even stick with the current "goal" design, I would like the goals to not be overloaded. That means, every goal object in the worker state....actually represents one goal! For example--checkand--repairshould be far more separated from the "regular build" case, as the goal, in the plain sense of the word "goal", is distinct.
The issues I want to solve listed above all depend on 3, so it is tempting to start "top down" instead, and directly attack those issues. The problem is that the lower level issues percolate up, such that the length of https://en.wiktionary.org/wiki/Chesterton%27s_fence is extremely daunting, making it very hard to refactor with confidence. The bottom up approach forces to solve other problems first, but makes us safely move complexity out of the way so by the time we get to step 3, the Chesterton's fences are elsewhere, not in our way.
Bottom-up plan details
This is what I've figured out so far, within steps 1 and 2.
Note that prior any of this done, we have this structure:
DerivationGoal::buildDonecalls a huge number ofcleanup*virtual functions, most of which are only defined for the local building case.DerivationGoal::tryToBuildandLocalDerivationGoal::tryLocalBuildare the sole callers of this. That is one remote-path and one local-path one.
So we we see have a different-shared-different "hourglasss" structure here: different cleanup, single buildDone, different "main do the work" function callers.
This makes me think the low-level plan should look like this:
- Duplicate
buildDoneto both cases, and simplify- cleanup callbacks go away
- See what other overridden functions remain, try to inline them too.
- (Assuming the above is successful) now that the local path is "one big coroutine", turn all the class variables into local variables.
This basically gets us through step 1 in the plan above.