Windows: cover libstore in CI, and let it build content-addressed derivations - #16411
Open
awsmadi wants to merge 7 commits into
Open
Windows: cover libstore in CI, and let it build content-addressed derivations#16411awsmadi wants to merge 7 commits into
awsmadi wants to merge 7 commits into
Conversation
`unitTests` built only `nix-util-tests`, which links neither libmain nor libstore, so nothing in libstore was exercised on Windows at all. Output checks, content-addressed outputs and fixed-output derivations are handled in platform-neutral code that the Windows builder inherits, but that inheritance was never verified. Adding the suite reports 784 tests from 87 suites under Wine, of which 729 pass and 24 fail. Every failure is a test hardcoding `/nix/store`, which is not an absolute path on Windows because `std::filesystem::path::is_absolute()` requires a root *name* as well as a root directory. Those are fixed separately; this commit is the wiring that makes them visible. Assisted-by: Claude Code (claude-opus-5)
The outer build exports `NIX_STORE` pointing at the host's POSIX store directory, and Wine passes it straight through to the Windows test binary. `StoreDirSetting` consults `NIX_STORE_DIR` then `NIX_STORE` before its own default, so a store declared `FilePathType::Native` ends up validating that POSIX value with `std::filesystem::path::is_absolute()` -- false on Windows, which requires a root name as well as a root directory. Clearing both lets each store type fall back to its own default: the Unix default stays `/nix/store`, and the native one becomes `getProgramData()/nix/store` as intended. Measured on nix-store-tests under Wine: 24 failures before, 8 after, with passes going from 729 to 745 of 784. Assisted-by: Claude Code (claude-opus-5)
Wiring nix-store-tests into the Windows CI job surfaced 24 failures. Sixteen were the `NIX_STORE` leak fixed in the previous commit; these are the remaining eight, which are three separate pre-existing defects rather than test bugs. **`LocalOverlayStoreConfig` was unconstructible on Windows.** `upperLayer` was a `Setting<AbsolutePath>` whose default was the sentinel `/upper-layer-must-be-set`. `AbsolutePath` validates on construction and a POSIX-rooted sentinel is not absolute on Windows, so the config threw before the caller's parameters were even considered. It is now `Setting<std::optional<AbsolutePath>>` defaulting to `std::nullopt`, which is what "must be set" actually means; the serialiser for that type already existed, and the constructor already rejected the unset case, so the sentinel was never meaningfully read. **`parseStorePath` did not normalise on Windows.** It wrapped the input in `std::filesystem::path` and compared `parent_path()`, so `<storeDir>/./x`, `<storeDir>/y/../x` and a trailing separator were rejected instead of being normalised and accepted. It now canonicalises in the syntax the store directory itself uses: a Unix-style store dir stays Unix-style on Windows, which is what `FilePathType::Unix` is for and what allows a Windows client to address a Unix store. Normalising natively would rewrite the separators to `\\` and break the comparison instead. The pre-existing comment questioning these semantics is replaced with one stating them. **A C-API test asserted the Unix default store dir.** The default store is local, hence native, so on Windows it is `getProgramData()/nix/store` rather than `NIX_STORE_DIR`. Measured under Wine: 784 tests, 753 passing, zero failures (was 729 passing with 24 failures). Assisted-by: Claude Code (claude-opus-5)
Two gaps meant the suite added earlier would not actually have protected anything. The job's build step named `unitTests.nix-util-tests` explicitly, so adding an attribute to `ci/gha/tests/windows.nix` was not enough to make CI run it. More importantly the job carried `continue-on-error: true`, so a Windows failure has never blocked a merge. A suite that is allowed to be red provides no protection, and libstore was not covered on Windows at all until now, so the two together meant Windows could regress silently. Dropping it makes `windows unit tests` a real gate. Safe to do now that the suite is green: 784 tests, 753 passing, zero failures under Wine. Assisted-by: Claude Code (claude-opus-5)
…put on Windows
`UnixDerivationBuilderImpl::startBuild` decided the scratch path per output
and populated `inputRewrites` from it. Nothing in that loop is POSIX --- it
uses `needsHashRewrite()`, `makeFallbackPath()`, `scratchOutputs`,
`hashPlaceholder()` and `redirectedOutputs`, all of which are either already
shared or move here with it --- so it now lives on `DerivationBuilderImpl` as
`prepareScratchOutputs()`, along with the two virtuals it needs.
`needsHashRewrite()` keeps its `true` default and the chroot builder keeps its
override; Windows has no chroot, so the default is right there.
The Windows builder had its own cut-down version of that loop which assumed
every output was input-addressed:
if (!ia)
throw UnimplementedError(
"only input-addressed derivation outputs are supported on Windows");
That is what actually stopped content-addressed and fixed-output derivations
from building on Windows. They were refused before the build started, not by
anything downstream, so the platform-neutral output checking and
content-addressing in `registerOutputs` was unreachable for them. Calling the
shared version instead removes the restriction, because it works off
`initialOutputs` and copes with outputs whose final path is not known up
front. It also means `inputRewrites` is populated on Windows for the first
time, which the next commit makes use of.
Verified: native and `x86_64-w64-mingw32` both compile clean;
nix-store-tests 792/787 natively, unchanged; under Wine nix-store-tests
784/753 and nix-util-tests 799/791, both with zero failures.
Assisted-by: Claude Code (claude-opus-5)
The previous commit populates `inputRewrites` on Windows; nothing consumed it, so `hashPlaceholder(outputName)` still reached the builder verbatim and a derivation could not name its own outputs. The Unix builder substitutes in three places --- the environment block, the command line, and `extraFiles` Assisted-by: Claude Code (claude-opus-5) --- and the first two now happen here too. Without a sandbox this substitution is the *only* mechanism by which a Windows derivation can refer to its outputs, since there is no chroot that could present them at their final paths instead. Also refreshes the class comment, which still claimed content-addressed and fixed-output derivations were unsupported and that there was never anything to rewrite. Both stopped being true one commit ago. `extraFiles` remains unimplemented on Windows and is left for a follow-up: it needs a `writeBuilderFile` equivalent, and the natural one to reuse is the `OsFilename`-typed version from NixOS#15244, which has not landed. Verified: pre-commit 7/7; `x86_64-w64-mingw32` compiles clean; nix-store-tests 792/787 natively and 784/753 under Wine, both unchanged and with zero failures.
`getFSSourceAccessor` passed the literal "/" as its root.
`std::filesystem::path("/").is_absolute()` is false on Windows --- that path
has a root directory but no root name, and `is_absolute()` requires both ---
so `WindowsSourceAccessor`'s constructor assertion fired. `EvalState` reaches
here through `getFSSourceAccessor`, which meant every command that evaluates
anything aborted:
Assertion failed: root.empty() || root.is_absolute(),
file src/libutil/posix-source-accessor.cc, line 648
An empty root is how this accessor already spells "no prefix, the paths I am
given are absolute already": `WindowsSourceAccessor::makeAbsPath` handles it
explicitly, and the assertion permits it. That is also the honest description
of Windows, which has no single filesystem root because paths are rooted per
drive.
Measured with a cross-compiled `nix.exe` under Wine. Before, `nix eval --expr
'1 + 1'` aborted on the assertion; after, it prints 2, and `"a" + "b"` prints
"ab". `nix --version` and `nix store info` worked either way, since neither
constructs an evaluator.
Reading a `.nix` file still fails, with a different bug --- a native path gets
a "/" prepended, giving `path '/Z:\tmp\t.nix' does not exist`. That is
concatenation rather than validation and is left for a separate change.
Verified: pre-commit 7/7; native nix-util-tests 813/811 and nix-store-tests
792/787, both unchanged; nix-store-tests under Wine 784/753 with zero
failures, confirmed by a forced rebuild rather than a cache hit.
Assisted-by: Claude Code (claude-opus-5)
awsmadi
force-pushed
the
feat/windows-builder-parity
branch
from
September 1, 2026 21:28
b745957 to
ef289c2
Compare
awsmadi
force-pushed
the
feat/windows-builder-parity
branch
from
September 2, 2026 17:05
ef289c2 to
9afc0ca
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
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.
Windows had no libstore test coverage at all, and the Windows derivation builder refused any output whose final path was not known before the build. This wires libstore into the Windows CI job, fixes what that exposed, and moves the shared part of the builder's output setup somewhere both platforms can use it.
Measured under Wine:
nix-store-testsgoes from not being built to 784 tests with 753 passing and zero failures.Windows CI now covers libstore, and can fail
ci/gha/tests/windows.nixbuilt onlynix-util-tests, which as its own comment noted "links neither libmain nor libstore". So output checking, content-addressing and fixed-output derivations were all reached through platform-neutral code that nothing on Windows ever exercised.The job also carried
continue-on-error: true, so a Windows failure had never blocked a merge. Both together meant Windows could regress silently. That flag is gone, which is only reasonable because the suite is green.One environment fix was needed to get there: the outer build exports
NIX_STOREpointing at the host's POSIX store directory and Wine passes it through, so a store declaredFilePathType::Nativevalidated that value withis_absolute()and rejected it. Clearing it lets each store type use its own default. That alone took the failure count from 24 to 8.Three pre-existing Windows defects the coverage exposed
LocalOverlayStoreConfigcould not be constructed on Windows at all.upperLayerwas aSetting<AbsolutePath>defaulting to the sentinel/upper-layer-must-be-set.AbsolutePathvalidates on construction, and a POSIX-rooted sentinel is not absolute on Windows, so the config threw before the caller's parameters were looked at. It is nowSetting<std::optional<AbsolutePath>>defaulting tostd::nullopt, which is what "must be set" means; the serialiser for that type already existed and the constructor already rejected the unset case, so the sentinel was never meaningfully read.parseStorePathdid not normalise on Windows. It wrapped the input instd::filesystem::pathand comparedparent_path(), so<storeDir>/./x,<storeDir>/y/../xand a trailing separator were rejected rather than normalised. It now canonicalises in whatever syntax the store directory itself uses: a Unix-style store dir stays Unix-style on Windows, which is whatFilePathType::Unixis for and what lets a Windows client address a Unix store. Normalising natively would rewrite the separators and break the comparison instead. The comment saying these semantics were "unclear" is replaced with one stating them.A C-API test asserted the Unix default store dir. The default store is local, hence native, so on Windows it is
getProgramData()/nix/store.Content-addressed and fixed-output derivations on Windows
WindowsDerivationBuilderImpl::startBuildhad its own cut-down copy of the scratch-output loop which assumed every output was input-addressed:That is what stopped content-addressed and fixed-output derivations from building there. They were refused before the build started, so the platform-neutral output checking and content-addressing in
registerOutputswas unreachable for them.The real loop lived in
UnixDerivationBuilderImpl::startBuildbut contains nothing POSIX — onlyneedsHashRewrite(),makeFallbackPath(),scratchOutputs,hashPlaceholder()andredirectedOutputs. It moves toDerivationBuilderImpl::prepareScratchOutputs()with the two virtuals it needs.needsHashRewrite()keeps itstruedefault and the chroot builder keeps its override; Windows has no chroot, so the default is correct there.With that shared, the Windows builder also applies
inputRewritesto its environment block and command line, as Unix does. Without a sandbox that substitution is the only way a Windows derivation can name its own outputs, since there is no chroot that could present them at their final paths instead.extraFilesis still unimplemented on Windows. It needs awriteBuilderFileequivalent, and the natural one to reuse is theOsFilename-typed version from #15244, which has not landed.The evaluator could not start on Windows
getFSSourceAccessorpassed the literal"/"as its root.std::filesystem::path("/").is_absolute()is false on Windows — that path has a root directory but no root name, andis_absolute()wants both — soWindowsSourceAccessor's constructor assertion fired.EvalStatereaches here throughgetFSSourceAccessor, so every command that evaluated anything aborted:An empty root is how this accessor already spells "no prefix, the paths I am given are absolute already" —
WindowsSourceAccessor::makeAbsPathhandles it explicitly and the assertion permits it — and it is the honest description of Windows, where paths are rooted per drive rather than under one root.Measured with a cross-compiled
nix.exeunder Wine: before,nix eval --expr '1 + 1'aborted; after, it prints2.nix --versionandnix store infoworked either way, since neither constructs an evaluator.These are one bug, not five
Every fix above is the same mistake at a different site: treating a POSIX-rooted path as absolute on Windows, where
is_absolute()requires a root name as well as a root directory. The overlay-store sentinel, the SSL cert path in #16383,parseStorePath, and the source accessor are all instances.There is a sixth site, not fixed here, and it needs a decision rather than a patch — filed separately. Reading a
.nixfile givespath '/Z:\tmp\t.nix' does not exist, becauseCanonPathunconditionally prepends/while the caller hands it a native path. That one is a type confusion between virtual and native paths, and picking a fix requires deciding whatSourcePathmeans on Windows.Verification
checks.x86_64-linux.pre-commit: 7/7 hooksnix-store-tests792/787 andnix-util-tests813/811, both unchanged from before these commitsnix-store-testsunder Wine: 784 run, 753 pass, zero failures, confirmed by a forced rebuild rather than a cache hitx86_64-w64-mingw32cross-compile clean