Merge pull request #6268 from thufschmitt/remove-the-variant-in-hashmodulo

Simplify the handling of the hash modulo

src/libexpr/primops.cc

@@ -1222,34 +1222,26 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * *
                 DerivationOutput::Deferred { });
         }
 
-        // Regular, non-CA derivation should always return a single hash and not
+        auto hashModulo = hashDerivationModulo(*state.store, Derivation(drv), true);
-        // hash per output.
+        switch (hashModulo.kind) {
-        auto hashModulo = hashDerivationModulo(*state.store, drv, true);
-        std::visit(overloaded {
-            [&](const DrvHash & drvHash) {
-                auto & h = drvHash.hash;
-                switch (drvHash.kind) {
-                case DrvHash::Kind::Deferred:
-                    /* Outputs already deferred, nothing to do */
-                    break;
         case DrvHash::Kind::Regular:
-                    for (auto & [outputName, output] : drv.outputs) {
+            for (auto & i : outputs) {
-                        auto outPath = state.store->makeOutputPath(outputName, h, drvName);
+                auto h = hashModulo.hashes.at(i);
-                        drv.env[outputName] = state.store->printStorePath(outPath);
+                auto outPath = state.store->makeOutputPath(i, h, drvName);
-                        output = DerivationOutput::InputAddressed {
+                drv.env[i] = state.store->printStorePath(outPath);
+                drv.outputs.insert_or_assign(
+                    i,
+                    DerivationOutputInputAddressed {
                         .path = std::move(outPath),
-                        };
+                    });
             }
             break;
+            ;
+        case DrvHash::Kind::Deferred:
+            for (auto & i : outputs) {
+                drv.outputs.insert_or_assign(i, DerivationOutputDeferred {});
+            }
         }
-            },
-            [&](const CaOutputHashes &) {
-                // Shouldn't happen as the toplevel derivation is not CA.
-                assert(false);
-            },
-        },
-        hashModulo.raw());
-
     }
 
     /* Write the resulting term into the Nix store directory. */

src/libstore/derivations.cc

@@ -474,7 +474,7 @@ Sync<DrvHashes> drvHashes;
 /* Look up the derivation by value and memoize the
    `hashDerivationModulo` call.
  */
-static const DrvHashModulo pathDerivationModulo(Store & store, const StorePath & drvPath)
+static const DrvHash pathDerivationModulo(Store & store, const StorePath & drvPath)
 {
     {
         auto hashes = drvHashes.lock();
@@ -509,7 +509,7 @@ static const DrvHashModulo pathDerivationModulo(Store & store, const StorePath &
    don't leak the provenance of fixed outputs, reducing pointless cache
    misses as the build itself won't know this.
  */
-DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool maskOutputs)
+DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOutputs)
 {
     auto type = drv.type();
 
@@ -524,7 +524,10 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m
                 + store.printStorePath(dof.path(store, drv.name, i.first)));
             outputHashes.insert_or_assign(i.first, std::move(hash));
         }
-        return outputHashes;
+        return DrvHash{
+            .hashes = outputHashes,
+            .kind = DrvHash::Kind::Regular,
+        };
     }
 
     auto kind = std::visit(overloaded {
@@ -540,65 +543,36 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m
         },
     }, drv.type().raw());
 
-    /* For other derivations, replace the inputs paths with recursive
-       calls to this function. */
     std::map<std::string, StringSet> inputs2;
     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 {
+        if (res.kind == DrvHash::Kind::Deferred)
-            // Regular non-CA derivation, replace derivation
+            kind = DrvHash::Kind::Deferred;
-            [&](const DrvHash & drvHash) {
+        for (auto & outputName : inputOutputs) {
-                kind |= drvHash.kind;
+            const auto h = res.hashes.at(outputName);
-                inputs2.insert_or_assign(drvHash.hash.to_string(Base16, false), inputOutputs);
+            inputs2[h.to_string(Base16, false)].insert(outputName);
-            },
-            // CA derivation's output hashes
-            [&](const CaOutputHashes & outputHashes) {
-                std::set<std::string> justOut = { "out" };
-                for (auto & output : inputOutputs) {
-                    /* Put each one in with a single "out" output.. */
-                    const auto h = outputHashes.at(output);
-                    inputs2.insert_or_assign(
-                        h.to_string(Base16, false),
-                        justOut);
         }
-            },
-        }, res.raw());
     }
 
     auto hash = hashString(htSHA256, drv.unparse(store, maskOutputs, &inputs2));
 
