From d34ee4e0aad965325a4b0e929573f971f1d2b8ea Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 13 Nov 2025 02:27:19 -0500 Subject: [PATCH] Dedup some derivation initialization logic, and test `nix derivation add`, and its C API counterpart, now works a bit closer to `builtins.derivation` in that they don't require the user to fill-in input addressed paths correctly ahead of time. The logic for this is carefully deduplicated, between all 3 entry points, and also between the existing `checkInvariants` function. There are some more functional tests, and there are also many more unit tests. --- src/libexpr/primops.cc | 23 +- src/libstore-c/nix_api_store.cc | 8 +- .../invariants/bad-depends-on-drv-pre.json | 27 ++ .../derivation/invariants/bad-env-var.json | 18 ++ .../data/derivation/invariants/bad-path.json | 20 ++ .../invariants/depends-on-drv-pre.json | 25 ++ ...filled-in-deferred-empty-env-var-post.json | 20 ++ .../filled-in-deferred-empty-env-var-pre.json | 18 ++ .../filled-in-deferred-no-env-var-post.json | 20 ++ .../filled-in-deferred-no-env-var-pre.json | 17 ++ .../invariants/filled-in-idempotent.json | 20 ++ .../external-formats.cc} | 45 +-- src/libstore-tests/derivation/invariants.cc | 264 ++++++++++++++++++ src/libstore-tests/derivation/test-support.hh | 52 ++++ src/libstore-tests/meson.build | 3 +- src/libstore/derivations.cc | 225 +++++++++++---- src/libstore/include/nix/store/derivations.hh | 60 ++++ .../nix/util/tests/characterization.hh | 4 +- .../nix/util/tests/json-characterization.hh | 29 ++ src/nix/derivation-add.cc | 4 +- tests/functional/derivation-json.sh | 17 +- 21 files changed, 785 insertions(+), 134 deletions(-) create mode 100644 src/libstore-tests/data/derivation/invariants/bad-depends-on-drv-pre.json create mode 100644 src/libstore-tests/data/derivation/invariants/bad-env-var.json create mode 100644 src/libstore-tests/data/derivation/invariants/bad-path.json create mode 100644 src/libstore-tests/data/derivation/invariants/depends-on-drv-pre.json create mode 100644 src/libstore-tests/data/derivation/invariants/filled-in-deferred-empty-env-var-post.json create mode 100644 src/libstore-tests/data/derivation/invariants/filled-in-deferred-empty-env-var-pre.json create mode 100644 src/libstore-tests/data/derivation/invariants/filled-in-deferred-no-env-var-post.json create mode 100644 src/libstore-tests/data/derivation/invariants/filled-in-deferred-no-env-var-pre.json create mode 100644 src/libstore-tests/data/derivation/invariants/filled-in-idempotent.json rename src/libstore-tests/{derivation.cc => derivation/external-formats.cc} (91%) create mode 100644 src/libstore-tests/derivation/invariants.cc create mode 100644 src/libstore-tests/derivation/test-support.hh diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 98ed1b450..d548685f4 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1768,28 +1768,7 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName drv.outputs.insert_or_assign(i, DerivationOutput::Deferred{}); } - auto hashModulo = hashDerivationModulo(*state.store, Derivation(drv), true); - switch (hashModulo.kind) { - case DrvHash::Kind::Regular: - for (auto & i : outputs) { - auto h = get(hashModulo.hashes, i); - if (!h) - state.error("derivation produced no hash for output '%s'", i).atPos(v).debugThrow(); - auto outPath = state.store->makeOutputPath(i, *h, drvName); - drv.env[i] = state.store->printStorePath(outPath); - drv.outputs.insert_or_assign( - i, - DerivationOutput::InputAddressed{ - .path = std::move(outPath), - }); - } - break; - ; - case DrvHash::Kind::Deferred: - for (auto & i : outputs) { - drv.outputs.insert_or_assign(i, DerivationOutput::Deferred{}); - } - } + drv.fillInOutputPaths(*state.store); } /* Write the resulting term into the Nix store directory. */ diff --git a/src/libstore-c/nix_api_store.cc b/src/libstore-c/nix_api_store.cc index 024ed9785..b95e5b749 100644 --- a/src/libstore-c/nix_api_store.cc +++ b/src/libstore-c/nix_api_store.cc @@ -223,13 +223,7 @@ nix_derivation * nix_derivation_from_json(nix_c_context * context, Store * store if (context) context->last_err_code = NIX_OK; try { - auto drv = static_cast(nlohmann::json::parse(json)); - - auto drvPath = nix::writeDerivation(*store->ptr, drv, nix::NoRepair, /* read only */ true); - - drv.checkInvariants(*store->ptr, drvPath); - - return new nix_derivation{drv}; + return new nix_derivation{nix::Derivation::parseJsonAndValidate(*store->ptr, nlohmann::json::parse(json))}; } NIXC_CATCH_ERRS_NULL } diff --git a/src/libstore-tests/data/derivation/invariants/bad-depends-on-drv-pre.json b/src/libstore-tests/data/derivation/invariants/bad-depends-on-drv-pre.json new file mode 100644 index 000000000..8454cf548 --- /dev/null +++ b/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 +} diff --git a/src/libstore-tests/data/derivation/invariants/bad-env-var.json b/src/libstore-tests/data/derivation/invariants/bad-env-var.json new file mode 100644 index 000000000..cb0c9492f --- /dev/null +++ b/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 +} diff --git a/src/libstore-tests/data/derivation/invariants/bad-path.json b/src/libstore-tests/data/derivation/invariants/bad-path.json new file mode 100644 index 000000000..688f2d4e6 --- /dev/null +++ b/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 +} diff --git a/src/libstore-tests/data/derivation/invariants/depends-on-drv-pre.json b/src/libstore-tests/data/derivation/invariants/depends-on-drv-pre.json new file mode 100644 index 000000000..d782cc756 --- /dev/null +++ b/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 +} diff --git a/src/libstore-tests/data/derivation/invariants/filled-in-deferred-empty-env-var-post.json b/src/libstore-tests/data/derivation/invariants/filled-in-deferred-empty-env-var-post.json new file mode 100644 index 000000000..c5abdf692 --- /dev/null +++ b/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 +} diff --git a/src/libstore-tests/data/derivation/invariants/filled-in-deferred-empty-env-var-pre.json b/src/libstore-tests/data/derivation/invariants/filled-in-deferred-empty-env-var-pre.json new file mode 100644 index 000000000..bc5338925 --- /dev/null +++ b/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 +} diff --git a/src/libstore-tests/data/derivation/invariants/filled-in-deferred-no-env-var-post.json b/src/libstore-tests/data/derivation/invariants/filled-in-deferred-no-env-var-post.json new file mode 100644 index 000000000..709d7bca0 --- /dev/null +++ b/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 +} diff --git a/src/libstore-tests/data/derivation/invariants/filled-in-deferred-no-env-var-pre.json b/src/libstore-tests/data/derivation/invariants/filled-in-deferred-no-env-var-pre.json new file mode 100644 index 000000000..194e33086 --- /dev/null +++ b/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 +} diff --git a/src/libstore-tests/data/derivation/invariants/filled-in-idempotent.json b/src/libstore-tests/data/derivation/invariants/filled-in-idempotent.json new file mode 100644 index 000000000..9b99fb812 --- /dev/null +++ b/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 +} diff --git a/src/libstore-tests/derivation.cc b/src/libstore-tests/derivation/external-formats.cc similarity index 91% rename from src/libstore-tests/derivation.cc rename to src/libstore-tests/derivation/external-formats.cc index 6b33e5442..056eeaa8a 100644 --- a/src/libstore-tests/derivation.cc +++ b/src/libstore-tests/derivation/external-formats.cc @@ -1,57 +1,14 @@ #include #include -#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( diff --git a/src/libstore-tests/derivation/invariants.cc b/src/libstore-tests/derivation/invariants.cc new file mode 100644 index 000000000..6d7cee968 --- /dev/null +++ b/src/libstore-tests/derivation/invariants.cc @@ -0,0 +1,264 @@ +#include +#include + +#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 +{ + std::filesystem::path unitTestData = getUnitTestData() / "derivation" / "invariants"; + +protected: + FillInOutputPathsTest() + : LibStoreTest([]() { + auto config = make_ref(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(&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(&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 diff --git a/src/libstore-tests/derivation/test-support.hh b/src/libstore-tests/derivation/test-support.hh new file mode 100644 index 000000000..f48e6caef --- /dev/null +++ b/src/libstore-tests/derivation/test-support.hh @@ -0,0 +1,52 @@ +#pragma once + +#include + +#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 diff --git a/src/libstore-tests/meson.build b/src/libstore-tests/meson.build index f76df8bcb..58f624611 100644 --- a/src/libstore-tests/meson.build +++ b/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', diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index f96109a6c..3108bb3dc 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -1210,6 +1210,100 @@ std::optional Derivation::tryResolve( return resolved; } +/** + * Process `InputAddressed`, `Deferred`, and `CAFixed` outputs. + * + * For `InputAddressed` outputs or `Deferred` outputs: + * + * - with `Regular` hash kind, validate `InputAddressed` outputs have the + * correct path (throws if mismatch), invoke `deferredCallback` for + * `Deferred` outputs with the computed output path, then invoke + * `envCallback` with the path. + * + * - with `Deferred` hash kind, validate that the output is either + * `InputAddressed` (error) or `Deferred` (correct). + * + * For `CAFixed` outputs, invoke `envCallback` with the computed path. + */ +static void processForOutputPaths( + Store & store, auto && drv, std::string_view drvName, auto && deferredCallback, auto && envCallback) +{ + std::optional hashesModulo; + + for (auto & [outputName, output] : drv.outputs) { + auto envHasRightPath = [&](const StorePath & actual) { + envCallback(outputName, drv.env.find(outputName), actual); + }; + auto hash = [&] { + if (!hashesModulo) { + // somewhat expensive so we do lazily + hashesModulo = hashDerivationModulo(store, drv, true); + } + switch (hashesModulo->kind) { + case DrvHash::Kind::Regular: { + auto h = get(hashesModulo->hashes, outputName); + if (!h) + throw Error("derivation produced no hash for output '%s'", outputName); + auto outPath = store.makeOutputPath(outputName, *h, drvName); + std::visit( + overloaded{ + [&](const DerivationOutput::InputAddressed & doia) { + if (doia.path == outPath) { + return; // Correct case + } + // Error case, an explicilty wrong path is always an error. + throw Error( + "derivation has incorrect output '%s', should be '%s'", + store.printStorePath(doia.path), + store.printStorePath(outPath)); + }, + [&](const DerivationOutput::Deferred &) { + // The callback will either fill in the right path or throw an error + deferredCallback(outputName, output, outPath); + }, + [&](const auto &) { + // should never happen, based on where `hash` is called. + unreachable(); + }, + }, + output.raw); + envHasRightPath(outPath); + break; + } + case DrvHash::Kind::Deferred: + std::visit( + overloaded{ + [&](const DerivationOutput::InputAddressed & doia) { + // Error case, an explicilty wrong path is always an error. + throw Error( + "derivation has incorrect output '%s', should be deferred", + store.printStorePath(doia.path)); + }, + [&](const DerivationOutput::Deferred &) { + // Correct: Deferred output with Deferred hash kind + }, + [&](const auto &) { + // should never happen, based on where `hash` is called. + unreachable(); + }, + }, + output.raw); + break; + } + }; + std::visit( + overloaded{ + [&](const DerivationOutput::InputAddressed &) { hash(); }, + [&](const DerivationOutput::Deferred &) { hash(); }, + [&](const DerivationOutput::CAFixed & dof) { envHasRightPath(dof.path(store, drvName, outputName)); }, + [&](const auto &) { + // Nothing to do for other output types + }, + }, + output.raw); + } +} + void Derivation::checkInvariants(Store & store, const StorePath & drvPath) const { assert(drvPath.isDerivation()); @@ -1217,65 +1311,96 @@ void Derivation::checkInvariants(Store & store, const StorePath & drvPath) const drvName = drvName.substr(0, drvName.size() - drvExtension.size()); if (drvName != name) { - throw Error("Derivation '%s' has name '%s' which does not match its path", store.printStorePath(drvPath), name); + throw Error("derivation '%s' has name '%s' which does not match its path", store.printStorePath(drvPath), name); } - auto envHasRightPath = [&](const StorePath & actual, const std::string & varName) { - auto j = env.find(varName); - if (j == env.end() || store.parseStorePath(j->second) != actual) - throw Error( - "derivation '%s' has incorrect environment variable '%s', should be '%s'", - store.printStorePath(drvPath), - varName, - store.printStorePath(actual)); - }; + try { + checkInvariants(store); + } catch (Error & e) { + e.addTrace({}, "while checking derivation '%s'", store.printStorePath(drvPath)); + throw; + } +} +void Derivation::checkInvariants(Store & store) const +{ // Don't need the answer, but do this anyways to assert is proper // combination. The code below is more general and naturally allows // combinations that are currently prohibited. type(); - std::optional hashesModulo; - for (auto & i : outputs) { - std::visit( - overloaded{ - [&](const DerivationOutput::InputAddressed & doia) { - if (!hashesModulo) { - // somewhat expensive so we do lazily - hashesModulo = hashDerivationModulo(store, *this, true); - } - auto currentOutputHash = get(hashesModulo->hashes, i.first); - if (!currentOutputHash) - throw Error( - "derivation '%s' has unexpected output '%s' (local-store / hashesModulo) named '%s'", - store.printStorePath(drvPath), - store.printStorePath(doia.path), - i.first); - StorePath recomputed = store.makeOutputPath(i.first, *currentOutputHash, drvName); - if (doia.path != recomputed) - throw Error( - "derivation '%s' has incorrect output '%s', should be '%s'", - store.printStorePath(drvPath), - store.printStorePath(doia.path), - store.printStorePath(recomputed)); - envHasRightPath(doia.path, i.first); - }, - [&](const DerivationOutput::CAFixed & dof) { - auto path = dof.path(store, drvName, i.first); - envHasRightPath(path, i.first); - }, - [&](const DerivationOutput::CAFloating &) { - /* Nothing to check */ - }, - [&](const DerivationOutput::Deferred &) { - /* Nothing to check */ - }, - [&](const DerivationOutput::Impure &) { - /* Nothing to check */ - }, - }, - i.second.raw); + processForOutputPaths( + store, + *this, + name, + [&](const std::string & outputName, const DerivationOutput & output, const StorePath & outPath) { + throw Error("derivation has incorrect deferred output, should be '%s'", store.printStorePath(outPath)); + }, + [&](const std::string & outputName, decltype(env)::const_iterator j, const StorePath & actual) { + if (j == env.end()) + throw Error( + "derivation has missing environment variable '%s', should be '%s' but is not present", + outputName, + store.printStorePath(actual)); + if (j->second != store.printStorePath(actual)) + throw Error( + "derivation has incorrect environment variable '%s', should be '%s' but is actually '%s'", + outputName, + store.printStorePath(actual), + j->second); + }); +} + +void Derivation::fillInOutputPaths(Store & store) +{ + processForOutputPaths( + store, + *this, + name, + [&](const std::string & outputName, DerivationOutput & output, StorePath outPath) { + // Fill in output path for Deferred outputs + output = DerivationOutput::InputAddressed{ + .path = std::move(outPath), + }; + }, + [&](const std::string & outputName, decltype(env)::iterator j, const StorePath & actual) { + if (j == env.end()) { + // fill it in + env.insert(j, {outputName, store.printStorePath(actual)}); + return; + } + auto & value = j->second; + if (value == "") { + // fill it in + j->second = store.printStorePath(actual); + return; + } + if (j->second == store.printStorePath(actual)) { + // do nothing, already filled in + return; + } + throw Error( + "derivation has incorrect environment variable '%s', should be '%s' but is actually '%s'", + outputName, + store.printStorePath(actual), + value); + }); +} + +Derivation Derivation::parseJsonAndValidate(Store & store, const nlohmann::json & json) +{ + auto drv = static_cast(json); + + drv.fillInOutputPaths(store); + + try { + drv.checkInvariants(store); + } catch (Error & e) { + e.addTrace({}, "while checking derivation from JSON with name '%s'", drv.name); + throw; } + + return drv; } const Hash impureOutputHash = hashString(HashAlgorithm::SHA256, "impure"); diff --git a/src/libstore/include/nix/store/derivations.hh b/src/libstore/include/nix/store/derivations.hh index 259314d3f..21bda2481 100644 --- a/src/libstore/include/nix/store/derivations.hh +++ b/src/libstore/include/nix/store/derivations.hh @@ -368,9 +368,46 @@ struct Derivation : BasicDerivation * This is mainly a matter of checking the outputs, where our C++ * representation supports all sorts of combinations we do not yet * allow. + * + * This overload does not validate the derivation name or add path + * context to errors. Use this when you don't have a `StorePath` or + * when you want to handle error context yourself. + * + * @param store The store to use for validation + */ + void checkInvariants(Store & store) const; + + /** + * This overload does everything the base `checkInvariants` does, + * but also validates that the derivation name matches the path, and + * improves any error messages that occur using the derivation path. + * + * @param store The store to use for validation + * @param drvPath The path to this derivation */ void checkInvariants(Store & store, const StorePath & drvPath) const; + /** + * Fill in output paths as needed. + * + * For input-addressed derivations (ready or deferred), it computes + * the derivation hash modulo and based on the result: + * + * - If `Regular`: converts `Deferred` outputs to `InputAddressed`, and + * ensures all `InputAddressed` (exist or just et) have he right + * computed paths. Also updates the environment variables with + * output paths. + * + * - If `Deferred`: converts `InputAddressed` to `Deferred`. + * + * Also for fixed-output content-addressed derivations, likewise + * updates output paths in env vars. + * + * @param store The store to use for path computation + * @param drvName The derivation name (without .drv extension) + */ + void fillInOutputPaths(Store & store); + Derivation() = default; Derivation(const BasicDerivation & bd) @@ -383,6 +420,29 @@ struct Derivation : BasicDerivation { } + /** + * Parse a derivation from JSON, and also perform various + * conveniences such as: + * + * 1. Filling in output paths in as needed/required. + * + * 2. Checking invariants in general. + * + * In the future it might also do things like: + * + * - assist with the migration from older JSON formats. + * + * - (a somewhat exmaple of the above) initialize + * `DerivationOptions` from their traditional encoding inside the + * `env` and `structuredAttrs`. + * + * @param store The store to use for path computation and validation + * @param json The JSON representation of the derivation + * @return A validated derivation with output paths filled in + * @throws Error if parsing fails, output paths can't be computed, or validation fails + */ + static Derivation parseJsonAndValidate(Store & store, const nlohmann::json & json); + bool operator==(const Derivation &) const = default; // TODO libc++ 16 (used by darwin) missing `std::map::operator <=>`, can't do yet. // auto operator <=> (const Derivation &) const = default; diff --git a/src/libutil-test-support/include/nix/util/tests/characterization.hh b/src/libutil-test-support/include/nix/util/tests/characterization.hh index 0434590f7..d8fad1df9 100644 --- a/src/libutil-test-support/include/nix/util/tests/characterization.hh +++ b/src/libutil-test-support/include/nix/util/tests/characterization.hh @@ -31,16 +31,14 @@ static inline bool testAccept() /** * Mixin class for writing characterization tests */ -class CharacterizationTest : public virtual ::testing::Test +struct CharacterizationTest : virtual ::testing::Test { -protected: /** * While the "golden master" for this characterization test is * located. It should not be shared with any other test. */ virtual std::filesystem::path goldenMaster(PathView testStem) const = 0; -public: /** * Golden test for reading * diff --git a/src/libutil-test-support/include/nix/util/tests/json-characterization.hh b/src/libutil-test-support/include/nix/util/tests/json-characterization.hh index 0ee6fd2fd..bd70ba830 100644 --- a/src/libutil-test-support/include/nix/util/tests/json-characterization.hh +++ b/src/libutil-test-support/include/nix/util/tests/json-characterization.hh @@ -39,6 +39,30 @@ void writeJsonTest(CharacterizationTest & test, PathView testStem, const T & val [](const auto & file, const auto & got) { return writeFile(file, got.dump(2) + "\n"); }); } +/** + * Golden test in the middle of something + */ +template +void checkpointJson(CharacterizationTest & test, PathView testStem, const T & got) +{ + using namespace nlohmann; + + auto file = test.goldenMaster(Path{testStem} + ".json"); + + json gotJson = static_cast(got); + + if (testAccept()) { + std::filesystem::create_directories(file.parent_path()); + writeFile(file, gotJson.dump(2) + "\n"); + ADD_FAILURE() << "Updating golden master " << file; + } else { + json expectedJson = json::parse(readFile(file)); + ASSERT_EQ(gotJson, expectedJson); + T expected = adl_serializer::from_json(expectedJson); + ASSERT_EQ(got, expected); + } +} + /** * Mixin class for writing characterization tests for `nlohmann::json` * conversions for a given type. @@ -67,6 +91,11 @@ struct JsonCharacterizationTest : virtual CharacterizationTest { nix::writeJsonTest(*this, testStem, value); } + + void checkpointJson(PathView testStem, const T & value) + { + nix::checkpointJson(*this, testStem, value); + } }; } // namespace nix diff --git a/src/nix/derivation-add.cc b/src/nix/derivation-add.cc index 48e935092..bbaa87597 100644 --- a/src/nix/derivation-add.cc +++ b/src/nix/derivation-add.cc @@ -33,12 +33,10 @@ struct CmdAddDerivation : MixDryRun, StoreCommand { auto json = nlohmann::json::parse(drainFD(STDIN_FILENO)); - auto drv = static_cast(json); + auto drv = Derivation::parseJsonAndValidate(*store, json); auto drvPath = writeDerivation(*store, drv, NoRepair, /* read only */ dryRun); - drv.checkInvariants(*store, drvPath); - writeDerivation(*store, drv, NoRepair, dryRun); logger->cout("%s", store->printStorePath(drvPath)); diff --git a/tests/functional/derivation-json.sh b/tests/functional/derivation-json.sh index 06f934cfe..d07776e90 100755 --- a/tests/functional/derivation-json.sh +++ b/tests/functional/derivation-json.sh @@ -4,11 +4,20 @@ source common.sh drvPath=$(nix-instantiate simple.nix) -nix derivation show "$drvPath" | jq .[] > "$TEST_HOME"/simple.json - -drvPath2=$(nix derivation add < "$TEST_HOME"/simple.json) +nix derivation show "$drvPath" | jq '.[]' > "$TEST_HOME/simple.json" +# Round tripping to JSON works +drvPath2=$(nix derivation add < "$TEST_HOME/simple.json") [[ "$drvPath" = "$drvPath2" ]] +# Derivaiton is input addressed, all outputs have a path +jq -e '.outputs | .[] | has("path")' < "$TEST_HOME/simple.json" + # Input addressed derivations cannot be renamed. -jq '.name = "foo"' < "$TEST_HOME"/simple.json | expectStderr 1 nix derivation add | grepQuiet "has incorrect output" +jq '.name = "foo"' < "$TEST_HOME/simple.json" | expectStderr 1 nix derivation add | grepQuiet "has incorrect output" + +# If we remove the input addressed to make it a deferred derivation, we +# still get the same result because Nix will see that need not be +# deferred and fill in the right input address for us. +drvPath3=$(jq '.outputs |= map_values(del(.path))' < "$TEST_HOME/simple.json" | nix derivation add) +[[ "$drvPath" = "$drvPath3" ]]