Merge pull request #7601 from obsidiansystems/string-installables

Make more string values work as installables

src/libcmd/installable-attr-path.cc

@@ -46,7 +46,15 @@ std::pair<Value *, PosIdx> 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);
 

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<ExtraPathInfo>(),
-            }};
+        if (std::optional derivedPathWithInfo = trySinglePathToDerivedPaths(
+            v,
+            noPos,
+            fmt("while evaluating the flake output attribute '%s'", attrPath)))
+        {
+            return { *derivedPathWithInfo };
         }
-
-        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<ExtraPathInfo>(),
-                }};
-            } else
-                throw Error("flake output attribute '%s' evaluates to the string '%s' which is not a store path", attrPath, s);
-        }
-
         else
             throw Error("flake output attribute '%s' is not a derivation or path", attrPath);
     }

src/libcmd/installable-value.cc

@@ -41,4 +41,26 @@ ref<InstallableValue> InstallableValue::require(ref<Installable> installable)
     return ref { castedInstallable };
 }
 
+std::optional<DerivedPathWithInfo> 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<ExtraPathInfo>(),
+        }};
+    }
+
+    else if (v.type() == nString) {
+        return {{
+            .path = state->coerceToDerivedPath(pos, v, errorCtx),
+            .info = make_ref<ExtraPathInfo>(),
+        }};
+    }
+
+    else return std::nullopt;
+}
+
 }

src/libcmd/installable-value.hh

@@ -98,6 +98,24 @@ struct InstallableValue : Installable
 
     static InstallableValue & require(Installable & installable);
     static ref<InstallableValue> require(ref<Installable> 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<DerivedPathWithInfo> trySinglePathToDerivedPaths(Value & v, const PosIdx pos, std::string_view errorCtx);
 };
 
 }

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<StorePath> 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
@@ -2297,6 +2318,80 @@ StorePath EvalState::coerceToStorePath(const PosIdx pos, Value & v, NixStringCon
 }
 
 
+std::pair<DerivedPath, std::string_view> 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<EvalError>();
+    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<EvalError>();
+        },
+        [&](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<EvalError>();
+        },
+        [&](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<OutputsSpec::Names>(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<EvalError>();
+        }
+    }, derivedPath.raw());
+    return derivedPath;
+}
+
+
 bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_view errorCtx)
 {
     forceValue(v1, noPos);

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<DerivedPath, std::string_view> 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:
 
     /**
@@ -576,12 +599,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<StorePath> optOutputPath);
+
     void concatLists(Value & v, size_t nrLists, Value * * lists, const PosIdx pos, std::string_view errorCtx);
 
     /**

src/libexpr/primops.cc

@@ -129,40 +129,31 @@ 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. */
+/**
+ * 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 BasicDerivation & drv,
     const std::pair<std::string, DerivationOutput> & o)
 {
-    auto optOutputPath = o.second.path(*state.store, drv.name, o.first);
-    attrs.alloc(o.first).mkString(
-        optOutputPath
-            ? state.store->printStorePath(*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),
-        NixStringContext {
-            NixStringContextElem::Built {
-                .drvPath = drvPath,
-                .output = o.first,
-            }
-        });
+    state.mkOutputString(
+        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
@@ -193,7 +184,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 +1396,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);
 }

src/libexpr/tests/derived-path.cc

@@ -0,0 +1,65 @@
+#include <nlohmann/json.hpp>
+#include <gtest/gtest.h>
+#include <rapidcheck/gtest.h>
+
+#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 */

tests/build.sh

@@ -57,6 +57,30 @@ nix build -f multiple-outputs.nix --json 'e^*' --no-link | jq --exit-status '
     (.outputs | keys == ["a_a", "b", "c"]))
 '
 
+# test buidling from non-drv attr path
+
+nix build -f multiple-outputs.nix --json 'e.a_a.outPath' --no-link | jq --exit-status '
+  (.[0] |
+    (.drvPath | match(".*multiple-outputs-e.drv")) and
+    (.outputs | keys == ["a_a"]))
+'
+
+# Illegal type of string context
+expectStderr 1 nix build -f multiple-outputs.nix 'e.a_a.drvPath' \
+  | grepQuiet "has a context which refers to a complete source and binary closure."
+
+# No string context
+expectStderr 1 nix build --expr '""' --no-link \
+  | grepQuiet "has 0 entries in its context. It should only have exactly one entry"
+
+# Too much string context
+expectStderr 1 nix build --impure --expr 'with (import ./multiple-outputs.nix).e.a_a; "${drvPath}${outPath}"' --no-link \
+  | grepQuiet "has 2 entries in its context. It should only have exactly one entry"
+
+nix build --impure --json --expr 'builtins.unsafeDiscardOutputDependency (import ./multiple-outputs.nix).e.a_a.drvPath' --no-link | jq --exit-status '
+  (.[0] | .path | match(".*multiple-outputs-e.drv"))
+'
+
 # Test building from raw store path to drv not expression.
 
 drv=$(nix eval -f multiple-outputs.nix --raw a.drvPath)

tests/flakes/build-paths.sh

@@ -41,10 +41,27 @@ cat > $flake1Dir/flake.nix <<EOF
     a8 = builtins.storePath $dep;
 
     a9 = "$dep";
+
+    drvCall = with import ./config.nix; mkDerivation {
+      name = "simple";
+      builder = ./simple.builder.sh;
+      PATH = "";
+      goodPath = path;
+    };
+
+    a10 = builtins.unsafeDiscardOutputDependency self.drvCall.drvPath;
+
+    a11 = self.drvCall.drvPath;
+
+    a12 = self.drvCall.outPath;
+
+    a13 = "\${self.drvCall.drvPath}\${self.drvCall.outPath}";
   };
 }
 EOF
 
+cp ../simple.nix ../simple.builder.sh ../config.nix $flake1Dir/
+
 echo bar > $flake1Dir/foo
 
 nix build --json --out-link $TEST_ROOT/result $flake1Dir#a1
@@ -63,4 +80,17 @@ nix build --json --out-link $TEST_ROOT/result $flake1Dir#a6
 nix build --impure --json --out-link $TEST_ROOT/result $flake1Dir#a8
 diff common.sh $TEST_ROOT/result
 
-(! nix build --impure --json --out-link $TEST_ROOT/result $flake1Dir#a9)
+expectStderr 1 nix build --impure --json --out-link $TEST_ROOT/result $flake1Dir#a9 \
+  | grepQuiet "has 0 entries in its context. It should only have exactly one entry"
+
+nix build --json --out-link $TEST_ROOT/result $flake1Dir#a10
+[[ $(readlink -e $TEST_ROOT/result) = *simple.drv ]]
+
+expectStderr 1 nix build --json --out-link $TEST_ROOT/result $flake1Dir#a11 \
+  | grepQuiet "has a context which refers to a complete source and binary closure"
+
+nix build --json --out-link $TEST_ROOT/result $flake1Dir#a12
+[[ -e $TEST_ROOT/result/hello ]]
+
+expectStderr 1 nix build --impure --json --out-link $TEST_ROOT/result $flake1Dir#a13 \
+  | grepQuiet "has 2 entries in its context. It should only have exactly one entry"