Repository navigation
Fold the build hook into libstore while keeping custom build-hook programs working - #16587
Open
ArkhamKnight25 wants to merge 6 commits into
Open
ArkhamKnight25 wants to merge 6 commits into
ArkhamKnight25 wants to merge 6 commits into
Conversation
xokdvium
reviewed
Oct 10, 2026
Comment on lines
+229
to
+233
| /* Our SQLite connection reuses the parent's WAL index descriptor; | ||
| the dup2()s below must not overwrite it. */ | ||
| for (int fd : {4, 5}) | ||
| if (fdIsStoreWALIndex(fd)) | ||
| throw Error("descriptor %d is the store's SQLite WAL index; cannot fork the build hook", fd); |
Contributor
There was a problem hiding this comment.
Tbh this is the reason why this stuff should be in a separate executable. Let's not do more fork-without-exec.
Contributor
There was a problem hiding this comment.
We don't need to re-exec ourselves though. Several ways:
-
- Dynamically linked - just execute a binary from libexec that only links to libstore
-
- For full static builds on linux do memfd + fexecve
This branch has not been deployed
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
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
Remote builds go through the build hook, which picks a builder and runs the build there. Today the default hook is a second Nix process: the worker forks, execs
nix __build-remote, and sends it every setting over a pipe so the new process can rebuild its configuration.This moves the hook into libstore and runs the default hook in a forked child of the worker, without exec. The child opens its own store from the worker's
StoreConfigand speaks the existing protocol over the existing pipes. Nix no longer has to find its own executable or dump its settings for the default case, and the hook code can be tested as a library. A custombuild-hookprogram is still started with fork+exec, exactly as before.Context
Part of my GSoC project on remote building, mentored by @lisanna-dettwyler.
Related: #15533 (future of the build-hook mechanism), #16504 (
spawnProgram, merged), and #16579, which movesHookInstanceontospawnProgramand touches the same code (see the review guide).Implementation strategy
Six commits:
Move remote-builder selection into libstore. The slot-lock, ranking and upload-lock code of
nix __build-remoteis extracted intoacquireMachineSlot,openUploadLockand their helpers inmachine-selection.{hh,cc}. The existing program is switched to the new functions.Make the hook a library function. The body of
nix __build-remotebecomesserveBuildHook()insrc/libstore/unix/build/build-remote.cc; the command is now a thin wrapper that reads the settings dump, opens a store and calls it. One behaviour change in the move: the first request is read before# decline-permanentlyis written. Before, the parent's write could fail with EPIPE and it would restart the hook.Run the default hook without exec.
HookInstance::builtin()forks withstartProcessand callsserveBuildHook()in the child. The child closes the inherited descriptors except SQLite's (its own connection reuses the parent's per-process SQLite state, which refers to them by number), keeps the saved mount-namespace descriptors that a root client needs to start ssh, switches to the JSON logger and opens its own store. Where the path behind a descriptor cannot be looked up (anything other than Linux and macOS), the default runsnix __build-remotefound onPATH(before, the absolute path of the running binary). Custom hooks go throughHookInstance::external(), which is the old constructor unchanged.Choosing between the two uses a new
Setting::isDefault().isOverridden()cannot be used: it is reset after the systemnix.confis read, so a hook configured there would be taken for the default. I also removed the code inmain.ccthat replaced the default with the absolute path of the running binary; it made every installation look as if it had a custom hook. As a side effect,nix config show build-hooknow printsnix __build-remote.Signals in the built-in hook. The child skips
initNix, so it starts the signal-handler thread itself. This changes behaviour: the old program reset SIGTERM to its default and died on it, while the built-in hook treats SIGTERM as an interrupt and unwinds at the next interrupt check; a hook blocked inside ssh is still killed by the parent afterbuild-hook-kill-timeout. The same thread restores the SIGPIPE blocking that the old program had throughinitNixand that the fork loses incommonChildInit, so a dropped ssh connection does not kill the hook silently.No fork without builders. If
buildersresolves to no machine (including the default@/etc/nix/machineswhen the file does not exist), the parent declines on the spot instead of forking and opening a second database connection.Tripwire for the SQLite WAL index. If fd 4 or 5 is the store's
db.sqlite-shm, the child fails instead ofdup2ing over it. Not reachable as long as descriptors 0–2 are open when the store is opened (the big lock, the database and its WAL come first, so-shmlands at 6 or above); it turns a silent hang into an error.Alternatives
spawnProgram. Keeps the self-exec and the settings dump, but starts a clean process and avoids forking while SQLite connections and other threads are live. This is what libstore/unix: Use spawnProgram for the build hook #16579 does forHookInstance.SQLITE_PROTOCOLand retries forever; the hook hangs. That is why the SQLite descriptors are kept.SIGALRMandalarm()) and blocks in ssh for minutes. A process keeps that away from the worker and can be killed on its own.Open questions
tryBuildHook. I could not reproduce it, and tests cannot rule it out. Is this acceptable? Commits 1 and 2 stand on their own, andserveBuildHook()can be started throughspawnPrograminstead if the fork is not wanted.User-visible changes
nix config show build-hookprintsnix __build-remoteinstead of an absolute path.psunder the worker's command line.build-hook = nix __build-remoteby hand selects the same in-process hook (on Linux and macOS; elsewhere both run the program).Review guide
Commits 1 and 2 are code movement; diff them as such. Commits 3 to 6 are the new logic: child setup in
HookInstance::builtin(), descriptor handling, signals, and the dispatch intryBuildHook. #16579 renameschild.hhtochild-impl.hh, which this branch includes; whichever lands second rebases.Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.