Allow dynamic derivation deps in `inputDrvs`

We use the same nested map representation we used for goals, again in
order to save space. We might someday want to combine with `inputDrvs`,
by doing `V = bool` instead of `V = std::set<OutputName>`, but we are
not doing that yet for sake of a smaller diff.

The ATerm format for Derivations also needs to be extended, in addition
to the in-memory format. To accomodate this, we added a new basic
versioning scheme, so old versions of Nix will get nice errors. (And
going forward, if the ATerm format changes again the errors will be even
better.)

`parsedStrings`, an internal function used as part of parsing
derivations in A-Term format, used to consume the final `]` but expect
the initial `[` to already be consumed. This made for what looked like
unbalanced brackets at callsites, which was confusing. Now it consumes
both which is hopefully less confusing.

As part of testing, we also created a unit test for the A-Term format for
regular non-experimental derivations too.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

Apply suggestions from code review

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

1 files changed, 271 insertions, 60 deletions

diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc
index 43d3dc4a2..67069c3c9 100644
--- a/src/libstore/derivations.cc
+++ b/src/libstore/derivations.cc
@@ -136,7 +136,7 @@ StorePath writeDerivation(Store & store,
     const Derivation & drv, RepairFlag repair, bool readOnly)
 {
     auto references = drv.inputSrcs;
-    for (auto & i : drv.inputDrvs)
+    for (auto & i : drv.inputDrvs.map)
         references.insert(i.first);
     /* Note that the outputs of a derivation are *not* references
        (that can be missing (of course) and should not necessarily be
@@ -208,22 +208,25 @@ static bool endOfList(std::istream & str)
 static StringSet parseStrings(std::istream & str, bool arePaths)
 {
     StringSet res;
+    expect(str, "[");
     while (!endOfList(str))
         res.insert(arePaths ? parsePath(str) : parseString(str));
     return res;
 }
 
 
-static DerivationOutput parseDerivationOutput(const Store & store,
-    std::string_view pathS, std::string_view hashAlgo, std::string_view hashS)
+static DerivationOutput parseDerivationOutput(
+    const Store & store,
+    std::string_view pathS, std::string_view hashAlgo, std::string_view hashS,
+    const ExperimentalFeatureSettings & xpSettings)
 {
     if (hashAlgo != "") {
         ContentAddressMethod method = ContentAddressMethod::parsePrefix(hashAlgo);
         if (method == TextIngestionMethod {})
-            experimentalFeatureSettings.require(Xp::DynamicDerivations);
+            xpSettings.require(Xp::DynamicDerivations);
         const auto hashType = parseHashType(hashAlgo);
         if (hashS == "impure") {
-            experimentalFeatureSettings.require(Xp::ImpureDerivations);
+            xpSettings.require(Xp::ImpureDerivations);
             if (pathS != "")
                 throw FormatError("impure derivation output should not specify output path");
             return DerivationOutput::Impure {
@@ -240,7 +243,7 @@ static DerivationOutput parseDerivationOutput(const Store & store,
                 },
             };
         } else {
-            experimentalFeatureSettings.require(Xp::CaDerivations);
+            xpSettings.require(Xp::CaDerivations);
             if (pathS != "")
                 throw FormatError("content-addressed derivation output should not specify output path");
             return DerivationOutput::CAFloating {
@@ -259,29 +262,116 @@ static DerivationOutput parseDerivationOutput(const Store & store,
     }
 }
 
-static DerivationOutput parseDerivationOutput(const Store & store, std::istringstream & str)
+static DerivationOutput parseDerivationOutput(
+    const Store & store, std::istringstream & str,
+    const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings)
 {
     expect(str, ","); const auto pathS = parseString(str);
     expect(str, ","); const auto hashAlgo = parseString(str);
     expect(str, ","); const auto hash = parseString(str);
     expect(str, ")");
 
-    return parseDerivationOutput(store, pathS, hashAlgo, hash);
+    return parseDerivationOutput(store, pathS, hashAlgo, hash, xpSettings);
+}
+
+/**
+ * All ATerm Derivation format versions currently known.
+ *
+ * Unknown versions are rejected at the parsing stage.
+ */
+enum struct DerivationATermVersion {
+    /**
+     * Older unversioned form
+     */
+    Traditional,
+
+    /**
+     * Newer versioned form; only this version so far.
+     */
+    DynamicDerivations,
+};
+
+static DerivedPathMap<StringSet>::ChildNode parseDerivedPathMapNode(
+    const Store & store,
+    std::istringstream & str,
+    DerivationATermVersion version)
+{
+    DerivedPathMap<StringSet>::ChildNode node;
+
+    auto parseNonDynamic = [&]() {
+        node.value = parseStrings(str, false);
+    };
+
+    // Older derivation should never use new form, but newer
+    // derivaiton can use old form.
+    switch (version) {
+    case DerivationATermVersion::Traditional:
+        parseNonDynamic();
+        break;
+    case DerivationATermVersion::DynamicDerivations:
+        switch (str.peek()) {
+        case '[':
+            parseNonDynamic();
+            break;
+        case '(':
+            expect(str, "(");
+            node.value = parseStrings(str, false);
+            expect(str, ",[");
+            while (!endOfList(str)) {
+                expect(str, "(");
+                auto outputName = parseString(str);
+                expect(str, ",");
+                node.childMap.insert_or_assign(outputName, parseDerivedPathMapNode(store, str, version));
+                expect(str, ")");
+            }
+            expect(str, ")");
+            break;
+        default:
+            throw FormatError("invalid inputDrvs entry in derivation");
+        }
+        break;
+    default:
+        // invalid format, not a parse error but internal error
+        assert(false);
+    }
+    return node;
 }
 
 
-Derivation parseDerivation(const Store & store, std::string && s, std::string_view name)
+Derivation parseDerivation(
+    const Store & store, std::string && s, std::string_view name,
+    const ExperimentalFeatureSettings & xpSettings)
 {
     Derivation drv;
     drv.name = name;
 
     std::istringstream str(std::move(s));
-    expect(str, "Derive([");
+    expect(str, "D");
+    DerivationATermVersion version;
+    switch (str.peek()) {
+    case 'e':
+        expect(str, "erive(");
+        version = DerivationATermVersion::Traditional;
+        break;
+    case 'r':
+        expect(str, "rvWithVersion(");
+        auto versionS = parseString(str);
+        if (versionS == "xp-dyn-drv") {
+            // Only verison we have so far
+            version = DerivationATermVersion::DynamicDerivations;
+            xpSettings.require(Xp::DynamicDerivations);
+        } else {
+            throw FormatError("Unknown derivation ATerm format version '%s'", versionS);
+        }
+        expect(str, ",");
+        break;
+    }
 
     /* Parse the list of outputs. */
+    expect(str, "[");
     while (!endOfList(str)) {
         expect(str, "("); std::string id = parseString(str);
-        auto output = parseDerivationOutput(store, str);
+        auto output = parseDerivationOutput(store, str, xpSettings);
         drv.outputs.emplace(std::move(id), std::move(output));
     }
 
@@ -290,12 +380,12 @@ Derivation parseDerivation(const Store & store, std::string && s, std::string_vi
     while (!endOfList(str)) {
         expect(str, "(");
         Path drvPath = parsePath(str);
-        expect(str, ",[");
-        drv.inputDrvs.insert_or_assign(store.parseStorePath(drvPath), parseStrings(str, false));
+        expect(str, ",");
+        drv.inputDrvs.map.insert_or_assign(store.parseStorePath(drvPath), parseDerivedPathMapNode(store, str, version));
         expect(str, ")");
     }
 
-    expect(str, ",["); drv.inputSrcs = store.parseStorePathSet(parseStrings(str, true));
+    expect(str, ","); drv.inputSrcs = store.parseStorePathSet(parseStrings(str, true));
     expect(str, ","); drv.platform = parseString(str);
     expect(str, ","); drv.builder = parseString(str);
 
@@ -379,14 +469,67 @@ static void printUnquotedStrings(std::string & res, ForwardIterator i, ForwardIt
 }
 
 
+static void unparseDerivedPathMapNode(const Store & store, std::string & s, const DerivedPathMap<StringSet>::ChildNode & node)
+{
+    s += ',';
+    if (node.childMap.empty()) {
+        printUnquotedStrings(s, node.value.begin(), node.value.end());
+    } else {
+        s += "(";
+        printUnquotedStrings(s, node.value.begin(), node.value.end());
+        s += ",[";
+        bool first = true;
+        for (auto & [outputName, childNode] : node.childMap) {
+            if (first) first = false; else s += ',';
+            s += '('; printUnquotedString(s, outputName);
+            unparseDerivedPathMapNode(store, s, childNode);
+            s += ')';
+        }
+        s += "])";
+    }
+}
+
+
+/**
+ * Does the derivation have a dependency on the output of a dynamic
+ * derivation?
+ *
+ * In other words, does it on the output of derivation that is itself an
+ * ouput of a derivation? This corresponds to a dependency that is an
+ * inductive derived path with more than one layer of
+ * `DerivedPath::Built`.
+ */
+static bool hasDynamicDrvDep(const Derivation & drv)
+{
+    return
+        std::find_if(
+            drv.inputDrvs.map.begin(),
+            drv.inputDrvs.map.end(),
+            [](auto & kv) { return !kv.second.childMap.empty(); })
+        != drv.inputDrvs.map.end();
+}
+
+
 std::string Derivation::unparse(const Store & store, bool maskOutputs,
-    std::map<std::string, StringSet> * actualInputs) const
+    DerivedPathMap<StringSet>::ChildNode::Map * actualInputs) const
 {
     std::string s;
     s.reserve(65536);
-    s += "Derive([";
+
+    /* Use older unversioned form if possible, for wider compat. Use
+       newer form only if we need it, which we do for
+       `Xp::DynamicDerivations`. */
+    if (hasDynamicDrvDep(*this)) {
+        s += "DrvWithVersion(";
+        // Only version we have so far
+        printUnquotedString(s, "xp-dyn-drv");
+        s += ",";
+    } else {
+        s += "Derive(";
+    }
 
     bool first = true;
+    s += "[";
     for (auto & i : outputs) {
         if (first) first = false; else s += ',';
         s += '('; printUnquotedString(s, i.first);
@@ -424,17 +567,17 @@ std::string Derivation::unparse(const Store & store, bool maskOutputs,
     s += "],[";
     first = true;
     if (actualInputs) {
-        for (auto & i : *actualInputs) {
+        for (auto & [drvHashModulo, childMap] : *actualInputs) {
             if (first) first = false; else s += ',';
-            s += '('; printUnquotedString(s, i.first);
-            s += ','; printUnquotedStrings(s, i.second.begin(), i.second.end());
+            s += '('; printUnquotedString(s, drvHashModulo);
+            unparseDerivedPathMapNode(store, s, childMap);
             s += ')';
         }
     } else {
-        for (auto & i : inputDrvs) {
+        for (auto & [drvPath, childMap] : inputDrvs.map) {
             if (first) first = false; else s += ',';
-            s += '('; printUnquotedString(s, store.printStorePath(i.first));
-            s += ','; printUnquotedStrings(s, i.second.begin(), i.second.end());
+            s += '('; printUnquotedString(s, store.printStorePath(drvPath));
+            unparseDerivedPathMapNode(store, s, childMap);
             s += ')';
         }
     }
@@ -668,18 +811,16 @@ DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOut
         }
     }, drv.type().raw);
 
-    std::map<std::string, StringSet> inputs2;
-    for (auto & [drvPath, inputOutputs0] : drv.inputDrvs) {
-        // Avoid lambda capture restriction with standard / Clang
-        auto & inputOutputs = inputOutputs0;
+    DerivedPathMap<StringSet>::ChildNode::Map inputs2;
+    for (auto & [drvPath, node] : drv.inputDrvs.map) {
         const auto & res = pathDerivationModulo(store, drvPath);
         if (res.kind == DrvHash::Kind::Deferred)
             kind = DrvHash::Kind::Deferred;
-        for (auto & outputName : inputOutputs) {
+        for (auto & outputName : node.value) {
             const auto h = get(res.hashes, outputName);
             if (!h)
                 throw Error("no hash for output '%s' of derivation '%s'", outputName, drv.name);
-            inputs2[h->to_string(Base16, false)].insert(outputName);
+            inputs2[h->to_string(Base16, false)].value.insert(outputName);
         }
     }
 
@@ -709,7 +850,7 @@ static DerivationOutput readDerivationOutput(Source & in, const Store & store)
     const auto hashAlgo = readString(in);
     const auto hash = readString(in);
 
-    return parseDerivationOutput(store, pathS, hashAlgo, hash);
+    return parseDerivationOutput(store, pathS, hashAlgo, hash, experimentalFeatureSettings);
 }
 
 StringSet BasicDerivation::outputNames() const
@@ -824,6 +965,8 @@ std::string hashPlaceholder(const OutputNameView outputName)
 
 static void rewriteDerivation(Store & store, BasicDerivation & drv, const StringMap & rewrites)
 {
+    debug("Rewriting the derivation");
+
     for (auto & rewrite : rewrites) {
         debug("rewriting %s as %s", rewrite.first, rewrite.second);
     }
@@ -862,14 +1005,70 @@ std::optional<BasicDerivation> Derivation::tryResolve(Store & store) const
 {
     std::map<std::pair<StorePath, std::string>, StorePath> inputDrvOutputs;
 
-    for (auto & input : inputDrvs)
-        for (auto & [outputName, outputPath] : store.queryPartialDerivationOutputMap(input.first))
-            if (outputPath)
-                inputDrvOutputs.insert_or_assign({input.first, outputName}, *outputPath);
+    std::function<void(const StorePath &, const DerivedPathMap<StringSet>::ChildNode &)> accum;
+    accum = [&](auto & inputDrv, auto & node) {
+        for (auto & [outputName, outputPath] : store.queryPartialDerivationOutputMap(inputDrv)) {
+            if (outputPath) {
+                inputDrvOutputs.insert_or_assign({inputDrv, outputName}, *outputPath);
+                if (auto p = get(node.childMap, outputName))
+                    accum(*outputPath, *p);
+            }
+        }
+    };
+
+    for (auto & [inputDrv, node] : inputDrvs.map)
+        accum(inputDrv, node);
 
     return tryResolve(store, inputDrvOutputs);
 }
 
+static bool tryResolveInput(
+    Store & store, StorePathSet & inputSrcs, StringMap & inputRewrites,
+    const DownstreamPlaceholder * placeholderOpt,
+    const StorePath & inputDrv, const DerivedPathMap<StringSet>::ChildNode & inputNode,
+    const std::map<std::pair<StorePath, std::string>, StorePath> & inputDrvOutputs)
+{
+    auto getOutput = [&](const std::string & outputName) {
+        auto * actualPathOpt = get(inputDrvOutputs, { inputDrv, outputName });
+        if (!actualPathOpt)
+            warn("output %s of input %s missing, aborting the resolving",
+                outputName,
+                store.printStorePath(inputDrv)
+            );
+        return actualPathOpt;
+    };
+
+    auto getPlaceholder = [&](const std::string & outputName) {
+        return placeholderOpt
+            ? DownstreamPlaceholder::unknownDerivation(*placeholderOpt, outputName)
+            : DownstreamPlaceholder::unknownCaOutput(inputDrv, outputName);
+    };
+
+    for (auto & outputName : inputNode.value) {
+        auto actualPathOpt = getOutput(outputName);
+        if (!actualPathOpt) return false;
+        auto actualPath = *actualPathOpt;
+        if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) {
+            inputRewrites.emplace(
+                getPlaceholder(outputName).render(),
+                store.printStorePath(actualPath));
+        }
+        inputSrcs.insert(std::move(actualPath));
+    }
+
+    for (auto & [outputName, childNode] : inputNode.childMap) {
+        auto actualPathOpt = getOutput(outputName);
+        if (!actualPathOpt) return false;
+        auto actualPath = *actualPathOpt;
+        auto nextPlaceholder = getPlaceholder(outputName);
+        if (!tryResolveInput(store, inputSrcs, inputRewrites,
+            &nextPlaceholder, actualPath, childNode,
+            inputDrvOutputs))
+            return false;
+    }
+    return true;
+}
+
 std::optional<BasicDerivation> Derivation::tryResolve(
     Store & store,
     const std::map<std::pair<StorePath, std::string>, StorePath> & inputDrvOutputs) const
@@ -879,23 +1078,10 @@ std::optional<BasicDerivation> Derivation::tryResolve(
     // Input paths that we'll want to rewrite in the derivation
     StringMap inputRewrites;
 
-    for (auto & [inputDrv, inputOutputs] : inputDrvs) {
-        for (auto & outputName : inputOutputs) {
-            if (auto actualPath = get(inputDrvOutputs, { inputDrv, outputName })) {
-                if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) {
-                    inputRewrites.emplace(
-                        DownstreamPlaceholder::unknownCaOutput(inputDrv, outputName).render(),
-                        store.printStorePath(*actualPath));
-                }
-                resolved.inputSrcs.insert(*actualPath);
-            } else {
-                warn("output '%s' of input '%s' missing, aborting the resolving",
-                    outputName,
-                    store.printStorePath(inputDrv));
-                return {};
-            }
-        }
-    }
+    for (auto & [inputDrv, inputNode] : inputDrvs.map)
+        if (!tryResolveInput(store, resolved.inputSrcs, inputRewrites,
+            nullptr, inputDrv, inputNode, inputDrvOutputs))
+            return std::nullopt;
 
     rewriteDerivation(store, resolved, inputRewrites);
 
@@ -1084,10 +1270,25 @@ nlohmann::json Derivation::toJSON(const Store & store) const
     }
 
     {
-        auto& inputDrvsObj = res["inputDrvs"];
-        inputDrvsObj = nlohmann::json ::object();
-        for (auto & input : inputDrvs)
-            inputDrvsObj[store.printStorePath(input.first)] = input.second;
+        std::function<nlohmann::json(const DerivedPathMap<StringSet>::ChildNode &)> doInput;
+        doInput = [&](const auto & inputNode) {
+            auto value = nlohmann::json::object();
+            value["outputs"] = inputNode.value;
+            {
+                auto next = nlohmann::json::object();
+                for (auto & [outputId, childNode] : inputNode.childMap)
+                    next[outputId] = doInput(childNode);
+                value["dynamicOutputs"] = std::move(next);
+            }
+            return value;
+        };
+        {
+            auto& inputDrvsObj = res["inputDrvs"];
+            inputDrvsObj = nlohmann::json::object();
+            for (auto & [inputDrv, inputNode] : inputDrvs.map) {
+                inputDrvsObj[store.printStorePath(inputDrv)] = doInput(inputNode);
+            }
+        }
     }
 
     res["system"] = platform;
@@ -1101,7 +1302,8 @@ nlohmann::json Derivation::toJSON(const Store & store) const
 
 Derivation Derivation::fromJSON(
     const Store & store,
-    const nlohmann::json & json)
+    const nlohmann::json & json,
+    const ExperimentalFeatureSettings & xpSettings)
 {
     using nlohmann::detail::value_t;
 
@@ -1133,12 +1335,21 @@ Derivation Derivation::fromJSON(
     }
 
     try {
+        std::function<DerivedPathMap<StringSet>::ChildNode(const nlohmann::json &)> doInput;
+        doInput = [&](const auto & json) {
+            DerivedPathMap<StringSet>::ChildNode node;
+            node.value = static_cast<const StringSet &>(
+                ensureType(valueAt(json, "outputs"), value_t::array));
+            for (auto & [outputId, childNode] : ensureType(valueAt(json, "dynamicOutputs"), value_t::object).items()) {
+                xpSettings.require(Xp::DynamicDerivations);
+                node.childMap[outputId] = doInput(childNode);
+            }
+            return node;
+        };
         auto & inputDrvsObj = ensureType(valueAt(json, "inputDrvs"), value_t::object);
-        for (auto & [inputDrvPath, inputOutputs] : inputDrvsObj.items()) {
-            ensureType(inputOutputs, value_t::array);
-            res.inputDrvs[store.parseStorePath(inputDrvPath)] =
-                static_cast<const StringSet &>(inputOutputs);
-        }
+        for (auto & [inputDrvPath, inputOutputs] : inputDrvsObj.items())
+            res.inputDrvs.map[store.parseStorePath(inputDrvPath)] =
+                doInput(inputOutputs);
     } catch (Error & e) {
         e.addTrace({}, "while reading key 'inputDrvs'");
         throw;