-    return DrvHash { .hash = hash, .kind = kind };
+    std::map<std::string, Hash> outputHashes;
-}
+    for (const auto & [outputName, _] : drv.outputs) {
-
+        outputHashes.insert_or_assign(outputName, hash);
-
-void operator |= (DrvHash::Kind & self, const DrvHash::Kind & other) noexcept
-{
-    switch (other) {
-    case DrvHash::Kind::Regular:
-        break;
-    case DrvHash::Kind::Deferred:
-        self = other;
-        break;
     }
+
+    return DrvHash {
+        .hashes = outputHashes,
+        .kind = kind,
+    };
 }
 
 
 std::map<std::string, Hash> staticOutputHashes(Store & store, const Derivation & drv)
 {
-    std::map<std::string, Hash> res;
+    return hashDerivationModulo(store, drv, true).hashes;
-    std::visit(overloaded {
-        [&](const DrvHash & drvHash) {
-            for (auto & outputName : drv.outputNames()) {
-                res.insert({outputName, drvHash.hash});
-            }
-        },
-        [&](const CaOutputHashes & outputHashes) {
-            res = outputHashes;
-        },
-    }, hashDerivationModulo(store, drv, true).raw());
-    return res;
 }
 
 
@@ -747,7 +721,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<DerivationOutput::Deferred>(output.raw())) {
-            auto & h = hashModulo.requireNoFixedNonDeferred();
+            auto & h = hashModulo.hashes.at(outputName);
             auto outPath = store.makeOutputPath(outputName, h, drv.name);
             drv.env[outputName] = store.printStorePath(outPath);
             output = DerivationOutput::InputAddressed {
@@ -758,13 +732,6 @@ 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)

src/libstore/derivations.hh

@@ -202,12 +202,14 @@ bool isDerivation(const std::string & fileName);
    the output name is "out". */
 std::string outputPathName(std::string_view drvName, std::string_view outputName);
 
-// known CA drv's output hashes, current just for fixed-output derivations
-// whose output hashes are always known since they are fixed up-front.
-typedef std::map<std::string, Hash> CaOutputHashes;
 
+// The hashes modulo of a derivation.
+//
+// Each output is given a hash, although in practice only the content-addressed
+// derivations (fixed-output or not) will have a different hash for each
+// output.
 struct DrvHash {
-    Hash hash;
+    std::map<std::string, Hash> hashes;
 
     enum struct Kind: bool {
         // Statically determined derivations.
@@ -222,28 +224,6 @@ struct DrvHash {
 
 void operator |= (DrvHash::Kind & self, const DrvHash::Kind & other) noexcept;
 
-typedef std::variant<
-    // 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.
 
@@ -267,7 +247,7 @@ struct DrvHashModulo : _DrvHashModuloRaw {
    ATerm, after subderivations have been likewise expunged from that
    derivation.
  */
-DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool maskOutputs);
+DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOutputs);
 
 /*
    Return a map associating each output to a hash that uniquely identifies its
@@ -276,7 +256,7 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m
 std::map<std::string, Hash> staticOutputHashes(Store& store, const Derivation& drv);
 
 /* Memoisation of hashDerivationModulo(). */
-typedef std::map<StorePath, DrvHashModulo> DrvHashes;
+typedef std::map<StorePath, DrvHash> DrvHashes;
 
 // FIXME: global, though at least thread-safe.
 extern Sync<DrvHashes> drvHashes;

src/libstore/local-store.cc

@@ -695,16 +695,15 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat
     // combinations that are currently prohibited.
     drv.type();
 
-    std::optional<Hash> h;
+    std::optional<DrvHash> hashesModulo;
     for (auto & i : drv.outputs) {
         std::visit(overloaded {
             [&](const DerivationOutput::InputAddressed & doia) {
-                if (!h) {
+                if (!hashesModulo) {
                     // somewhat expensive so we do lazily
-                    auto h0 = hashDerivationModulo(*this, drv, true);
+                    hashesModulo = hashDerivationModulo(*this, drv, true);
-                    h = h0.requireNoFixedNonDeferred();
                 }
-                StorePath recomputed = makeOutputPath(i.first, *h, drvName);
+                StorePath recomputed = makeOutputPath(i.first, hashesModulo->hashes.at(i.first), drvName);
                 if (doia.path != recomputed)
                     throw Error("derivation '%s' has incorrect output '%s', should be '%s'",
                         printStorePath(drvPath), printStorePath(doia.path), printStorePath(recomputed));

src/nix/develop.cc

@@ -204,10 +204,10 @@ static StorePath getDerivationEnvironment(ref<Store> store, ref<Store> evalStore
             output.second = DerivationOutput::Deferred { };
             drv.env[output.first] = "";
         }
-        auto h0 = hashDerivationModulo(*evalStore, drv, true);
+        auto hashesModulo = hashDerivationModulo(*evalStore, drv, true);
-        const Hash & h = h0.requireNoFixedNonDeferred();
 
         for (auto & output : drv.outputs) {
+            Hash h = hashesModulo.hashes.at(output.first);
             auto outPath = store->makeOutputPath(output.first, h, drv.name);
             output.second = DerivationOutput::InputAddressed {
                 .path = outPath,