Merge pull request #14560 from obsidiansystems/fill-in-outputs

Dedup some derivation initialization logic, and test

src/libstore-tests/data/derivation/invariants/bad-depends-on-drv-pre.json

@@ -0,0 +1,27 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "InputAddressed throws when should be deferred",
+    "out": ""
+  },
+  "inputs": {
+    "drvs": {
+      "lg4c4b8r9hlczwprl6kgnzfd9mc1xmkk-dependency.drv": {
+        "dynamicOutputs": {},
+        "outputs": [
+          "out"
+        ]
+      }
+    },
+    "srcs": []
+  },
+  "name": "depends-on-drv",
+  "outputs": {
+    "out": {
+      "path": "c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"
+    }
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/bad-env-var.json

@@ -0,0 +1,18 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Wrong env var value throws error",
+    "out": "/nix/store/c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "bad-env-var",
+  "outputs": {
+    "out": {}
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/bad-path.json

@@ -0,0 +1,20 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Wrong InputAddressed path throws error",
+    "out": "/nix/store/c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "bad-path",
+  "outputs": {
+    "out": {
+      "path": "c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"
+    }
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/depends-on-drv-pre.json

@@ -0,0 +1,25 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Deferred stays deferred with CA dependencies",
+    "out": ""
+  },
+  "inputs": {
+    "drvs": {
+      "lg4c4b8r9hlczwprl6kgnzfd9mc1xmkk-dependency.drv": {
+        "dynamicOutputs": {},
+        "outputs": [
+          "out"
+        ]
+      }
+    },
+    "srcs": []
+  },
+  "name": "depends-on-drv",
+  "outputs": {
+    "out": {}
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/filled-in-deferred-empty-env-var-post.json

@@ -0,0 +1,20 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Fill in deferred output with empty env var",
+    "out": "/nix/store/bilpz1nq8qi9r3bzsp72n34yjgqg43ws-filled-in-deferred-empty-env-var"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "filled-in-deferred-empty-env-var",
+  "outputs": {
+    "out": {
+      "path": "bilpz1nq8qi9r3bzsp72n34yjgqg43ws-filled-in-deferred-empty-env-var"
+    }
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/filled-in-deferred-empty-env-var-pre.json

@@ -0,0 +1,18 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Fill in deferred output with empty env var",
+    "out": ""
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "filled-in-deferred-empty-env-var",
+  "outputs": {
+    "out": {}
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/filled-in-deferred-no-env-var-post.json

@@ -0,0 +1,20 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Fill in deferred with missing env var",
+    "out": "/nix/store/wpk9qrgg77fyswhailap0gicgw98izx9-filled-in-deferred-no-env-var"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "filled-in-deferred-no-env-var",
+  "outputs": {
+    "out": {
+      "path": "wpk9qrgg77fyswhailap0gicgw98izx9-filled-in-deferred-no-env-var"
+    }
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/filled-in-deferred-no-env-var-pre.json

@@ -0,0 +1,17 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Fill in deferred with missing env var"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "filled-in-deferred-no-env-var",
+  "outputs": {
+    "out": {}
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/data/derivation/invariants/filled-in-idempotent.json

@@ -0,0 +1,20 @@
+{
+  "args": [],
+  "builder": "/bin/sh",
+  "env": {
+    "__doc": "Correct path stays unchanged",
+    "out": "/nix/store/w4bk7hpyxzgy2gx8fsa8f952435pll3i-filled-in-already"
+  },
+  "inputs": {
+    "drvs": {},
+    "srcs": []
+  },
+  "name": "filled-in-already",
+  "outputs": {
+    "out": {
+      "path": "w4bk7hpyxzgy2gx8fsa8f952435pll3i-filled-in-already"
+    }
+  },
+  "system": "x86_64-linux",
+  "version": 4
+}

src/libstore-tests/derivation/external-formats.cc

@@ -1,57 +1,14 @@
 #include <nlohmann/json.hpp>
 #include <gtest/gtest.h>
 
-#include "nix/util/experimental-features.hh"
 #include "nix/store/derivations.hh"
-
-#include "nix/store/tests/libstore.hh"
+#include "derivation/test-support.hh"
 #include "nix/util/tests/json-characterization.hh"
 
 namespace nix {
 
 using nlohmann::json;
 
-class DerivationTest : public virtual CharacterizationTest, public LibStoreTest
-{
-    std::filesystem::path unitTestData = getUnitTestData() / "derivation";
-
-public:
-    std::filesystem::path goldenMaster(std::string_view testStem) const override
-    {
-        return unitTestData / testStem;
-    }
-
-    /**
-     * We set these in tests rather than the regular globals so we don't have
-     * to worry about race conditions if the tests run concurrently.
-     */
-    ExperimentalFeatureSettings mockXpSettings;
-};
-
-class CaDerivationTest : public DerivationTest
-{
-    void SetUp() override
-    {
-        mockXpSettings.set("experimental-features", "ca-derivations");
-    }
-};
-
-class DynDerivationTest : public DerivationTest
-{
-    void SetUp() override
-    {
-        mockXpSettings.set("experimental-features", "dynamic-derivations ca-derivations");
-    }
-};
-
-class ImpureDerivationTest : public DerivationTest
-{
-    void SetUp() override
-    {
-        mockXpSettings.set("experimental-features", "impure-derivations");
-    }
-};
-
 TEST_F(DerivationTest, BadATerm_version)
 {
     ASSERT_THROW(

src/libstore-tests/derivation/invariants.cc

@@ -0,0 +1,264 @@
+#include <gtest/gtest.h>
+#include <nlohmann/json.hpp>
+
+#include "nix/store/derivations.hh"
+#include "nix/store/tests/libstore.hh"
+#include "nix/store/dummy-store-impl.hh"
+#include "nix/util/tests/json-characterization.hh"
+
+#include "derivation/test-support.hh"
+
+namespace nix {
+
+class FillInOutputPathsTest : public LibStoreTest, public JsonCharacterizationTest<Derivation>
+{
+    std::filesystem::path unitTestData = getUnitTestData() / "derivation" / "invariants";
+
+protected:
+    FillInOutputPathsTest()
+        : LibStoreTest([]() {
+            auto config = make_ref<DummyStoreConfig>(DummyStoreConfig::Params{});
+            config->readOnly = false;
+            return config->openDummyStore();
+        }())
+    {
+    }
+
+    /**
+     * Create a CA floating output derivation and write it to the store.
+     * This is useful for creating dependencies that will cause downstream
+     * derivations to remain deferred.
+     */
+    StorePath makeCAFloatingDependency(std::string_view name)
+    {
+        Derivation depDrv;
+        depDrv.name = name;
+        depDrv.platform = "x86_64-linux";
+        depDrv.builder = "/bin/sh";
+        depDrv.outputs = {
+            {
+                "out",
+                // will ensure that downstream is deferred
+                DerivationOutput{DerivationOutput::CAFloating{
+                    .method = ContentAddressMethod::Raw::NixArchive,
+                    .hashAlgo = HashAlgorithm::SHA256,
+                }},
+            },
+        };
+        depDrv.env = {{"out", ""}};
+
+        // Fill in the dependency derivation's output paths
+        depDrv.fillInOutputPaths(*store);
+
+        // Write the dependency to the store
+        return writeDerivation(*store, depDrv, NoRepair);
+    }
+
+public:
+    std::filesystem::path goldenMaster(std::string_view testStem) const override
+    {
+        return unitTestData / testStem;
+    }
+};
+
+TEST_F(FillInOutputPathsTest, fillsDeferredOutputs_emptyStringEnvVar)
+{
+    using nlohmann::json;
+
+    // Before: Derivation with deferred output
+    Derivation drv;
+    drv.name = "filled-in-deferred-empty-env-var";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::Deferred{}}},
+    };
+    drv.env = {{"__doc", "Fill in deferred output with empty env var"}, {"out", ""}};
+
+    // Serialize before state
+    checkpointJson("filled-in-deferred-empty-env-var-pre", drv);
+
+    drv.fillInOutputPaths(*store);
+
+    // Serialize after state
+    checkpointJson("filled-in-deferred-empty-env-var-post", drv);
+
+    // After: Should have been converted to InputAddressed
+    auto * outputP = std::get_if<DerivationOutput::InputAddressed>(&drv.outputs.at("out").raw);
+    ASSERT_TRUE(outputP);
+    auto & output = *outputP;
+
+    // Environment variable should be filled in
+    EXPECT_EQ(drv.env.at("out"), store->printStorePath(output.path));
+}
+
+TEST_F(FillInOutputPathsTest, fillsDeferredOutputs_empty_string_var)
+{
+    using nlohmann::json;
+
+    // Before: Derivation with deferred output
+    Derivation drv;
+    drv.name = "filled-in-deferred-no-env-var";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::Deferred{}}},
+    };
+    drv.env = {
+        {"__doc", "Fill in deferred with missing env var"},
+    };
+
+    // Serialize before state
+    checkpointJson("filled-in-deferred-no-env-var-pre", drv);
+
+    drv.fillInOutputPaths(*store);
+
+    // Serialize after state
+    checkpointJson("filled-in-deferred-no-env-var-post", drv);
+
+    // After: Should have been converted to InputAddressed
+    auto * outputP = std::get_if<DerivationOutput::InputAddressed>(&drv.outputs.at("out").raw);
+    ASSERT_TRUE(outputP);
+    auto & output = *outputP;
+
+    // Environment variable should be filled in
+    EXPECT_EQ(drv.env.at("out"), store->printStorePath(output.path));
+}
+
+TEST_F(FillInOutputPathsTest, preservesInputAddressedOutputs)
+{
+    auto expectedPath = StorePath{"w4bk7hpyxzgy2gx8fsa8f952435pll3i-filled-in-already"};
+
+    Derivation drv;
+    drv.name = "filled-in-already";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::InputAddressed{.path = expectedPath}}},
+    };
+    drv.env = {
+        {"__doc", "Correct path stays unchanged"},
+        {"out", store->printStorePath(expectedPath)},
+    };
+
+    // Serialize before state
+    checkpointJson("filled-in-idempotent", drv);
+
+    auto drvBefore = drv;
+
+    drv.fillInOutputPaths(*store);
+
+    // Should still be no change
+    EXPECT_EQ(drv, drvBefore);
+}
+
+TEST_F(FillInOutputPathsTest, throwsOnIncorrectInputAddressedPath)
+{
+    auto wrongPath = StorePath{"c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"};
+
+    Derivation drv;
+    drv.name = "bad-path";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::InputAddressed{.path = wrongPath}}},
+    };
+    drv.env = {
+        {"__doc", "Wrong InputAddressed path throws error"},
+        {"out", store->printStorePath(wrongPath)},
+    };
+
+    // Serialize before state
+    checkpointJson("bad-path", drv);
+
+    ASSERT_THROW(drv.fillInOutputPaths(*store), Error);
+}
+
+TEST_F(FillInOutputPathsTest, throwsOnIncorrectEnvVar)
+{
+    auto wrongPath = StorePath{"c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"};
+
+    Derivation drv;
+    drv.name = "bad-env-var";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::Deferred{}}},
+    };
+    drv.env = {
+        {"__doc", "Wrong env var value throws error"},
+        {"out", store->printStorePath(wrongPath)},
+    };
+
+    // Serialize before state
+    checkpointJson("bad-env-var", drv);
+
+    ASSERT_THROW(drv.fillInOutputPaths(*store), Error);
+}
+
+TEST_F(FillInOutputPathsTest, preservesDeferredWithInputDrvs)
+{
+    using nlohmann::json;
+
+    // Create a CA floating dependency derivation
+    auto depDrvPath = makeCAFloatingDependency("dependency");
+
+    // Create a derivation that depends on the dependency
+    Derivation drv;
+    drv.name = "depends-on-drv";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::Deferred{}}},
+    };
+    drv.env = {
+        {"__doc", "Deferred stays deferred with CA dependencies"},
+        {"out", ""},
+    };
+    // Add the real input derivation dependency
+    drv.inputDrvs = {.map = {{depDrvPath, {.value = {"out"}}}}};
+
+    // Serialize before state
+    checkpointJson("depends-on-drv-pre", drv);
+
+    auto drvBefore = drv;
+
+    // Apply fillInOutputPaths
+    drv.fillInOutputPaths(*store);
+
+    // Derivation should be unchanged
+    EXPECT_EQ(drv, drvBefore);
+}
+
+TEST_F(FillInOutputPathsTest, throwsOnPatWhenShouldBeDeffered)
+{
+    using nlohmann::json;
+
+    // Create a CA floating dependency derivation
+    auto depDrvPath = makeCAFloatingDependency("dependency");
+
+    auto wrongPath = StorePath{"c015dhfh5l0lp6wxyvdn7bmwhbbr6hr9-wrong-name"};
+
+    // Create a derivation that depends on the dependency
+    Derivation drv;
+    drv.name = "depends-on-drv";
+    drv.platform = "x86_64-linux";
+    drv.builder = "/bin/sh";
+    drv.outputs = {
+        {"out", DerivationOutput{DerivationOutput::InputAddressed{.path = wrongPath}}},
+    };
+    drv.env = {
+        {"__doc", "InputAddressed throws when should be deferred"},
+        {"out", ""},
+    };
+    // Add the real input derivation dependency
+    drv.inputDrvs = {.map = {{depDrvPath, {.value = {"out"}}}}};
+
+    // Serialize before state
+    checkpointJson("bad-depends-on-drv-pre", drv);
+
+    // Apply fillInOutputPaths
+    ASSERT_THROW(drv.fillInOutputPaths(*store), Error);
+}
+
+} // namespace nix

