From 9550c3862fb4bb021b977b47b5e710c0cdf6c74e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 30 Jan 2023 07:05:35 -0500 Subject: Remove unneeded argument for `mkOutputString` --- src/libexpr/primops.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 0be39fa7d..017aa25d1 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -145,10 +145,9 @@ static void mkOutputString( EvalState & state, BindingsBuilder & attrs, const StorePath & drvPath, - const BasicDerivation & drv, const std::pair & o) { - auto optOutputPath = o.second.path(*state.store, drv.name, o.first); + auto optOutputPath = o.second.path(*state.store, Derivation::nameFromPath(drvPath), o.first); attrs.alloc(o.first).mkString( optOutputPath ? state.store->printStorePath(*optOutputPath) @@ -193,7 +192,7 @@ static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * v state.mkList(outputsVal, drv.outputs.size()); for (const auto & [i, o] : enumerate(drv.outputs)) { - mkOutputString(state, attrs, *storePath, drv, o); + mkOutputString(state, attrs, *storePath, o); (outputsVal.listElems()[i] = state.allocValue())->mkString(o.first); } @@ -1405,7 +1404,7 @@ drvName, Bindings * attrs, Value & v) NixStringContextElem::DrvDeep { .drvPath = drvPath }, }); for (auto & i : drv.outputs) - mkOutputString(state, result, drvPath, drv, i); + mkOutputString(state, result, drvPath, i); v.mkAttrs(result); } -- cgit v1.2.3 From 0a9afce3b9191d71347a572a89e90167ba50c854 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 10 May 2023 19:07:33 -0400 Subject: Split `mkOutputString` in two This well help us with some unit testing --- src/libexpr/primops.cc | 59 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 21 deletions(-) (limited to 'src') diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 017aa25d1..336d756a0 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -129,41 +129,58 @@ static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, co } } -/* Add and attribute to the given attribute map from the output name to - the output path, or a placeholder. - - Where possible the path is used, but for floating CA derivations we - may not know it. For sake of determinism we always assume we don't - and instead put in a place holder. In either case, however, the - string context will contain the drv path and output name, so - downstream derivations will have the proper dependency, and in - addition, before building, the placeholder will be rewritten to be - the actual path. - - The 'drv' and 'drvPath' outputs must correspond. */ +/** + * Inverse of one of the `EvalState::coerceToDerivedPath()` cases. + */ static void mkOutputString( EvalState & state, - BindingsBuilder & attrs, + Value & value, const StorePath & drvPath, - const std::pair & o) + const std::string outputName, + std::optional optOutputPath) { - auto optOutputPath = o.second.path(*state.store, Derivation::nameFromPath(drvPath), o.first); - attrs.alloc(o.first).mkString( + value.mkString( optOutputPath - ? state.store->printStorePath(*optOutputPath) + ? state.store->printStorePath(*std::move(optOutputPath)) /* Downstream we would substitute this for an actual path once we build the floating CA derivation */ - /* FIXME: we need to depend on the basic derivation, not - derivation */ - : downstreamPlaceholder(*state.store, drvPath, o.first), + : downstreamPlaceholder(*state.store, drvPath, outputName), NixStringContext { NixStringContextElem::Built { .drvPath = drvPath, - .output = o.first, + .output = outputName, } }); } +/** + * Add and attribute to the given attribute map from the output name to + * the output path, or a placeholder. + * + * Where possible the path is used, but for floating CA derivations we + * may not know it. For sake of determinism we always assume we don't + * and instead put in a place holder. In either case, however, the + * string context will contain the drv path and output name, so + * downstream derivations will have the proper dependency, and in + * addition, before building, the placeholder will be rewritten to be + * the actual path. + * + * The 'drv' and 'drvPath' outputs must correspond. + */ +static void mkOutputString( + EvalState & state, + BindingsBuilder & attrs, + const StorePath & drvPath, + const std::pair & o) +{ + mkOutputString( + state, + attrs.alloc(o.first), + drvPath, + o.first, + o.second.path(*state.store, Derivation::nameFromPath(drvPath), o.first)); +} + /* Load and evaluate an expression from path specified by the argument. */ static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * vScope, Value & v) -- cgit v1.2.3 From 8e1a99026887ee836a68ae3e238fb0f2e1562447 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 10 May 2023 20:47:25 -0400 Subject: Expose `mkOutputString` as method of `EvalState` --- src/libexpr/eval.cc | 21 +++++++++++++++++++++ src/libexpr/eval.hh | 33 +++++++++++++++++++++++++++++---- src/libexpr/primops.cc | 27 +-------------------------- 3 files changed, 51 insertions(+), 30 deletions(-) (limited to 'src') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 0b4243670..679f46153 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1047,6 +1047,27 @@ void EvalState::mkStorePathString(const StorePath & p, Value & v) } +void EvalState::mkOutputString( + Value & value, + const StorePath & drvPath, + const std::string outputName, + std::optional optOutputPath) +{ + value.mkString( + optOutputPath + ? store->printStorePath(*std::move(optOutputPath)) + /* Downstream we would substitute this for an actual path once + we build the floating CA derivation */ + : downstreamPlaceholder(*store, drvPath, outputName), + NixStringContext { + NixStringContextElem::Built { + .drvPath = drvPath, + .output = outputName, + } + }); +} + + /* Create a thunk for the delayed computation of the given expression in the given environment. But if the expression is a variable, then look it up right away. This significantly reduces the number diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index bb3ac2b22..3b2164a19 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -576,12 +576,37 @@ public: void mkThunk_(Value & v, Expr * expr); void mkPos(Value & v, PosIdx pos); - /* Create a string representing a store path. - - The string is the printed store path with a context containing a single - `Opaque` element of that store path. */ + /** + * 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. + */ void mkStorePathString(const StorePath & storePath, Value & v); + /** + * Create a string representing a `DerivedPath::Built`. + * + * 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 outputName Name of the output + * + * @param optOutputPath 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()`). + */ + void mkOutputString( + Value & value, + const StorePath & drvPath, + const std::string outputName, + std::optional optOutputPath); + void concatLists(Value & v, size_t nrLists, Value * * lists, const PosIdx pos, std::string_view errorCtx); /** diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 336d756a0..6fbd66389 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -129,30 +129,6 @@ static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, co } } -/** - * Inverse of one of the `EvalState::coerceToDerivedPath()` cases. - */ -static void mkOutputString( - EvalState & state, - Value & value, - const StorePath & drvPath, - const std::string outputName, - std::optional optOutputPath) -{ - value.mkString( - optOutputPath - ? state.store->printStorePath(*std::move(optOutputPath)) - /* Downstream we would substitute this for an actual path once - we build the floating CA derivation */ - : downstreamPlaceholder(*state.store, drvPath, outputName), - NixStringContext { - NixStringContextElem::Built { - .drvPath = drvPath, - .output = outputName, - } - }); -} - /** * Add and attribute to the given attribute map from the output name to * the output path, or a placeholder. @@ -173,8 +149,7 @@ static void mkOutputString( const StorePath & drvPath, const std::pair & o) { - mkOutputString( - state, + state.mkOutputString( attrs.alloc(o.first), drvPath, o.first, -- cgit v1.2.3 From 5a23b80b0aa711877a05b18e2f8f221f8bd4844a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 10 May 2023 19:25:52 -0400 Subject: Create `EvalState::coerceToDerivedPath` This gives us some round trips to test. `EvalState::coerceToDerivedPathUnchecked` is a factored out helper just for unit testing. --- src/libexpr/eval.cc | 74 +++++++++++++++++++++++++++++++++++++++ src/libexpr/eval.hh | 23 ++++++++++++ src/libexpr/tests/derived-path.cc | 65 ++++++++++++++++++++++++++++++++++ 3 files changed, 162 insertions(+) create mode 100644 src/libexpr/tests/derived-path.cc (limited to 'src') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 679f46153..740a5e677 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2318,6 +2318,80 @@ StorePath EvalState::coerceToStorePath(const PosIdx pos, Value & v, NixStringCon } +std::pair EvalState::coerceToDerivedPathUnchecked(const PosIdx pos, Value & v, std::string_view errorCtx) +{ + NixStringContext context; + auto s = forceString(v, context, pos, errorCtx); + auto csize = context.size(); + if (csize != 1) + error( + "string '%s' has %d entries in its context. It should only have exactly one entry", + s, csize) + .withTrace(pos, errorCtx).debugThrow(); + auto derivedPath = std::visit(overloaded { + [&](NixStringContextElem::Opaque && o) -> DerivedPath { + return DerivedPath::Opaque { + .path = std::move(o.path), + }; + }, + [&](NixStringContextElem::DrvDeep &&) -> DerivedPath { + error( + "string '%s' has a context which refers to a complete source and binary closure. This is not supported at this time", + s).withTrace(pos, errorCtx).debugThrow(); + }, + [&](NixStringContextElem::Built && b) -> DerivedPath { + return DerivedPath::Built { + .drvPath = std::move(b.drvPath), + .outputs = OutputsSpec::Names { std::move(b.output) }, + }; + }, + }, ((NixStringContextElem &&) *context.begin()).raw()); + return { + std::move(derivedPath), + std::move(s), + }; +} + + +DerivedPath EvalState::coerceToDerivedPath(const PosIdx pos, Value & v, std::string_view errorCtx) +{ + auto [derivedPath, s_] = coerceToDerivedPathUnchecked(pos, v, errorCtx); + auto s = s_; + std::visit(overloaded { + [&](const DerivedPath::Opaque & o) { + auto sExpected = store->printStorePath(o.path); + if (s != sExpected) + error( + "path string '%s' has context with the different path '%s'", + s, sExpected) + .withTrace(pos, errorCtx).debugThrow(); + }, + [&](const DerivedPath::Built & b) { + // TODO need derived path with single output to make this + // total. Will add as part of RFC 92 work and then this is + // cleaned up. + auto output = *std::get(b.outputs).begin(); + + auto drv = store->readDerivation(b.drvPath); + auto i = drv.outputs.find(output); + if (i == drv.outputs.end()) + throw Error("derivation '%s' does not have output '%s'", store->printStorePath(b.drvPath), output); + auto optOutputPath = i->second.path(*store, drv.name, output); + // This is testing for the case of CA derivations + auto sExpected = optOutputPath + ? store->printStorePath(*optOutputPath) + : downstreamPlaceholder(*store, b.drvPath, output); + if (s != sExpected) + 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, output, store->printStorePath(b.drvPath), sExpected) + .withTrace(pos, errorCtx).debugThrow(); + } + }, derivedPath.raw()); + return derivedPath; +} + + bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_view errorCtx) { forceValue(v1, noPos); diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 3b2164a19..a90ff34c0 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -21,6 +21,7 @@ namespace nix { class Store; class EvalState; class StorePath; +struct DerivedPath; enum RepairFlag : bool; @@ -473,6 +474,28 @@ public: */ StorePath coerceToStorePath(const PosIdx pos, Value & v, NixStringContext & context, std::string_view errorCtx); + /** + * Part of `coerceToDerivedPath()` without any store IO which is exposed for unit testing only. + */ + std::pair coerceToDerivedPathUnchecked(const PosIdx pos, Value & v, std::string_view errorCtx); + + /** + * Coerce to `DerivedPath`. + * + * Must be a string which is either a literal store path or a + * "placeholder (see `downstreamPlaceholder()`). + * + * Even more importantly, the string context must be exactly one + * element, which is either a `NixStringContextElem::Opaque` or + * `NixStringContextElem::Built`. (`NixStringContextEleme::DrvDeep` + * is not permitted). + * + * The string is parsed based on the context --- the context is the + * source of truth, and ultimately tells us what we want, and then + * we ensure the string corresponds to it. + */ + DerivedPath coerceToDerivedPath(const PosIdx pos, Value & v, std::string_view errorCtx); + public: /** diff --git a/src/libexpr/tests/derived-path.cc b/src/libexpr/tests/derived-path.cc new file mode 100644 index 000000000..8210efef2 --- /dev/null +++ b/src/libexpr/tests/derived-path.cc @@ -0,0 +1,65 @@ +#include +#include +#include + +#include "tests/derived-path.hh" +#include "tests/libexpr.hh" + +namespace nix { + +// Testing of trivial expressions +class DerivedPathExpressionTest : public LibExprTest {}; + +// FIXME: `RC_GTEST_FIXTURE_PROP` isn't calling `SetUpTestSuite` because it is +// no a real fixture. +// +// See https://github.com/emil-e/rapidcheck/blob/master/doc/gtest.md#rc_gtest_fixture_propfixture-name-args +TEST_F(DerivedPathExpressionTest, force_init) +{ +} + +RC_GTEST_FIXTURE_PROP( + DerivedPathExpressionTest, + prop_opaque_path_round_trip, + (const DerivedPath::Opaque & o)) +{ + auto * v = state.allocValue(); + state.mkStorePathString(o.path, *v); + auto d = state.coerceToDerivedPath(noPos, *v, ""); + RC_ASSERT(DerivedPath { o } == d); +} + +// TODO use DerivedPath::Built for parameter once it supports a single output +// path only. + +RC_GTEST_FIXTURE_PROP( + DerivedPathExpressionTest, + prop_built_path_placeholder_round_trip, + (const StorePath & drvPath, const StorePathName & outputName)) +{ + auto * v = state.allocValue(); + state.mkOutputString(*v, drvPath, outputName.name, std::nullopt); + auto [d, _] = state.coerceToDerivedPathUnchecked(noPos, *v, ""); + DerivedPath::Built b { + .drvPath = drvPath, + .outputs = OutputsSpec::Names { outputName.name }, + }; + RC_ASSERT(DerivedPath { b } == d); +} + +RC_GTEST_FIXTURE_PROP( + DerivedPathExpressionTest, + prop_built_path_out_path_round_trip, + (const StorePath & drvPath, const StorePathName & outputName, const StorePath & outPath)) +{ + auto * v = state.allocValue(); + state.mkOutputString(*v, drvPath, outputName.name, outPath); + auto [d, _] = state.coerceToDerivedPathUnchecked(noPos, *v, ""); + DerivedPath::Built b { + .drvPath = drvPath, + .outputs = OutputsSpec::Names { outputName.name }, + }; + RC_ASSERT(DerivedPath { b } == d); +} + +} /* namespace nix */ -- cgit v1.2.3 From d2162e7acdad4818788d229ed2d4600412b2be52 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 10 Jan 2023 17:18:59 -0500 Subject: Make more string values work as installables As discussed in #7417, it would be good to make more string values work as installables. That is to say, if an installable refers to a value, and the value is a string, it used to not work at all, since #7484, it works somewhat, and this PR make it work some more. The new cases that are added for `BuiltPath` contexts: - Fixed input- or content-addressed derivation: ``` nix-repl> hello.out.outPath "/nix/store/jppfl2bp1zhx8sgs2mgifmsx6dv16mv2-hello-2.12" nix-repl> :p builtins.getContext hello.out.outPath { "/nix/store/c7jrxqjhdda93lhbkanqfs07x2bzazbm-hello-2.12.drv" = { outputs = [ "out" ]; }; } The string matches the specified single output of that derivation, so it should also be valid. - Floating content-addressed derivation: ``` nix-repl> (hello.overrideAttrs (_: { __contentAddressed = true; })).out.outPath "/1a08j26xqc0zm8agps8anxpjji410yvsx4pcgyn4bfan1ddkx2g0" nix-repl> :p builtins.getContext (hello.overrideAttrs (_: { __contentAddressed = true; })).out.outPath { "/nix/store/qc645pyf9wl37c6qvqzaqkwsm1gp48al-hello-2.12.drv" = { outputs = [ "out" ]; }; } ``` The string is not a path but a placeholder, however it also matches the context, and because it is a CA derivation we have no better option. This should also be valid. We may also want to think about richer attrset based values (also discussed in that issue and #6507), but this change "completes" our string-based building blocks, from which the others can be desugared into or at least described/document/taught in terms of. Progress towards #7417 Co-authored-by: Robert Hensing --- src/libcmd/installable-attr-path.cc | 10 +++++++++- src/libcmd/installable-flake.cc | 30 ++++++------------------------ src/libcmd/installable-value.cc | 22 ++++++++++++++++++++++ src/libcmd/installable-value.hh | 18 ++++++++++++++++++ 4 files changed, 55 insertions(+), 25 deletions(-) (limited to 'src') diff --git a/src/libcmd/installable-attr-path.cc b/src/libcmd/installable-attr-path.cc index cf513126d..b35ca2910 100644 --- a/src/libcmd/installable-attr-path.cc +++ b/src/libcmd/installable-attr-path.cc @@ -46,7 +46,15 @@ std::pair InstallableAttrPath::toValue(EvalState & state) DerivedPathsWithInfo InstallableAttrPath::toDerivedPaths() { - auto v = toValue(*state).first; + auto [v, pos] = toValue(*state); + + if (std::optional derivedPathWithInfo = trySinglePathToDerivedPaths( + *v, + pos, + fmt("while evaluating the attribute '%s'", attrPath))) + { + return { *derivedPathWithInfo }; + } Bindings & autoArgs = *cmd.getAutoArgs(*state); diff --git a/src/libcmd/installable-flake.cc b/src/libcmd/installable-flake.cc index 37e59cfdf..eb944240b 100644 --- a/src/libcmd/installable-flake.cc +++ b/src/libcmd/installable-flake.cc @@ -95,31 +95,13 @@ DerivedPathsWithInfo InstallableFlake::toDerivedPaths() // FIXME: use eval cache? auto v = attr->forceValue(); - if (v.type() == nPath) { - auto storePath = v.path().fetchToStore(state->store); - return {{ - .path = DerivedPath::Opaque { - .path = std::move(storePath), - }, - .info = make_ref(), - }}; - } - - else if (v.type() == nString) { - NixStringContext context; - auto s = state->forceString(v, context, noPos, fmt("while evaluating the flake output attribute '%s'", attrPath)); - auto storePath = state->store->maybeParseStorePath(s); - if (storePath && context.count(NixStringContextElem::Opaque { .path = *storePath })) { - return {{ - .path = DerivedPath::Opaque { - .path = std::move(*storePath), - }, - .info = make_ref(), - }}; - } else - throw Error("flake output attribute '%s' evaluates to the string '%s' which is not a store path", attrPath, s); + if (std::optional derivedPathWithInfo = trySinglePathToDerivedPaths( + v, + noPos, + fmt("while evaluating the flake output attribute '%s'", attrPath))) + { + return { *derivedPathWithInfo }; } - else throw Error("flake output attribute '%s' is not a derivation or path", attrPath); } diff --git a/src/libcmd/installable-value.cc b/src/libcmd/installable-value.cc index 3a7ede4e2..1eff293cc 100644 --- a/src/libcmd/installable-value.cc +++ b/src/libcmd/installable-value.cc @@ -41,4 +41,26 @@ ref InstallableValue::require(ref installable) return ref { castedInstallable }; } +std::optional InstallableValue::trySinglePathToDerivedPaths(Value & v, const PosIdx pos, std::string_view errorCtx) +{ + if (v.type() == nPath) { + auto storePath = v.path().fetchToStore(state->store); + return {{ + .path = DerivedPath::Opaque { + .path = std::move(storePath), + }, + .info = make_ref(), + }}; + } + + else if (v.type() == nString) { + return {{ + .path = state->coerceToDerivedPath(pos, v, errorCtx), + .info = make_ref(), + }}; + } + + else return std::nullopt; +} + } diff --git a/src/libcmd/installable-value.hh b/src/libcmd/installable-value.hh index 5ab7eee16..3138ce8ec 100644 --- a/src/libcmd/installable-value.hh +++ b/src/libcmd/installable-value.hh @@ -98,6 +98,24 @@ struct InstallableValue : Installable static InstallableValue & require(Installable & installable); static ref require(ref installable); + +protected: + + /** + * Handles either a plain path, or a string with a single string + * context elem in the right format. The latter case is handled by + * `EvalState::coerceToDerivedPath()`; see it for details. + * + * @param v Value that is hopefully a string or path per the above. + * + * @param pos Position of value to aid with diagnostics. + * + * @param errorCtx Arbitrary message for use in potential error message when something is wrong with `v`. + * + * @result A derived path (with empty info, for now) if the value + * matched the above criteria. + */ + std::optional trySinglePathToDerivedPaths(Value & v, const PosIdx pos, std::string_view errorCtx); }; } -- cgit v1.2.3