Gsoc/builder inputs rebased - #2
Open
ArkhamKnight25 wants to merge 578 commits into
Open
Conversation
Check if a schema migration is actually needed before acquiring exclusive on the big nix store lock, and always hold a shared lock throughout the duration of the LocalStore. Checking if it's needed prevents the build hook from trying to acquire exclusive while the parent is holding shared. Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>
This was the sanest solution I could come up with for settings that are defined in a base class that ends up being inherited from for different store types (since store slugs are different for each of those).
In 8ba7ebc (Replace hashes that appear in cache.nixos.org with hashes which are unlikely to do so (for the diff from 3.14.0 to 3.15.0), 2025-12-23) / NixOS#14923, real store paths in documentation were removed and replaced with fake ones to avoid docs getting accidental runtime dependencies. However, this missed one string in the `nix why-depends` documentation, which quoted the prefix of the string and now seems incongruous. Fix the prefix so it's consistent with the full path earlier in the documentation.
Using operator() caused very ugly clang formatting, e.g.
state.stmts->RegisterValidPath
.use()(printStorePath(info.path))(info.narHash.to_string(HashFormat::Base16, true))(
info.registrationTime == 0 ? time(nullptr) : info.registrationTime)(
info.deriver ? printStorePath(*info.deriver) : "",
(bool) info.deriver)(info.narSize, info.narSize != 0)(info.ultimate ? 1 : 0, info.ultimate)(
concatStringsSep(" ", Signature::toStrings(info.sigs)),
!info.sigs.empty())(renderContentAddress(info.ca), (bool) info.ca)
This was especially painful when there was a merge conflict involving
this type of code.
Now it looks like this:
state.stmts->RegisterValidPath.use()
.apply(printStorePath(info.path))
.apply(info.narHash.to_string(HashFormat::Base16, true))
.apply(info.registrationTime == 0 ? time(nullptr) : info.registrationTime)
.apply(info.deriver ? printStorePath(*info.deriver) : "", (bool) info.deriver)
.apply(info.narSize, info.narSize != 0)
.apply(info.ultimate ? 1 : 0, info.ultimate)
.apply(concatStringsSep(" ", Signature::toStrings(info.sigs)), !info.sigs.empty())
.apply(renderContentAddress(info.ca), (bool) info.ca)
Fixes NixOS#15980. We should treat all interruptions as recoverable.
Bumps [aws-actions/configure-aws-credentials](https://github.com/aws-actions/configure-aws-credentials) from 6.1.3 to 6.2.0. - [Release notes](https://github.com/aws-actions/configure-aws-credentials/releases) - [Changelog](https://github.com/aws-actions/configure-aws-credentials/blob/main/CHANGELOG.md) - [Commits](aws-actions/configure-aws-credentials@99214aa...e7f100c) --- updated-dependencies: - dependency-name: aws-actions/configure-aws-credentials dependency-version: 6.2.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
BDWGC has a tendency of being too loud with warnings, so silence it by making the warnings go into debug() logging. Nixpkgs has some annoying hacks to deal with this [1], which would be nice to do away with. [1]: https://github.com/NixOS/nixpkgs/blob/b66a6fb0aae47b295a5662b19aecd9eb08bab65e/pkgs/top-level/nixpkgs-basic-release-checks.nix#L51-L52
The GC's deleteReferrersClosure has a BFS phase that checks shared->tempRoots for each path, and a deletion phase that removes all visited paths. The "pending" mechanism synchronises with addTempRoot's socket handler, but only for the single path currently being visited in the BFS. Once the BFS moves on, pending is overwritten, so a concurrent addTempRoot for an already-visited path would add it to tempRoots without blocking, and the deletion phase would then delete it anyway because it never re-checked. Fix by re-checking tempRoots and setting pending before each deletion, mirroring the same check+pending pattern used in the BFS phase. This closes the window where a path could be deleted despite having a valid temproot from a concurrent evaluator. Assisted-by: Claude Opus 4.6
…ore paths The GC could delete store paths that were actively in use because several code paths accessed store paths without first registering them as temporary roots: - fetchers: Input::getAccessorUnchecked used ensurePath without addTempRoot for locked inputs with narHash, allowing GC to delete the store path between the validity check and accessor reads. This was the primary cause of "path does not exist" errors during concurrent GC and flake evaluation. - eval cache: getStringWithContext checked isValidPath without addTempRoot, creating a TOCTOU race where GC could delete context paths between the validity check and their use. - local store: multiple LocalStore instances in the same process (e.g. main store + eval store) shared the same temp root filename (based on PID only), causing the second instance to delete the first's temp root file on creation. Use a per-instance atomic counter to generate unique filenames. Assisted-by: Claude Opus 4.6 (1M context)
…-race gc: fix race where concurrent addTempRoot is ignored during deletion (fixed AI commit trailers)
…-locking libstore: only acquire big write lock when needed, hold read lock
Stable 2.34.x daemons are newer than 2.34.0pre20251217, but predate the `realisation-with-path-not-hash` protocol feature, so the suite ran against them and crashed.
Bump nixpkgs to 26.05
SQLiteStmt::Use: Rename operator() -> apply()
…ws-actions/configure-aws-credentials-6.2.0 build(deps): bump aws-actions/configure-aws-credentials from 6.1.3 to 6.2.0
…tions Don't memoise Interrupted errors
Add missing man pages
Closes NixOS#13470 Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>
…gs-from-build Add `--out-link` and `--print-out-paths` to `nix flake check`
We generally want to minimize the number of top-level commands, especially for infrequently used non-porcelain operations.
Rename nix build-trace -> nix store build-trace
This immediately fails the "slow" derivation if the FIFO exists but can't be written to. This way test misconfigurations will be distinguishable from this case regressing.
When running in NixOS in some configs, the sandbox users are not the same as the user running the tests. Make it writable to the sandbox users.
doc/why-depends: fix example store hash prefix
Make command descriptions consistently lower case
Reserve a couple of worker operation codes
Ensure the callback is only called once. Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@saronic.com>
Fix crash in restricted store queryRealisationUncached
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` 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: - `Store::getBuilder`, which returns an owning reference to something implementing Builder. - `LocalBuilder`, a wrapper around `Worker` to enable thread-safety and optimized ensurePath. 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>
Separate building/scheduling from storage
libutil: Fix Config::toKeyValue, make libutil-tests less noisy
Use C23 `embed` instead of `gen_headers`
Assisted-by: Codex (GPT-5)
libfetchers: namespace input fingerprint by scheme
…ial-manual Appropriate exclusion of rl-next.md when officialRelease = true
Working with templates adds some overhead, but parameterizing the inputs is an important prerequisite for storing the options inside the derivation. Subsequent commits can clean this up significantly. Co-authored-by: Amaan Qureshi <git@amaanq.com> Assisted-by: Claude:opus-4.8
This is much nicer to read than imperatively mutating fields one by one. Assisted-by: Claude:opus-4.8
libstore: parameterize the inputs of `Derivation`
The previous code had excessive message duplication yet did not include the line of the attribute. Replace with a new `warnAttr` closure to simplify. Co-Authored-By: John Ericson <John.Ericson@Obsidian.Systems>
libexpr: Give better warning messages for derivation attributes
Expose a limited Nix daemon into derivations that request it so builders can add objects and register outputs before they complete. The manual is updated accordingly.
…rotocol Implement new builder-rpc-v0 feature
warning: input 'git-hooks-nix' has an override for a non-existent input 'gitignore' removed upstream in cachix/git-hooks.nix#721
This saves tens of thousands of calls to SQLite and to makeFixedOutputPathFromCA(). (E.g. instantiating nixpkgs#firefox was doing 4744 fetcher cache checks for /pkgs/stdenv/generic/source-stdenv.sh.)
Populate the srcToStore cache when we have a fetcher cache hit
flake: drop unused gitignore override
This fixes the following Sentry crash report: libnixutil.so.2.34.80xff807ce5e404 nix::panic (error.cc:459) libnixutil.so.2.34.80xff807ceed68c __wrap___assert_fail (wrap-assert-fail.cc:18) libnixutil.so.2.34.80xff807cee2834 nix::Pid::wait (processes.cc:100) libnixstore.so.2.34.80xff807cc4fcfc operator() (derivation-builder.cc:1102) libnixstore.so.2.34.80xff807cc4fcfc nix::DerivationBuilderImpl::processSandboxSetupMessages (derivation-builder.cc:1111) libnixstore.so.2.34.80xff807cc5d390 .LTHUNK39.lto_priv.4 (linux-derivation-builder.cc:477) libnixstore.so.2.34.80xff807cc4b10c nix::DerivationBuilderImpl::startBuild (derivation-builder.cc:895) This happens because in the Linux sandbox, if the sandbox helper fails, `pid` is not initialised yet so we can't wait for it.
This surfaces the bug fixed by boostorg/context#337 in our test suite.
… while abandoning the coroutine Applies a patch to our boost.context dependency to fix NixOS#16174. An alternative would be to apply basically the same workaround to our suspension points, but that would be more fragile. Other packagers might want to apply the boost patch too (maybe once that's merged upstream), since the issue is likely to affect more stuff than just nix. The bug is subtle enough that it went unnoticed for quite some time.
…ages-crash Fix assertion failure in processSandboxSetupMessages()
Use vfork() in a safer manner in runProgram2
…t-exceptions Add boost.context patch for std::uncaught_exceptions() misreporting 0 while abandoning the coroutine
libfetchers: Simplify git sink, test much more things and align overwrite semantics closer to libarchive
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().
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.