Gsoc/builder virtual method - #1
Open
ArkhamKnight25 wants to merge 245 commits into
Open
Conversation
Cutting a release currently blocks on the full hydraJobs evaluation (~900 builds including sanitizers, clang-tidy, static, NixOS VM and installer tests), even though upload-release only consumes ~25 of them. On recent maintenance evals the long CI tail and darwin queue depth pushed the wait into the multi-day range while the actual artifacts were ready within an hour. Hydra hard-codes flake jobsets to outputs.hydraJobs, so the subset is exposed through a legacy jobset expression that re-enters the flake via builtins.getFlake on the locked GitHub ref. Going through the ref rather than the checked-out store path preserves rev/lastModified and thus the version suffix, keeping derivations bit-identical to the flake jobset so both share builds through the binary cache. A release aggregate job provides a single gating signal for upload-release. The release process now creates a release-$VERSION jobset alongside maintenance-$VERSION and waits on that instead of the full matrix. Requires adding https://releases.nixos.org/ to allowed-uris on hydra.nixos.org, since the locked nixpkgs input is a tarball from there and legacy jobsets run under restrict-eval.
Several things. Derivation is the primary memory hog in the build loop, with the total memory usage getting into gigabytes just to hold the derivations in memory. Previously I've optimised most goals to share a single ref<const Derivation>, but turns out I missed DerivationResolutionGoal. Also fixes a bug in the move assignment of Goal::Co that didn't destroy the coroutine frame of the Goal being moved into. Just this single commit lowers the memory usage of the build loop by 25% in my tests (from 4.2G -> 3.2G) on a 70k derivation closure.
ChildEvents has no business being in the coroutines promise type - it bloats it unnecessarily. The whole promise type was like 500 bytes just because it stored TimedOut inline - now it's behind a unique_ptr as it should have been. Shaves off like 300MB of memory usage on my stress-testing benchmark.
… lambda, get rid of allocationsA std::function allocates for the captures, but with C++23 we have a much better way of writing recursive lambdas - so use it.
Having such helpers in the namespace scope in a translation unit is a bad time when using unity builds. At some point this was duplicated between several files and caused name collisions with unity builds. A lambda is much cleaner and doesn't have the same footgun. Lambdas can have template parameters too now.
This avoids the fetched path from getting garbage collected if it is already valid.
When a URL with embedded credentials (scheme://user:pass@host/...) is passed to builtins.fetchurl or any other downloadFile caller, the password is printed verbatim in progress, retry-warning and error messages because they all format request.uri directly: warning: unable to download 'http://user:SECRET@host/file': ...; retrying in 256 ms Add FileTransferRequest::displayUri() which returns uri with the userinfo component stripped, and use it at every diagnostic call site. request.uri itself is unchanged so CURLOPT_URL, result.urls and fetcher-cache keys are unaffected.
libstore: don't print URL userinfo in FileTransfer diagnostics
It was only printing the base name, which isn't how we usually print store paths.
mountInput modifies lockedRef.input to stuff narHash into it.
…d::function One slight blemish I noticed while touching this code. With C++23 we can simplify things.
Turns out the readOnlyMode was always dangling in the C API and nobody noticed... Previous commits just started accessing it, which was caught by ASan. This is the minimal fix I can think of.
This is necessary for making flake store paths lazier and also slightly more concise anyway.
This is thankfully not used by anything else anymore. Good riddance, since all usages of it had bugs in them.
Bundle of fixes
…SourceAccessor This significantly simplifies path filtering and gets rid of raw file system accesses.
…mlinks This would be useful for caching parent directory file descriptors in the source accessor. I don't think the windows code works too well now, so I left the callback there as a FIXME to be called. It's just a perf improvement in general and shouldn't break if it never gets called.
Avoids drifting from upload-release.pl when new cross targets such as x86_64-unknown-freebsd are added to fallback-paths. Also drop the reference to a not-yet-existing Python rewrite of the upload script. Addresses review comments on NixOS#15640.
This more honestly wraps the underlying FS accessor (we want to use the unix-specific dirfd-based one).
With the addition of the file descriptor based source accessor (that does caching) we now create a fresh instance of the accessor and the last path component is not followed, so the transformation is safe (actually tests fail without this because some directories get unlinked and result in ENOENT if the file descriptor gets cached).
The API is the same and uses the unix-specific implementation. We can also drop all makeParentCanonical calls, because the function follows symlinks in the parents already when opening the dirFd/file. One slight wrinkle it that we first have to do absPath - this is fine.
Unlike previously, we now follow symlinks in parents (not the last path component), but this is secure and what versions like 2.18 did.
…ons) and use in recursive traversal This ensures race-free traversal throughout.
This completely bypasses coroutines and serialisation/deserialisation overhead and could start using reflinking in the future too.
…for-search nix search: switch to boost::regex for ~1.74x speedup
…-follows-warning fix: remove follows after git-hooks bump
build: embed C API symbols in release binaries
…race Remove non-atomically initialised variable vImportedDrvToDerivation
There is a race between activity and logger teardown, so we switch from a unique_ptr to a raw pointer and leak the logger so it persists across all activity teardowns. Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>
In pr NixOS#15082 I introduced parser-based binding detection, but `parseReplBindings` silently swallowed "unexpected end of file" errors from incomplete input (e.g. unclosed multi-line strings). This caused the REPL to fall through to expression parsing as opposed to binding parsing, leading to an "unexpected '='" error instead of prompting for continuation lines. Fixes NixOS#15801
…te input detection Extract isIncompleteInput helper to deduplicate the "unexpected end of file" string matching that was repeated in parseString and parseReplBindings. Simplify parseReplBindings to return nullptr for "not valid binding syntax" instead of rethrowing a stored exception_ptr that was always swallowed by the caller. This makes its contract explicit: returns ExprAttrs* on success, nullptr if not bindings, or throws IncompleteReplExpr if the input is incomplete. This in turn simplifies processLine, which no longer needs a catch-and-rethrow dance to let IncompleteReplExpr propagate past a ParseError catch. Add a doc comment to IncompleteReplExpr explaining why the exception subtype is needed: evaluation can also produce "unexpected end of file" ParseErrors (e.g. import of a broken file), but those must be reported as errors, not trigger continuation.
IncompleteReplExpr is a control flow signal, not a parse error. Making it a ParseError subclass meant that any `catch (ParseError &)` could accidentally swallow the continuation signal, which is exactly the kind of bug that required the catch-and-rethrow workaround removed in the previous commit.
…g-continuation Fix repl multiline binding continuation
FreeBSD sets EMLINK instead of ELOOP when opening symlinks with `O_NOFOLLOW` in order to distinguish different error cases. Change all occurances of ELOOP to handle this difference.
HttpBinaryCacheStore: Don't ignore 401/407 errors
Fix FreeBSD makeFSSourceAccessor on symlinks
This will be used later in the fixed-output derivation hash mismatch handling path in the case where we fail to lock the correct path, to invoke this error early. No functional change intended.
…tching FOD While trying to lock the final output path of a hash-mismatching fixed-output derivation, a deadlock can arise if another derivation is building and has locked the same output path. Therefore, if locking fails, just throw the hash-mismatch error and bail. This does mean the output would not exist after, but I think the situation still makes sense, since this can be explained as another derivation that produces this output having been cancelled.
A deadlock was fixed for a case where a slow fixed-output derivation and a fast hash-mismatching fixed output derivation whose correct output path is the same as the slow one. Add a test case for this, so that it doesn't regress.
…t-leak Fix logging segfault by leaking logger
The option has been broken since Nix 2.4. The flag was accepted, but not used. This change plumbs it through to computeFSClosure.
A concurrent garbage collection can remove an entry from the links directory between the moment optimisePath_ checks for it and the moment it creates the hard link. This produced "cannot create hard link: No such file or directory" and failed the whole build. Treat the vanished link as a benign race and skip optimising the path, since a later pass will dedup it. Guard both the lstat (now maybeLstat) and the create_hard_link call, mirroring the existing too_many_links and file_exists handling. Relates to NixOS#7273 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The logger needs to be manually stopped because it is being leaked. Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>
Stop leaked logger in Finally block
…utputs Fix nix-copy-closure --include-outputs
…oved-links libstore: skip optimisation when GC removes a link concurrently
derivation-builder: Allow locking final output to fail for hash-mismatching FOD; plus refactor and test case
This is the major first step of NixOS#5025. Motivation ---------- Right now, there is a bit of conceptual tension between `--store` and `--builders`: - With `--store`, it is very convenient to think that the store knows how to build. One specifies a store, and gets a different method of building (local, some sort of remote) and scheduling (the remote store can take an entire derivation graph, multiple jobs) accordingly. - With `--builders` one has a local scheduler. Stores either act as a passive "workbench" for building (the local case) or we give them a single job (a single ready-to-build derivation) at a time. For historical reasons, this doesn't even use the store interface, except on the "other side" of the build hook. Issue NixOS#5025 is about using the store interface for `--builders`. In this case, we want to invert the relationship between `Store` and `Worker`. We have no use in this case for various `Store` methods creating a `Worker` behind the scenes, because we already have our `Worker`: - `LocalStore`s don't need any build method at all, we can just have our `Worker` directly use the local store, as it does with the default `worker.store` today. - remote stores supporting building (`ssh://` and `ssh-ng://`) are only fed a single job at a time, their remote-side scheduling being overkill for the task at hand. But we can't just delete the building methods of `--store` that we don't need anymore, because that would break `--store` building. We need to support both cases, where some stores effectively build/schedule and `Worker` can also own/borrow stores to be a single, unified scheduler. This change ----------- The way we satisfy both goals is by: - Pulling the building methods out of `Store` into a new `Builder` class - Having some stores also give/implement `Builder` The separation of `Store` vs `Builder` works for the `--builders` use-case, and the project of making that leverage `Store` and other C++ interfaces directly without indirecting through build hook or other ad-hoc implementation swapping methods. Here's how: as opposed to default `Store::` method implementations creating a `Worker` on the fly, `Worker` will implement `Builder`, and those methods, now on `Builder`, will become `Worker`'s own implementation. Local building To implement this conceptual switch, the methods that directly delegated to the worker are now instead ripped off `Store` and put in the new `Builder` class. (For example, `build/entry-points.cc` now contains all `Worker` methods (virtual method impls of `Builder`) and not `Store` method impls.) `Worker` also now owns `ref<...> srcStore, destStore`, directly out of issue NixOS#5025. (`Store & store, evalStore` are kept as aliasing references to reduce churn.) (`Worker` should be renamed to `LocalBuilder`, since it is the local build scheduler, and additionally knows how to build in local stores.) Remote building What about the `--store` case? The remote stores have a new method to provide a `Builder` of their choice given an `evalStore`. (This reflects the fact that `Builder` no longer has `evalStore` parameters on its methods.) That new method is a new interface `BuildStore` which `RemoteStore` and `LegacySSHStore` implement. Each one has an unexposed `Builder` implementation which will just do everything over RPC, like today. Putting it all together Introduce: - `getLocalBuilder`, a freestanding function to make a `Worker`. (Really, the details of `Worker` should be considered private to the `build/*.cc` files) - `getDefaultBuilder`, a freestanding function which will call `BuildStore::getBuilder` for `BuildStore`s, and use `getLocalBuilder` otherwise. Future work ----------- Issue NixOS#1221 The next step of the NixOS#5025 saga is issue NixOS#1221. To solve that issue, `Worker` will not use the build hook, but instead work via C++. In particular, it will do this: - if the builder is a `BuildStore`, use an appropriate method (possibly yet to be created) on the remote building store's (remote) `Builder`. - if the builder is a `LocalStore` `Worker` should *not* create another `Worker` (as the `build-remote` program would do today) but instead directly manage building in that local store, so we avoid **n** `Worker` instances scheduling independently (which is stupid discoordination). - (Otherwise fail, which matches what happens today, actually, just in fewer steps.) Simplifying the RPC case I also suspect that longer term, those stores will just implement `Builder` directly, as `evalStore` doesn't really make sense for RPC endpoints when the remote side has no idea what the caller is doing with other stores. We won't need `BuildStore` anymore then. Other improvements ------------------ Recursive Nix Speaking of avoiding redundant schedulers: `RestrictedStore`, when it used to override the store building methods, would spin up a new `Worker` for each recursive Nix build call. This is again bad --- we should have a central scheduler that takes in dynamic jobs, same for recursive Nix and dynamic derivations. Now this is *almost*, but not quite, fixed. Three changes were made: - `RestrictedBuilder` was split out from `RestrictedStore` to wrap the build methods. - `processConnection` takes an optional `Builder` parameter, using it directly rather than spinning one up with `getDefaultBuilder`. - `DerivationBuilder` took a callback to process the connection for recursive Nix, so the caller could provide the `processConnection` call with the ambient worker in order to reuse it. This would have solved the redundant scheduler problem very nicely! This unfortunately deadlocked, so instead the caller explicitly creates a fresh worker (as before, but not hidden beneath a gazillion abstractions) with a TODO saying the deadlock should be fixed and this should not be done. `LegacySSHStore` fix As a final note, the old `LegacySSHStore` did not override `buildPathsWithResults`, which meant that when specifying an `ssh://` store, the local scheduler was being erroneously used for some commands. Now, `LegacySSHBuilder::buildPathsWithResults` uses a single `buildPathsRaw` call (which sends the serve protocol `BuildPaths` command and returns `std::variant<BuildResultSuccessStatus, BuildError>` with the error message already read from the wire), and then queries realisations to reconstruct the `BuildResult`s --- code similar to the old fallback code for `ssh-ng://`. Use std::shared_ptr for processConnection's builder This avoids the need to pass a raw pointer. Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com> Rename BuildStore to BuildStore Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com> Co-authored-by: John Ericson <John.Ericson@Obsidian.Systems>
ArkhamKnight25
force-pushed
the
gsoc/builder-virtual-method
branch
3 times, most recently
from
June 30, 2026 04:40
94422a5 to
120c9dc
Compare
Add buildDerivation() and buildPathsWithResults() overloads to the Builder interface that take a StorePathSet of inputs and copy them into the builder's store before building, instead of relying on a separate copyPaths() beforehand. Shipping the inputs together with the build request lets a builder store decide where to build based on those inputs (for example, choosing a machine by total input size) before they are copied. This is not possible when copying and building are two separate calls. RemoteBuilder and LegacySSHBuilder open the local store as the source and copy the inputs into the builder's store; RestrictedBuilder rejects the operation. The copy honours the builders-use-substitutes setting. build-remote folds its standalone dependency copy into these overloads: the trusted/CA path calls buildDerivation() with the input paths, and the untrusted path ships the inputs together with the derivation's closure via buildPathsWithResults().
ArkhamKnight25
force-pushed
the
gsoc/builder-virtual-method
branch
from
June 30, 2026 05:21
120c9dc to
aee006b
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
20 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
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.