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 <roberth@users.noreply.github.com>
This commit is contained in:
John Ericson 2021-03-10 04:22:56 +00:00
parent a04720e68c
commit e7c39ff00b
6 changed files with 122 additions and 71 deletions

View file

@ -1027,24 +1027,67 @@ void EvalState::mkStorePathString(const StorePath & p, Value & v)
} }
std::string EvalState::mkOutputStringRaw(
const SingleDerivedPath::Built & b,
std::optional<StorePath> 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( void EvalState::mkOutputString(
Value & value, Value & value,
const StorePath & drvPath, const SingleDerivedPath::Built & b,
const std::string outputName,
std::optional<StorePath> optStaticOutputPath, std::optional<StorePath> optStaticOutputPath,
const ExperimentalFeatureSettings & xpSettings) const ExperimentalFeatureSettings & xpSettings)
{ {
value.mkString( value.mkString(
optStaticOutputPath mkOutputStringRaw(b, optStaticOutputPath, xpSettings),
? store->printStorePath(*std::move(optStaticOutputPath)) NixStringContext { b });
/* Downstream we would substitute this for an actual path once }
we build the floating CA derivation */
: DownstreamPlaceholder::unknownCaOutput(drvPath, outputName, xpSettings).render(),
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<StorePath> {
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 { NixStringContext {
NixStringContextElem::Built { std::visit([](auto && v) -> NixStringContextElem { return v; }, p),
.drvPath = makeConstantStorePathRef(drvPath),
.output = outputName,
}
}); });
} }
@ -2333,39 +2376,25 @@ SingleDerivedPath EvalState::coerceToSingleDerivedPath(const PosIdx pos, Value &
{ {
auto [derivedPath, s_] = coerceToSingleDerivedPathUnchecked(pos, v, errorCtx); auto [derivedPath, s_] = coerceToSingleDerivedPathUnchecked(pos, v, errorCtx);
auto s = s_; auto s = s_;
std::visit(overloaded { auto sExpected = mkSingleDerivedPathStringRaw(derivedPath);
[&](const SingleDerivedPath::Opaque & o) { if (s != sExpected) {
auto sExpected = store->printStorePath(o.path); /* `std::visit` is used here just to provide a more precise
if (s != sExpected) error message. */
std::visit(overloaded {
[&](const SingleDerivedPath::Opaque & o) {
error( error(
"path string '%s' has context with the different path '%s'", "path string '%s' has context with the different path '%s'",
s, sExpected) s, sExpected)
.withTrace(pos, errorCtx).debugThrow<EvalError>(); .withTrace(pos, errorCtx).debugThrow<EvalError>();
}, },
[&](const SingleDerivedPath::Built & b) { [&](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)
error( 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'", "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) s, b.output, b.drvPath->to_string(*store), sExpected)
.withTrace(pos, errorCtx).debugThrow<EvalError>(); .withTrace(pos, errorCtx).debugThrow<EvalError>();
} }
}, derivedPath.raw()); }, derivedPath.raw());
}
return derivedPath; return derivedPath;
} }

View file

@ -668,37 +668,46 @@ public:
/** /**
* Create a string representing a store path. * Create a string representing a store path.
* *
* The string is the printed store path with a context containing a single * The string is the printed store path with a context containing a
* `NixStringContextElem::Opaque` element of that store path. * single `NixStringContextElem::Opaque` element of that store path.
*/ */
void mkStorePathString(const StorePath & storePath, Value & v); 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 * The string is the printed store path with a context containing a
* `NixStringContextElem::Built` element of the drv path and output name. * single `NixStringContextElem::Built` element of the drv path and
* output name.
* *
* @param value Value we are settings * @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
* @param optStaticOutputPath Optional output path for that string. Must * input-addressed or fixed output. Will be printed to form string
* be passed if and only if output store object is input-addressed. * if passed, otherwise a placeholder will be used (see
* Will be printed to form string if passed, otherwise a placeholder * `DownstreamPlaceholder`).
* will be used (see `DownstreamPlaceholder`).
* *
* @param xpSettings Stop-gap to avoid globals during unit tests. * @param xpSettings Stop-gap to avoid globals during unit tests.
*/ */
void mkOutputString( void mkOutputString(
Value & value, Value & value,
const StorePath & drvPath, const SingleDerivedPath::Built & b,
const std::string outputName,
std::optional<StorePath> optStaticOutputPath, std::optional<StorePath> optStaticOutputPath,
const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings); 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); void concatLists(Value & v, size_t nrLists, Value * * lists, const PosIdx pos, std::string_view errorCtx);
/** /**
@ -714,6 +723,22 @@ public:
private: 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<StorePath> 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 nrEnvs = 0;
unsigned long nrValuesInEnvs = 0; unsigned long nrValuesInEnvs = 0;
unsigned long nrValues = 0; unsigned long nrValues = 0;

View file

@ -156,8 +156,10 @@ static void mkOutputString(
{ {
state.mkOutputString( state.mkOutputString(
attrs.alloc(o.first), attrs.alloc(o.first),
drvPath, SingleDerivedPath::Built {
o.first, .drvPath = makeConstantStorePathRef(drvPath),
.output = o.first,
},
o.second.path(*state.store, Derivation::nameFromPath(drvPath), o.first)); o.second.path(*state.store, Derivation::nameFromPath(drvPath), o.first));
} }

View file

@ -34,8 +34,8 @@ RC_GTEST_FIXTURE_PROP(
RC_GTEST_FIXTURE_PROP( RC_GTEST_FIXTURE_PROP(
DerivedPathExpressionTest, DerivedPathExpressionTest,
prop_built_path_placeholder_round_trip, prop_derived_path_built_placeholder_round_trip,
(const StorePath & drvPath, const StorePathName & outputName)) (const SingleDerivedPath::Built & b))
{ {
/** /**
* We set these in tests rather than the regular globals so we don't have * 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"); mockXpSettings.set("experimental-features", "ca-derivations");
auto * v = state.allocValue(); 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, ""); auto [d, _] = state.coerceToSingleDerivedPathUnchecked(noPos, *v, "");
SingleDerivedPath::Built b {
.drvPath = makeConstantStorePathRef(drvPath),
.output = outputName.name,
};
RC_ASSERT(SingleDerivedPath { b } == d); RC_ASSERT(SingleDerivedPath { b } == d);
} }
RC_GTEST_FIXTURE_PROP( RC_GTEST_FIXTURE_PROP(
DerivedPathExpressionTest, DerivedPathExpressionTest,
prop_built_path_out_path_round_trip, prop_derived_path_built_out_path_round_trip,
(const StorePath & drvPath, const StorePathName & outputName, const StorePath & outPath)) (const SingleDerivedPath::Built & b, const StorePath & outPath))
{ {
auto * v = state.allocValue(); auto * v = state.allocValue();
state.mkOutputString(*v, drvPath, outputName.name, outPath); state.mkOutputString(*v, b, outPath);
auto [d, _] = state.coerceToSingleDerivedPathUnchecked(noPos, *v, ""); auto [d, _] = state.coerceToSingleDerivedPathUnchecked(noPos, *v, "");
SingleDerivedPath::Built b {
.drvPath = makeConstantStorePathRef(drvPath),
.output = outputName.name,
};
RC_ASSERT(SingleDerivedPath { b } == d); RC_ASSERT(SingleDerivedPath { b } == d);
} }

View file

@ -39,16 +39,18 @@ DownstreamPlaceholder DownstreamPlaceholder::unknownDerivation(
} }
DownstreamPlaceholder DownstreamPlaceholder::fromSingleDerivedPathBuilt( DownstreamPlaceholder DownstreamPlaceholder::fromSingleDerivedPathBuilt(
const SingleDerivedPath::Built & b) const SingleDerivedPath::Built & b,
const ExperimentalFeatureSettings & xpSettings)
{ {
return std::visit(overloaded { return std::visit(overloaded {
[&](const SingleDerivedPath::Opaque & o) { [&](const SingleDerivedPath::Opaque & o) {
return DownstreamPlaceholder::unknownCaOutput(o.path, b.output); return DownstreamPlaceholder::unknownCaOutput(o.path, b.output, xpSettings);
}, },
[&](const SingleDerivedPath::Built & b2) { [&](const SingleDerivedPath::Built & b2) {
return DownstreamPlaceholder::unknownDerivation( return DownstreamPlaceholder::unknownDerivation(
DownstreamPlaceholder::fromSingleDerivedPathBuilt(b2), DownstreamPlaceholder::fromSingleDerivedPathBuilt(b2, xpSettings),
b.output); b.output,
xpSettings);
}, },
}, b.drvPath->raw()); }, b.drvPath->raw());
} }

View file

@ -84,7 +84,8 @@ public:
* `SingleDerivedPath::Built.drvPath` chain. * `SingleDerivedPath::Built.drvPath` chain.
*/ */
static DownstreamPlaceholder fromSingleDerivedPathBuilt( static DownstreamPlaceholder fromSingleDerivedPathBuilt(
const SingleDerivedPath::Built & built); const SingleDerivedPath::Built & built,
const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings);
}; };
} }