src/libstore-tests/derivation/test-support.hh

@@ -0,0 +1,52 @@
+#pragma once
+
+#include <gtest/gtest.h>
+
+#include "nix/util/experimental-features.hh"
+#include "nix/store/tests/libstore.hh"
+#include "nix/util/tests/characterization.hh"
+
+namespace nix {
+
+class DerivationTest : public virtual CharacterizationTest, public LibStoreTest
+{
+    std::filesystem::path unitTestData = getUnitTestData() / "derivation";
+
+public:
+    std::filesystem::path goldenMaster(std::string_view testStem) const override
+    {
+        return unitTestData / testStem;
+    }
+
+    /**
+     * We set these in tests rather than the regular globals so we don't have
+     * to worry about race conditions if the tests run concurrently.
+     */
+    ExperimentalFeatureSettings mockXpSettings;
+};
+
+class CaDerivationTest : public DerivationTest
+{
+    void SetUp() override
+    {
+        mockXpSettings.set("experimental-features", "ca-derivations");
+    }
+};
+
+class DynDerivationTest : public DerivationTest
+{
+    void SetUp() override
+    {
+        mockXpSettings.set("experimental-features", "dynamic-derivations ca-derivations");
+    }
+};
+
+class ImpureDerivationTest : public DerivationTest
+{
+    void SetUp() override
+    {
+        mockXpSettings.set("experimental-features", "impure-derivations");
+    }
+};
+
+} // namespace nix

src/libstore-tests/meson.build

@@ -58,7 +58,8 @@ sources = files(
   'common-protocol.cc',
   'content-address.cc',
   'derivation-advanced-attrs.cc',
-  'derivation.cc',
+  'derivation/external-formats.cc',
+  'derivation/invariants.cc',
   'derived-path.cc',
   'downstream-placeholder.cc',
   'dummy-store.cc',