Windows: cover libstore in CI, and let it build content-addressed derivations

.github/workflows/ci.yml

@@ -146,7 +146,6 @@ jobs:
     needs: basic-checks
     name: windows unit tests
     runs-on: ubuntu-24.04
-    continue-on-error: true
     timeout-minutes: 60
     steps:
     - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
@@ -159,7 +158,7 @@ jobs:
         dogfood: ${{ github.event_name == 'workflow_dispatch' && inputs.dogfood || github.event_name != 'workflow_dispatch' }}
     - name: Build and test Windows components
       run: |
-        nix build --file ci/gha/tests/windows.nix crossBuild unitTests.nix-util-tests -L
+        nix build --file ci/gha/tests/windows.nix crossBuild unitTests.nix-util-tests unitTests.nix-store-tests -L
 
   installer_test:
     needs: [tests]

ci/gha/tests/windows.nix

@@ -16,6 +16,13 @@ let
       # hide/show sequences, making logs unreadable in GitHub Actions.
       buildCommand = ''
         set -o pipefail
+        # The outer build sets `NIX_STORE` to the host's POSIX store directory,
+        # and Wine passes it through to the Windows test binary. A store
+        # configured as `FilePathType::Native` then validates that value with
+        # `std::filesystem::path::is_absolute()`, which is false for a
+        # POSIX-rooted path on Windows because it has no root name. Clearing
+        # both lets each store type fall back to its own correct default.
+        unset NIX_STORE NIX_STORE_DIR
         {
           ${prev.buildCommand}
         } 2>&1 | ansi2txt
@@ -26,8 +33,8 @@ in
 {
   unitTests = {
     "nix-util-tests" = fixOutput packages."nix-util-tests-x86_64-w64-mingw32".passthru.tests.run;
+    "nix-store-tests" = fixOutput packages."nix-store-tests-x86_64-w64-mingw32".passthru.tests.run;
   };
 
-  # `unitTests` builds one suite, which links neither libmain nor libstore.
   crossBuild = packages."nix-everything-x86_64-w64-mingw32";
 }

src/libstore-tests/local-overlay-store.cc

@@ -44,14 +44,19 @@ TEST(LocalOverlayStore, upperLayer_notOverridden)
 
 TEST(LocalOverlayStore, upperLayer_overridden)
 {
+#ifdef _WIN32
+    constexpr std::string_view upper = "C:\\some\\upper";
+#else
+    constexpr std::string_view upper = "/some/upper";
+#endif
     LocalOverlayStoreConfig config{
         "",
         {
-            {"upper-layer", "/some/upper"},
+            {"upper-layer", std::string{upper}},
         },
     };
     EXPECT_TRUE(config.upperLayer.isOverridden());
-    EXPECT_EQ(config.upperLayer.get(), std::filesystem::path{"/some/upper"});
+    EXPECT_EQ(config.upperLayer.get(), std::optional<AbsolutePath>{std::string{upper}});
 }
 
 } // namespace nix

src/libstore-tests/nix_api_store.cc

@@ -11,6 +11,9 @@
 #include "nix/util/tests/string_callback.hh"
 #include "nix/util/tests/test-data.hh"
 #include "nix/util/url.hh"
+#ifdef _WIN32
+#  include "nix/util/windows-known-folders.hh"
+#endif
 
 #include "store-tests-config.hh"
 
@@ -50,8 +53,17 @@ TEST_F(nix_api_util_context, nix_store_get_storedir_default)
     assert_ctx_ok();
     ASSERT_EQ(NIX_OK, ret);
 
-    // These tests run with a unique storeDir, but not a relocated store
+    // These tests run with a unique storeDir, but not a relocated store.
+    //
+    // The default store is a local store, whose store dir is a *native* path.
+    // `NIX_STORE_DIR` is the Unix spelling, and on Windows it is not even a
+    // valid absolute path, so the native default is used there instead. See
+    // `StoreConfigBase::StoreDirSetting` for where that default comes from.
+#ifdef _WIN32
+    ASSERT_EQ((nix::windows::known_folders::getProgramData() / "nix" / "store").string(), str);
+#else
     ASSERT_STREQ(NIX_STORE_DIR, str.c_str());
+#endif
 
     nix_store_free(store);
 }

src/libstore/build/derivation-builder-impl.cc

@@ -764,4 +764,88 @@ SingleDrvOutputs DerivationBuilderImpl::checkSubmittedOutputs()
     return builtOutputs;
 }
 
