From 0b71da23e8306fff2633ae3192d223a307b4b219 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 15 Jan 2023 18:58:56 -0500 Subject: [PATCH] Revert "Simplify the handling of the hash modulo" This reverts commit 390269ed8784b1a73a3310e63eb96a4b62861654. --- src/libexpr/primops.cc | 53 +++++++++-------- src/libstore/derivations.cc | 114 +++++++++++++++++++++--------------- src/libstore/derivations.hh | 41 +++++++++---- src/nix/develop.cc | 4 +- 4 files changed, 125 insertions(+), 87 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 89b5de222802..b8fba53c7c51 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1355,31 +1355,34 @@ drvName, Bindings * attrs, Value & v) DerivationOutput::Deferred { }); } - auto hashModulo = hashDerivationModulo(*state.store, Derivation(drv), true); - switch (hashModulo.kind) { - case DrvHash::Kind::Regular: - for (auto & i : outputs) { - auto h = get(hashModulo.hashes, i); - if (!h) - throw AssertionError({ - .msg = hintfmt("derivation produced no hash for output '%s'", i), - .errPos = state.positions[noPos], - }); - auto outPath = state.store->makeOutputPath(i, *h, drvName); - 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 {}); - } - } + // Regular, non-CA derivation should always return a single hash and not + // hash per output. + 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) { + auto outPath = state.store->makeOutputPath(outputName, h, drvName); + drv.env[outputName] = state.store->printStorePath(outPath); + output = DerivationOutput::InputAddressed { + .path = std::move(outPath), + }; + } + break; + } + }, + [&](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. */ diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index b831b2fe5360..ae1956d00441 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -583,7 +583,7 @@ Sync drvHashes; /* Look up the derivation by value and memoize the `hashDerivationModulo` call. */ -static const DrvHash pathDerivationModulo(Store & store, const StorePath & drvPath) +static const DrvHashModulo pathDerivationModulo(Store & store, const StorePath & drvPath) { { auto hashes = drvHashes.lock(); @@ -618,7 +618,7 @@ static const DrvHash pathDerivationModulo(Store & store, const StorePath & drvPa don't leak the provenance of fixed outputs, reducing pointless cache misses as the build itself won't know this. */ -DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOutputs) +DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool maskOutputs) { auto type = drv.type(); @@ -633,20 +633,7 @@ DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOut + store.printStorePath(dof.path(store, drv.name, i.first))); outputHashes.insert_or_assign(i.first, std::move(hash)); } - return DrvHash { - .hashes = outputHashes, - .kind = DrvHash::Kind::Regular, - }; - } - - if (!type.isPure()) { - std::map outputHashes; - for (const auto & [outputName, _] : drv.outputs) - outputHashes.insert_or_assign(outputName, impureOutputHash); - return DrvHash { - .hashes = outputHashes, - .kind = DrvHash::Kind::Deferred, - }; + return outputHashes; } auto kind = std::visit(overloaded { @@ -661,42 +648,71 @@ DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOut : DrvHash::Kind::Deferred; }, [](const DerivationType::Impure &) -> DrvHash::Kind { - assert(false); + return DrvHash::Kind::Deferred; } }, drv.type().raw()); + /* For other derivations, replace the inputs paths with recursive + calls to this function. */ std::map inputs2; for (auto & [drvPath, inputOutputs0] : drv.inputDrvs) { // Avoid lambda capture restriction with standard / Clang auto & inputOutputs = inputOutputs0; const auto & res = pathDerivationModulo(store, drvPath); - if (res.kind == DrvHash::Kind::Deferred) - kind = DrvHash::Kind::Deferred; - for (auto & outputName : inputOutputs) { - 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); - } + std::visit(overloaded { + // Regular non-CA derivation, replace derivation + [&](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 justOut = { "out" }; + for (auto & outputName : inputOutputs) { + /* Put each one in with a single "out" output.. */ + const auto h = get(outputHashes, outputName); + if (!h) + throw Error("no hash for output '%s' of derivation '%s'", outputName, drv.name); + inputs2.insert_or_assign( + h->to_string(Base16, false), + justOut); + } + }, + }, res.raw()); } auto hash = hashString(htSHA256, drv.unparse(store, maskOutputs, &inputs2)); - std::map outputHashes; - for (const auto & [outputName, _] : drv.outputs) { - outputHashes.insert_or_assign(outputName, hash); - } + return DrvHash { .hash = hash, .kind = kind }; +} - return DrvHash { - .hashes = outputHashes, - .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; + } } std::map staticOutputHashes(Store & store, const Derivation & drv) { - return hashDerivationModulo(store, drv, true).hashes; + std::map res; + 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; } @@ -841,11 +857,8 @@ 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(output.raw())) { - auto h = get(hashModulo.hashes, outputName); - if (!h) - throw Error("derivation '%s' output '%s' has no hash (derivations.cc/rewriteDerivation)", - drv.name, outputName); - auto outPath = store.makeOutputPath(outputName, *h, drv.name); + auto & h = hashModulo.requireNoFixedNonDeferred(); + auto outPath = store.makeOutputPath(outputName, h, drv.name); drv.env[outputName] = store.printStorePath(outPath); output = DerivationOutput::InputAddressed { .path = std::move(outPath), @@ -855,6 +868,16 @@ static void rewriteDerivation(Store & store, BasicDerivation & drv, const String } +const Hash & DrvHashModulo::requireNoFixedNonDeferred() const { + auto * drvHashOpt = std::get_if(&raw()); + // FIXME say which derivation + if (!drvHashOpt) + throw Error("derivation has improper sort of hashing (derivations.cc/requireNoFixedNonDeferred)"); + if (drvHashOpt->kind != DrvHash::Kind::Regular) + throw Error("derivation has as deferred hash when it should have a regular one (derivations.cc/requireNoFixedNonDeferred)"); + return drvHashOpt->hash; +} + std::optional Derivation::tryResolve(Store & store) const { std::map, StorePath> inputDrvOutputs; @@ -922,19 +945,16 @@ void Derivation::checkInvariants(Store & store, const StorePath & drvPath) const // combinations that are currently prohibited. type(); - std::optional hashesModulo; + std::optional h; for (auto & i : outputs) { std::visit(overloaded { [&](const DerivationOutput::InputAddressed & doia) { - if (!hashesModulo) { + if (!h) { // somewhat expensive so we do lazily - hashesModulo = hashDerivationModulo(store, *this, true); + auto h0 = hashDerivationModulo(store, *this, true); + h = h0.requireNoFixedNonDeferred(); } - auto currentOutputHash = get(hashesModulo->hashes, i.first); - if (!currentOutputHash) - throw Error("derivation '%s' has unexpected output '%s' (local-store / hashesModulo) named '%s'", - store.printStorePath(drvPath), store.printStorePath(doia.path), i.first); - StorePath recomputed = store.makeOutputPath(i.first, *currentOutputHash, drvName); + StorePath recomputed = store.makeOutputPath(i.first, *h, drvName); if (doia.path != recomputed) throw Error("derivation '%s' has incorrect output '%s', should be '%s'", store.printStorePath(drvPath), store.printStorePath(doia.path), store.printStorePath(recomputed)); diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index fa79f77fd0be..47ede9bcac95 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -402,19 +402,12 @@ bool isDerivation(std::string_view fileName); */ 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 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 { - /** - * Map from output names to hashes - */ - std::map hashes; + Hash hash; enum struct Kind : bool { /** @@ -438,6 +431,28 @@ 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(*this); + } +}; + /** * Returns hashes with the details of fixed-output subderivations * expunged. @@ -462,7 +477,7 @@ void operator |= (DrvHash::Kind & self, const DrvHash::Kind & other) noexcept; * ATerm, after subderivations have been likewise expunged from that * derivation. */ -DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOutputs); +DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool maskOutputs); /** * Return a map associating each output to a hash that uniquely identifies its @@ -475,7 +490,7 @@ std::map staticOutputHashes(Store & store, const Derivation & /** * Memoisation of hashDerivationModulo(). */ -typedef std::map DrvHashes; +typedef std::map DrvHashes; // FIXME: global, though at least thread-safe. extern Sync drvHashes; diff --git a/src/nix/develop.cc b/src/nix/develop.cc index c033804e4a07..2c2b6493503d 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -218,10 +218,10 @@ static StorePath getDerivationEnvironment(ref store, ref evalStore output.second = DerivationOutput::Deferred { }; drv.env[output.first] = ""; } - auto hashesModulo = hashDerivationModulo(*evalStore, drv, true); + auto h0 = 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,