From e7c39ff00b81dfdc48ebe7f41847db84ba779676 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 10 Mar 2021 04:22:56 +0000 Subject: [PATCH] Rework evaluator `SingleDerivedPath` infra `EvalState::mkSingleDerivedPathString` previously contained its own inverse (printing, rather than parsing) in order to validate what was parsed. Now that is pulled out into its own separate function: `EvalState::coerceToSingleDerivedPath`. In additional that pulled out logic is deduplicated with `EvalState::mkOutputString` via `EvalState::mkOutputStringRaw`, which is itself deduplicated (and generalized) with `DownstreamPlaceholder::mkOutputStringRaw`. All these changes make the unit tests simpler. (We would ideally write more unit tests for `mkSingleDerivedPathString` `coerceToSingleDerivedPath` directly, but we cannot yet do that because the IO in reading the store path won't work when the dummy store cannot hold anything. Someday we'll have a proper in-memory store which will work for this.) Co-authored-by: Robert Hensing --- src/libexpr/eval.cc | 101 ++++++++++++++++--------- src/libexpr/eval.hh | 53 +++++++++---- src/libexpr/primops.cc | 6 +- src/libexpr/tests/derived-path.cc | 20 ++--- src/libstore/downstream-placeholder.cc | 10 ++- src/libstore/downstream-placeholder.hh | 3 +- 6 files changed, 122 insertions(+), 71 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 063e34c56..79e2c4727 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1027,24 +1027,67 @@ void EvalState::mkStorePathString(const StorePath & p, Value & v) } +std::string EvalState::mkOutputStringRaw( + const SingleDerivedPath::Built & b, + std::optional optStaticOutputPath, + const ExperimentalFeatureSettings & xpSettings) +{ + /* In practice, this is testing for the case of CA derivations, or + dynamic derivations. */ + return optStaticOutputPath + ? store->printStorePath(*std::move(optStaticOutputPath)) + /* Downstream we would substitute this for an actual path once + we build the floating CA derivation */ + : DownstreamPlaceholder::fromSingleDerivedPathBuilt(b, xpSettings).render(); +} + + void EvalState::mkOutputString( Value & value, - const StorePath & drvPath, - const std::string outputName, + const SingleDerivedPath::Built & b, std::optional optStaticOutputPath, const ExperimentalFeatureSettings & xpSettings) { value.mkString( - optStaticOutputPath - ? store->printStorePath(*std::move(optStaticOutputPath)) - /* Downstream we would substitute this for an actual path once - we build the floating CA derivation */ - : DownstreamPlaceholder::unknownCaOutput(drvPath, outputName, xpSettings).render(), + mkOutputStringRaw(b, optStaticOutputPath, xpSettings), + NixStringContext { b }); +} + + +std::string EvalState::mkSingleDerivedPathStringRaw( + const SingleDerivedPath & p) +{ + return std::visit(overloaded { + [&](const SingleDerivedPath::Opaque & o) { + return store->printStorePath(o.path); + }, + [&](const SingleDerivedPath::Built & b) { + auto optStaticOutputPath = std::visit(overloaded { + [&](const SingleDerivedPath::Opaque & o) { + auto drv = store->readDerivation(o.path); + auto i = drv.outputs.find(b.output); + if (i == drv.outputs.end()) + throw Error("derivation '%s' does not have output '%s'", b.drvPath->to_string(*store), b.output); + return i->second.path(*store, drv.name, b.output); + }, + [&](const SingleDerivedPath::Built & o) -> std::optional { + return std::nullopt; + }, + }, b.drvPath->raw()); + return mkOutputStringRaw(b, optStaticOutputPath); + } + }, p.raw()); +} + + +void EvalState::mkSingleDerivedPathString( + const SingleDerivedPath & p, + Value & v) +{ + v.mkString( + mkSingleDerivedPathStringRaw(p), NixStringContext { - NixStringContextElem::Built { - .drvPath = makeConstantStorePathRef(drvPath), - .output = outputName, - } + std::visit([](auto && v) -> NixStringContextElem { return v; }, p), }); } @@ -2333,39 +2376,25 @@ SingleDerivedPath EvalState::coerceToSingleDerivedPath(const PosIdx pos, Value & { auto [derivedPath, s_] = coerceToSingleDerivedPathUnchecked(pos, v, errorCtx); auto s = s_; - std::visit(overloaded { - [&](const SingleDerivedPath::Opaque & o) { - auto sExpected = store->printStorePath(o.path); - if (s != sExpected) + auto sExpected = mkSingleDerivedPathStringRaw(derivedPath); + if (s != sExpected) { + /* `std::visit` is used here just to provide a more precise + error message. */ + std::visit(overloaded { + [&](const SingleDerivedPath::Opaque & o) { error( "path string '%s' has context with the different path '%s'", s, sExpected) .withTrace(pos, errorCtx).debugThrow(); - }, - [&](const SingleDerivedPath::Built & b) { - auto sExpected = std::visit(overloaded { - [&](const SingleDerivedPath::Opaque & o) { - auto drv = store->readDerivation(o.path); - auto i = drv.outputs.find(b.output); - if (i == drv.outputs.end()) - throw Error("derivation '%s' does not have output '%s'", b.drvPath->to_string(*store), b.output); - auto optStaticOutputPath = i->second.path(*store, drv.name, b.output); - // This is testing for the case of CA derivations - return optStaticOutputPath - ? store->printStorePath(*optStaticOutputPath) - : DownstreamPlaceholder::fromSingleDerivedPathBuilt(b).render(); - }, - [&](const SingleDerivedPath::Built & o) { - return DownstreamPlaceholder::fromSingleDerivedPathBuilt(b).render(); - }, - }, b.drvPath->raw()); - if (s != sExpected) + }, + [&](const SingleDerivedPath::Built & b) { error( "string '%s' has context with the output '%s' from derivation '%s', but the string is not the right placeholder for this derivation output. It should be '%s'", s, b.output, b.drvPath->to_string(*store), sExpected) .withTrace(pos, errorCtx).debugThrow(); - } - }, derivedPath.raw()); + } + }, derivedPath.raw()); + } return derivedPath; } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 662c123b9..0c3eb6505 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -668,37 +668,46 @@ public: /** * Create a string representing a store path. * - * The string is the printed store path with a context containing a single - * `NixStringContextElem::Opaque` element of that store path. + * The string is the printed store path with a context containing a + * single `NixStringContextElem::Opaque` element of that store path. */ void mkStorePathString(const StorePath & storePath, Value & v); /** - * Create a string representing a `DerivedPath::Built`. + * Create a string representing a `SingleDerivedPath::Built`. * - * The string is the printed store path with a context containing a single - * `NixStringContextElem::Built` element of the drv path and output name. + * The string is the printed store path with a context containing a + * single `NixStringContextElem::Built` element of the drv path and + * output name. * * @param value Value we are settings * - * @param drvPath Path the drv whose output we are making a string for + * @param b the drv whose output we are making a string for, and the + * output * - * @param outputName Name of the output - * - * @param optStaticOutputPath Optional output path for that string. Must - * be passed if and only if output store object is input-addressed. - * Will be printed to form string if passed, otherwise a placeholder - * will be used (see `DownstreamPlaceholder`). + * @param optStaticOutputPath Optional output path for that string. + * Must be passed if and only if output store object is + * input-addressed or fixed output. Will be printed to form string + * if passed, otherwise a placeholder will be used (see + * `DownstreamPlaceholder`). * * @param xpSettings Stop-gap to avoid globals during unit tests. */ void mkOutputString( Value & value, - const StorePath & drvPath, - const std::string outputName, + const SingleDerivedPath::Built & b, std::optional optStaticOutputPath, const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings); + /** + * Create a string representing a `SingleDerivedPath`. + * + * A combination of `mkStorePathString` and `mkOutputString`. + */ + void mkSingleDerivedPathString( + const SingleDerivedPath & p, + Value & v); + void concatLists(Value & v, size_t nrLists, Value * * lists, const PosIdx pos, std::string_view errorCtx); /** @@ -714,6 +723,22 @@ public: private: + /** + * Like `mkOutputString` but just creates a raw string, not an + * string Value, which would also have a string context. + */ + std::string mkOutputStringRaw( + const SingleDerivedPath::Built & b, + std::optional optStaticOutputPath, + const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings); + + /** + * Like `mkSingleDerivedPathStringRaw` but just creates a raw string + * Value, which would also have a string context. + */ + std::string mkSingleDerivedPathStringRaw( + const SingleDerivedPath & p); + unsigned long nrEnvs = 0; unsigned long nrValuesInEnvs = 0; unsigned long nrValues = 0; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 54943b481..be78bca02 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -156,8 +156,10 @@ static void mkOutputString( { state.mkOutputString( attrs.alloc(o.first), - drvPath, - o.first, + SingleDerivedPath::Built { + .drvPath = makeConstantStorePathRef(drvPath), + .output = o.first, + }, o.second.path(*state.store, Derivation::nameFromPath(drvPath), o.first)); } diff --git a/src/libexpr/tests/derived-path.cc b/src/libexpr/tests/derived-path.cc index 2a5ca64f6..d01735db8 100644 --- a/src/libexpr/tests/derived-path.cc +++ b/src/libexpr/tests/derived-path.cc @@ -34,8 +34,8 @@ RC_GTEST_FIXTURE_PROP( RC_GTEST_FIXTURE_PROP( DerivedPathExpressionTest, - prop_built_path_placeholder_round_trip, - (const StorePath & drvPath, const StorePathName & outputName)) + prop_derived_path_built_placeholder_round_trip, + (const SingleDerivedPath::Built & b)) { /** * We set these in tests rather than the regular globals so we don't have @@ -45,27 +45,19 @@ RC_GTEST_FIXTURE_PROP( mockXpSettings.set("experimental-features", "ca-derivations"); auto * v = state.allocValue(); - state.mkOutputString(*v, drvPath, outputName.name, std::nullopt, mockXpSettings); + state.mkOutputString(*v, b, std::nullopt, mockXpSettings); auto [d, _] = state.coerceToSingleDerivedPathUnchecked(noPos, *v, ""); - SingleDerivedPath::Built b { - .drvPath = makeConstantStorePathRef(drvPath), - .output = outputName.name, - }; RC_ASSERT(SingleDerivedPath { b } == d); } RC_GTEST_FIXTURE_PROP( DerivedPathExpressionTest, - prop_built_path_out_path_round_trip, - (const StorePath & drvPath, const StorePathName & outputName, const StorePath & outPath)) + prop_derived_path_built_out_path_round_trip, + (const SingleDerivedPath::Built & b, const StorePath & outPath)) { auto * v = state.allocValue(); - state.mkOutputString(*v, drvPath, outputName.name, outPath); + state.mkOutputString(*v, b, outPath); auto [d, _] = state.coerceToSingleDerivedPathUnchecked(noPos, *v, ""); - SingleDerivedPath::Built b { - .drvPath = makeConstantStorePathRef(drvPath), - .output = outputName.name, - }; RC_ASSERT(SingleDerivedPath { b } == d); } diff --git a/src/libstore/downstream-placeholder.cc b/src/libstore/downstream-placeholder.cc index 885b3604d..d951b7b7d 100644 --- a/src/libstore/downstream-placeholder.cc +++ b/src/libstore/downstream-placeholder.cc @@ -39,16 +39,18 @@ DownstreamPlaceholder DownstreamPlaceholder::unknownDerivation( } DownstreamPlaceholder DownstreamPlaceholder::fromSingleDerivedPathBuilt( - const SingleDerivedPath::Built & b) + const SingleDerivedPath::Built & b, + const ExperimentalFeatureSettings & xpSettings) { return std::visit(overloaded { [&](const SingleDerivedPath::Opaque & o) { - return DownstreamPlaceholder::unknownCaOutput(o.path, b.output); + return DownstreamPlaceholder::unknownCaOutput(o.path, b.output, xpSettings); }, [&](const SingleDerivedPath::Built & b2) { return DownstreamPlaceholder::unknownDerivation( - DownstreamPlaceholder::fromSingleDerivedPathBuilt(b2), - b.output); + DownstreamPlaceholder::fromSingleDerivedPathBuilt(b2, xpSettings), + b.output, + xpSettings); }, }, b.drvPath->raw()); } diff --git a/src/libstore/downstream-placeholder.hh b/src/libstore/downstream-placeholder.hh index 9372dcd58..d58a2ac14 100644 --- a/src/libstore/downstream-placeholder.hh +++ b/src/libstore/downstream-placeholder.hh @@ -84,7 +84,8 @@ public: * `SingleDerivedPath::Built.drvPath` chain. */ static DownstreamPlaceholder fromSingleDerivedPathBuilt( - const SingleDerivedPath::Built & built); + const SingleDerivedPath::Built & built, + const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings); }; }