Merge pull request #6229 from obsidiansystems/refactor-hash-modulo

Overhaul derivation hash modulo somewhat

5 files changed, 131 insertions, 72 deletions

diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index 3124025aa..0980e75f4 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -1207,32 +1207,37 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * *
         // hash per output.
         auto hashModulo = hashDerivationModulo(*state.store, Derivation(drv), true);
         std::visit(overloaded {
-            [&](Hash & h) {
-                for (auto & i : outputs) {
-                    auto outPath = state.store->makeOutputPath(i, h, drvName);
-                    drv.env[i] = state.store->printStorePath(outPath);
-                    drv.outputs.insert_or_assign(i,
-                        DerivationOutput {
-                            .output = DerivationOutputInputAddressed {
-                                .path = std::move(outPath),
-                            },
-                        });
+            [&](const DrvHash & drvHash) {
+                auto & h = drvHash.hash;
+                switch (drvHash.kind) {
+                case DrvHash::Kind::Deferred:
+                    for (auto & i : outputs) {
+                        drv.outputs.insert_or_assign(i,
+                            DerivationOutput {
+                                .output = DerivationOutputDeferred{},
+                            });
+                    }
+                    break;
+                case DrvHash::Kind::Regular:
+                    for (auto & i : outputs) {
+                        auto outPath = state.store->makeOutputPath(i, h, drvName);
+                        drv.env[i] = state.store->printStorePath(outPath);
+                        drv.outputs.insert_or_assign(i,
+                            DerivationOutput {
+                                .output = DerivationOutputInputAddressed {
+                                    .path = std::move(outPath),
+                                },
+                            });
+                    }
+                    break;
                 }
             },
-            [&](CaOutputHashes &) {
+            [&](const CaOutputHashes &) {
                 // Shouldn't happen as the toplevel derivation is not CA.
                 assert(false);
             },
-            [&](DeferredHash &) {
-                for (auto & i : outputs) {
-                    drv.outputs.insert_or_assign(i,
-                        DerivationOutput {
-                            .output = DerivationOutputDeferred{},
-                        });
-                }
-            },
         },
-        hashModulo);
+        hashModulo.raw());
 
     }
 
diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc
index a49be0057..1fe45bd87 100644
--- a/src/libstore/derivations.cc
+++ b/src/libstore/derivations.cc
@@ -510,7 +510,7 @@ static const DrvHashModulo pathDerivationModulo(Store & store, const StorePath &
  */
 DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool maskOutputs)
 {
-    bool isDeferred = false;
+    auto kind = DrvHash::Kind::Regular;
     /* Return a fixed hash for fixed-output derivations. */
     switch (drv.type()) {
     case DerivationType::CAFixed: {
@@ -526,7 +526,7 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m
         return outputHashes;
     }
     case DerivationType::CAFloating:
-        isDeferred = true;
+        kind = DrvHash::Kind::Deferred;
         break;
     case DerivationType::InputAddressed:
         break;
@@ -537,21 +537,20 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m
     /* For other derivations, replace the inputs paths with recursive
        calls to this function. */
     std::map<std::string, StringSet> inputs2;
-    for (auto & i : drv.inputDrvs) {
-        const auto & res = pathDerivationModulo(store, i.first);
+    for (auto & [drvPath, inputOutputs0] : drv.inputDrvs) {
+        // Avoid lambda capture restriction with standard / Clang
+        auto & inputOutputs = inputOutputs0;
+        const auto & res = pathDerivationModulo(store, drvPath);
         std::visit(overloaded {
             // Regular non-CA derivation, replace derivation
-            [&](const Hash & drvHash) {
-                inputs2.insert_or_assign(drvHash.to_string(Base16, false), i.second);
-            },
-            [&](const DeferredHash & deferredHash) {
-                isDeferred = true;
-                inputs2.insert_or_assign(deferredHash.hash.to_string(Base16, false), i.second);
+            [&](const DrvHash & drvHash) {
+                kind |= drvHash.kind;
+                inputs2.insert_or_assign(drvHash.hash.to_string(Base16, false), inputOutputs);
             },
             // CA derivation's output hashes
             [&](const CaOutputHashes & outputHashes) {
                 std::set<std::string> justOut = { "out" };
-                for (auto & output : i.second) {
+                for (auto & output : inputOutputs) {
                     /* Put each one in with a single "out" output.. */
                     const auto h = outputHashes.at(output);
                     inputs2.insert_or_assign(
@@ -559,15 +558,24 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m
                         justOut);
                 }
             },
-        }, res);
+        }, res.raw());
     }
 
     auto hash = hashString(htSHA256, drv.unparse(store, maskOutputs, &inputs2));
 
-    if (isDeferred)
-        return DeferredHash { hash };
-    else
-        return hash;
+    return DrvHash { .hash = hash, .kind = kind };
+}
+
+
+void operator |= (DrvHash::Kind & self, const DrvHash::Kind & other) noexcept
+{
+    switch (other) {
+    case DrvHash::Kind::Regular:
+        break;
+    case DrvHash::Kind::Deferred:
+        self = other;
+        break;
+    }
 }
 
 
@@ -575,20 +583,15 @@ std::map<std::string, Hash> staticOutputHashes(Store & store, const Derivation &
 {
     std::map<std::string, Hash> res;
     std::visit(overloaded {
-        [&](const Hash & drvHash) {
-            for (auto & outputName : drv.outputNames()) {
-                res.insert({outputName, drvHash});
-            }
-        },
-        [&](const DeferredHash & deferredHash) {
+        [&](const DrvHash & drvHash) {
             for (auto & outputName : drv.outputNames()) {
-                res.insert({outputName, deferredHash.hash});
+                res.insert({outputName, drvHash.hash});
             }
         },
         [&](const CaOutputHashes & outputHashes) {
             res = outputHashes;
         },
-    }, hashDerivationModulo(store, drv, true));
+    }, hashDerivationModulo(store, drv, true).raw());
     return res;
 }
 
@@ -738,7 +741,7 @@ static void rewriteDerivation(Store & store, BasicDerivation & drv, const String
     auto hashModulo = hashDerivationModulo(store, Derivation(drv), true);
     for (auto & [outputName, output] : drv.outputs) {
         if (std::holds_alternative<DerivationOutputDeferred>(output.output)) {
-            Hash h = std::get<Hash>(hashModulo);
+            auto & h = hashModulo.requireNoFixedNonDeferred();
             auto outPath = store.makeOutputPath(outputName, h, drv.name);
             drv.env[outputName] = store.printStorePath(outPath);
             output = DerivationOutput {
@@ -751,31 +754,51 @@ static void rewriteDerivation(Store & store, BasicDerivation & drv, const String
 
 }
 
+const Hash & DrvHashModulo::requireNoFixedNonDeferred() const {
+    auto * drvHashOpt = std::get_if<DrvHash>(&raw());
+    assert(drvHashOpt);
+    assert(drvHashOpt->kind == DrvHash::Kind::Regular);
+    return drvHashOpt->hash;
+}
+
+static bool tryResolveInput(
+    Store & store, StorePathSet & inputSrcs, StringMap & inputRewrites,
+    const StorePath & inputDrv, const StringSet & inputOutputs)
+{
+    auto inputDrvOutputs = store.queryPartialDerivationOutputMap(inputDrv);
+
+    auto getOutput = [&](const std::string & outputName) {
+        auto & actualPathOpt = inputDrvOutputs.at(outputName);
+        if (!actualPathOpt)
+            warn("output %s of input %s missing, aborting the resolving",
+                outputName,
+                store.printStorePath(inputDrv)
+            );
+        return actualPathOpt;
+    };
+
+    for (auto & outputName : inputOutputs) {
+        auto actualPathOpt = getOutput(outputName);
+        if (!actualPathOpt) return false;
+        auto actualPath = *actualPathOpt;
+        inputRewrites.emplace(
+            downstreamPlaceholder(store, inputDrv, outputName),
+            store.printStorePath(actualPath));
+        inputSrcs.insert(std::move(actualPath));
+    }
+
+    return true;
+}
+
 std::optional<BasicDerivation> Derivation::tryResolve(Store & store) {
     BasicDerivation resolved { *this };
 
     // Input paths that we'll want to rewrite in the derivation
     StringMap inputRewrites;
 
-    for (auto & input : inputDrvs) {
-        auto inputDrvOutputs = store.queryPartialDerivationOutputMap(input.first);
-        StringSet newOutputNames;
-        for (auto & outputName : input.second) {
-            auto actualPathOpt = inputDrvOutputs.at(outputName);
-            if (!actualPathOpt) {
-                warn("output %s of input %s missing, aborting the resolving",
-                    outputName,
-                    store.printStorePath(input.first)
-                );
-                return std::nullopt;
-            }
-            auto actualPath = *actualPathOpt;
-            inputRewrites.emplace(
-                downstreamPlaceholder(store, input.first, outputName),
-                store.printStorePath(actualPath));
-            resolved.inputSrcs.insert(std::move(actualPath));
-        }
-    }
+    for (auto & [inputDrv, inputOutputs] : inputDrvs)
+        if (!tryResolveInput(store, resolved.inputSrcs, inputRewrites, inputDrv, inputOutputs))
+            return std::nullopt;
 
     rewriteDerivation(store, resolved, inputRewrites);
 
diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh
index 132de82b6..2fb18d7f7 100644
--- a/src/libstore/derivations.hh
+++ b/src/libstore/derivations.hh
@@ -175,13 +175,43 @@ std::string outputPathName(std::string_view drvName, std::string_view outputName
 // whose output hashes are always known since they are fixed up-front.
 typedef std::map<std::string, Hash> CaOutputHashes;
 
-struct DeferredHash { Hash hash; };
+struct DrvHash {
+    Hash hash;
+
+    enum struct Kind {
+        // Statically determined derivations.
+        // This hash will be directly used to compute the output paths
+        Regular,
+        // Floating-output derivations (and their dependencies).
+        Deferred,
+    };
+
+    Kind kind;
+};
+
+void operator |= (DrvHash::Kind & self, const DrvHash::Kind & other) noexcept;
 
 typedef std::variant<
-    Hash, // regular DRV normalized hash
-    CaOutputHashes, // Fixed-output derivation hashes
-    DeferredHash // Deferred hashes for floating outputs drvs and their dependencies
-> DrvHashModulo;
+    // Regular normalized derivation hash, and whether it was deferred (because
+    // an ancestor derivation is a floating content addressed derivation).
+    DrvHash,
+    // Fixed-output derivation hashes
+    CaOutputHashes
+> DrvHashModuloRaw;
+
+struct DrvHashModulo : DrvHashModuloRaw {
+    using Raw = DrvHashModuloRaw;
+    using Raw::Raw;
+
+    /* Get hash, throwing if it is per-output CA hashes or a
+       deferred Drv hash.
+     */
+    const Hash & requireNoFixedNonDeferred() const;
+
+    inline const Raw & raw() const {
+        return static_cast<const Raw &>(*this);
+    }
+};
 
 /* Returns hashes with the details of fixed-output subderivations
    expunged.
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 1ee71b1c0..9230be15a 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -701,8 +701,8 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat
             [&](const DerivationOutputInputAddressed & doia) {
                 if (!h) {
                     // somewhat expensive so we do lazily
-                    auto temp = hashDerivationModulo(*this, drv, true);
-                    h = std::get<Hash>(temp);
+                    auto h0 = hashDerivationModulo(*this, drv, true);
+                    h = h0.requireNoFixedNonDeferred();
                 }
                 StorePath recomputed = makeOutputPath(i.first, *h, drvName);
                 if (doia.path != recomputed)
diff --git a/src/nix/develop.cc b/src/nix/develop.cc
index 8af5da9d0..dafcafd79 100644
--- a/src/nix/develop.cc
+++ b/src/nix/develop.cc
@@ -206,7 +206,8 @@ static StorePath getDerivationEnvironment(ref<Store> store, ref<Store> evalStore
             output.second = { .output = DerivationOutputInputAddressed { .path = StorePath::dummy } };
             drv.env[output.first] = "";
         }
-        Hash h = std::get<0>(hashDerivationModulo(*evalStore, drv, true));
+        auto h0 = hashDerivationModulo(*evalStore, drv, true);
+        const Hash & h = h0.requireNoFixedNonDeferred();
 
         for (auto & output : drv.outputs) {
             auto outPath = store->makeOutputPath(output.first, h, drv.name);