From ea30f152b738fe65f216f3173d3a19adaaef5323 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 19 May 2023 10:48:53 +0200 Subject: [PATCH] fetchClosure: Allow input addressed paths in pure mode When explicitly requested by the caller, as suggested in the meeting (https://github.com/NixOS/nix/pull/8090#issuecomment-1531139324) > @edolstra: { toPath } vs { fromPath } is too implicit I've opted for the `inputAddressed = true` requirement, because it we did not agree on renaming the path attributes. > @roberth: more explicit > @edolstra: except for the direction; not immediately clear in which direction the rewriting happens This is in fact the most explicit syntax and a bit redundant, which is good, because that redundancy lets us deliver an error message that reminds expression authors that CA provides a better experience to their users. --- src/libexpr/primops/fetchClosure.cc | 54 +++++++++++++++++++++++------ tests/fetchClosure.sh | 16 +++++++-- 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index 8f8e033a5..96c1df544 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -13,6 +13,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg std::optional fromPath; bool enableRewriting = false; std::optional toPath; + bool inputAddressed = false; for (auto & attr : *args[0]->attrs) { const auto & attrName = state.symbols[attr.name]; @@ -38,6 +39,9 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg fromStoreUrl = state.forceStringNoCtx(*attr.value, attr.pos, attrHint()); + else if (attrName == "inputAddressed") + inputAddressed = state.forceBool(*attr.value, attr.pos, attrHint()); + else throw Error({ .msg = hintfmt("attribute '%s' isn't supported in call to 'fetchClosure'", attrName), @@ -51,6 +55,18 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg .errPos = state.positions[pos] }); + if (inputAddressed) { + if (toPath && toPath != fromPath) + throw Error({ + .msg = hintfmt("attribute '%s' is set to true, but 'toPath' does not match 'fromPath'. 'toPath' should be equal, or should be omitted. Instead 'toPath' was '%s' and 'fromPath' was '%s'", + "inputAddressed", + state.store->printStorePath(*toPath), + state.store->printStorePath(*fromPath)), + .errPos = state.positions[pos] + }); + assert(!enableRewriting); + } + if (!fromStoreUrl) throw Error({ .msg = hintfmt("attribute '%s' is missing in call to 'fetchClosure'", "fromStore"), @@ -104,15 +120,28 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg toPath = fromPath; } - /* In pure mode, require a CA path. */ - if (evalSettings.pureEval) { + /* We want input addressing to be explicit, to inform readers and to give + expression authors an opportunity to improve their user experience. */ + if (!inputAddressed) { auto info = state.store->queryPathInfo(*toPath); - if (!info->isContentAddressed(*state.store)) - throw Error({ - .msg = hintfmt("in pure mode, 'fetchClosure' requires a content-addressed path, which '%s' isn't", - state.store->printStorePath(*toPath)), - .errPos = state.positions[pos] - }); + if (!info->isContentAddressed(*state.store)) { + if (enableRewriting) { + throw Error({ + // Ideally we'd compute the path for them, but this error message is unlikely to occur in practice, so we keep it simple. + .msg = hintfmt("Rewriting was requested, but 'toPath' is not content addressed. This is impossible. Please change 'toPath' to the correct path, or to a non-existing path, and try again", + state.store->printStorePath(*toPath)), + .errPos = state.positions[pos] + }); + } else { + // We just checked toPath, but we report fromPath, because that's what the user certainly passed. + assert (toPath == fromPath); + throw Error({ + .msg = hintfmt("The 'fromPath' value '%s' is input addressed, but input addressing was not requested. If you do intend to return an input addressed store path, add 'inputAddressed = true;' to the 'fetchClosure' arguments. Note that content addressing does not require users to configure a trusted binary cache public key on their systems, and is therefore preferred.", + state.store->printStorePath(*fromPath)), + .errPos = state.positions[pos] + }); + } + } } state.mkStorePathString(*toPath, v); @@ -138,7 +167,7 @@ static RegisterPrimOp primop_fetchClosure({ `/nix/store/ldbh...`. If `fromPath` is already content-addressed, or if you are - allowing impure evaluation (`--impure`), then `toPath` may be + allowing input addressing (`inputAddressed = true;`), then `toPath` may be omitted. To find out the correct value for `toPath` given a `fromPath`, @@ -153,8 +182,11 @@ static RegisterPrimOp primop_fetchClosure({ allows you to use a previously built store path in a Nix expression. However, it is more reproducible because it requires specifying a binary cache from which the path can be fetched. - Also, requiring a content-addressed final store path avoids the - need for users to configure binary cache public keys. + Also, the default requirement of a content-addressed final store path + avoids the need for users to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys). + + This function is only available if you enable the experimental + feature `fetch-closure`. )", .fun = prim_fetchClosure, .experimentalFeature = Xp::FetchClosure, diff --git a/tests/fetchClosure.sh b/tests/fetchClosure.sh index 21650eb06..4b8198e94 100644 --- a/tests/fetchClosure.sh +++ b/tests/fetchClosure.sh @@ -35,16 +35,28 @@ clearStore if [[ "$NIX_REMOTE" != "daemon" ]]; then - # In impure mode, we can use non-CA paths. - [[ $(nix eval --raw --no-require-sigs --impure --expr " + # We can use non-CA paths when we ask explicitly. + [[ $(nix eval --raw --no-require-sigs --expr " builtins.fetchClosure { fromStore = \"file://$cacheDir\"; fromPath = $nonCaPath; + inputAddressed = true; } ") = $nonCaPath ]] [ -e $nonCaPath ] + # .. but only if we ask explicitly. + expectStderr 1 nix eval --raw --no-require-sigs --expr " + builtins.fetchClosure { + fromStore = \"file://$cacheDir\"; + fromPath = $nonCaPath; + } + " | grepQuiet -E "The .fromPath. value .* is input addressed, but input addressing was not requested. If you do intend to return an input addressed store path, add .inputAddressed = true;. to the .fetchClosure. arguments." + + [ -e $nonCaPath ] + + fi # 'toPath' set to empty string should fail but print the expected path.