+StorePath DerivationBuilderImpl::makeFallbackPath(OutputNameView outputName)
+{
+    // This is a bogus path type, constructed this way to ensure that it doesn't collide with any other store path
+    // See doc/manual/source/protocols/store-path.md for details
+    // TODO: We may want to separate the responsibilities of constructing the path fingerprint and of actually doing the
+    // hashing
+    auto pathType = "rewrite:" + std::string(drvPath.to_string()) + ":name:" + std::string(outputName);
+    return store.makeStorePath(
+        pathType,
+        // pass an all-zeroes hash
+        Hash(HashAlgorithm::SHA256),
+        outputPathName(drv.name, outputName));
+}
+
+StorePath DerivationBuilderImpl::makeFallbackPath(const StorePath & path)
+{
+    // This is a bogus path type, constructed this way to ensure that it doesn't collide with any other store path
+    // See doc/manual/source/protocols/store-path.md for details
+    auto pathType = "rewrite:" + std::string(drvPath.to_string()) + ":" + std::string(path.to_string());
+    return store.makeStorePath(
+        pathType,
+        // pass an all-zeroes hash
+        Hash(HashAlgorithm::SHA256),
+        path.name());
+}
+
+void DerivationBuilderImpl::prepareScratchOutputs()
+{
+    for (auto & [outputName, status] : initialOutputs) {
+        /* Set scratch path we'll actually use during the build.
+
+           If we're not doing a chroot build, but we have some valid
+           output paths.  Since we can't just overwrite or delete
+           them, we have to do hash rewriting: i.e. in the
+           environment/arguments passed to the build, we replace the
+           hashes of the valid outputs with unique dummy strings;
+           after the build, we discard the redirected outputs
+           corresponding to the valid outputs, and rewrite the
+           contents of the new outputs to replace the dummy strings
+           with the actual hashes. */
+        auto scratchPath = !status.known ? makeFallbackPath(outputName)
+                           : !needsHashRewrite()
+                               /* Can always use original path in sandbox */
+                               ? status.known->path
+                               : !status.known->isPresent()
+                                     /* If path doesn't yet exist can just use it */
+                                     ? status.known->path
+                                     : buildMode != bmRepair && !status.known->isValid()
+                                           /* If we aren't repairing we'll delete a corrupted path, so we
+                                              can use original path */
+                                           ? status.known->path
+                                           : /* If we are repairing or the path is totally valid, we'll need
+                                                to use a temporary path */
+                                           makeFallbackPath(status.known->path);
+        scratchOutputs.insert_or_assign(outputName, scratchPath);
+
+        /* Substitute output placeholders with the scratch output paths.
+           We'll use during the build. */
+        inputRewrites[hashPlaceholder(outputName)] = store.printStorePath(scratchPath);
+
+        /* Additional tasks if we know the final path a priori. */
+        if (!status.known)
+            continue;
+        auto fixedFinalPath = status.known->path;
+
+        /* Additional tasks if the final and scratch are both known and
+           differ. */
+        if (fixedFinalPath == scratchPath)
+            continue;
+
+        /* Ensure scratch path is ours to use. */
+        deletePath(store.printStorePath(scratchPath));
+
+        /* Rewrite and unrewrite paths */
+        {
+            std::string h1{fixedFinalPath.hashPart()};
+            std::string h2{scratchPath.hashPart()};
+            inputRewrites[h1] = h2;
+        }
+
+        redirectedOutputs.insert_or_assign(std::move(fixedFinalPath), std::move(scratchPath));
+    }
+}
+
 } // namespace nix

src/libstore/build/derivation-builder-impl.hh

@@ -127,6 +127,44 @@ protected:
      * and attach them to the derivation
      */
     SingleDrvOutputs checkSubmittedOutputs();
+
+    /**
+     * Whether we need to perform hash rewriting if there are valid output paths.
+     *
+     * Only a sandbox that can present the outputs at their final paths can skip
+     * this, which on Unix means a chroot. Windows has no such mechanism, so the
+     * default is the answer there.
+     */
+    virtual bool needsHashRewrite()
+    {
+        return true;
+    }
+
+    /**
+     * Create alternative path calculated from but distinct from the
+     * input, so we can avoid overwriting outputs (or other store paths)
+     * that already exist.
+     */
+    StorePath makeFallbackPath(const StorePath & path);
+
+    /**
+     * Make a path to another based on the output name along with the
+     * derivation hash.
+     *
+     * @todo Add option to randomize, so we can audit whether our
+     * rewrites caught everything
+     */
+    StorePath makeFallbackPath(OutputNameView outputName);
+
+    /**
+     * Decide the scratch path for each output and populate `inputRewrites`
+     * so the builder sees placeholders substituted.
+     *
+     * Platform-neutral, and needed by every builder: without it
+     * `hashPlaceholder(outputName)` is never substituted, so a derivation
+     * cannot refer to its own outputs.
+     */
+    void prepareScratchOutputs();
 };
 
 } // namespace nix

src/libstore/include/nix/store/local-overlay-store.hh

@@ -35,9 +35,15 @@ public:
           Must be used as OverlayFS lower layer for this store's store dir.
         )"};
 
