Draft
Conversation
amaanq
force-pushed
the
drv-builder-raii
branch
from
e98b318 to
3a4600c
Compare
amaanq
force-pushed
the
drv-builder-raii
branch
9 times, most recently
from
82cd254 to
4716030
Compare
github-actions
bot
added
the
with-tests
This shrinks the big derivation builder compilation unit, and hopefully puts on on the road to being able to do all derivation builder cleanup with RAII.
This is what we do for other OS-specific functionality.
…erface Add a virtual `cleanupOnDestruction() noexcept` method to `DerivationBuilder`. This is needed so that `DerivationBuilderDeleter` can call it directly on any `DerivationBuilder` pointer without requiring a `dynamic_cast` to `DerivationBuilderImpl`. `DerivationBuilderImpl` overrides it with the existing cleanup logic.
…cUnixDerivationBuilder
Move the identical setupSeccomp function from both linux-derivation-builder.cc and linux-chroot-derivation-builder.cc into a new shared file to eliminate code duplication.
Once per impl, that is. This hopefully makes the code easier to read.
Convert `tmpDirFd`, `inputRewrites`, and `outputRewrites` from member fields to local variables since they are only used within a single method: - `tmpDirFd` is only used in `startBuild()` - `inputRewrites` is only used in `startBuild()` - `outputRewrites` is only used in `unprepareBuild()` Applied to all four builder implementations. TODO there are more variables like this which can be converted.
The five builder files (`generic-unix`, `linux`, `linux-chroot`, `darwin`,
`external`) contained massive duplication: identical `computeScratchOutputs`
loops, `stopDaemon` methods, `processSandboxSetupMessages` blocks,
`setupRecursiveNixDaemon` code, PTY setup, AWS credential resolution,
builtin builder dispatch, privilege dropping, `execve` wrappers, disk
space checks, `unprepareBuild` preambles, cleanup logic, impure path
validation, and pre-build hook parsing.
This commit extracts all of these into free functions in
`derivation-builder-common.{cc,hh}`. Each builder now calls the common
functions, keeping only platform-specific logic inline. The `inputRewrites`,
`outputRewrites`, and `tmpDirFd` locals are promoted to fields so that
the extracted functions can access them without excessive parameter counts.
Replaces the `stopDaemon` and `setupRecursiveNixDaemon` free functions with a `RecursiveNixDaemon` struct whose `stop()` and `start()` methods reference `socket`, `thread`, and `workerThreads` as members. The 3 duplicated fields in all 5 derivation builder implementations collapse to a single `RecursiveNixDaemon daemon` member. Also folds remaining single-use locals (`env`, `inputRewrites`, `tmpDirFd`, etc.) back into the functions that use them and removes dead declarations from the builder headers.
Gives `RestrictionContext` a stored reference to the input paths and provides non-virtual `originalPaths()`, `isAllowed()`, and a default `addDependencyImpl()`. This removes identical boilerplate from all 5 derivation builders: `originalPaths()`, both `isAllowed()` overrides, `addDependencyImpl()`, and the unnecessary `friend struct RestrictedStore` declaration. `LinuxChrootDerivationBuilder` keeps its `addDependencyImpl` override for bind-mounting, delegating to the base implementation. `RecursiveNixDaemon::start()` drops two redundant parameters (`params`, `addedPaths`) now reachable through the `DerivationBuilder` reference.
amaanq
force-pushed
the
drv-builder-raii
branch
from
5b2d842 to
55c9872
Compare
amaanq
force-pushed
the
drv-builder-raii
branch
from
55c9872 to
6b6294d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
We want to clean up the code around derivation builders and make it easier to add addl support for windows and other oses in the future
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.