From c092fa4702215fdb61611c5dd28194401d056170 Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 23 Sep 2020 16:30:42 +0200 Subject: [PATCH 1/3] Allow non-CA derivations to depend on CA derivations --- src/libexpr/primops.cc | 39 +++++++++++----- src/libstore/build/derivation-goal.cc | 12 ++++- src/libstore/derivations.cc | 65 ++++++++++++++++++++++++--- src/libstore/derivations.hh | 14 +++++- src/libstore/local-store.cc | 4 +- src/nix/show-derivation.cc | 1 + tests/content-addressed.nix | 11 ++++- tests/content-addressed.sh | 1 + 8 files changed, 124 insertions(+), 23 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 2b304aab0fb..236433ef1d7 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1089,18 +1089,35 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * // Regular, non-CA derivation should always return a single hash and not // hash per output. - Hash h = std::get<0>(hashDerivationModulo(*state.store, Derivation(drv), true)); + 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), + }, + }); + } + }, + [&](CaOutputHashes) { + // Shouldn't happen as the toplevel derivation is not CA. + assert(false); + }, + [&](UnknownHashes) { + for (auto & i : outputs) { + drv.outputs.insert_or_assign(i, + DerivationOutput { + .output = DerivationOutputDeferred{}, + }); + } + }, + }, + hashModulo); - 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), - }, - }); - } } /* Write the resulting term into the Nix store directory. */ diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index db0c2bb6ce9..cc8737fd58b 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -493,7 +493,8 @@ void DerivationGoal::inputsRealised() if (useDerivation) { auto & fullDrv = *dynamic_cast(drv.get()); - if (!fullDrv.inputDrvs.empty() && fullDrv.type() == DerivationType::CAFloating) { + if ((!fullDrv.inputDrvs.empty() && + fullDrv.type() == DerivationType::CAFloating) || fullDrv.type() == DerivationType::DeferredInputAddressed) { /* We are be able to resolve this derivation based on the now-known results of dependencies. If so, we become a stub goal aliasing that resolved derivation goal */ @@ -3166,6 +3167,15 @@ void DerivationGoal::registerOutputs() [&](DerivationOutputCAFloating dof) { return newInfoFromCA(dof); }, + [&](DerivationOutputDeferred) { + // No derivation should reach that point without having been + // rewritten first + assert(false); + // Ugly, but the compiler insists on having this return a value + // of type `ValidPathInfo` despite the `assert(false)`, so + // let's provide it + return *(ValidPathInfo*)0; + }, }, output.output); /* Calculate where we'll move the output files. In the checking case we diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 07b4e772b9b..3b3a2539149 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -21,6 +21,9 @@ std::optional DerivationOutput::path(const Store & store, std::string [](DerivationOutputCAFloating dof) -> std::optional { return std::nullopt; }, + [](DerivationOutputDeferred) -> std::optional { + return std::nullopt; + }, }, output); } @@ -37,6 +40,7 @@ bool derivationIsCA(DerivationType dt) { case DerivationType::InputAddressed: return false; case DerivationType::CAFixed: return true; case DerivationType::CAFloating: return true; + case DerivationType::DeferredInputAddressed: return false; }; // Since enums can have non-variant values, but making a `default:` would // disable exhaustiveness warnings. @@ -48,6 +52,7 @@ bool derivationIsFixed(DerivationType dt) { case DerivationType::InputAddressed: return false; case DerivationType::CAFixed: return true; case DerivationType::CAFloating: return false; + case DerivationType::DeferredInputAddressed: return false; }; assert(false); } @@ -57,6 +62,7 @@ bool derivationIsImpure(DerivationType dt) { case DerivationType::InputAddressed: return false; case DerivationType::CAFixed: return true; case DerivationType::CAFloating: return false; + case DerivationType::DeferredInputAddressed: return false; }; assert(false); } @@ -180,6 +186,11 @@ static DerivationOutput parseDerivationOutput(const Store & store, }; } } else { + if (pathS == "") { + return DerivationOutput { + .output = DerivationOutputDeferred { } + }; + } validatePath(pathS); return DerivationOutput { .output = DerivationOutputInputAddressed { @@ -325,6 +336,11 @@ string Derivation::unparse(const Store & store, bool maskOutputs, s += ','; printUnquotedString(s, makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType)); s += ','; printUnquotedString(s, ""); }, + [&](DerivationOutputDeferred) { + s += ','; printUnquotedString(s, ""); + s += ','; printUnquotedString(s, ""); + s += ','; printUnquotedString(s, ""); + } }, i.second.output); s += ')'; } @@ -389,7 +405,7 @@ std::string outputPathName(std::string_view drvName, std::string_view outputName DerivationType BasicDerivation::type() const { - std::set inputAddressedOutputs, fixedCAOutputs, floatingCAOutputs; + std::set inputAddressedOutputs, fixedCAOutputs, floatingCAOutputs, deferredIAOutputs; std::optional floatingHashType; for (auto & i : outputs) { std::visit(overloaded { @@ -408,22 +424,27 @@ DerivationType BasicDerivation::type() const throw Error("All floating outputs must use the same hash type"); } }, + [&](DerivationOutputDeferred _) { + deferredIAOutputs.insert(i.first); + }, }, i.second.output); } - if (inputAddressedOutputs.empty() && fixedCAOutputs.empty() && floatingCAOutputs.empty()) { + if (inputAddressedOutputs.empty() && fixedCAOutputs.empty() && floatingCAOutputs.empty() && deferredIAOutputs.empty()) { throw Error("Must have at least one output"); - } else if (! inputAddressedOutputs.empty() && fixedCAOutputs.empty() && floatingCAOutputs.empty()) { + } else if (! inputAddressedOutputs.empty() && fixedCAOutputs.empty() && floatingCAOutputs.empty() && deferredIAOutputs.empty()) { return DerivationType::InputAddressed; - } else if (inputAddressedOutputs.empty() && ! fixedCAOutputs.empty() && floatingCAOutputs.empty()) { + } else if (inputAddressedOutputs.empty() && ! fixedCAOutputs.empty() && floatingCAOutputs.empty() && deferredIAOutputs.empty()) { if (fixedCAOutputs.size() > 1) // FIXME: Experimental feature? throw Error("Only one fixed output is allowed for now"); if (*fixedCAOutputs.begin() != "out") throw Error("Single fixed output must be named \"out\""); return DerivationType::CAFixed; - } else if (inputAddressedOutputs.empty() && fixedCAOutputs.empty() && ! floatingCAOutputs.empty()) { + } else if (inputAddressedOutputs.empty() && fixedCAOutputs.empty() && ! floatingCAOutputs.empty() && deferredIAOutputs.empty()) { return DerivationType::CAFloating; + } else if (inputAddressedOutputs.empty() && fixedCAOutputs.empty() && floatingCAOutputs.empty() && !deferredIAOutputs.empty()) { + return DerivationType::DeferredInputAddressed; } else { throw Error("Can't mix derivation output types"); } @@ -454,6 +475,8 @@ static const DrvHashModulo & pathDerivationModulo(Store & store, const StorePath return h->second; } +UnknownHashes unknownHashes; + /* See the header for interface details. These are the implementation details. For fixed-output derivations, each hash in the map is not the @@ -476,7 +499,7 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m /* Return a fixed hash for fixed-output derivations. */ switch (drv.type()) { case DerivationType::CAFloating: - throw Error("Regular input-addressed derivations are not yet allowed to depend on CA derivations"); + return unknownHashes; case DerivationType::CAFixed: { std::map outputHashes; for (const auto & i : drv.outputs) { @@ -491,12 +514,15 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m } case DerivationType::InputAddressed: break; + case DerivationType::DeferredInputAddressed: + break; } /* For other derivations, replace the inputs paths with recursive calls to this function. */ std::map inputs2; for (auto & i : drv.inputDrvs) { + bool hasUnknownHash = false; const auto & res = pathDerivationModulo(store, i.first); std::visit(overloaded { // Regular non-CA derivation, replace derivation @@ -514,7 +540,13 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m justOut); } }, + [&](UnknownHashes) { + hasUnknownHash = true; + }, }, res); + if (hasUnknownHash) { + return unknownHashes; + } } return hashString(htSHA256, drv.unparse(store, maskOutputs, &inputs2)); @@ -620,6 +652,11 @@ void writeDerivation(Sink & out, const Store & store, const BasicDerivation & dr << (makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType)) << ""; }, + [&](DerivationOutputDeferred) { + out << "" + << "" + << ""; + }, }, i.second.output); } worker_proto::write(store, out, drv.inputSrcs); @@ -645,7 +682,6 @@ std::string downstreamPlaceholder(const Store & store, const StorePath & drvPath } -// N.B. Outputs are left unchanged static void rewriteDerivation(Store & store, BasicDerivation & drv, const StringMap & rewrites) { debug("Rewriting the derivation"); @@ -666,6 +702,21 @@ static void rewriteDerivation(Store & store, BasicDerivation & drv, const String newEnv.emplace(envName, envValue); } drv.env = newEnv; + + auto hashModulo = hashDerivationModulo(store, Derivation(drv), true); + for (auto & [outputName, output] : drv.outputs) { + if (std::holds_alternative(output.output)) { + Hash h = std::get(hashModulo); + auto outPath = store.makeOutputPath(outputName, h, drv.name); + drv.env[outputName] = store.printStorePath(outPath); + output = DerivationOutput { + .output = DerivationOutputInputAddressed { + .path = std::move(outPath), + }, + }; + } + } + } diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 6d292b2e54e..f9ba935e66b 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -41,12 +41,18 @@ struct DerivationOutputCAFloating HashType hashType; }; +/* Input-addressed output which depends on a (CA) derivation whose hash isn't + * known atm + */ +struct DerivationOutputDeferred {}; + struct DerivationOutput { std::variant< DerivationOutputInputAddressed, DerivationOutputCAFixed, - DerivationOutputCAFloating + DerivationOutputCAFloating, + DerivationOutputDeferred > output; std::optional hashAlgoOpt(const Store & store) const; /* Note, when you use this function you should make sure that you're passing @@ -72,6 +78,7 @@ typedef std::map StringPairs; enum struct DerivationType : uint8_t { InputAddressed, + DeferredInputAddressed, CAFixed, CAFloating, }; @@ -167,9 +174,12 @@ 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 CaOutputHashes; +struct UnknownHashes {}; + typedef std::variant< Hash, // regular DRV normalized hash - CaOutputHashes + CaOutputHashes, // Fixed-output derivation hashes + UnknownHashes // Deferred hashes for floating outputs drvs and their dependencies > DrvHashModulo; /* Returns hashes with the details of fixed-output subderivations diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index d29236a9c84..d29a681798b 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -573,6 +573,8 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat [&](DerivationOutputCAFloating _) { /* Nothing to check */ }, + [&](DerivationOutputDeferred) { + }, }, i.second.output); } } @@ -817,7 +819,7 @@ std::map> LocalStore::queryPartialDerivati } /* can't just use else-if instead of `!haveCached` because we need to unlock `drvPathResolutions` before it is locked in `Derivation::resolve`. */ - if (!haveCached && drv.type() == DerivationType::CAFloating) { + if (!haveCached && (drv.type() == DerivationType::CAFloating || drv.type() == DerivationType::DeferredInputAddressed)) { /* Try resolve drv and use that path instead. */ auto attempt = drv.tryResolve(*this); if (!attempt) diff --git a/src/nix/show-derivation.cc b/src/nix/show-derivation.cc index 2542537d399..6d4f295d7e5 100644 --- a/src/nix/show-derivation.cc +++ b/src/nix/show-derivation.cc @@ -82,6 +82,7 @@ struct CmdShowDerivation : InstallablesCommand [&](DerivationOutputCAFloating dof) { outputObj.attr("hashAlgo", makeFileIngestionPrefix(dof.method) + printHashType(dof.hashType)); }, + [&](DerivationOutputDeferred) {}, }, output.output); } } diff --git a/tests/content-addressed.nix b/tests/content-addressed.nix index 3dcf916c3ca..8ca96d4bf1a 100644 --- a/tests/content-addressed.nix +++ b/tests/content-addressed.nix @@ -15,7 +15,7 @@ rec { ''; }; rootCA = mkDerivation { - name = "dependent"; + name = "rootCA"; outputs = [ "out" "dev" ]; buildCommand = '' echo "building a CA derivation" @@ -51,4 +51,13 @@ rec { outputHashMode = "recursive"; outputHashAlgo = "sha256"; }; + dependentNonCA = mkDerivation { + name = "dependent-non-ca"; + buildCommand = '' + echo "Didn't cut-off" + echo "building dependent-non-ca" + mkdir -p $out + echo ${rootCA}/non-ca-hello > $out/dep + ''; + }; } diff --git a/tests/content-addressed.sh b/tests/content-addressed.sh index 61ec03fe3cd..547919660ad 100644 --- a/tests/content-addressed.sh +++ b/tests/content-addressed.sh @@ -22,6 +22,7 @@ secondSeedArgs=(-j0) # dependent derivations always being already built. #testDerivation dependentCA testDerivation transitivelyDependentCA +testDerivation dependentNonCA nix-instantiate --experimental-features ca-derivations ./content-addressed.nix -A rootCA --arg seed 5 nix-collect-garbage --experimental-features ca-derivations --option keep-derivations true From bc081bcd816542d66f1578788b93df4d7e07b135 Mon Sep 17 00:00:00 2001 From: regnat Date: Thu, 24 Sep 2020 10:11:58 +0200 Subject: [PATCH 2/3] Inline `unkownHashes` See https://github.com/NixOS/nix/pull/4056#discussion_r493661632 --- src/libstore/derivations.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 3b3a2539149..517ecfaa205 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -475,8 +475,6 @@ static const DrvHashModulo & pathDerivationModulo(Store & store, const StorePath return h->second; } -UnknownHashes unknownHashes; - /* See the header for interface details. These are the implementation details. For fixed-output derivations, each hash in the map is not the @@ -499,7 +497,7 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m /* Return a fixed hash for fixed-output derivations. */ switch (drv.type()) { case DerivationType::CAFloating: - return unknownHashes; + return UnknownHashes {}; case DerivationType::CAFixed: { std::map outputHashes; for (const auto & i : drv.outputs) { @@ -545,7 +543,7 @@ DrvHashModulo hashDerivationModulo(Store & store, const Derivation & drv, bool m }, }, res); if (hasUnknownHash) { - return unknownHashes; + return UnknownHashes {}; } } From ab21ab65016275c224d1d40c42bdfed80dfbcbb0 Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 7 Oct 2020 12:24:53 +0200 Subject: [PATCH 3/3] Test the remote caching of non-ca-depending-on-ca derivations Although the non-resolved derivation will never get a cache-hit (it doesn't have an output path to query the cache for anyways), we might get one on the resolved derivation. --- tests/content-addressed.sh | 57 +++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/tests/content-addressed.sh b/tests/content-addressed.sh index 547919660ad..bdab09c86c6 100644 --- a/tests/content-addressed.sh +++ b/tests/content-addressed.sh @@ -5,24 +5,49 @@ source common.sh drv=$(nix-instantiate --experimental-features ca-derivations ./content-addressed.nix -A rootCA --arg seed 1) nix --experimental-features 'nix-command ca-derivations' show-derivation --derivation "$drv" --arg seed 1 -testDerivation () { +buildAttr () { local derivationPath=$1 - local commonArgs=("--experimental-features" "ca-derivations" "./content-addressed.nix" "-A" "$derivationPath" "--no-out-link") + shift + local args=("--experimental-features" "ca-derivations" "./content-addressed.nix" "-A" "$derivationPath" "--no-out-link") + args+=("$@") + nix-build "${args[@]}" +} + +testRemoteCache () { + clearCache + local outPath=$(buildAttr dependentNonCA) + nix copy --to file://$cacheDir $outPath + clearStore + buildAttr dependentNonCA --option substituters file://$cacheDir --no-require-sigs |& (! grep "building dependent-non-ca") +} + +testDeterministicCA () { + [[ $(buildAttr rootCA) = $(buildAttr rootCA) ]] +} + +testCutoffFor () { local out1 out2 - out1=$(nix-build "${commonArgs[@]}" --arg seed 1) - out2=$(nix-build "${commonArgs[@]}" --arg seed 2 "${secondSeedArgs[@]}") + out1=$(buildAttr $1) + # The seed only changes the root derivation, and not it's output, so the + # dependent derivations should only need to be built once. + out2=$(buildAttr $1 -j0) test "$out1" == "$out2" } -testDerivation rootCA -# The seed only changes the root derivation, and not it's output, so the -# dependent derivations should only need to be built once. -secondSeedArgs=(-j0) -# Don't directly build depenentCA, that way we'll make sure we dodn't rely on -# dependent derivations always being already built. -#testDerivation dependentCA -testDerivation transitivelyDependentCA -testDerivation dependentNonCA - -nix-instantiate --experimental-features ca-derivations ./content-addressed.nix -A rootCA --arg seed 5 -nix-collect-garbage --experimental-features ca-derivations --option keep-derivations true +testCutoff () { + # Don't directly build depenentCA, that way we'll make sure we dodn't rely on + # dependent derivations always being already built. + #testDerivation dependentCA + testCutoffFor transitivelyDependentCA + testCutoffFor dependentNonCA +} + +testGC () { + nix-instantiate --experimental-features ca-derivations ./content-addressed.nix -A rootCA --arg seed 5 + nix-collect-garbage --experimental-features ca-derivations --option keep-derivations true +} + +testRemoteCache +testDeterministicCA +testCutoff +testGC