-    const Setting<AbsolutePath> upperLayer{
+    /* Has no default: the constructor rejects a config that leaves it unset.
+       Spelling that as `std::nullopt` rather than a sentinel path matters
+       because `AbsolutePath` validates on construction, and a POSIX-rooted
+       sentinel is not absolute on Windows -- `is_absolute()` wants a root name
+       as well as a root directory -- so the sentinel made the config
+       unconstructible there regardless of what the caller passed. */
+    const Setting<std::optional<AbsolutePath>> upperLayer{
         (StoreConfig *) this,
-        "/upper-layer-must-be-set",
+        std::nullopt,
         "upper-layer",
         R"(
           Directory containing the OverlayFS upper layer for this store's store dir.

src/libstore/local-overlay-store.cc

@@ -39,7 +39,7 @@ StoreReference LocalOverlayStoreConfig::getReference() const
 
 std::filesystem::path LocalOverlayStoreConfig::toUpperPath(const StorePath & path) const
 {
-    return upperLayer.get() / path.to_string();
+    return *upperLayer.get() / path.to_string();
 }
 
 LocalOverlayStore::LocalOverlayStore(ref<const Config> config)
@@ -49,7 +49,7 @@ LocalOverlayStore::LocalOverlayStore(ref<const Config> config)
     , config{config}
     , lowerStore(openStore(config->lowerStoreUri.get()).dynamic_pointer_cast<LocalFSStore>())
 {
-    if (!config->upperLayer.isOverridden())
+    if (!config->upperLayer.get())
         throw Error("overlay store at %s requires the 'upper-layer' setting", PathFmt(config->realStoreDir.get()));
 
     if (config->checkMount.get()) {
@@ -70,9 +70,9 @@ LocalOverlayStore::LocalOverlayStore(ref<const Config> config)
         };
 
         auto expectedLowerDir = lowerStore->config.realStoreDir.get();
-        if (!checkOption("lowerdir", expectedLowerDir) || !checkOption("upperdir", config->upperLayer.get())) {
+        if (!checkOption("lowerdir", expectedLowerDir) || !checkOption("upperdir", *config->upperLayer.get())) {
             debug("expected lowerdir: %s", PathFmt(lowerStore->config.realStoreDir.get()));
-            debug("expected upperdir: %s", PathFmt(config->upperLayer.get()));
+            debug("expected upperdir: %s", PathFmt(*config->upperLayer.get()));
             debug("actual mount: %s", mountInfo);
             throw Error("overlay filesystem %s mounted incorrectly", PathFmt(config->realStoreDir.get()));
         }

src/libstore/store-dir-config.cc

@@ -10,14 +10,19 @@ StorePath StoreDirConfig::parseStorePath(std::string_view path) const
 {
     if (path.empty())
         throw BadStorePath("empty path is not a valid store path");
-    // On Windows, `/nix/store` is not a canonical path. More broadly it
-    // is unclear whether this function should be using the native
-    // notion of a canonical path at all. For example, it makes to
-    // support remote stores whose store dir is a non-native path (e.g.
-    // Windows <-> Unix ssh-ing).
+    /* Canonicalise in whatever syntax the store directory itself uses, not
+       necessarily the native one. A store dir is a *logical* path: a
+       Unix-style one stays Unix-style even on Windows -- that is what
+       `FilePathType::Unix` means, and it is what lets a Windows client talk to
+       a Unix store over ssh -- so normalising it with native semantics would
+       rewrite the separators to `\` and the comparison below would stop
+       matching. Previously Windows did not normalise at all, so
+       `<storeDir>/./x`, `<storeDir>/y/../x` and a trailing separator were all
+       rejected rather than accepted-and-normalised. */
     auto p =
 #ifdef _WIN32
-        std::filesystem::path(path)
+        storeDir.starts_with('/') ? std::filesystem::path(CanonPath(std::string(path)).abs())
+                                  : std::filesystem::path(path).lexically_normal()
 #else
         canonPath(std::string(path))
 #endif

src/libstore/unix/build/unix-derivation-builder-impl.hh

@@ -131,14 +131,6 @@ protected:
 
     friend struct RestrictedStore;
 
-    /**
-     * Whether we need to perform hash rewriting if there are valid output paths.
-     */
-    virtual bool needsHashRewrite()
-    {
-        return true;
-    }
-
 public:
 
     std::optional<Descriptor> startBuild() override;
@@ -332,22 +324,6 @@ public:
 private:
 
     bool decideWhetherDiskFull();
-
-    /**
-     * Create alternative path calculated from but distinct from the
-     * input, so we can avoid overwriting outputs (or other store paths)
-     * that already exist.
-     */
-    StorePath makeFallbackPath(const StorePath & path);
-
-    /**
-     * Make a path to another based on the output name along with the
-     * derivation hash.
-     *
-     * @todo Add option to randomize, so we can audit whether our
-     * rewrites caught everything
-     */
-    StorePath makeFallbackPath(OutputNameView outputName);
 };
 
 } // namespace nix