From 0f6d596df5866d3f08e743ac0f1a282daf6b2155 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 19 May 2023 09:49:18 +0200 Subject: [PATCH 01/38] fetchClosure: Factor out attribute hint --- src/libexpr/primops/fetchClosure.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index bae849f61..26c2b90bf 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -16,11 +16,13 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg for (auto & attr : *args[0]->attrs) { const auto & attrName = state.symbols[attr.name]; + auto attrHint = [&]() -> std::string { + return "while evaluating the '" + attrName + "' attribute passed to builtins.fetchClosure"; + }; if (attrName == "fromPath") { NixStringContext context; - fromPath = state.coerceToStorePath(attr.pos, *attr.value, context, - "while evaluating the 'fromPath' attribute passed to builtins.fetchClosure"); + fromPath = state.coerceToStorePath(attr.pos, *attr.value, context, attrHint()); } else if (attrName == "toPath") { @@ -28,14 +30,13 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg toCA = true; if (attr.value->type() != nString || attr.value->string.s != std::string("")) { NixStringContext context; - toPath = state.coerceToStorePath(attr.pos, *attr.value, context, - "while evaluating the 'toPath' attribute passed to builtins.fetchClosure"); + toPath = state.coerceToStorePath(attr.pos, *attr.value, context, attrHint()); } } else if (attrName == "fromStore") fromStoreUrl = state.forceStringNoCtx(*attr.value, attr.pos, - "while evaluating the 'fromStore' attribute passed to builtins.fetchClosure"); + attrHint()); else throw Error({ From 7e5b6d2c4550a3007946271768e114531d7b89f0 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 19 May 2023 10:03:25 +0200 Subject: [PATCH 02/38] fetchClosure: Refactor: rename toCA -> enableRewriting --- src/libexpr/primops/fetchClosure.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index 26c2b90bf..8f8e033a5 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -11,7 +11,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg std::optional fromStoreUrl; std::optional fromPath; - bool toCA = false; + bool enableRewriting = false; std::optional toPath; for (auto & attr : *args[0]->attrs) { @@ -27,7 +27,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg else if (attrName == "toPath") { state.forceValue(*attr.value, attr.pos); - toCA = true; + enableRewriting = true; if (attr.value->type() != nString || attr.value->string.s != std::string("")) { NixStringContext context; toPath = state.coerceToStorePath(attr.pos, *attr.value, context, attrHint()); @@ -75,7 +75,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg auto fromStore = openStore(parsedURL.to_string()); - if (toCA) { + if (enableRewriting) { if (!toPath || !state.store->isValidPath(*toPath)) { auto remappings = makeContentAddressed(*fromStore, *state.store, { *fromPath }); auto i = remappings.find(*fromPath); From ea30f152b738fe65f216f3173d3a19adaaef5323 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 19 May 2023 10:48:53 +0200 Subject: [PATCH 03/38] 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. From 508aa58e671583bf06fa3597629839827c8d1bd7 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 19 May 2023 16:25:23 +0200 Subject: [PATCH 04/38] fetchClosure: Always check that inputAddressed matches the result --- src/libexpr/primops/fetchClosure.cc | 42 +++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index 96c1df544..c66fed633 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -13,7 +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; + std::optional inputAddressedMaybe; for (auto & attr : *args[0]->attrs) { const auto & attrName = state.symbols[attr.name]; @@ -40,7 +40,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg attrHint()); else if (attrName == "inputAddressed") - inputAddressed = state.forceBool(*attr.value, attr.pos, attrHint()); + inputAddressedMaybe = state.forceBool(*attr.value, attr.pos, attrHint()); else throw Error({ @@ -55,6 +55,8 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg .errPos = state.positions[pos] }); + bool inputAddressed = inputAddressedMaybe.value_or(false); + if (inputAddressed) { if (toPath && toPath != fromPath) throw Error({ @@ -120,15 +122,39 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg toPath = fromPath; } - /* 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); + auto info = state.store->queryPathInfo(*toPath); + + /* If inputAddressed is set, make sure the result is as expected. */ + if (inputAddressedMaybe) { + bool expectCA = !inputAddressed; + if (info->isContentAddressed(*state.store) != expectCA) { + if (inputAddressed) + throw Error({ + .msg = hintfmt("The 'fetchClosure' result, '%s' is not input addressed, despite 'inputAddressed' being set to true. It is ok or even preferable to return a content addressed path, so you probably want to remove the 'inputAddressed' attribute", + state.store->printStorePath(*toPath)), + .errPos = state.positions[pos] + }); + else + throw Error({ + .msg = hintfmt("The 'fetchClosure' result, '%s' is input addressed, despite 'inputAddressed' being set to false. Please make sure you passed the correct path(s) or set 'inputAddressed' to true if you intended to return an input addressed path", + state.store->printStorePath(*toPath)), + .errPos = state.positions[pos] + }); + } + } else { + /* + While it's fine to omit the inputAddressed attribute for CA paths, + we want input addressing to be explicit, to inform readers and to give + expression authors an opportunity to improve their user experience; + see message below. + */ if (!info->isContentAddressed(*state.store)) { if (enableRewriting) { + // We don't perform the rewriting when outPath already exists, as an optimisation. + // However, we can quickly detect a mistake if the toPath is input addressed. + // Ideally we'd compute the path for them here, but this error message is unlikely to occur in practice, so we keep it simple. 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", + .msg = hintfmt("The 'toPath' value '%s' is input addressed, so it can't possibly be the result of rewriting. You may set 'toPath' to an empty string to figure out the correct path.", state.store->printStorePath(*toPath)), .errPos = state.positions[pos] }); From 8dca95386c9ab8b13886716ec31e1b2936a3ef10 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 5 Jun 2023 11:11:53 +0200 Subject: [PATCH 05/38] fetchClosure: Disallow toPath for inputAddressed = true --- src/libexpr/primops/fetchClosure.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index c66fed633..6d2271853 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -58,12 +58,11 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg bool inputAddressed = inputAddressedMaybe.value_or(false); if (inputAddressed) { - if (toPath && toPath != fromPath) + if (toPath) 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'", + .msg = hintfmt("attribute '%s' is set to true, but '%s' is also set. Please remove one of them", "inputAddressed", - state.store->printStorePath(*toPath), - state.store->printStorePath(*fromPath)), + "toPath"), .errPos = state.positions[pos] }); assert(!enableRewriting); From 55888633dd323c83cd1db3773d82dc954da34888 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 5 Jun 2023 11:37:41 +0200 Subject: [PATCH 06/38] makeContentAddressed: Add single path helper --- src/libexpr/primops/fetchClosure.cc | 10 ++++------ src/libstore/make-content-addressed.cc | 11 +++++++++++ src/libstore/make-content-addressed.hh | 13 ++++++++++++- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index 6d2271853..6a15c826f 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -94,14 +94,12 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg if (enableRewriting) { if (!toPath || !state.store->isValidPath(*toPath)) { - auto remappings = makeContentAddressed(*fromStore, *state.store, { *fromPath }); - auto i = remappings.find(*fromPath); - assert(i != remappings.end()); - if (toPath && *toPath != i->second) + auto rewrittenPath = makeContentAddressed(*fromStore, *state.store, *fromPath); + if (toPath && *toPath != rewrittenPath) throw Error({ .msg = hintfmt("rewriting '%s' to content-addressed form yielded '%s', while '%s' was expected", state.store->printStorePath(*fromPath), - state.store->printStorePath(i->second), + state.store->printStorePath(rewrittenPath), state.store->printStorePath(*toPath)), .errPos = state.positions[pos] }); @@ -111,7 +109,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg "rewriting '%s' to content-addressed form yielded '%s'; " "please set this in the 'toPath' attribute passed to 'fetchClosure'", state.store->printStorePath(*fromPath), - state.store->printStorePath(i->second)), + state.store->printStorePath(rewrittenPath)), .errPos = state.positions[pos] }); } diff --git a/src/libstore/make-content-addressed.cc b/src/libstore/make-content-addressed.cc index 53fe04704..626a22480 100644 --- a/src/libstore/make-content-addressed.cc +++ b/src/libstore/make-content-addressed.cc @@ -80,4 +80,15 @@ std::map makeContentAddressed( return remappings; } +StorePath makeContentAddressed( + Store & srcStore, + Store & dstStore, + const StorePath & fromPath) +{ + auto remappings = makeContentAddressed(srcStore, dstStore, StorePathSet { fromPath }); + auto i = remappings.find(fromPath); + assert(i != remappings.end()); + return i->second; +} + } diff --git a/src/libstore/make-content-addressed.hh b/src/libstore/make-content-addressed.hh index 2ce6ec7bc..60bb2b477 100644 --- a/src/libstore/make-content-addressed.hh +++ b/src/libstore/make-content-addressed.hh @@ -5,9 +5,20 @@ namespace nix { +/** Rewrite a closure of store paths to be completely content addressed. + */ std::map makeContentAddressed( Store & srcStore, Store & dstStore, - const StorePathSet & storePaths); + const StorePathSet & rootPaths); + +/** Rewrite a closure of a store path to be completely content addressed. + * + * This is a convenience function for the case where you only have one root path. + */ +StorePath makeContentAddressed( + Store & srcStore, + Store & dstStore, + const StorePath & rootPath); } From 5bdca46117c701cd1cd47700c755367442537157 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 5 Jun 2023 12:35:03 +0200 Subject: [PATCH 07/38] fetchClosure: Split into three cases --- src/libexpr/primops/fetchClosure.cc | 177 ++++++++++++++++------------ 1 file changed, 101 insertions(+), 76 deletions(-) diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index 6a15c826f..e8c32de96 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -5,6 +5,101 @@ namespace nix { +/** + * Handler for the content addressed case. + * + * @param state The evaluator state and store to write to. + * @param fromStore The store containing the path to rewrite. + * @param fromPath The source path to be rewritten. + * @param toPathMaybe The path to write the rewritten path to. If empty, the error shows the actual path. + * @param v The return `Value` + */ +static void runFetchClosureWithRewrite(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, const std::optional & toPathMaybe, Value &v) { + + // establish toPath or throw + + if (!toPathMaybe || !state.store->isValidPath(*toPathMaybe)) { + auto rewrittenPath = makeContentAddressed(fromStore, *state.store, fromPath); + if (toPathMaybe && *toPathMaybe != rewrittenPath) + throw Error({ + .msg = hintfmt("rewriting '%s' to content-addressed form yielded '%s', while '%s' was expected", + state.store->printStorePath(fromPath), + state.store->printStorePath(rewrittenPath), + state.store->printStorePath(*toPathMaybe)), + .errPos = state.positions[pos] + }); + if (!toPathMaybe) + throw Error({ + .msg = hintfmt( + "rewriting '%s' to content-addressed form yielded '%s'; " + "please set this in the 'toPath' attribute passed to 'fetchClosure'", + state.store->printStorePath(fromPath), + state.store->printStorePath(rewrittenPath)), + .errPos = state.positions[pos] + }); + } + + auto toPath = *toPathMaybe; + + // check and return + + auto resultInfo = state.store->queryPathInfo(toPath); + + if (!resultInfo->isContentAddressed(*state.store)) { + // We don't perform the rewriting when outPath already exists, as an optimisation. + // However, we can quickly detect a mistake if the toPath is input addressed. + throw Error({ + .msg = hintfmt("The 'toPath' value '%s' is input addressed, so it can't possibly be the result of rewriting. You may set 'toPath' to an empty string to figure out the correct path.", + state.store->printStorePath(toPath)), + .errPos = state.positions[pos] + }); + } + + state.mkStorePathString(toPath, v); +} + +/** + * Fetch the closure and make sure it's content addressed. + */ +static void runFetchClosureWithContentAddressedPath(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, Value & v) { + + if (!state.store->isValidPath(fromPath)) + copyClosure(fromStore, *state.store, RealisedPath::Set { fromPath }); + + auto info = state.store->queryPathInfo(fromPath); + + if (!info->isContentAddressed(*state.store)) { + 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(fromPath, v); +} + +/** + * Fetch the closure and make sure it's input addressed. + */ +static void runFetchClosureWithInputAddressedPath(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, Value & v) { + + if (!state.store->isValidPath(fromPath)) + copyClosure(fromStore, *state.store, RealisedPath::Set { fromPath }); + + auto info = state.store->queryPathInfo(fromPath); + + if (info->isContentAddressed(*state.store)) { + throw Error({ + .msg = hintfmt("The 'fetchClosure' result, '%s' is not input addressed, despite 'inputAddressed' being set to true. It is preferable to return a content addressed path, so remove the 'inputAddressed' attribute to ensure content addressing is used in the future", + state.store->printStorePath(fromPath)), + .errPos = state.positions[pos] + }); + } + + state.mkStorePathString(fromPath, v); +} + static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * args, Value & v) { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.fetchClosure"); @@ -92,82 +187,12 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg auto fromStore = openStore(parsedURL.to_string()); - if (enableRewriting) { - if (!toPath || !state.store->isValidPath(*toPath)) { - auto rewrittenPath = makeContentAddressed(*fromStore, *state.store, *fromPath); - if (toPath && *toPath != rewrittenPath) - throw Error({ - .msg = hintfmt("rewriting '%s' to content-addressed form yielded '%s', while '%s' was expected", - state.store->printStorePath(*fromPath), - state.store->printStorePath(rewrittenPath), - state.store->printStorePath(*toPath)), - .errPos = state.positions[pos] - }); - if (!toPath) - throw Error({ - .msg = hintfmt( - "rewriting '%s' to content-addressed form yielded '%s'; " - "please set this in the 'toPath' attribute passed to 'fetchClosure'", - state.store->printStorePath(*fromPath), - state.store->printStorePath(rewrittenPath)), - .errPos = state.positions[pos] - }); - } - } else { - if (!state.store->isValidPath(*fromPath)) - copyClosure(*fromStore, *state.store, RealisedPath::Set { *fromPath }); - toPath = fromPath; - } - - auto info = state.store->queryPathInfo(*toPath); - - /* If inputAddressed is set, make sure the result is as expected. */ - if (inputAddressedMaybe) { - bool expectCA = !inputAddressed; - if (info->isContentAddressed(*state.store) != expectCA) { - if (inputAddressed) - throw Error({ - .msg = hintfmt("The 'fetchClosure' result, '%s' is not input addressed, despite 'inputAddressed' being set to true. It is ok or even preferable to return a content addressed path, so you probably want to remove the 'inputAddressed' attribute", - state.store->printStorePath(*toPath)), - .errPos = state.positions[pos] - }); - else - throw Error({ - .msg = hintfmt("The 'fetchClosure' result, '%s' is input addressed, despite 'inputAddressed' being set to false. Please make sure you passed the correct path(s) or set 'inputAddressed' to true if you intended to return an input addressed path", - state.store->printStorePath(*toPath)), - .errPos = state.positions[pos] - }); - } - } else { - /* - While it's fine to omit the inputAddressed attribute for CA paths, - we want input addressing to be explicit, to inform readers and to give - expression authors an opportunity to improve their user experience; - see message below. - */ - if (!info->isContentAddressed(*state.store)) { - if (enableRewriting) { - // We don't perform the rewriting when outPath already exists, as an optimisation. - // However, we can quickly detect a mistake if the toPath is input addressed. - // Ideally we'd compute the path for them here, but this error message is unlikely to occur in practice, so we keep it simple. - throw Error({ - .msg = hintfmt("The 'toPath' value '%s' is input addressed, so it can't possibly be the result of rewriting. You may set 'toPath' to an empty string to figure out the correct path.", - 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); + if (enableRewriting) + runFetchClosureWithRewrite(state, pos, *fromStore, *fromPath, toPath, v); + else if (inputAddressed) + runFetchClosureWithInputAddressedPath(state, pos, *fromStore, *fromPath, v); + else + runFetchClosureWithContentAddressedPath(state, pos, *fromStore, *fromPath, v); } static RegisterPrimOp primop_fetchClosure({ From dc79636007ff6982300980ba7eb1c2be3e45bece Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 5 Jun 2023 12:55:58 +0200 Subject: [PATCH 08/38] fetchClosure: Refactor: replace enableRewriting A single variable is nice and self-contained. --- src/libexpr/primops/fetchClosure.cc | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index e8c32de96..e048a2629 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -100,14 +100,15 @@ static void runFetchClosureWithInputAddressedPath(EvalState & state, const PosId state.mkStorePathString(fromPath, v); } +typedef std::optional StorePathOrGap; + static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * args, Value & v) { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.fetchClosure"); std::optional fromStoreUrl; std::optional fromPath; - bool enableRewriting = false; - std::optional toPath; + std::optional toPath; std::optional inputAddressedMaybe; for (auto & attr : *args[0]->attrs) { @@ -123,8 +124,11 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg else if (attrName == "toPath") { state.forceValue(*attr.value, attr.pos); - enableRewriting = true; - if (attr.value->type() != nString || attr.value->string.s != std::string("")) { + bool isEmptyString = attr.value->type() == nString && attr.value->string.s == std::string(""); + if (isEmptyString) { + toPath = StorePathOrGap {}; + } + else { NixStringContext context; toPath = state.coerceToStorePath(attr.pos, *attr.value, context, attrHint()); } @@ -160,7 +164,6 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg "toPath"), .errPos = state.positions[pos] }); - assert(!enableRewriting); } if (!fromStoreUrl) @@ -187,8 +190,8 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg auto fromStore = openStore(parsedURL.to_string()); - if (enableRewriting) - runFetchClosureWithRewrite(state, pos, *fromStore, *fromPath, toPath, v); + if (toPath) + runFetchClosureWithRewrite(state, pos, *fromStore, *fromPath, *toPath, v); else if (inputAddressed) runFetchClosureWithInputAddressedPath(state, pos, *fromStore, *fromPath, v); else From 32c69e2b17ad7bd9721b584c3733629fe6d8c7f7 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 5 Jun 2023 16:25:02 +0200 Subject: [PATCH 09/38] doc: Typo --- doc/manual/src/language/builtins-prefix.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/manual/src/language/builtins-prefix.md b/doc/manual/src/language/builtins-prefix.md index 35e3dccc3..7b2321466 100644 --- a/doc/manual/src/language/builtins-prefix.md +++ b/doc/manual/src/language/builtins-prefix.md @@ -3,7 +3,7 @@ This section lists the functions built into the Nix language evaluator. All built-in functions are available through the global [`builtins`](./builtin-constants.md#builtins-builtins) constant. -For convenience, some built-ins are can be accessed directly: +For convenience, some built-ins can be accessed directly: - [`derivation`](#builtins-derivation) - [`import`](#builtins-import) From 50de11d662ced6bfef792af54ad57553f1c3208a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 5 Jun 2023 16:25:22 +0200 Subject: [PATCH 10/38] doc: Improve `fetchClosure` documentation --- doc/manual/src/release-notes/rl-next.md | 2 + src/libexpr/primops.cc | 2 + src/libexpr/primops/fetchClosure.cc | 66 +++++++++++++++++++------ 3 files changed, 55 insertions(+), 15 deletions(-) diff --git a/doc/manual/src/release-notes/rl-next.md b/doc/manual/src/release-notes/rl-next.md index 8479b166a..e64839fa2 100644 --- a/doc/manual/src/release-notes/rl-next.md +++ b/doc/manual/src/release-notes/rl-next.md @@ -2,5 +2,7 @@ - [`nix-channel`](../command-ref/nix-channel.md) now supports a `--list-generations` subcommand +- The function [`builtins.fetchClosure`](../language/builtins.md#builtins-fetchClosure) can now fetch input-addressed paths in [pure mode](../command-ref/conf-file.md#conf-pure-eval). + - Nix now allows unprivileged/[`allowed-users`](../command-ref/conf-file.md#conf-allowed-users) to sign paths. Previously, only [`trusted-users`](../command-ref/conf-file.md#conf-trusted-users) users could sign paths. diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 5dfad470a..f4bdf8fc6 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1502,6 +1502,8 @@ static RegisterPrimOp primop_storePath({ in a new path (e.g. `/nix/store/ld01dnzc…-source-source`). Not available in [pure evaluation mode](@docroot@/command-ref/conf-file.md#conf-pure-eval). + + See also [`builtins.fetchClosure`](#builtins-fetchClosure). )", .fun = prim_storePath, }); diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index e048a2629..e344c9513 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -202,40 +202,76 @@ static RegisterPrimOp primop_fetchClosure({ .name = "__fetchClosure", .args = {"args"}, .doc = R"( - Fetch a Nix store closure from a binary cache, rewriting it into - content-addressed form. For example, + Fetch a Nix store closure from a binary cache. - ```nix - builtins.fetchClosure { - fromStore = "https://cache.nixos.org"; - fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1; - toPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1; - } - ``` + This function can be used in three ways: - fetches `/nix/store/r2jd...` from the specified binary cache, + - Fetch any store path and rewrite it to a fully content-addressed store path. Example: + + ```nix + builtins.fetchClosure { + fromStore = "https://cache.nixos.org"; + fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1; + toPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1; + } + ``` + + - Fetch a content-addressed store path. + + ```nix + builtins.fetchClosure { + fromStore = "https://cache.nixos.org"; + fromPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1; + } + ``` + + - Fetch an [input-addressed store path](@docroot@/glossary.md#gloss-input-addressed-store-object) as is. This depends on user configuration, which is less preferable. + + ```nix + builtins.fetchClosure { + fromStore = "https://cache.nixos.org"; + fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1; + inputAddressed = true; + } + ``` + + **Rewriting a store path** + + This first example fetches `/nix/store/r2jd...` from the specified binary cache, and rewrites it into the content-addressed store path `/nix/store/ldbh...`. - If `fromPath` is already content-addressed, or if you are - allowing input addressing (`inputAddressed = true;`), then `toPath` may be - omitted. + By rewriting the store paths, you can add the contents to any store without requiring that you or perhaps your users configure any extra trust. To find out the correct value for `toPath` given a `fromPath`, - you can use `nix store make-content-addressed`: + you can use [`nix store make-content-addressed`](@docroot@/command-ref/new-cli/nix3-store-make-content-addressed.md): ```console # nix store make-content-addressed --from https://cache.nixos.org /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1 rewrote '/nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1' to '/nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1' ``` - This function is similar to `builtins.storePath` in that it + Alternatively, you may set `toPath = ""` and find the correct `toPath` in the error message. + + **Not rewriting** + + `toPath` may be omitted when either + - `fromPath` is already content-addressed, or + - `inputAddressed = true;` is set. + + Fetching an [input addressed path](@docroot@/glossary.md#gloss-input-addressed-store-object) is not recommended because it requires that the source be trusted by the Nix configuration. + + **`builtins.storePath`** + + This function is similar to [`builtins.storePath`](#builtins-storePath) in that it 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, 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). + **Experimental feature** + This function is only available if you enable the experimental feature `fetch-closure`. )", From 40052c76132d79561aa50ec9349ad22b51c64505 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 7 Jun 2023 12:47:18 +0200 Subject: [PATCH 11/38] fetchClosure: Docs and error message improvements Co-authored-by: Valentin Gagarin --- doc/manual/src/release-notes/rl-next.md | 2 +- src/libexpr/primops/fetchClosure.cc | 38 ++++++++++++++++--------- tests/fetchClosure.sh | 2 +- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/doc/manual/src/release-notes/rl-next.md b/doc/manual/src/release-notes/rl-next.md index e64839fa2..139d07188 100644 --- a/doc/manual/src/release-notes/rl-next.md +++ b/doc/manual/src/release-notes/rl-next.md @@ -2,7 +2,7 @@ - [`nix-channel`](../command-ref/nix-channel.md) now supports a `--list-generations` subcommand -- The function [`builtins.fetchClosure`](../language/builtins.md#builtins-fetchClosure) can now fetch input-addressed paths in [pure mode](../command-ref/conf-file.md#conf-pure-eval). +* The function [`builtins.fetchClosure`](../language/builtins.md#builtins-fetchClosure) can now fetch input-addressed paths in [pure evaluation mode](../command-ref/conf-file.md#conf-pure-eval), as those are not impure. - Nix now allows unprivileged/[`allowed-users`](../command-ref/conf-file.md#conf-allowed-users) to sign paths. Previously, only [`trusted-users`](../command-ref/conf-file.md#conf-trusted-users) users could sign paths. diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index e344c9513..5114df553 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -8,11 +8,11 @@ namespace nix { /** * Handler for the content addressed case. * - * @param state The evaluator state and store to write to. - * @param fromStore The store containing the path to rewrite. - * @param fromPath The source path to be rewritten. - * @param toPathMaybe The path to write the rewritten path to. If empty, the error shows the actual path. - * @param v The return `Value` + * @param state Evaluator state and store to write to. + * @param fromStore Store containing the path to rewrite. + * @param fromPath Source path to be rewritten. + * @param toPathMaybe Path to write the rewritten path to. If empty, the error shows the actual path. + * @param v Return `Value` */ static void runFetchClosureWithRewrite(EvalState & state, const PosIdx pos, Store & fromStore, const StorePath & fromPath, const std::optional & toPathMaybe, Value &v) { @@ -31,8 +31,8 @@ static void runFetchClosureWithRewrite(EvalState & state, const PosIdx pos, Stor if (!toPathMaybe) throw Error({ .msg = hintfmt( - "rewriting '%s' to content-addressed form yielded '%s'; " - "please set this in the 'toPath' attribute passed to 'fetchClosure'", + "rewriting '%s' to content-addressed form yielded '%s'\n" + "Use this value for the 'toPath' attribute passed to 'fetchClosure'", state.store->printStorePath(fromPath), state.store->printStorePath(rewrittenPath)), .errPos = state.positions[pos] @@ -49,7 +49,9 @@ static void runFetchClosureWithRewrite(EvalState & state, const PosIdx pos, Stor // We don't perform the rewriting when outPath already exists, as an optimisation. // However, we can quickly detect a mistake if the toPath is input addressed. throw Error({ - .msg = hintfmt("The 'toPath' value '%s' is input addressed, so it can't possibly be the result of rewriting. You may set 'toPath' to an empty string to figure out the correct path.", + .msg = hintfmt( + "The 'toPath' value '%s' is input-addressed, so it can't possibly be the result of rewriting to a content-addressed path.\n\n" + "Set 'toPath' to an empty string to make Nix report the correct content-addressed path.", state.store->printStorePath(toPath)), .errPos = state.positions[pos] }); @@ -70,7 +72,9 @@ static void runFetchClosureWithContentAddressedPath(EvalState & state, const Pos if (!info->isContentAddressed(*state.store)) { 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.", + .msg = hintfmt( + "The 'fromPath' value '%s' is input-addressed, but 'inputAddressed' is set to 'false' (default).\n\n" + "If you do intend to fetch 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] }); @@ -91,7 +95,9 @@ static void runFetchClosureWithInputAddressedPath(EvalState & state, const PosId if (info->isContentAddressed(*state.store)) { throw Error({ - .msg = hintfmt("The 'fetchClosure' result, '%s' is not input addressed, despite 'inputAddressed' being set to true. It is preferable to return a content addressed path, so remove the 'inputAddressed' attribute to ensure content addressing is used in the future", + .msg = hintfmt( + "The store object referred to by 'fromPath' at '%s' is not input-addressed, but 'inputAddressed' is set to 'true'.\n\n" + "Remove the 'inputAddressed' attribute (it defaults to 'false') to expect 'fromPath' to be content-addressed", state.store->printStorePath(fromPath)), .errPos = state.positions[pos] }); @@ -202,11 +208,13 @@ static RegisterPrimOp primop_fetchClosure({ .name = "__fetchClosure", .args = {"args"}, .doc = R"( - Fetch a Nix store closure from a binary cache. + Fetch a store path [closure](@docroot@/glossary.md#gloss-closure) from a binary cache. This function can be used in three ways: - - Fetch any store path and rewrite it to a fully content-addressed store path. Example: + - Fetch any store path and rewrite it to a fully content-addressed store path. + + Example: ```nix builtins.fetchClosure { @@ -218,6 +226,8 @@ static RegisterPrimOp primop_fetchClosure({ - Fetch a content-addressed store path. +Example: + ```nix builtins.fetchClosure { fromStore = "https://cache.nixos.org"; @@ -227,6 +237,8 @@ static RegisterPrimOp primop_fetchClosure({ - Fetch an [input-addressed store path](@docroot@/glossary.md#gloss-input-addressed-store-object) as is. This depends on user configuration, which is less preferable. + Example: + ```nix builtins.fetchClosure { fromStore = "https://cache.nixos.org"; @@ -244,7 +256,7 @@ static RegisterPrimOp primop_fetchClosure({ By rewriting the store paths, you can add the contents to any store without requiring that you or perhaps your users configure any extra trust. To find out the correct value for `toPath` given a `fromPath`, - you can use [`nix store make-content-addressed`](@docroot@/command-ref/new-cli/nix3-store-make-content-addressed.md): + use [`nix store make-content-addressed`](@docroot@/command-ref/new-cli/nix3-store-make-content-addressed.md): ```console # nix store make-content-addressed --from https://cache.nixos.org /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1 diff --git a/tests/fetchClosure.sh b/tests/fetchClosure.sh index 4b8198e94..0f1c64372 100644 --- a/tests/fetchClosure.sh +++ b/tests/fetchClosure.sh @@ -52,7 +52,7 @@ if [[ "$NIX_REMOTE" != "daemon" ]]; then 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." + " | grepQuiet -E "The .fromPath. value .* is input-addressed, but .inputAddressed. is set to .false." [ -e $nonCaPath ] From 1db81f710725eb83597b1761076e9ca28317a52e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 19 Jun 2023 23:22:37 +0200 Subject: [PATCH 12/38] tests/fetchClosure: Improve coverage of new and some existing flows --- tests/fetchClosure.sh | 79 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 69 insertions(+), 10 deletions(-) diff --git a/tests/fetchClosure.sh b/tests/fetchClosure.sh index 0f1c64372..a02d1ce7a 100644 --- a/tests/fetchClosure.sh +++ b/tests/fetchClosure.sh @@ -33,8 +33,26 @@ clearStore [ ! -e $nonCaPath ] [ -e $caPath ] +clearStore + +# The daemon will reject input addressed paths unless configured to trust the +# cache key or the user. This behavior should be covered by another test, so we +# skip this part when using the daemon. if [[ "$NIX_REMOTE" != "daemon" ]]; then + # If we want to return a non-CA path, we have to be explicit about it. + 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 .inputAddressed. is set to .false." + + # TODO: Should the closure be rejected, despite single user mode? + # [ ! -e $nonCaPath ] + + [ ! -e $caPath ] + # We can use non-CA paths when we ask explicitly. [[ $(nix eval --raw --no-require-sigs --expr " builtins.fetchClosure { @@ -45,20 +63,13 @@ if [[ "$NIX_REMOTE" != "daemon" ]]; then ") = $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 .inputAddressed. is set to .false." - - [ -e $nonCaPath ] + [ ! -e $caPath ] fi +[ ! -e $caPath ] + # 'toPath' set to empty string should fail but print the expected path. expectStderr 1 nix eval -v --json --expr " builtins.fetchClosure { @@ -71,6 +82,10 @@ expectStderr 1 nix eval -v --json --expr " # If fromPath is CA, then toPath isn't needed. nix copy --to file://$cacheDir $caPath +clearStore + +[ ! -e $caPath ] + [[ $(nix eval -v --raw --expr " builtins.fetchClosure { fromStore = \"file://$cacheDir\"; @@ -78,6 +93,8 @@ nix copy --to file://$cacheDir $caPath } ") = $caPath ]] +[ -e $caPath ] + # Check that URL query parameters aren't allowed. clearStore narCache=$TEST_ROOT/nar-cache @@ -89,3 +106,45 @@ rm -rf $narCache } ") (! [ -e $narCache ]) + +# If toPath is specified but wrong, we check it (only) when the path is missing. +clearStore + +badPath=$(echo $caPath | sed -e 's!/store/................................-!/store/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-!') + +[ ! -e $badPath ] + +expectStderr 1 nix eval -v --raw --expr " + builtins.fetchClosure { + fromStore = \"file://$cacheDir\"; + fromPath = $nonCaPath; + toPath = $badPath; + } +" | grep "error: rewriting.*$nonCaPath.*yielded.*$caPath.*while.*$badPath.*was expected" + +[ ! -e $badPath ] + +# We only check it when missing, as a performance optimization similar to what we do for fixed output derivations. So if it's already there, we don't check it. +# It would be nice for this to fail, but checking it would be too(?) slow. +[ -e $caPath ] + +[[ $(nix eval -v --raw --expr " + builtins.fetchClosure { + fromStore = \"file://$cacheDir\"; + fromPath = $badPath; + toPath = $caPath; + } +") = $caPath ]] + + +# However, if the output address is unexpected, we can report it + + +expectStderr 1 nix eval -v --raw --expr " + builtins.fetchClosure { + fromStore = \"file://$cacheDir\"; + fromPath = $caPath; + inputAddressed = true; + } +" | grepQuiet 'error.*The store object referred to by.*fromPath.* at .* is not input-addressed, but .*inputAddressed.* is set to .*true.*' + From fefb94713295d63d30a8bba257c41c3eb54e0998 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 19 Jun 2023 23:22:56 +0200 Subject: [PATCH 13/38] tests/signing.sh: Check signature checking error message We should check error messages, so that we know the command fails for the right reason. Alternatively, a mere typo can run the test undetected. --- tests/signing.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/signing.sh b/tests/signing.sh index 9b673c609..f14b53e7f 100644 --- a/tests/signing.sh +++ b/tests/signing.sh @@ -84,7 +84,7 @@ info=$(nix path-info --store file://$cacheDir --json $outPath2) # Copying to a diverted store should fail due to a lack of signatures by trusted keys. chmod -R u+w $TEST_ROOT/store0 || true rm -rf $TEST_ROOT/store0 -(! nix copy --to $TEST_ROOT/store0 $outPath) +expectStderr 1 nix copy --to $TEST_ROOT/store0 $outPath | grepQuiet -E 'cannot add path .* because it lacks a signature by a trusted key' # But succeed if we supply the public keys. nix copy --to $TEST_ROOT/store0 $outPath --trusted-public-keys $pk1 From 4b2f155f0a22179e46d2c55cb1bd074c885fd08e Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Thu, 6 Jul 2023 09:00:49 -0700 Subject: [PATCH 14/38] nix-daemon.service: Add TasksMax=infinity --- misc/systemd/nix-daemon.service.in | 1 + 1 file changed, 1 insertion(+) diff --git a/misc/systemd/nix-daemon.service.in b/misc/systemd/nix-daemon.service.in index f46413630..4b4474475 100644 --- a/misc/systemd/nix-daemon.service.in +++ b/misc/systemd/nix-daemon.service.in @@ -10,6 +10,7 @@ ConditionPathIsReadWrite=@localstatedir@/nix/daemon-socket ExecStart=@@bindir@/nix-daemon nix-daemon --daemon KillMode=process LimitNOFILE=1048576 +TasksMax=infinity [Install] WantedBy=multi-user.target From 537e8beb770ed92dd1d5a20796a86620c4c61389 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 7 Jul 2023 11:00:40 +0200 Subject: [PATCH 15/38] fetchClosure: Apply suggestions from code review Co-authored-by: Valentin Gagarin --- src/libexpr/primops/fetchClosure.cc | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index 5114df553..22bae19b7 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -74,7 +74,10 @@ static void runFetchClosureWithContentAddressedPath(EvalState & state, const Pos throw Error({ .msg = hintfmt( "The 'fromPath' value '%s' is input-addressed, but 'inputAddressed' is set to 'false' (default).\n\n" - "If you do intend to fetch 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.", + "If you do intend to fetch an input-addressed store path, add\n\n" + " inputAddressed = true;\n\n" + "to the 'fetchClosure' arguments.\n\n" + "Note that to ensure authenticity input-addressed store paths, users must configure a trusted binary cache public key on their systems. This is not needed for content-addressed paths.", state.store->printStorePath(fromPath)), .errPos = state.positions[pos] }); @@ -226,7 +229,7 @@ static RegisterPrimOp primop_fetchClosure({ - Fetch a content-addressed store path. -Example: + Example: ```nix builtins.fetchClosure { @@ -263,7 +266,7 @@ Example: rewrote '/nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1' to '/nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1' ``` - Alternatively, you may set `toPath = ""` and find the correct `toPath` in the error message. + Alternatively, set `toPath = ""` and find the correct `toPath` in the error message. **Not rewriting** @@ -279,13 +282,7 @@ Example: 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, 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). - - **Experimental feature** - - This function is only available if you enable the experimental - feature `fetch-closure`. + Also, using content-addressed store paths does not require users to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys) to ensure their authenticity. )", .fun = prim_fetchClosure, .experimentalFeature = Xp::FetchClosure, From b4b02d084f066d6391074ac807e532e36e4eccd9 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 7 Jul 2023 11:40:40 +0200 Subject: [PATCH 16/38] fetchClosure: Interleave the examples in the docs --- src/libexpr/primops/fetchClosure.cc | 79 ++++++++++++++--------------- 1 file changed, 37 insertions(+), 42 deletions(-) diff --git a/src/libexpr/primops/fetchClosure.cc b/src/libexpr/primops/fetchClosure.cc index 22bae19b7..7fe8203f4 100644 --- a/src/libexpr/primops/fetchClosure.cc +++ b/src/libexpr/primops/fetchClosure.cc @@ -211,52 +211,42 @@ static RegisterPrimOp primop_fetchClosure({ .name = "__fetchClosure", .args = {"args"}, .doc = R"( - Fetch a store path [closure](@docroot@/glossary.md#gloss-closure) from a binary cache. + Fetch a store path [closure](@docroot@/glossary.md#gloss-closure) from a binary cache, and return the store path as a string with context. - This function can be used in three ways: + This function can be invoked in three ways, that we will discuss in order of preference. - - Fetch any store path and rewrite it to a fully content-addressed store path. - - Example: - - ```nix - builtins.fetchClosure { - fromStore = "https://cache.nixos.org"; - fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1; - toPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1; - } - ``` - - - Fetch a content-addressed store path. + **Fetch a content-addressed store path** Example: - ```nix - builtins.fetchClosure { - fromStore = "https://cache.nixos.org"; - fromPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1; - } - ``` + ```nix + builtins.fetchClosure { + fromStore = "https://cache.nixos.org"; + fromPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1; + } + ``` - - Fetch an [input-addressed store path](@docroot@/glossary.md#gloss-input-addressed-store-object) as is. This depends on user configuration, which is less preferable. + This is the simplest invocation, and it does not require the user of the expression to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys) to ensure their authenticity. - Example: + If your store path is [input addressed](@docroot@/glossary.md#gloss-input-addressed-store-object) instead of content addressed, consider the other two invocations. - ```nix - builtins.fetchClosure { - fromStore = "https://cache.nixos.org"; - fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1; - inputAddressed = true; - } - ``` + **Fetch any store path and rewrite it to a fully content-addressed store path** - **Rewriting a store path** + Example: - This first example fetches `/nix/store/r2jd...` from the specified binary cache, + ```nix + builtins.fetchClosure { + fromStore = "https://cache.nixos.org"; + fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1; + toPath = /nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1; + } + ``` + + This example fetches `/nix/store/r2jd...` from the specified binary cache, and rewrites it into the content-addressed store path `/nix/store/ldbh...`. - By rewriting the store paths, you can add the contents to any store without requiring that you or perhaps your users configure any extra trust. + Like the previous example, no extra configuration or privileges are required. To find out the correct value for `toPath` given a `fromPath`, use [`nix store make-content-addressed`](@docroot@/command-ref/new-cli/nix3-store-make-content-addressed.md): @@ -268,20 +258,25 @@ static RegisterPrimOp primop_fetchClosure({ Alternatively, set `toPath = ""` and find the correct `toPath` in the error message. - **Not rewriting** + **Fetch an input-addressed store path as is** - `toPath` may be omitted when either - - `fromPath` is already content-addressed, or - - `inputAddressed = true;` is set. + Example: - Fetching an [input addressed path](@docroot@/glossary.md#gloss-input-addressed-store-object) is not recommended because it requires that the source be trusted by the Nix configuration. + ```nix + builtins.fetchClosure { + fromStore = "https://cache.nixos.org"; + fromPath = /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1; + inputAddressed = true; + } + ``` + + It is possible to fetch an [input-addressed store path](@docroot@/glossary.md#gloss-input-addressed-store-object) and return it as is. + However, this is the least preferred way of invoking `fetchClosure`, because it requires that the input-addressed paths are trusted by the Nix configuration. **`builtins.storePath`** - This function is similar to [`builtins.storePath`](#builtins-storePath) in that it - 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. + `fetchClosure` is similar to [`builtins.storePath`](#builtins-storePath) in that it allows you to use a previously built store path in a Nix expression. + However, `fetchClosure` is more reproducible because it specifies a binary cache from which the path can be fetched. Also, using content-addressed store paths does not require users to configure [`trusted-public-keys`](@docroot@/command-ref/conf-file.md#conf-trusted-public-keys) to ensure their authenticity. )", .fun = prim_fetchClosure, From 9fc82de49388a58240234d10893673566432f7ab Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 7 Jul 2023 15:07:30 +0200 Subject: [PATCH 17/38] signing.sh: Revert test improvement because it fails on GHA + macOS --- tests/signing.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/signing.sh b/tests/signing.sh index f14b53e7f..942b51630 100644 --- a/tests/signing.sh +++ b/tests/signing.sh @@ -84,7 +84,11 @@ info=$(nix path-info --store file://$cacheDir --json $outPath2) # Copying to a diverted store should fail due to a lack of signatures by trusted keys. chmod -R u+w $TEST_ROOT/store0 || true rm -rf $TEST_ROOT/store0 -expectStderr 1 nix copy --to $TEST_ROOT/store0 $outPath | grepQuiet -E 'cannot add path .* because it lacks a signature by a trusted key' + +# Fails or very flaky only on GHA + macOS: +# expectStderr 1 nix copy --to $TEST_ROOT/store0 $outPath | grepQuiet -E 'cannot add path .* because it lacks a signature by a trusted key' +# but this works: +(! nix copy --to $TEST_ROOT/store0 $outPath) # But succeed if we supply the public keys. nix copy --to $TEST_ROOT/store0 $outPath --trusted-public-keys $pk1 From d76bf29c5f55f63f10741290c8d415c880a80ac2 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Fri, 7 Jul 2023 07:52:16 -0700 Subject: [PATCH 18/38] Choose a reasonable number similar to LimitNOFile --- misc/systemd/nix-daemon.service.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/misc/systemd/nix-daemon.service.in b/misc/systemd/nix-daemon.service.in index 4b4474475..45fbea02c 100644 --- a/misc/systemd/nix-daemon.service.in +++ b/misc/systemd/nix-daemon.service.in @@ -10,7 +10,7 @@ ConditionPathIsReadWrite=@localstatedir@/nix/daemon-socket ExecStart=@@bindir@/nix-daemon nix-daemon --daemon KillMode=process LimitNOFILE=1048576 -TasksMax=infinity +TasksMax=1048576 [Install] WantedBy=multi-user.target From 87dcd090470ed6e56a2744cbe1490d2cf235d5c0 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 23 Jun 2023 12:31:09 -0400 Subject: [PATCH 19/38] Clean up `resolveSearchPathElem` We should use `std::optional` not `std::pair` for an optional string. --- src/libexpr/eval.cc | 14 +++++++------- src/libexpr/eval.hh | 8 ++++++-- src/libexpr/parser.y | 45 +++++++++++++++++++++++--------------------- 3 files changed, 37 insertions(+), 30 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 706a19024..97a264085 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -571,22 +571,22 @@ EvalState::EvalState( allowedPaths = PathSet(); for (auto & i : searchPath) { - auto r = resolveSearchPathElem(i); - if (!r.first) continue; + auto r = resolveSearchPathElem(i.path); + if (!r) continue; - auto path = r.second; + auto path = *std::move(r); - if (store->isInStore(r.second)) { + if (store->isInStore(path)) { try { StorePathSet closure; - store->computeFSClosure(store->toStorePath(r.second).first, closure); + store->computeFSClosure(store->toStorePath(path).first, closure); for (auto & path : closure) allowPath(path); } catch (InvalidPath &) { - allowPath(r.second); + allowPath(path); } } else - allowPath(r.second); + allowPath(path); } } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index e3676c1b7..e1a540a7f 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -317,7 +317,7 @@ private: SearchPath searchPath; - std::map> searchPathResolved; + std::map> searchPathResolved; /** * Cache used by checkSourcePath(). @@ -434,9 +434,13 @@ public: SourcePath findFile(SearchPath & searchPath, const std::string_view path, const PosIdx pos = noPos); /** + * Try to resolve a search path value (not the optinal key part) + * * If the specified search path element is a URI, download it. + * + * If it is not found, return `std::nullopt` */ - std::pair resolveSearchPathElem(const SearchPathElem & elem); + std::optional resolveSearchPathElem(const std::string & value); /** * Evaluate an expression to normal form diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 0d0004f9f..f839e804c 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -772,9 +772,9 @@ SourcePath EvalState::findFile(SearchPath & searchPath, const std::string_view p continue; suffix = path.size() == s ? "" : concatStrings("/", path.substr(s)); } - auto r = resolveSearchPathElem(i); - if (!r.first) continue; - Path res = r.second + suffix; + auto r = resolveSearchPathElem(i.path); + if (!r) continue; + Path res = *r + suffix; if (pathExists(res)) return CanonPath(canonPath(res)); } @@ -791,49 +791,52 @@ SourcePath EvalState::findFile(SearchPath & searchPath, const std::string_view p } -std::pair EvalState::resolveSearchPathElem(const SearchPathElem & elem) +std::optional EvalState::resolveSearchPathElem(const std::string & value) { - auto i = searchPathResolved.find(elem.path); + auto i = searchPathResolved.find(value); if (i != searchPathResolved.end()) return i->second; - std::pair res; + std::optional res; - if (EvalSettings::isPseudoUrl(elem.path)) { + if (EvalSettings::isPseudoUrl(value)) { try { auto storePath = fetchers::downloadTarball( - store, EvalSettings::resolvePseudoUrl(elem.path), "source", false).tree.storePath; - res = { true, store->toRealPath(storePath) }; + store, EvalSettings::resolvePseudoUrl(value), "source", false).tree.storePath; + res = { store->toRealPath(storePath) }; } catch (FileTransferError & e) { logWarning({ - .msg = hintfmt("Nix search path entry '%1%' cannot be downloaded, ignoring", elem.path) + .msg = hintfmt("Nix search path entry '%1%' cannot be downloaded, ignoring", value) }); - res = { false, "" }; + res = std::nullopt; } } - else if (hasPrefix(elem.path, "flake:")) { + else if (hasPrefix(value, "flake:")) { experimentalFeatureSettings.require(Xp::Flakes); - auto flakeRef = parseFlakeRef(elem.path.substr(6), {}, true, false); - debug("fetching flake search path element '%s''", elem.path); + auto flakeRef = parseFlakeRef(value.substr(6), {}, true, false); + debug("fetching flake search path element '%s''", value); auto storePath = flakeRef.resolve(store).fetchTree(store).first.storePath; - res = { true, store->toRealPath(storePath) }; + res = { store->toRealPath(storePath) }; } else { - auto path = absPath(elem.path); + auto path = absPath(value); if (pathExists(path)) - res = { true, path }; + res = { path }; else { logWarning({ - .msg = hintfmt("Nix search path entry '%1%' does not exist, ignoring", elem.path) + .msg = hintfmt("Nix search path entry '%1%' does not exist, ignoring", value) }); - res = { false, "" }; + res = std::nullopt; } } - debug("resolved search path element '%s' to '%s'", elem.path, res.second); + if (res) + debug("resolved search path element '%s' to '%s'", value, *res); + else + debug("failed to resolve search path element '%s'", value); - searchPathResolved[elem.path] = res; + searchPathResolved[value] = res; return res; } From be518e73ae331ac2f46e6b3a0ffdfeead26e3186 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 23 Jun 2023 13:51:25 -0400 Subject: [PATCH 20/38] Clean up `SearchPath` - Better types - Own header / C++ file pair - Test factored out methods - Pass parsed thing around more than strings Co-authored-by: Robert Hensing --- src/libcmd/common-eval-args.cc | 4 +- src/libcmd/common-eval-args.hh | 3 +- src/libcmd/repl.cc | 8 +-- src/libcmd/repl.hh | 2 +- src/libexpr/eval.cc | 12 ++-- src/libexpr/eval.hh | 18 ++---- src/libexpr/parser.y | 47 +++++--------- src/libexpr/primops.cc | 14 ++-- src/libexpr/search-path.cc | 56 ++++++++++++++++ src/libexpr/search-path.hh | 108 +++++++++++++++++++++++++++++++ src/libexpr/tests/search-path.cc | 90 ++++++++++++++++++++++++++ src/nix/upgrade-nix.cc | 2 +- 12 files changed, 300 insertions(+), 64 deletions(-) create mode 100644 src/libexpr/search-path.cc create mode 100644 src/libexpr/search-path.hh create mode 100644 src/libexpr/tests/search-path.cc diff --git a/src/libcmd/common-eval-args.cc b/src/libcmd/common-eval-args.cc index 7f97364a1..3df2c71a5 100644 --- a/src/libcmd/common-eval-args.cc +++ b/src/libcmd/common-eval-args.cc @@ -105,7 +105,9 @@ MixEvalArgs::MixEvalArgs() )", .category = category, .labels = {"path"}, - .handler = {[&](std::string s) { searchPath.push_back(s); }} + .handler = {[&](std::string s) { + searchPath.elements.emplace_back(SearchPath::Elem::parse(s)); + }} }); addFlag({ diff --git a/src/libcmd/common-eval-args.hh b/src/libcmd/common-eval-args.hh index b65cb5b20..6359b2579 100644 --- a/src/libcmd/common-eval-args.hh +++ b/src/libcmd/common-eval-args.hh @@ -3,6 +3,7 @@ #include "args.hh" #include "common-args.hh" +#include "search-path.hh" namespace nix { @@ -19,7 +20,7 @@ struct MixEvalArgs : virtual Args, virtual MixRepair Bindings * getAutoArgs(EvalState & state); - Strings searchPath; + SearchPath searchPath; std::optional evalStoreUrl; diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 4b160a100..f9e9c2bf8 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -68,7 +68,7 @@ struct NixRepl const Path historyFile; - NixRepl(const Strings & searchPath, nix::ref store,ref state, + NixRepl(const SearchPath & searchPath, nix::ref store,ref state, std::function getValues); virtual ~NixRepl(); @@ -104,7 +104,7 @@ std::string removeWhitespace(std::string s) } -NixRepl::NixRepl(const Strings & searchPath, nix::ref store, ref state, +NixRepl::NixRepl(const SearchPath & searchPath, nix::ref store, ref state, std::function getValues) : AbstractNixRepl(state) , debugTraceIndex(0) @@ -1024,7 +1024,7 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m std::unique_ptr AbstractNixRepl::create( - const Strings & searchPath, nix::ref store, ref state, + const SearchPath & searchPath, nix::ref store, ref state, std::function getValues) { return std::make_unique( @@ -1044,7 +1044,7 @@ void AbstractNixRepl::runSimple( NixRepl::AnnotatedValues values; return values; }; - const Strings & searchPath = {}; + SearchPath searchPath = {}; auto repl = std::make_unique( searchPath, openStore(), diff --git a/src/libcmd/repl.hh b/src/libcmd/repl.hh index 731c8e6db..6d88883fe 100644 --- a/src/libcmd/repl.hh +++ b/src/libcmd/repl.hh @@ -25,7 +25,7 @@ struct AbstractNixRepl typedef std::vector> AnnotatedValues; static std::unique_ptr create( - const Strings & searchPath, nix::ref store, ref state, + const SearchPath & searchPath, nix::ref store, ref state, std::function getValues); static void runSimple( diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 97a264085..be1bdb806 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -498,7 +498,7 @@ ErrorBuilder & ErrorBuilder::withFrame(const Env & env, const Expr & expr) EvalState::EvalState( - const Strings & _searchPath, + const SearchPath & _searchPath, ref store, std::shared_ptr buildStore) : sWith(symbols.create("")) @@ -563,15 +563,17 @@ EvalState::EvalState( /* Initialise the Nix expression search path. */ if (!evalSettings.pureEval) { - for (auto & i : _searchPath) addToSearchPath(i); - for (auto & i : evalSettings.nixPath.get()) addToSearchPath(i); + for (auto & i : _searchPath.elements) + addToSearchPath(SearchPath::Elem {i}); + for (auto & i : evalSettings.nixPath.get()) + addToSearchPath(SearchPath::Elem::parse(i)); } if (evalSettings.restrictEval || evalSettings.pureEval) { allowedPaths = PathSet(); - for (auto & i : searchPath) { - auto r = resolveSearchPathElem(i.path); + for (auto & i : searchPath.elements) { + auto r = resolveSearchPathPath(i.path); if (!r) continue; auto path = *std::move(r); diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index e1a540a7f..277e77ad5 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -9,6 +9,7 @@ #include "config.hh" #include "experimental-features.hh" #include "input-accessor.hh" +#include "search-path.hh" #include #include @@ -122,15 +123,6 @@ std::string printValue(const EvalState & state, const Value & v); std::ostream & operator << (std::ostream & os, const ValueType t); -struct SearchPathElem -{ - std::string prefix; - // FIXME: maybe change this to an std::variant. - std::string path; -}; -typedef std::list SearchPath; - - /** * Initialise the Boehm GC, if applicable. */ @@ -344,12 +336,12 @@ private: public: EvalState( - const Strings & _searchPath, + const SearchPath & _searchPath, ref store, std::shared_ptr buildStore = nullptr); ~EvalState(); - void addToSearchPath(const std::string & s); + void addToSearchPath(SearchPath::Elem && elem); SearchPath getSearchPath() { return searchPath; } @@ -431,7 +423,7 @@ public: * Look up a file in the search path. */ SourcePath findFile(const std::string_view path); - SourcePath findFile(SearchPath & searchPath, const std::string_view path, const PosIdx pos = noPos); + SourcePath findFile(const SearchPath & searchPath, const std::string_view path, const PosIdx pos = noPos); /** * Try to resolve a search path value (not the optinal key part) @@ -440,7 +432,7 @@ public: * * If it is not found, return `std::nullopt` */ - std::optional resolveSearchPathElem(const std::string & value); + std::optional resolveSearchPathPath(const SearchPath::Path & path); /** * Evaluate an expression to normal form diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index f839e804c..77bcc31e0 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -734,22 +734,9 @@ Expr * EvalState::parseStdin() } -void EvalState::addToSearchPath(const std::string & s) +void EvalState::addToSearchPath(SearchPath::Elem && elem) { - size_t pos = s.find('='); - std::string prefix; - Path path; - if (pos == std::string::npos) { - path = s; - } else { - prefix = std::string(s, 0, pos); - path = std::string(s, pos + 1); - } - - searchPath.emplace_back(SearchPathElem { - .prefix = prefix, - .path = path, - }); + searchPath.elements.emplace_back(std::move(elem)); } @@ -759,22 +746,19 @@ SourcePath EvalState::findFile(const std::string_view path) } -SourcePath EvalState::findFile(SearchPath & searchPath, const std::string_view path, const PosIdx pos) +SourcePath EvalState::findFile(const SearchPath & searchPath, const std::string_view path, const PosIdx pos) { - for (auto & i : searchPath) { - std::string suffix; - if (i.prefix.empty()) - suffix = concatStrings("/", path); - else { - auto s = i.prefix.size(); - if (path.compare(0, s, i.prefix) != 0 || - (path.size() > s && path[s] != '/')) - continue; - suffix = path.size() == s ? "" : concatStrings("/", path.substr(s)); - } - auto r = resolveSearchPathElem(i.path); - if (!r) continue; - Path res = *r + suffix; + for (auto & i : searchPath.elements) { + auto suffixOpt = i.prefix.suffixIfPotentialMatch(path); + + if (!suffixOpt) continue; + auto suffix = *suffixOpt; + + auto rOpt = resolveSearchPathPath(i.path); + if (!rOpt) continue; + auto r = *rOpt; + + Path res = suffix == "" ? r : concatStrings(r, "/", suffix); if (pathExists(res)) return CanonPath(canonPath(res)); } @@ -791,8 +775,9 @@ SourcePath EvalState::findFile(SearchPath & searchPath, const std::string_view p } -std::optional EvalState::resolveSearchPathElem(const std::string & value) +std::optional EvalState::resolveSearchPathPath(const SearchPath::Path & value0) { + auto & value = value0.s; auto i = searchPathResolved.find(value); if (i != searchPathResolved.end()) return i->second; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 5dfad470a..b98b06db9 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1656,9 +1656,9 @@ static void prim_findFile(EvalState & state, const PosIdx pos, Value * * args, V })); } - searchPath.emplace_back(SearchPathElem { - .prefix = prefix, - .path = path, + searchPath.elements.emplace_back(SearchPath::Elem { + .prefix = SearchPath::Prefix { .s = prefix }, + .path = SearchPath::Path { .s = path }, }); } @@ -4319,12 +4319,12 @@ void EvalState::createBaseEnv() }); /* Add a value containing the current Nix expression search path. */ - mkList(v, searchPath.size()); + mkList(v, searchPath.elements.size()); int n = 0; - for (auto & i : searchPath) { + for (auto & i : searchPath.elements) { auto attrs = buildBindings(2); - attrs.alloc("path").mkString(i.path); - attrs.alloc("prefix").mkString(i.prefix); + attrs.alloc("path").mkString(i.path.s); + attrs.alloc("prefix").mkString(i.prefix.s); (v.listElems()[n++] = allocValue())->mkAttrs(attrs); } addConstant("__nixPath", v, { diff --git a/src/libexpr/search-path.cc b/src/libexpr/search-path.cc new file mode 100644 index 000000000..36bb4c3a5 --- /dev/null +++ b/src/libexpr/search-path.cc @@ -0,0 +1,56 @@ +#include "search-path.hh" +#include "util.hh" + +namespace nix { + +std::optional SearchPath::Prefix::suffixIfPotentialMatch( + std::string_view path) const +{ + auto n = s.size(); + + /* Non-empty prefix and suffix must be separated by a /, or the + prefix is not a valid path prefix. */ + bool needSeparator = n > 0 && (path.size() - n) > 0; + + if (needSeparator && path[n] != '/') { + return std::nullopt; + } + + /* Prefix must be prefix of this path. */ + if (path.compare(0, n, s) != 0) { + return std::nullopt; + } + + /* Skip next path separator. */ + return { + path.substr(needSeparator ? n + 1 : n) + }; +} + + +SearchPath::Elem SearchPath::Elem::parse(std::string_view rawElem) +{ + size_t pos = rawElem.find('='); + + return SearchPath::Elem { + .prefix = Prefix { + .s = pos == std::string::npos + ? std::string { "" } + : std::string { rawElem.substr(0, pos) }, + }, + .path = Path { + .s = std::string { rawElem.substr(pos + 1) }, + }, + }; +} + + +SearchPath parseSearchPath(const Strings & rawElems) +{ + SearchPath res; + for (auto & rawElem : rawElems) + res.elements.emplace_back(SearchPath::Elem::parse(rawElem)); + return res; +} + +} diff --git a/src/libexpr/search-path.hh b/src/libexpr/search-path.hh new file mode 100644 index 000000000..ce78135b5 --- /dev/null +++ b/src/libexpr/search-path.hh @@ -0,0 +1,108 @@ +#pragma once +///@file + +#include + +#include "types.hh" +#include "comparator.hh" + +namespace nix { + +/** + * A "search path" is a list of ways look for something, used with + * `builtins.findFile` and `< >` lookup expressions. + */ +struct SearchPath +{ + /** + * A single element of a `SearchPath`. + * + * Each element is tried in succession when looking up a path. The first + * element to completely match wins. + */ + struct Elem; + + /** + * The first part of a `SearchPath::Elem` pair. + * + * Called a "prefix" because it takes the form of a prefix of a file + * path (first `n` path components). When looking up a path, to use + * a `SearchPath::Elem`, its `Prefix` must match the path. + */ + struct Prefix; + + /** + * The second part of a `SearchPath::Elem` pair. + * + * It is either a path or a URL (with certain restrictions / extra + * structure). + * + * If the prefix of the path we are looking up matches, we then + * check if the rest of the path points to something that exists + * within the directory denoted by this. If so, the + * `SearchPath::Elem` as a whole matches, and that *something* being + * pointed to by the rest of the path we are looking up is the + * result. + */ + struct Path; + + /** + * The list of search path elements. Each one is checked for a path + * when looking up. (The actual lookup entry point is in `EvalState` + * not in this class.) + */ + std::list elements; + + /** + * Parse a string into a `SearchPath` + */ + static SearchPath parse(const Strings & rawElems); +}; + +struct SearchPath::Prefix +{ + /** + * Underlying string + * + * @todo Should we normalize this when constructing a `SearchPath::Prefix`? + */ + std::string s; + + GENERATE_CMP(SearchPath::Prefix, me->s); + + /** + * If the path possibly matches this search path element, return the + * suffix that we should look for inside the resolved value of the + * element + * Note the double optionality in the name. While we might have a matching prefix, the suffix may not exist. + */ + std::optional suffixIfPotentialMatch(std::string_view path) const; +}; + +struct SearchPath::Path +{ + /** + * The location of a search path item, as a path or URL. + * + * @todo Maybe change this to `std::variant`. + */ + std::string s; + + GENERATE_CMP(SearchPath::Path, me->s); +}; + +struct SearchPath::Elem +{ + + Prefix prefix; + Path path; + + GENERATE_CMP(SearchPath::Elem, me->prefix, me->path); + + /** + * Parse a string into a `SearchPath::Elem` + */ + static SearchPath::Elem parse(std::string_view rawElem); +}; + +} diff --git a/src/libexpr/tests/search-path.cc b/src/libexpr/tests/search-path.cc new file mode 100644 index 000000000..dbe7ab95f --- /dev/null +++ b/src/libexpr/tests/search-path.cc @@ -0,0 +1,90 @@ +#include +#include + +#include "search-path.hh" + +namespace nix { + +TEST(SearchPathElem, parse_justPath) { + ASSERT_EQ( + SearchPath::Elem::parse("foo"), + (SearchPath::Elem { + .prefix = SearchPath::Prefix { .s = "" }, + .path = SearchPath::Path { .s = "foo" }, + })); +} + +TEST(SearchPathElem, parse_emptyPrefix) { + ASSERT_EQ( + SearchPath::Elem::parse("=foo"), + (SearchPath::Elem { + .prefix = SearchPath::Prefix { .s = "" }, + .path = SearchPath::Path { .s = "foo" }, + })); +} + +TEST(SearchPathElem, parse_oneEq) { + ASSERT_EQ( + SearchPath::Elem::parse("foo=bar"), + (SearchPath::Elem { + .prefix = SearchPath::Prefix { .s = "foo" }, + .path = SearchPath::Path { .s = "bar" }, + })); +} + +TEST(SearchPathElem, parse_twoEqs) { + ASSERT_EQ( + SearchPath::Elem::parse("foo=bar=baz"), + (SearchPath::Elem { + .prefix = SearchPath::Prefix { .s = "foo" }, + .path = SearchPath::Path { .s = "bar=baz" }, + })); +} + + +TEST(SearchPathElem, suffixIfPotentialMatch_justPath) { + SearchPath::Prefix prefix { .s = "" }; + ASSERT_EQ(prefix.suffixIfPotentialMatch("any/thing"), std::optional { "any/thing" }); +} + +TEST(SearchPathElem, suffixIfPotentialMatch_misleadingPrefix1) { + SearchPath::Prefix prefix { .s = "foo" }; + ASSERT_EQ(prefix.suffixIfPotentialMatch("fooX"), std::nullopt); +} + +TEST(SearchPathElem, suffixIfPotentialMatch_misleadingPrefix2) { + SearchPath::Prefix prefix { .s = "foo" }; + ASSERT_EQ(prefix.suffixIfPotentialMatch("fooX/bar"), std::nullopt); +} + +TEST(SearchPathElem, suffixIfPotentialMatch_partialPrefix) { + SearchPath::Prefix prefix { .s = "fooX" }; + ASSERT_EQ(prefix.suffixIfPotentialMatch("foo"), std::nullopt); +} + +TEST(SearchPathElem, suffixIfPotentialMatch_exactPrefix) { + SearchPath::Prefix prefix { .s = "foo" }; + ASSERT_EQ(prefix.suffixIfPotentialMatch("foo"), std::optional { "" }); +} + +TEST(SearchPathElem, suffixIfPotentialMatch_multiKey) { + SearchPath::Prefix prefix { .s = "foo/bar" }; + ASSERT_EQ(prefix.suffixIfPotentialMatch("foo/bar/baz"), std::optional { "baz" }); +} + +TEST(SearchPathElem, suffixIfPotentialMatch_trailingSlash) { + SearchPath::Prefix prefix { .s = "foo" }; + ASSERT_EQ(prefix.suffixIfPotentialMatch("foo/"), std::optional { "" }); +} + +TEST(SearchPathElem, suffixIfPotentialMatch_trailingDoubleSlash) { + SearchPath::Prefix prefix { .s = "foo" }; + ASSERT_EQ(prefix.suffixIfPotentialMatch("foo//"), std::optional { "/" }); +} + +TEST(SearchPathElem, suffixIfPotentialMatch_trailingPath) { + SearchPath::Prefix prefix { .s = "foo" }; + ASSERT_EQ(prefix.suffixIfPotentialMatch("foo/bar/baz"), std::optional { "bar/baz" }); +} + +} diff --git a/src/nix/upgrade-nix.cc b/src/nix/upgrade-nix.cc index 3997c98bf..d05c23fb7 100644 --- a/src/nix/upgrade-nix.cc +++ b/src/nix/upgrade-nix.cc @@ -146,7 +146,7 @@ struct CmdUpgradeNix : MixDryRun, StoreCommand auto req = FileTransferRequest(storePathsUrl); auto res = getFileTransfer()->download(req); - auto state = std::make_unique(Strings(), store); + auto state = std::make_unique(SearchPath{}, store); auto v = state->allocValue(); state->eval(state->parseExprFromString(res.data, state->rootPath(CanonPath("/no-such-path"))), *v); Bindings & bindings(*state->allocBindings(0)); From 3d74e7b811ea8fc13e31127175be12ba8d39351c Mon Sep 17 00:00:00 2001 From: Yingchi Long Date: Mon, 10 Jul 2023 12:02:29 +0800 Subject: [PATCH 21/38] libexpr: remove std::move() for `basePath` in parser, it has no effect --- src/libexpr/parser.y | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 0d0004f9f..3336d3315 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -663,7 +663,7 @@ Expr * EvalState::parse( ParseData data { .state = *this, .symbols = symbols, - .basePath = std::move(basePath), + .basePath = basePath, .origin = {origin}, }; From 3fa0266e7a736e85e2faaa6c92ab23e809f89930 Mon Sep 17 00:00:00 2001 From: Bader AlAttar <68021685+balattar@users.noreply.github.com> Date: Mon, 10 Jul 2023 02:33:04 -0700 Subject: [PATCH 22/38] Fix some grammar in installables doc (#8682) --- src/nix/nix.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nix/nix.md b/src/nix/nix.md index 6d9e40dbc..e0f459d6b 100644 --- a/src/nix/nix.md +++ b/src/nix/nix.md @@ -63,7 +63,7 @@ The following types of installable are supported by most commands: - [Nix file](#nix-file), optionally qualified by an attribute path - [Nix expression](#nix-expression), optionally qualified by an attribute path -For most commands, if no installable is specified, `.` as assumed. +For most commands, if no installable is specified, `.` is assumed. That is, Nix will operate on the default flake output attribute of the flake in the current directory. ### Flake output attribute From c2c8187118c4bf2961aa175ff8667e1402a767c7 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 21 Jun 2023 23:15:24 -0400 Subject: [PATCH 23/38] Fix test file name It's UTF-8, not UFT-8. --- tests/lang/{parse-fail-uft8.nix => parse-fail-utf8.nix} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/lang/{parse-fail-uft8.nix => parse-fail-utf8.nix} (100%) diff --git a/tests/lang/parse-fail-uft8.nix b/tests/lang/parse-fail-utf8.nix similarity index 100% rename from tests/lang/parse-fail-uft8.nix rename to tests/lang/parse-fail-utf8.nix From 07dabcc90ed8f2a2e7b98d858a47de3e75d2c3a2 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Tue, 11 Jul 2023 10:44:03 +0100 Subject: [PATCH 24/38] Always attempt setgroups but allow failure to be ignored. --- src/libstore/build/local-derivation-goal.cc | 9 ++++++--- src/libstore/globals.hh | 2 +- tests/supplementary-groups.sh | 8 ++++---- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 53e6998e8..068b47f93 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -909,9 +909,12 @@ void LocalDerivationGoal::startBuilder() /* Drop additional groups here because we can't do it after we've created the new user namespace. */ - if (settings.dropSupplementaryGroups) - if (setgroups(0, 0) == -1) - throw SysError("setgroups failed. Set the drop-supplementary-groups option to false to skip this step."); + if (setgroups(0, 0) == -1) { + if (errno != EPERM) + throw SysError("setgroups failed"); + if (settings.requireDropSupplementaryGroups) + throw Error("setgroups failed. Set the require-drop-supplementary-groups option to false to skip this step."); + } ProcessOptions options; options.cloneFlags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD; diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index a19b43086..dbabf116a 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -524,7 +524,7 @@ public: Setting sandboxFallback{this, true, "sandbox-fallback", "Whether to disable sandboxing when the kernel doesn't allow it."}; - Setting dropSupplementaryGroups{this, getuid() == 0, "drop-supplementary-groups", + Setting requireDropSupplementaryGroups{this, true, "require-drop-supplementary-groups", R"( Whether to drop supplementary groups when building with sandboxing. This is normally a good idea if we are root and have the capability to diff --git a/tests/supplementary-groups.sh b/tests/supplementary-groups.sh index 47debc5e3..47c6ef605 100644 --- a/tests/supplementary-groups.sh +++ b/tests/supplementary-groups.sh @@ -20,14 +20,14 @@ unshare --mount --map-root-user bash < Date: Tue, 11 Jul 2023 10:57:03 +0100 Subject: [PATCH 25/38] Update description for require-drop-supplementary-groups. --- src/libstore/globals.hh | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index dbabf116a..601626d00 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -526,21 +526,15 @@ public: Setting requireDropSupplementaryGroups{this, true, "require-drop-supplementary-groups", R"( - Whether to drop supplementary groups when building with sandboxing. - This is normally a good idea if we are root and have the capability to - do so. + Following the principle of least privilege, + Nix will attempt to drop supplementary groups when building with sandboxing. - But if this "root" is mapped from a non-root user in a larger - namespace, we won't be able drop additional groups; they will be - mapped to nogroup in the child namespace. There does not seem to be a - workaround for this. + However this can fail under some circumstances. + For example, if the user lacks the CAP_SETGID capability. + Search setgroups(2) for EPERM to find more detailed information on this. - (But who can tell from reading user_namespaces(7)? See also https://lwn.net/Articles/621612/.) - - TODO: It might be good to create a middle ground option that allows - `setgroups` to fail if all additional groups are "nogroup" / the value - of `/proc/sys/fs/overflowuid`. This would handle the common - nested-sandboxing case identified above. + If you encounter such a failure, + you can instruct Nix to continue without dropping supplementary groups by setting this option to `false`. )"}; #if __linux__ From 2b4c59dd997c72069b6039783fea4c3b35f5cee7 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Tue, 11 Jul 2023 11:09:25 +0100 Subject: [PATCH 26/38] Be clearer about the security implications. --- src/libstore/globals.hh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 601626d00..dec132ff0 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -533,8 +533,9 @@ public: For example, if the user lacks the CAP_SETGID capability. Search setgroups(2) for EPERM to find more detailed information on this. - If you encounter such a failure, - you can instruct Nix to continue without dropping supplementary groups by setting this option to `false`. + If you encounter such a failure, setting this option to `false` will let you ignore it and continue. + But before doing so, you should consider the security implications carefully. + Not dropping supplementary groups means the build sandbox will be less restricted than intended. )"}; #if __linux__ From a193ec4052d9efa895681c438cc335296c7affea Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Tue, 11 Jul 2023 11:13:39 +0100 Subject: [PATCH 27/38] Default should depend on whether we are root. --- src/libstore/globals.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index dec132ff0..9a9b4903f 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -524,7 +524,7 @@ public: Setting sandboxFallback{this, true, "sandbox-fallback", "Whether to disable sandboxing when the kernel doesn't allow it."}; - Setting requireDropSupplementaryGroups{this, true, "require-drop-supplementary-groups", + Setting requireDropSupplementaryGroups{this, getuid() == 0, "require-drop-supplementary-groups", R"( Following the principle of least privilege, Nix will attempt to drop supplementary groups when building with sandboxing. From b8e8dfc3e8581a08bc179f75fbe61e04030088de Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Tue, 11 Jul 2023 11:24:11 +0100 Subject: [PATCH 28/38] Say a bit about default value in setting description. --- src/libstore/globals.hh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 9a9b4903f..81fa154bb 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -536,6 +536,10 @@ public: If you encounter such a failure, setting this option to `false` will let you ignore it and continue. But before doing so, you should consider the security implications carefully. Not dropping supplementary groups means the build sandbox will be less restricted than intended. + + This option defaults to `true` when the user is root + (since root usually has permissions to call setgroups) + and `false` otherwise. )"}; #if __linux__ From c1d39de1fbceebbb6b46abc4a21fb8e34423df99 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Thu, 15 Jun 2023 12:48:06 +0100 Subject: [PATCH 29/38] Check _NIX_TEST_NO_SANDBOX when setting _canUseSandbox. --- tests/common/vars-and-functions.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/common/vars-and-functions.sh.in b/tests/common/vars-and-functions.sh.in index dc7ce13cc..ad0623871 100644 --- a/tests/common/vars-and-functions.sh.in +++ b/tests/common/vars-and-functions.sh.in @@ -141,7 +141,7 @@ restartDaemon() { startDaemon } -if [[ $(uname) == Linux ]] && [[ -L /proc/self/ns/user ]] && unshare --user true; then +if [[ -z "${_NIX_TEST_NO_SANDBOX:-}" ]] && [[ $(uname) == Linux ]] && [[ -L /proc/self/ns/user ]] && unshare --user true; then _canUseSandbox=1 fi From 41412dc4ae0fec2d08335c19724276d99e0c6056 Mon Sep 17 00:00:00 2001 From: Ben Radford Date: Thu, 15 Jun 2023 12:58:59 +0100 Subject: [PATCH 30/38] Skip build-remote-trustless unless sandbox is supported. --- tests/build-remote-trustless-should-fail-0.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/build-remote-trustless-should-fail-0.sh b/tests/build-remote-trustless-should-fail-0.sh index fad1def59..b14101f83 100644 --- a/tests/build-remote-trustless-should-fail-0.sh +++ b/tests/build-remote-trustless-should-fail-0.sh @@ -2,6 +2,7 @@ source common.sh enableFeatures "daemon-trust-override" +requireSandboxSupport restartDaemon [[ $busybox =~ busybox ]] || skipTest "no busybox" From c70484454f52a3bba994ac0c155b4a6f35a5013c Mon Sep 17 00:00:00 2001 From: Mathnerd314 Date: Fri, 4 Sep 2015 14:23:08 -0600 Subject: [PATCH 31/38] Expanded test suite * Lang now verifies errors and parse output * Some new miscellaneous tests * Easy way to update the tests * Document workflow in manual * Use `!` not `~` as separater char for sed It is confusing to use `~` when we are talking about paths and home directories! * Test test suite itself (`test/lang-test/infra.sh`) Additionally, run shellcheck on `tests/lang.sh` to help ensure it is correct, now that is is more complex. Co-authored-by: Robert Hensing Co-authored-by: Valentin Gagarin --- .gitignore | 1 + doc/manual/src/contributing/testing.md | 25 ++++ tests/dependencies.sh | 3 + tests/lang-test-infra.sh | 86 ++++++++++++ tests/lang.sh | 123 +++++++++++++----- tests/lang/empty.exp | 0 tests/lang/eval-fail-abort.err.exp | 10 ++ tests/lang/eval-fail-antiquoted-path.err.exp | 1 + tests/lang/eval-fail-assert.err.exp | 36 +++++ tests/lang/eval-fail-bad-antiquote-1.err.exp | 10 ++ tests/lang/eval-fail-bad-antiquote-2.err.exp | 1 + tests/lang/eval-fail-bad-antiquote-3.err.exp | 10 ++ ...al-fail-bad-string-interpolation-1.err.exp | 10 ++ ...al-fail-bad-string-interpolation-2.err.exp | 1 + ...al-fail-bad-string-interpolation-3.err.exp | 10 ++ tests/lang/eval-fail-blackhole.err.exp | 18 +++ tests/lang/eval-fail-deepseq.err.exp | 26 ++++ ...-foldlStrict-strict-op-application.err.exp | 38 ++++++ .../eval-fail-fromTOML-timestamps.err.exp | 12 ++ tests/lang/eval-fail-hashfile-missing.err.exp | 19 +++ tests/lang/eval-fail-list.err.exp | 10 ++ tests/lang/eval-fail-list.nix | 1 + tests/lang/eval-fail-missing-arg.err.exp | 16 +++ tests/lang/eval-fail-nonexist-path.err.exp | 1 + tests/lang/eval-fail-path-slash.err.exp | 8 ++ tests/lang/eval-fail-recursion.err.exp | 16 +++ tests/lang/eval-fail-recursion.nix | 1 + tests/lang/eval-fail-remove.err.exp | 19 +++ tests/lang/eval-fail-scope-5.err.exp | 36 +++++ tests/lang/eval-fail-seq.err.exp | 18 +++ tests/lang/eval-fail-set-override.err.exp | 6 + tests/lang/eval-fail-set-override.nix | 1 + tests/lang/eval-fail-set.err.exp | 7 + tests/lang/eval-fail-set.nix | 1 + tests/lang/eval-fail-substring.err.exp | 12 ++ tests/lang/eval-fail-to-path.err.exp | 14 ++ tests/lang/eval-fail-undeclared-arg.err.exp | 17 +++ tests/lang/eval-okay-fromjson.nix | 14 +- tests/lang/eval-okay-overrides.nix | 2 +- tests/lang/eval-okay-print.err.exp | 1 + tests/lang/eval-okay-print.exp | 1 + tests/lang/eval-okay-print.nix | 1 + tests/lang/eval-okay-search-path.flags | 2 +- tests/lang/framework.sh | 33 +++++ tests/lang/parse-fail-dup-attrs-1.err.exp | 7 + tests/lang/parse-fail-dup-attrs-2.err.exp | 7 + tests/lang/parse-fail-dup-attrs-3.err.exp | 7 + tests/lang/parse-fail-dup-attrs-4.err.exp | 7 + tests/lang/parse-fail-dup-attrs-6.err.exp | 1 + tests/lang/parse-fail-dup-attrs-7.err.exp | 7 + tests/lang/parse-fail-dup-formals.err.exp | 6 + tests/lang/parse-fail-eof-in-string.err.exp | 7 + .../parse-fail-mixed-nested-attrs1.err.exp | 8 ++ .../parse-fail-mixed-nested-attrs2.err.exp | 8 ++ tests/lang/parse-fail-patterns-1.err.exp | 7 + .../parse-fail-regression-20060610.err.exp | 8 ++ tests/lang/parse-fail-undef-var-2.err.exp | 7 + tests/lang/parse-fail-undef-var.err.exp | 7 + tests/lang/parse-fail-utf8.err.exp | 6 + tests/lang/parse-okay-1.exp | 1 + tests/lang/parse-okay-crlf.exp | 1 + tests/lang/parse-okay-dup-attrs-5.exp | 1 + tests/lang/parse-okay-dup-attrs-6.exp | 1 + .../lang/parse-okay-mixed-nested-attrs-1.exp | 1 + .../lang/parse-okay-mixed-nested-attrs-2.exp | 1 + .../lang/parse-okay-mixed-nested-attrs-3.exp | 1 + tests/lang/parse-okay-regression-20041027.exp | 1 + tests/lang/parse-okay-regression-751.exp | 1 + tests/lang/parse-okay-subversion.exp | 1 + tests/lang/parse-okay-url.exp | 1 + tests/local.mk | 1 + tests/misc.sh | 6 + tests/restricted.sh | 2 + 73 files changed, 762 insertions(+), 36 deletions(-) create mode 100644 tests/lang-test-infra.sh mode change 100644 => 100755 tests/lang.sh create mode 100644 tests/lang/empty.exp create mode 100644 tests/lang/eval-fail-abort.err.exp create mode 100644 tests/lang/eval-fail-antiquoted-path.err.exp create mode 100644 tests/lang/eval-fail-assert.err.exp create mode 100644 tests/lang/eval-fail-bad-antiquote-1.err.exp create mode 100644 tests/lang/eval-fail-bad-antiquote-2.err.exp create mode 100644 tests/lang/eval-fail-bad-antiquote-3.err.exp create mode 100644 tests/lang/eval-fail-bad-string-interpolation-1.err.exp create mode 100644 tests/lang/eval-fail-bad-string-interpolation-2.err.exp create mode 100644 tests/lang/eval-fail-bad-string-interpolation-3.err.exp create mode 100644 tests/lang/eval-fail-blackhole.err.exp create mode 100644 tests/lang/eval-fail-deepseq.err.exp create mode 100644 tests/lang/eval-fail-foldlStrict-strict-op-application.err.exp create mode 100644 tests/lang/eval-fail-fromTOML-timestamps.err.exp create mode 100644 tests/lang/eval-fail-hashfile-missing.err.exp create mode 100644 tests/lang/eval-fail-list.err.exp create mode 100644 tests/lang/eval-fail-list.nix create mode 100644 tests/lang/eval-fail-missing-arg.err.exp create mode 100644 tests/lang/eval-fail-nonexist-path.err.exp create mode 100644 tests/lang/eval-fail-path-slash.err.exp create mode 100644 tests/lang/eval-fail-recursion.err.exp create mode 100644 tests/lang/eval-fail-recursion.nix create mode 100644 tests/lang/eval-fail-remove.err.exp create mode 100644 tests/lang/eval-fail-scope-5.err.exp create mode 100644 tests/lang/eval-fail-seq.err.exp create mode 100644 tests/lang/eval-fail-set-override.err.exp create mode 100644 tests/lang/eval-fail-set-override.nix create mode 100644 tests/lang/eval-fail-set.err.exp create mode 100644 tests/lang/eval-fail-set.nix create mode 100644 tests/lang/eval-fail-substring.err.exp create mode 100644 tests/lang/eval-fail-to-path.err.exp create mode 100644 tests/lang/eval-fail-undeclared-arg.err.exp create mode 100644 tests/lang/eval-okay-print.err.exp create mode 100644 tests/lang/eval-okay-print.exp create mode 100644 tests/lang/eval-okay-print.nix create mode 100644 tests/lang/framework.sh create mode 100644 tests/lang/parse-fail-dup-attrs-1.err.exp create mode 100644 tests/lang/parse-fail-dup-attrs-2.err.exp create mode 100644 tests/lang/parse-fail-dup-attrs-3.err.exp create mode 100644 tests/lang/parse-fail-dup-attrs-4.err.exp create mode 100644 tests/lang/parse-fail-dup-attrs-6.err.exp create mode 100644 tests/lang/parse-fail-dup-attrs-7.err.exp create mode 100644 tests/lang/parse-fail-dup-formals.err.exp create mode 100644 tests/lang/parse-fail-eof-in-string.err.exp create mode 100644 tests/lang/parse-fail-mixed-nested-attrs1.err.exp create mode 100644 tests/lang/parse-fail-mixed-nested-attrs2.err.exp create mode 100644 tests/lang/parse-fail-patterns-1.err.exp create mode 100644 tests/lang/parse-fail-regression-20060610.err.exp create mode 100644 tests/lang/parse-fail-undef-var-2.err.exp create mode 100644 tests/lang/parse-fail-undef-var.err.exp create mode 100644 tests/lang/parse-fail-utf8.err.exp create mode 100644 tests/lang/parse-okay-1.exp create mode 100644 tests/lang/parse-okay-crlf.exp create mode 100644 tests/lang/parse-okay-dup-attrs-5.exp create mode 100644 tests/lang/parse-okay-dup-attrs-6.exp create mode 100644 tests/lang/parse-okay-mixed-nested-attrs-1.exp create mode 100644 tests/lang/parse-okay-mixed-nested-attrs-2.exp create mode 100644 tests/lang/parse-okay-mixed-nested-attrs-3.exp create mode 100644 tests/lang/parse-okay-regression-20041027.exp create mode 100644 tests/lang/parse-okay-regression-751.exp create mode 100644 tests/lang/parse-okay-subversion.exp create mode 100644 tests/lang/parse-okay-url.exp diff --git a/.gitignore b/.gitignore index 969194650..93a9ff9ae 100644 --- a/.gitignore +++ b/.gitignore @@ -95,6 +95,7 @@ perl/Makefile.config # /tests/lang/ /tests/lang/*.out /tests/lang/*.out.xml +/tests/lang/*.err /tests/lang/*.ast /perl/lib/Nix/Config.pm diff --git a/doc/manual/src/contributing/testing.md b/doc/manual/src/contributing/testing.md index e5f80a928..a5253997d 100644 --- a/doc/manual/src/contributing/testing.md +++ b/doc/manual/src/contributing/testing.md @@ -86,6 +86,31 @@ GNU gdb (GDB) 12.1 One can debug the Nix invocation in all the usual ways. For example, enter `run` to start the Nix invocation. +### Characterization testing + +Occasionally, Nix utilizes a technique called [Characterization Testing](https://en.wikipedia.org/wiki/Characterization_test) as part of the functional tests. +This technique is to include the exact output/behavior of a former version of Nix in a test in order to check that Nix continues to produce the same behavior going forward. + +For example, this technique is used for the language tests, to check both the printed final value if evaluation was successful, and any errors and warnings encountered. + +It is frequently useful to regenerate the expected output. +To do that, rerun the failed test with `_NIX_TEST_ACCEPT=1`. +(At least, this is the convention we've used for `tests/lang.sh`. +If we add more characterization testing we should always strive to be consistent.) + +An interesting situation to document is the case when these tests are "overfitted". +The language tests are, again, an example of this. +The expected successful output of evaluation is supposed to be highly stable – we do not intend to make breaking changes to (the stable parts of) the Nix language. +However, the errors and warnings during evaluation (successful or not) are not stable in this way. +We are free to change how they are displayed at any time. + +It may be surprising that we would test non-normative behavior like diagnostic outputs. +Diagnostic outputs are indeed not a stable interface, but they still are important to users. +By recording the expected output, the test suite guards against accidental changes, and ensure the *result* (not just the code that implements it) of the diagnostic code paths are under code review. +Regressions are caught, and improvements always show up in code review. + +To ensure that characterization testing doesn't make it harder to intentionally change these interfaces, there always must be an easy way to regenerate the expected output, as we do with `_NIX_TEST_ACCEPT=1`. + ## Integration tests The integration tests are defined in the Nix flake under the `hydraJobs.tests` attribute. diff --git a/tests/dependencies.sh b/tests/dependencies.sh index f9da0c6bc..d5cd30396 100644 --- a/tests/dependencies.sh +++ b/tests/dependencies.sh @@ -15,6 +15,9 @@ if test -n "$dot"; then $dot < $TEST_ROOT/graph fi +# Test GraphML graph generation +nix-store -q --graphml "$drvPath" > $TEST_ROOT/graphml + outPath=$(nix-store -rvv "$drvPath") || fail "build failed" # Test Graphviz graph generation. diff --git a/tests/lang-test-infra.sh b/tests/lang-test-infra.sh new file mode 100644 index 000000000..30da8977b --- /dev/null +++ b/tests/lang-test-infra.sh @@ -0,0 +1,86 @@ +# Test the function for lang.sh +source common.sh + +source lang/framework.sh + +# We are testing this, so don't want outside world to affect us. +unset _NIX_TEST_ACCEPT + +# We'll only modify this in subshells so we don't need to reset it. +badDiff=0 + +# matches non-empty +echo Hi! > "$TEST_ROOT/got" +cp "$TEST_ROOT/got" "$TEST_ROOT/expected" +( + diffAndAcceptInner test "$TEST_ROOT/got" "$TEST_ROOT/expected" + (( "$badDiff" == 0 )) +) + +# matches empty, non-existant file is the same as empty file +echo -n > "$TEST_ROOT/got" +( + diffAndAcceptInner test "$TEST_ROOT/got" "$TEST_ROOT/does-not-exist" + (( "$badDiff" == 0 )) +) + +# doesn't matches non-empty, non-existant file is the same as empty file +echo Hi! > "$TEST_ROOT/got" +( + diffAndAcceptInner test "$TEST_ROOT/got" "$TEST_ROOT/does-not-exist" + (( "$badDiff" == 1 )) +) + +# doesn't match, `badDiff` set, file unchanged +echo Hi! > "$TEST_ROOT/got" +echo Bye! > "$TEST_ROOT/expected" +( + diffAndAcceptInner test "$TEST_ROOT/got" "$TEST_ROOT/expected" + (( "$badDiff" == 1 )) +) +[[ "$(echo Bye! )" == $(< "$TEST_ROOT/expected") ]] + +# _NIX_TEST_ACCEPT=1 matches non-empty +echo Hi! > "$TEST_ROOT/got" +cp "$TEST_ROOT/got" "$TEST_ROOT/expected" +( + _NIX_TEST_ACCEPT=1 diffAndAcceptInner test "$TEST_ROOT/got" "$TEST_ROOT/expected" + (( "$badDiff" == 0 )) +) + +# _NIX_TEST_ACCEPT doesn't match, `badDiff=1` set, file changed (was previously non-empty) +echo Hi! > "$TEST_ROOT/got" +echo Bye! > "$TEST_ROOT/expected" +( + _NIX_TEST_ACCEPT=1 diffAndAcceptInner test "$TEST_ROOT/got" "$TEST_ROOT/expected" + (( "$badDiff" == 1 )) +) +[[ "$(echo Hi! )" == $(< "$TEST_ROOT/expected") ]] +# second time succeeds +( + diffAndAcceptInner test "$TEST_ROOT/got" "$TEST_ROOT/expected" + (( "$badDiff" == 0 )) +) + +# _NIX_TEST_ACCEPT matches empty, non-existant file not created +echo -n > "$TEST_ROOT/got" +( + _NIX_TEST_ACCEPT=1 diffAndAcceptInner test "$TEST_ROOT/got" "$TEST_ROOT/does-not-exists" + (( "$badDiff" == 0 )) +) +[[ ! -f "$TEST_ROOT/does-not-exist" ]] + +# _NIX_TEST_ACCEPT doesn't match, output empty, file deleted +echo -n > "$TEST_ROOT/got" +echo Bye! > "$TEST_ROOT/expected" +badDiff=0 +( + _NIX_TEST_ACCEPT=1 diffAndAcceptInner test "$TEST_ROOT/got" "$TEST_ROOT/expected" + (( "$badDiff" == 1 )) +) +[[ ! -f "$TEST_ROOT/expected" ]] +# second time succeeds +( + diffAndAcceptInner test "$TEST_ROOT/got" "$TEST_ROOT/expected" + (( "$badDiff" == 0 )) +) diff --git a/tests/lang.sh b/tests/lang.sh old mode 100644 new mode 100755 index 8170cb39d..bfb942d3d --- a/tests/lang.sh +++ b/tests/lang.sh @@ -1,5 +1,17 @@ source common.sh +set -o pipefail + +source lang/framework.sh + +# specialize function a bit +function diffAndAccept() { + local -r testName="$1" + local -r got="lang/$testName.$2" + local -r expected="lang/$testName.$3" + diffAndAcceptInner "$testName" "$got" "$expected" +} + export TEST_VAR=foo # for eval-okay-getenv.nix export NIX_REMOTE=dummy:// export NIX_STORE_DIR=/nix/store @@ -20,63 +32,114 @@ nix-instantiate --eval -E 'let x = { repeating = x; tracing = builtins.trace x t set +x -fail=0 +badDiff=0 +badExitCode=0 for i in lang/parse-fail-*.nix; do echo "parsing $i (should fail)"; - i=$(basename $i .nix) - if ! expect 1 nix-instantiate --parse - < lang/$i.nix; then + i=$(basename "$i" .nix) + if expectStderr 1 nix-instantiate --parse - < "lang/$i.nix" > "lang/$i.err" + then + diffAndAccept "$i" err err.exp + else echo "FAIL: $i shouldn't parse" - fail=1 + badExitCode=1 fi done for i in lang/parse-okay-*.nix; do echo "parsing $i (should succeed)"; - i=$(basename $i .nix) - if ! expect 0 nix-instantiate --parse - < lang/$i.nix > lang/$i.out; then + i=$(basename "$i" .nix) + if + expect 0 nix-instantiate --parse - < "lang/$i.nix" \ + 1> >(sed "s!$(pwd)!/pwd!g" > "lang/$i.out") \ + 2> >(sed "s!$(pwd)!/pwd!g" > "lang/$i.err") + then + diffAndAccept "$i" out exp + diffAndAccept "$i" err err.exp + else echo "FAIL: $i should parse" - fail=1 + badExitCode=1 fi done for i in lang/eval-fail-*.nix; do echo "evaluating $i (should fail)"; - i=$(basename $i .nix) - if ! expect 1 nix-instantiate --eval lang/$i.nix; then + i=$(basename "$i" .nix) + if + expectStderr 1 nix-instantiate --show-trace "lang/$i.nix" \ + | sed "s!$(pwd)!/pwd!g" > "lang/$i.err" + then + diffAndAccept "$i" err err.exp + else echo "FAIL: $i shouldn't evaluate" - fail=1 + badExitCode=1 fi done for i in lang/eval-okay-*.nix; do echo "evaluating $i (should succeed)"; - i=$(basename $i .nix) + i=$(basename "$i" .nix) - if test -e lang/$i.exp; then - flags= - if test -e lang/$i.flags; then - flags=$(cat lang/$i.flags) - fi - if ! expect 0 env NIX_PATH=lang/dir3:lang/dir4 HOME=/fake-home nix-instantiate $flags --eval --strict lang/$i.nix > lang/$i.out; then + if test -e "lang/$i.exp.xml"; then + if expect 0 nix-instantiate --eval --xml --no-location --strict \ + "lang/$i.nix" > "lang/$i.out.xml" + then + diffAndAccept "$i" out.xml exp.xml + else echo "FAIL: $i should evaluate" - fail=1 - elif ! diff <(< lang/$i.out sed -e "s|$(pwd)|/pwd|g") lang/$i.exp; then - echo "FAIL: evaluation result of $i not as expected" - fail=1 + badExitCode=1 + fi + elif test ! -e "lang/$i.exp-disabled"; then + declare -a flags=() + if test -e "lang/$i.flags"; then + read -r -a flags < "lang/$i.flags" fi - fi - if test -e lang/$i.exp.xml; then - if ! expect 0 nix-instantiate --eval --xml --no-location --strict \ - lang/$i.nix > lang/$i.out.xml; then + if + expect 0 env \ + NIX_PATH=lang/dir3:lang/dir4 \ + HOME=/fake-home \ + nix-instantiate "${flags[@]}" --eval --strict "lang/$i.nix" \ + 1> "lang/$i.out" \ + 2> "lang/$i.err" + then + sed -i "s!$(pwd)!/pwd!g" "lang/$i.out" "lang/$i.err" + diffAndAccept "$i" out exp + diffAndAccept "$i" err err.exp + else echo "FAIL: $i should evaluate" - fail=1 - elif ! cmp -s lang/$i.out.xml lang/$i.exp.xml; then - echo "FAIL: XML evaluation result of $i not as expected" - fail=1 + badExitCode=1 fi fi done -exit $fail +if test -n "${_NIX_TEST_ACCEPT-}"; then + if (( "$badDiff" )); then + echo 'Output did mot match, but accepted output as the persisted expected output.' + echo 'That means the next time the tests are run, they should pass.' + else + echo 'NOTE: Environment variable _NIX_TEST_ACCEPT is defined,' + echo 'indicating the unexpected output should be accepted as the expected output going forward,' + echo 'but no tests had unexpected output so there was no expected output to update.' + fi + if (( "$badExitCode" )); then + exit "$badExitCode" + else + skipTest "regenerating golden masters" + fi +else + if (( "$badDiff" )); then + echo '' + echo 'You can rerun this test with:' + echo '' + echo ' _NIX_TEST_ACCEPT=1 make tests/lang.sh.test' + echo '' + echo 'to regenerate the files containing the expected output,' + echo 'and then view the git diff to decide whether a change is' + echo 'good/intentional or bad/unintentional.' + echo 'If the diff contains arbitrary or impure information,' + echo 'please improve the normalization that the test applies to the output.' + fi + exit $(( "$badExitCode" + "$badDiff" )) +fi diff --git a/tests/lang/empty.exp b/tests/lang/empty.exp new file mode 100644 index 000000000..e69de29bb diff --git a/tests/lang/eval-fail-abort.err.exp b/tests/lang/eval-fail-abort.err.exp new file mode 100644 index 000000000..345232d3f --- /dev/null +++ b/tests/lang/eval-fail-abort.err.exp @@ -0,0 +1,10 @@ +error: + … while calling the 'abort' builtin + + at /pwd/lang/eval-fail-abort.nix:1:14: + + 1| if true then abort "this should fail" else 1 + | ^ + 2| + + error: evaluation aborted with the following error message: 'this should fail' diff --git a/tests/lang/eval-fail-antiquoted-path.err.exp b/tests/lang/eval-fail-antiquoted-path.err.exp new file mode 100644 index 000000000..425deba42 --- /dev/null +++ b/tests/lang/eval-fail-antiquoted-path.err.exp @@ -0,0 +1 @@ +error: getting attributes of path ‘PWD/lang/fnord’: No such file or directory diff --git a/tests/lang/eval-fail-assert.err.exp b/tests/lang/eval-fail-assert.err.exp new file mode 100644 index 000000000..aeecd8167 --- /dev/null +++ b/tests/lang/eval-fail-assert.err.exp @@ -0,0 +1,36 @@ +error: + … while evaluating the attribute 'body' + + at /pwd/lang/eval-fail-assert.nix:4:3: + + 3| + 4| body = x "x"; + | ^ + 5| } + + … from call site + + at /pwd/lang/eval-fail-assert.nix:4:10: + + 3| + 4| body = x "x"; + | ^ + 5| } + + … while calling 'x' + + at /pwd/lang/eval-fail-assert.nix:2:7: + + 1| let { + 2| x = arg: assert arg == "y"; 123; + | ^ + 3| + + error: assertion '(arg == "y")' failed + + at /pwd/lang/eval-fail-assert.nix:2:12: + + 1| let { + 2| x = arg: assert arg == "y"; 123; + | ^ + 3| diff --git a/tests/lang/eval-fail-bad-antiquote-1.err.exp b/tests/lang/eval-fail-bad-antiquote-1.err.exp new file mode 100644 index 000000000..cf94f53bc --- /dev/null +++ b/tests/lang/eval-fail-bad-antiquote-1.err.exp @@ -0,0 +1,10 @@ +error: + … while evaluating a path segment + + at /pwd/lang/eval-fail-bad-antiquote-1.nix:1:2: + + 1| "${x: x}" + | ^ + 2| + + error: cannot coerce a function to a string diff --git a/tests/lang/eval-fail-bad-antiquote-2.err.exp b/tests/lang/eval-fail-bad-antiquote-2.err.exp new file mode 100644 index 000000000..c8fe39d12 --- /dev/null +++ b/tests/lang/eval-fail-bad-antiquote-2.err.exp @@ -0,0 +1 @@ +error: operation 'addToStoreFromDump' is not supported by store 'dummy' diff --git a/tests/lang/eval-fail-bad-antiquote-3.err.exp b/tests/lang/eval-fail-bad-antiquote-3.err.exp new file mode 100644 index 000000000..fbefbc826 --- /dev/null +++ b/tests/lang/eval-fail-bad-antiquote-3.err.exp @@ -0,0 +1,10 @@ +error: + … while evaluating a path segment + + at /pwd/lang/eval-fail-bad-antiquote-3.nix:1:3: + + 1| ''${x: x}'' + | ^ + 2| + + error: cannot coerce a function to a string diff --git a/tests/lang/eval-fail-bad-string-interpolation-1.err.exp b/tests/lang/eval-fail-bad-string-interpolation-1.err.exp new file mode 100644 index 000000000..eb73e9a52 --- /dev/null +++ b/tests/lang/eval-fail-bad-string-interpolation-1.err.exp @@ -0,0 +1,10 @@ +error: + … while evaluating a path segment + + at /pwd/lang/eval-fail-bad-string-interpolation-1.nix:1:2: + + 1| "${x: x}" + | ^ + 2| + + error: cannot coerce a function to a string diff --git a/tests/lang/eval-fail-bad-string-interpolation-2.err.exp b/tests/lang/eval-fail-bad-string-interpolation-2.err.exp new file mode 100644 index 000000000..c8fe39d12 --- /dev/null +++ b/tests/lang/eval-fail-bad-string-interpolation-2.err.exp @@ -0,0 +1 @@ +error: operation 'addToStoreFromDump' is not supported by store 'dummy' diff --git a/tests/lang/eval-fail-bad-string-interpolation-3.err.exp b/tests/lang/eval-fail-bad-string-interpolation-3.err.exp new file mode 100644 index 000000000..ac14f329b --- /dev/null +++ b/tests/lang/eval-fail-bad-string-interpolation-3.err.exp @@ -0,0 +1,10 @@ +error: + … while evaluating a path segment + + at /pwd/lang/eval-fail-bad-string-interpolation-3.nix:1:3: + + 1| ''${x: x}'' + | ^ + 2| + + error: cannot coerce a function to a string diff --git a/tests/lang/eval-fail-blackhole.err.exp b/tests/lang/eval-fail-blackhole.err.exp new file mode 100644 index 000000000..f0618d8ac --- /dev/null +++ b/tests/lang/eval-fail-blackhole.err.exp @@ -0,0 +1,18 @@ +error: + … while evaluating the attribute 'body' + + at /pwd/lang/eval-fail-blackhole.nix:2:3: + + 1| let { + 2| body = x; + | ^ + 3| x = y; + + error: infinite recursion encountered + + at /pwd/lang/eval-fail-blackhole.nix:3:7: + + 2| body = x; + 3| x = y; + | ^ + 4| y = x; diff --git a/tests/lang/eval-fail-deepseq.err.exp b/tests/lang/eval-fail-deepseq.err.exp new file mode 100644 index 000000000..5e204ba73 --- /dev/null +++ b/tests/lang/eval-fail-deepseq.err.exp @@ -0,0 +1,26 @@ +error: + … while calling the 'deepSeq' builtin + + at /pwd/lang/eval-fail-deepseq.nix:1:1: + + 1| builtins.deepSeq { x = abort "foo"; } 456 + | ^ + 2| + + … while evaluating the attribute 'x' + + at /pwd/lang/eval-fail-deepseq.nix:1:20: + + 1| builtins.deepSeq { x = abort "foo"; } 456 + | ^ + 2| + + … while calling the 'abort' builtin + + at /pwd/lang/eval-fail-deepseq.nix:1:24: + + 1| builtins.deepSeq { x = abort "foo"; } 456 + | ^ + 2| + + error: evaluation aborted with the following error message: 'foo' diff --git a/tests/lang/eval-fail-foldlStrict-strict-op-application.err.exp b/tests/lang/eval-fail-foldlStrict-strict-op-application.err.exp new file mode 100644 index 000000000..0069285fb --- /dev/null +++ b/tests/lang/eval-fail-foldlStrict-strict-op-application.err.exp @@ -0,0 +1,38 @@ +error: + … while calling the 'foldl'' builtin + + at /pwd/lang/eval-fail-foldlStrict-strict-op-application.nix:2:1: + + 1| # Tests that the result of applying op is forced even if the value is never used + 2| builtins.foldl' + | ^ + 3| (_: f: f null) + + … while calling anonymous lambda + + at /pwd/lang/eval-fail-foldlStrict-strict-op-application.nix:3:7: + + 2| builtins.foldl' + 3| (_: f: f null) + | ^ + 4| null + + … from call site + + at /pwd/lang/eval-fail-foldlStrict-strict-op-application.nix:3:10: + + 2| builtins.foldl' + 3| (_: f: f null) + | ^ + 4| null + + … while calling anonymous lambda + + at /pwd/lang/eval-fail-foldlStrict-strict-op-application.nix:5:6: + + 4| null + 5| [ (_: throw "Not the final value, but is still forced!") (_: 23) ] + | ^ + 6| + + error: Not the final value, but is still forced! diff --git a/tests/lang/eval-fail-fromTOML-timestamps.err.exp b/tests/lang/eval-fail-fromTOML-timestamps.err.exp new file mode 100644 index 000000000..f6bd19f5a --- /dev/null +++ b/tests/lang/eval-fail-fromTOML-timestamps.err.exp @@ -0,0 +1,12 @@ +error: + … while calling the 'fromTOML' builtin + + at /pwd/lang/eval-fail-fromTOML-timestamps.nix:1:1: + + 1| builtins.fromTOML '' + | ^ + 2| key = "value" + + error: while parsing a TOML string: Dates and times are not supported + + at «none»:0: (source not available) diff --git a/tests/lang/eval-fail-hashfile-missing.err.exp b/tests/lang/eval-fail-hashfile-missing.err.exp new file mode 100644 index 000000000..8e77dec1e --- /dev/null +++ b/tests/lang/eval-fail-hashfile-missing.err.exp @@ -0,0 +1,19 @@ +error: + … while calling the 'toString' builtin + + at /pwd/lang/eval-fail-hashfile-missing.nix:4:3: + + 3| in + 4| toString (builtins.concatLists (map (hash: map (builtins.hashFile hash) paths) ["md5" "sha1" "sha256" "sha512"])) + | ^ + 5| + + … while evaluating the first argument passed to builtins.toString + + at «none»:0: (source not available) + + … while calling the 'hashFile' builtin + + at «none»:0: (source not available) + + error: opening file '/pwd/lang/this-file-is-definitely-not-there-7392097': No such file or directory diff --git a/tests/lang/eval-fail-list.err.exp b/tests/lang/eval-fail-list.err.exp new file mode 100644 index 000000000..24d682118 --- /dev/null +++ b/tests/lang/eval-fail-list.err.exp @@ -0,0 +1,10 @@ +error: + … while evaluating one of the elements to concatenate + + at /pwd/lang/eval-fail-list.nix:1:2: + + 1| 8++1 + | ^ + 2| + + error: value is an integer while a list was expected diff --git a/tests/lang/eval-fail-list.nix b/tests/lang/eval-fail-list.nix new file mode 100644 index 000000000..fa749f2f7 --- /dev/null +++ b/tests/lang/eval-fail-list.nix @@ -0,0 +1 @@ +8++1 diff --git a/tests/lang/eval-fail-missing-arg.err.exp b/tests/lang/eval-fail-missing-arg.err.exp new file mode 100644 index 000000000..61fabf0d5 --- /dev/null +++ b/tests/lang/eval-fail-missing-arg.err.exp @@ -0,0 +1,16 @@ +error: + … from call site + + at /pwd/lang/eval-fail-missing-arg.nix:1:1: + + 1| ({x, y, z}: x + y + z) {x = "foo"; z = "bar";} + | ^ + 2| + + error: function 'anonymous lambda' called without required argument 'y' + + at /pwd/lang/eval-fail-missing-arg.nix:1:2: + + 1| ({x, y, z}: x + y + z) {x = "foo"; z = "bar";} + | ^ + 2| diff --git a/tests/lang/eval-fail-nonexist-path.err.exp b/tests/lang/eval-fail-nonexist-path.err.exp new file mode 100644 index 000000000..c8fe39d12 --- /dev/null +++ b/tests/lang/eval-fail-nonexist-path.err.exp @@ -0,0 +1 @@ +error: operation 'addToStoreFromDump' is not supported by store 'dummy' diff --git a/tests/lang/eval-fail-path-slash.err.exp b/tests/lang/eval-fail-path-slash.err.exp new file mode 100644 index 000000000..f0011c97f --- /dev/null +++ b/tests/lang/eval-fail-path-slash.err.exp @@ -0,0 +1,8 @@ +error: path has a trailing slash + + at /pwd/lang/eval-fail-path-slash.nix:6:12: + + 5| # and https://nixos.org/nix-dev/2016-June/020829.html + 6| /nix/store/ + | ^ + 7| diff --git a/tests/lang/eval-fail-recursion.err.exp b/tests/lang/eval-fail-recursion.err.exp new file mode 100644 index 000000000..af64133cb --- /dev/null +++ b/tests/lang/eval-fail-recursion.err.exp @@ -0,0 +1,16 @@ +error: + … in the right operand of the update (//) operator + + at /pwd/lang/eval-fail-recursion.nix:1:12: + + 1| let a = {} // a; in a.foo + | ^ + 2| + + error: infinite recursion encountered + + at /pwd/lang/eval-fail-recursion.nix:1:15: + + 1| let a = {} // a; in a.foo + | ^ + 2| diff --git a/tests/lang/eval-fail-recursion.nix b/tests/lang/eval-fail-recursion.nix new file mode 100644 index 000000000..075b5ed06 --- /dev/null +++ b/tests/lang/eval-fail-recursion.nix @@ -0,0 +1 @@ +let a = {} // a; in a.foo diff --git a/tests/lang/eval-fail-remove.err.exp b/tests/lang/eval-fail-remove.err.exp new file mode 100644 index 000000000..e82cdac98 --- /dev/null +++ b/tests/lang/eval-fail-remove.err.exp @@ -0,0 +1,19 @@ +error: + … while evaluating the attribute 'body' + + at /pwd/lang/eval-fail-remove.nix:4:3: + + 3| + 4| body = (removeAttrs attrs ["x"]).x; + | ^ + 5| } + + error: attribute 'x' missing + + at /pwd/lang/eval-fail-remove.nix:4:10: + + 3| + 4| body = (removeAttrs attrs ["x"]).x; + | ^ + 5| } + Did you mean y? diff --git a/tests/lang/eval-fail-scope-5.err.exp b/tests/lang/eval-fail-scope-5.err.exp new file mode 100644 index 000000000..22b6166f8 --- /dev/null +++ b/tests/lang/eval-fail-scope-5.err.exp @@ -0,0 +1,36 @@ +error: + … while evaluating the attribute 'body' + + at /pwd/lang/eval-fail-scope-5.nix:8:3: + + 7| + 8| body = f {}; + | ^ + 9| + + … from call site + + at /pwd/lang/eval-fail-scope-5.nix:8:10: + + 7| + 8| body = f {}; + | ^ + 9| + + … while calling 'f' + + at /pwd/lang/eval-fail-scope-5.nix:6:7: + + 5| + 6| f = {x ? y, y ? x}: x + y; + | ^ + 7| + + error: infinite recursion encountered + + at /pwd/lang/eval-fail-scope-5.nix:6:12: + + 5| + 6| f = {x ? y, y ? x}: x + y; + | ^ + 7| diff --git a/tests/lang/eval-fail-seq.err.exp b/tests/lang/eval-fail-seq.err.exp new file mode 100644 index 000000000..33a7e9491 --- /dev/null +++ b/tests/lang/eval-fail-seq.err.exp @@ -0,0 +1,18 @@ +error: + … while calling the 'seq' builtin + + at /pwd/lang/eval-fail-seq.nix:1:1: + + 1| builtins.seq (abort "foo") 2 + | ^ + 2| + + … while calling the 'abort' builtin + + at /pwd/lang/eval-fail-seq.nix:1:15: + + 1| builtins.seq (abort "foo") 2 + | ^ + 2| + + error: evaluation aborted with the following error message: 'foo' diff --git a/tests/lang/eval-fail-set-override.err.exp b/tests/lang/eval-fail-set-override.err.exp new file mode 100644 index 000000000..beb29d678 --- /dev/null +++ b/tests/lang/eval-fail-set-override.err.exp @@ -0,0 +1,6 @@ +error: + … while evaluating the `__overrides` attribute + + at «none»:0: (source not available) + + error: value is an integer while a set was expected diff --git a/tests/lang/eval-fail-set-override.nix b/tests/lang/eval-fail-set-override.nix new file mode 100644 index 000000000..03551c186 --- /dev/null +++ b/tests/lang/eval-fail-set-override.nix @@ -0,0 +1 @@ +rec { __overrides = 1; } diff --git a/tests/lang/eval-fail-set.err.exp b/tests/lang/eval-fail-set.err.exp new file mode 100644 index 000000000..0d0140508 --- /dev/null +++ b/tests/lang/eval-fail-set.err.exp @@ -0,0 +1,7 @@ +error: undefined variable 'x' + + at /pwd/lang/eval-fail-set.nix:1:3: + + 1| 8.x + | ^ + 2| diff --git a/tests/lang/eval-fail-set.nix b/tests/lang/eval-fail-set.nix new file mode 100644 index 000000000..c6b7980b6 --- /dev/null +++ b/tests/lang/eval-fail-set.nix @@ -0,0 +1 @@ +8.x diff --git a/tests/lang/eval-fail-substring.err.exp b/tests/lang/eval-fail-substring.err.exp new file mode 100644 index 000000000..dc26a00bd --- /dev/null +++ b/tests/lang/eval-fail-substring.err.exp @@ -0,0 +1,12 @@ +error: + … while calling the 'substring' builtin + + at /pwd/lang/eval-fail-substring.nix:1:1: + + 1| builtins.substring (builtins.sub 0 1) 1 "x" + | ^ + 2| + + error: negative start position in 'substring' + + at «none»:0: (source not available) diff --git a/tests/lang/eval-fail-to-path.err.exp b/tests/lang/eval-fail-to-path.err.exp new file mode 100644 index 000000000..43ed2bdfc --- /dev/null +++ b/tests/lang/eval-fail-to-path.err.exp @@ -0,0 +1,14 @@ +error: + … while calling the 'toPath' builtin + + at /pwd/lang/eval-fail-to-path.nix:1:1: + + 1| builtins.toPath "foo/bar" + | ^ + 2| + + … while evaluating the first argument passed to builtins.toPath + + at «none»:0: (source not available) + + error: string 'foo/bar' doesn't represent an absolute path diff --git a/tests/lang/eval-fail-undeclared-arg.err.exp b/tests/lang/eval-fail-undeclared-arg.err.exp new file mode 100644 index 000000000..30db743c7 --- /dev/null +++ b/tests/lang/eval-fail-undeclared-arg.err.exp @@ -0,0 +1,17 @@ +error: + … from call site + + at /pwd/lang/eval-fail-undeclared-arg.nix:1:1: + + 1| ({x, z}: x + z) {x = "foo"; y = "bla"; z = "bar";} + | ^ + 2| + + error: function 'anonymous lambda' called with unexpected argument 'y' + + at /pwd/lang/eval-fail-undeclared-arg.nix:1:2: + + 1| ({x, z}: x + z) {x = "foo"; y = "bla"; z = "bar";} + | ^ + 2| + Did you mean one of x or z? diff --git a/tests/lang/eval-okay-fromjson.nix b/tests/lang/eval-okay-fromjson.nix index e1c0f86cc..4c526b9ae 100644 --- a/tests/lang/eval-okay-fromjson.nix +++ b/tests/lang/eval-okay-fromjson.nix @@ -11,9 +11,12 @@ builtins.fromJSON "Width": 200, "Height": 250 }, + "Animated" : false, + "IDs": [116, 943, 234, 38793, true ,false,null, -100], + "Escapes": "\"\\\/\t\n\r\t", "Subtitle" : false, - "Latitude": 46.2051, - "Longitude": 6.0723 + "Latitude": 37.7668, + "Longitude": -122.3959 } } '' @@ -28,8 +31,11 @@ builtins.fromJSON Width = 200; Height = 250; }; + Animated = false; + IDs = [ 116 943 234 38793 true false null (0-100) ]; + Escapes = "\"\\\/\t\n\r\t"; # supported in JSON but not Nix: \b\f Subtitle = false; - Latitude = 46.2051; - Longitude = 6.0723; + Latitude = 37.7668; + Longitude = -122.3959; }; } diff --git a/tests/lang/eval-okay-overrides.nix b/tests/lang/eval-okay-overrides.nix index 358742b36..719bdc9c0 100644 --- a/tests/lang/eval-okay-overrides.nix +++ b/tests/lang/eval-okay-overrides.nix @@ -1,6 +1,6 @@ let - overrides = { a = 2; }; + overrides = { a = 2; b = 3; }; in (rec { __overrides = overrides; diff --git a/tests/lang/eval-okay-print.err.exp b/tests/lang/eval-okay-print.err.exp new file mode 100644 index 000000000..3fc99be3e --- /dev/null +++ b/tests/lang/eval-okay-print.err.exp @@ -0,0 +1 @@ +trace: [ ] diff --git a/tests/lang/eval-okay-print.exp b/tests/lang/eval-okay-print.exp new file mode 100644 index 000000000..0d960fb70 --- /dev/null +++ b/tests/lang/eval-okay-print.exp @@ -0,0 +1 @@ +[ null [ [ «repeated» ] ] ] diff --git a/tests/lang/eval-okay-print.nix b/tests/lang/eval-okay-print.nix new file mode 100644 index 000000000..d36ba4da3 --- /dev/null +++ b/tests/lang/eval-okay-print.nix @@ -0,0 +1 @@ +with builtins; trace [(1+1)] [ null toString (deepSeq "x") (a: a) (let x=[x]; in x) ] diff --git a/tests/lang/eval-okay-search-path.flags b/tests/lang/eval-okay-search-path.flags index a28e68210..dfad1c611 100644 --- a/tests/lang/eval-okay-search-path.flags +++ b/tests/lang/eval-okay-search-path.flags @@ -1 +1 @@ --I lang/dir1 -I lang/dir2 -I dir5=lang/dir3 \ No newline at end of file +-I lang/dir1 -I lang/dir2 -I dir5=lang/dir3 diff --git a/tests/lang/framework.sh b/tests/lang/framework.sh new file mode 100644 index 000000000..d6499e0e9 --- /dev/null +++ b/tests/lang/framework.sh @@ -0,0 +1,33 @@ +# Golden test support +# +# Test that the output of the given test matches what is expected. If +# `_NIX_TEST_ACCEPT` is non-empty also update the expected output so +# that next time the test succeeds. +function diffAndAcceptInner() { + local -r testName=$1 + local -r got="$2" + local -r expected="$3" + + # Absence of expected file indicates empty output expected. + if test -e "$expected"; then + local -r expectedOrEmpty="$expected" + else + local -r expectedOrEmpty=lang/empty.exp + fi + + # Diff so we get a nice message + if ! diff "$got" "$expectedOrEmpty"; then + echo "FAIL: evaluation result of $testName not as expected" + badDiff=1 + fi + + # Update expected if `_NIX_TEST_ACCEPT` is non-empty. + if test -n "${_NIX_TEST_ACCEPT-}"; then + cp "$got" "$expected" + # Delete empty expected files to avoid bloating the repo with + # empty files. + if ! test -s "$expected"; then + rm "$expected" + fi + fi +} diff --git a/tests/lang/parse-fail-dup-attrs-1.err.exp b/tests/lang/parse-fail-dup-attrs-1.err.exp new file mode 100644 index 000000000..4fe6b7a1f --- /dev/null +++ b/tests/lang/parse-fail-dup-attrs-1.err.exp @@ -0,0 +1,7 @@ +error: attribute 'x' already defined at «stdin»:1:3 + + at «stdin»:3:3: + + 2| y = 456; + 3| x = 789; + | ^ diff --git a/tests/lang/parse-fail-dup-attrs-2.err.exp b/tests/lang/parse-fail-dup-attrs-2.err.exp new file mode 100644 index 000000000..3aba2891f --- /dev/null +++ b/tests/lang/parse-fail-dup-attrs-2.err.exp @@ -0,0 +1,7 @@ +error: attribute 'x' already defined at «stdin»:9:5 + + at «stdin»:10:17: + + 9| x = 789; + 10| inherit (as) x; + | ^ diff --git a/tests/lang/parse-fail-dup-attrs-3.err.exp b/tests/lang/parse-fail-dup-attrs-3.err.exp new file mode 100644 index 000000000..3aba2891f --- /dev/null +++ b/tests/lang/parse-fail-dup-attrs-3.err.exp @@ -0,0 +1,7 @@ +error: attribute 'x' already defined at «stdin»:9:5 + + at «stdin»:10:17: + + 9| x = 789; + 10| inherit (as) x; + | ^ diff --git a/tests/lang/parse-fail-dup-attrs-4.err.exp b/tests/lang/parse-fail-dup-attrs-4.err.exp new file mode 100644 index 000000000..ff68446a1 --- /dev/null +++ b/tests/lang/parse-fail-dup-attrs-4.err.exp @@ -0,0 +1,7 @@ +error: attribute 'services.ssh.port' already defined at «stdin»:2:3 + + at «stdin»:3:3: + + 2| services.ssh.port = 22; + 3| services.ssh.port = 23; + | ^ diff --git a/tests/lang/parse-fail-dup-attrs-6.err.exp b/tests/lang/parse-fail-dup-attrs-6.err.exp new file mode 100644 index 000000000..74823fc25 --- /dev/null +++ b/tests/lang/parse-fail-dup-attrs-6.err.exp @@ -0,0 +1 @@ +error: attribute ‘services.ssh’ at (string):3:3 already defined at (string):2:3 diff --git a/tests/lang/parse-fail-dup-attrs-7.err.exp b/tests/lang/parse-fail-dup-attrs-7.err.exp new file mode 100644 index 000000000..512a499ca --- /dev/null +++ b/tests/lang/parse-fail-dup-attrs-7.err.exp @@ -0,0 +1,7 @@ +error: attribute 'x' already defined at «stdin»:6:12 + + at «stdin»:7:12: + + 6| inherit x; + 7| inherit x; + | ^ diff --git a/tests/lang/parse-fail-dup-formals.err.exp b/tests/lang/parse-fail-dup-formals.err.exp new file mode 100644 index 000000000..1d566fb33 --- /dev/null +++ b/tests/lang/parse-fail-dup-formals.err.exp @@ -0,0 +1,6 @@ +error: duplicate formal function argument 'x' + + at «stdin»:1:8: + + 1| {x, y, x}: x + | ^ diff --git a/tests/lang/parse-fail-eof-in-string.err.exp b/tests/lang/parse-fail-eof-in-string.err.exp new file mode 100644 index 000000000..f9fa72312 --- /dev/null +++ b/tests/lang/parse-fail-eof-in-string.err.exp @@ -0,0 +1,7 @@ +error: syntax error, unexpected end of file, expecting '"' + + at «stdin»:3:5: + + 2| # Note that this file must not end with a newline. + 3| a 1"$ + | ^ diff --git a/tests/lang/parse-fail-mixed-nested-attrs1.err.exp b/tests/lang/parse-fail-mixed-nested-attrs1.err.exp new file mode 100644 index 000000000..32f776795 --- /dev/null +++ b/tests/lang/parse-fail-mixed-nested-attrs1.err.exp @@ -0,0 +1,8 @@ +error: attribute 'z' already defined at «stdin»:3:16 + + at «stdin»:2:3: + + 1| { + 2| x.z = 3; + | ^ + 3| x = { y = 3; z = 3; }; diff --git a/tests/lang/parse-fail-mixed-nested-attrs2.err.exp b/tests/lang/parse-fail-mixed-nested-attrs2.err.exp new file mode 100644 index 000000000..0437cd50c --- /dev/null +++ b/tests/lang/parse-fail-mixed-nested-attrs2.err.exp @@ -0,0 +1,8 @@ +error: attribute 'y' already defined at «stdin»:3:9 + + at «stdin»:2:3: + + 1| { + 2| x.y.y = 3; + | ^ + 3| x = { y.y= 3; z = 3; }; diff --git a/tests/lang/parse-fail-patterns-1.err.exp b/tests/lang/parse-fail-patterns-1.err.exp new file mode 100644 index 000000000..634a04aaa --- /dev/null +++ b/tests/lang/parse-fail-patterns-1.err.exp @@ -0,0 +1,7 @@ +error: duplicate formal function argument 'args' + + at «stdin»:1:1: + + 1| args@{args, x, y, z}: x + | ^ + 2| diff --git a/tests/lang/parse-fail-regression-20060610.err.exp b/tests/lang/parse-fail-regression-20060610.err.exp new file mode 100644 index 000000000..167d01e85 --- /dev/null +++ b/tests/lang/parse-fail-regression-20060610.err.exp @@ -0,0 +1,8 @@ +error: undefined variable 'gcc' + + at «stdin»:8:12: + + 7| + 8| body = ({ + | ^ + 9| inherit gcc; diff --git a/tests/lang/parse-fail-undef-var-2.err.exp b/tests/lang/parse-fail-undef-var-2.err.exp new file mode 100644 index 000000000..77c96bbd2 --- /dev/null +++ b/tests/lang/parse-fail-undef-var-2.err.exp @@ -0,0 +1,7 @@ +error: syntax error, unexpected ':', expecting '}' + + at «stdin»:3:13: + + 2| + 3| f = {x, y : + | ^ diff --git a/tests/lang/parse-fail-undef-var.err.exp b/tests/lang/parse-fail-undef-var.err.exp new file mode 100644 index 000000000..48e88747f --- /dev/null +++ b/tests/lang/parse-fail-undef-var.err.exp @@ -0,0 +1,7 @@ +error: undefined variable 'y' + + at «stdin»:1:4: + + 1| x: y + | ^ + 2| diff --git a/tests/lang/parse-fail-utf8.err.exp b/tests/lang/parse-fail-utf8.err.exp new file mode 100644 index 000000000..6087479a3 --- /dev/null +++ b/tests/lang/parse-fail-utf8.err.exp @@ -0,0 +1,6 @@ +error: syntax error, unexpected invalid token, expecting end of file + + at «stdin»:1:5: + + 1| 123 à + | ^ diff --git a/tests/lang/parse-okay-1.exp b/tests/lang/parse-okay-1.exp new file mode 100644 index 000000000..d5ab5f18a --- /dev/null +++ b/tests/lang/parse-okay-1.exp @@ -0,0 +1 @@ +({ x, y, z }: ((x + y) + z)) diff --git a/tests/lang/parse-okay-crlf.exp b/tests/lang/parse-okay-crlf.exp new file mode 100644 index 000000000..4213609fc --- /dev/null +++ b/tests/lang/parse-okay-crlf.exp @@ -0,0 +1 @@ +rec { foo = "multi\nline\n string\n test\r"; x = y; y = 123; z = 456; } diff --git a/tests/lang/parse-okay-dup-attrs-5.exp b/tests/lang/parse-okay-dup-attrs-5.exp new file mode 100644 index 000000000..88b0b036f --- /dev/null +++ b/tests/lang/parse-okay-dup-attrs-5.exp @@ -0,0 +1 @@ +{ services = { ssh = { enable = true; port = 23; }; }; } diff --git a/tests/lang/parse-okay-dup-attrs-6.exp b/tests/lang/parse-okay-dup-attrs-6.exp new file mode 100644 index 000000000..88b0b036f --- /dev/null +++ b/tests/lang/parse-okay-dup-attrs-6.exp @@ -0,0 +1 @@ +{ services = { ssh = { enable = true; port = 23; }; }; } diff --git a/tests/lang/parse-okay-mixed-nested-attrs-1.exp b/tests/lang/parse-okay-mixed-nested-attrs-1.exp new file mode 100644 index 000000000..89c66f760 --- /dev/null +++ b/tests/lang/parse-okay-mixed-nested-attrs-1.exp @@ -0,0 +1 @@ +{ x = { q = 3; y = 3; z = 3; }; } diff --git a/tests/lang/parse-okay-mixed-nested-attrs-2.exp b/tests/lang/parse-okay-mixed-nested-attrs-2.exp new file mode 100644 index 000000000..89c66f760 --- /dev/null +++ b/tests/lang/parse-okay-mixed-nested-attrs-2.exp @@ -0,0 +1 @@ +{ x = { q = 3; y = 3; z = 3; }; } diff --git a/tests/lang/parse-okay-mixed-nested-attrs-3.exp b/tests/lang/parse-okay-mixed-nested-attrs-3.exp new file mode 100644 index 000000000..b89a59734 --- /dev/null +++ b/tests/lang/parse-okay-mixed-nested-attrs-3.exp @@ -0,0 +1 @@ +{ services = { httpd = { enable = true; }; ssh = { enable = true; port = 123; }; }; } diff --git a/tests/lang/parse-okay-regression-20041027.exp b/tests/lang/parse-okay-regression-20041027.exp new file mode 100644 index 000000000..9df7219e4 --- /dev/null +++ b/tests/lang/parse-okay-regression-20041027.exp @@ -0,0 +1 @@ +({ fetchurl, stdenv }: ((stdenv).mkDerivation { name = "libXi-6.0.1"; src = (fetchurl { md5 = "7e935a42428d63a387b3c048be0f2756"; url = "http://freedesktop.org/~xlibs/release/libXi-6.0.1.tar.bz2"; }); })) diff --git a/tests/lang/parse-okay-regression-751.exp b/tests/lang/parse-okay-regression-751.exp new file mode 100644 index 000000000..e2ed886fe --- /dev/null +++ b/tests/lang/parse-okay-regression-751.exp @@ -0,0 +1 @@ +(let const = (a: "const"); in ((const { x = "q"; }))) diff --git a/tests/lang/parse-okay-subversion.exp b/tests/lang/parse-okay-subversion.exp new file mode 100644 index 000000000..4168ee8bf --- /dev/null +++ b/tests/lang/parse-okay-subversion.exp @@ -0,0 +1 @@ +({ fetchurl, localServer ? false, httpServer ? false, sslSupport ? false, pythonBindings ? false, javaSwigBindings ? false, javahlBindings ? false, stdenv, openssl ? null, httpd ? null, db4 ? null, expat, swig ? null, j2sdk ? null }: assert (expat != null); assert (localServer -> (db4 != null)); assert (httpServer -> ((httpd != null) && ((httpd).expat == expat))); assert (sslSupport -> ((openssl != null) && (httpServer -> ((httpd).openssl == openssl)))); assert (pythonBindings -> ((swig != null) && (swig).pythonSupport)); assert (javaSwigBindings -> ((swig != null) && (swig).javaSupport)); assert (javahlBindings -> (j2sdk != null)); ((stdenv).mkDerivation { builder = /foo/bar; db4 = (if localServer then db4 else null); inherit expat ; inherit httpServer ; httpd = (if httpServer then httpd else null); j2sdk = (if javaSwigBindings then (swig).j2sdk else (if javahlBindings then j2sdk else null)); inherit javaSwigBindings ; inherit javahlBindings ; inherit localServer ; name = "subversion-1.1.1"; openssl = (if sslSupport then openssl else null); patches = (if javahlBindings then [ (/javahl.patch) ] else [ ]); python = (if pythonBindings then (swig).python else null); inherit pythonBindings ; src = (fetchurl { md5 = "a180c3fe91680389c210c99def54d9e0"; url = "http://subversion.tigris.org/tarballs/subversion-1.1.1.tar.bz2"; }); inherit sslSupport ; swig = (if (pythonBindings || javaSwigBindings) then swig else null); })) diff --git a/tests/lang/parse-okay-url.exp b/tests/lang/parse-okay-url.exp new file mode 100644 index 000000000..e5f0829b0 --- /dev/null +++ b/tests/lang/parse-okay-url.exp @@ -0,0 +1 @@ +[ ("x:x") ("https://svn.cs.uu.nl:12443/repos/trace/trunk") ("http://www2.mplayerhq.hu/MPlayer/releases/fonts/font-arial-iso-8859-1.tar.bz2") ("http://losser.st-lab.cs.uu.nl/~armijn/.nix/gcc-3.3.4-static-nix.tar.gz") ("http://fpdownload.macromedia.com/get/shockwave/flash/english/linux/7.0r25/install_flash_player_7_linux.tar.gz") ("https://ftp5.gwdg.de/pub/linux/archlinux/extra/os/x86_64/unzip-6.0-14-x86_64.pkg.tar.zst") ("ftp://ftp.gtk.org/pub/gtk/v1.2/gtk+-1.2.10.tar.gz") ] diff --git a/tests/local.mk b/tests/local.mk index 88848926b..bb80c5317 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -20,6 +20,7 @@ nix_tests = \ remote-store.sh \ legacy-ssh-store.sh \ lang.sh \ + lang-test-infra.sh \ experimental-features.sh \ fetchMercurial.sh \ gc-auto.sh \ diff --git a/tests/misc.sh b/tests/misc.sh index 60d58310e..af96d20bd 100644 --- a/tests/misc.sh +++ b/tests/misc.sh @@ -24,3 +24,9 @@ eval_stdin_res=$(echo 'let a = {} // a; in a.foo' | nix-instantiate --eval -E - echo $eval_stdin_res | grep "at «stdin»:1:15:" echo $eval_stdin_res | grep "infinite recursion encountered" +# Attribute path errors +expectStderr 1 nix-instantiate --eval -E '{}' -A '"x' | grepQuiet "missing closing quote in selection path" +expectStderr 1 nix-instantiate --eval -E '[]' -A 'x' | grepQuiet "should be a set" +expectStderr 1 nix-instantiate --eval -E '{}' -A '1' | grepQuiet "should be a list" +expectStderr 1 nix-instantiate --eval -E '{}' -A '.' | grepQuiet "empty attribute name" +expectStderr 1 nix-instantiate --eval -E '[]' -A '1' | grepQuiet "out of range" diff --git a/tests/restricted.sh b/tests/restricted.sh index 776893a56..17f310a4b 100644 --- a/tests/restricted.sh +++ b/tests/restricted.sh @@ -49,3 +49,5 @@ output="$(nix eval --raw --restrict-eval -I "$traverseDir" \ 2>&1 || :)" echo "$output" | grep "is forbidden" echo "$output" | grepInverse -F restricted-secret + +expectStderr 1 nix-instantiate --restrict-eval true ./dependencies.nix | grepQuiet "forbidden in restricted mode" From 0309f6b5b8ca354e1a43d320928d3d546ce7f878 Mon Sep 17 00:00:00 2001 From: Ben Radford <104896700+benradf@users.noreply.github.com> Date: Wed, 12 Jul 2023 12:32:57 +0100 Subject: [PATCH 32/38] Update src/libstore/globals.hh Co-authored-by: Valentin Gagarin --- src/libstore/globals.hh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 81fa154bb..d8fda9707 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -530,8 +530,8 @@ public: Nix will attempt to drop supplementary groups when building with sandboxing. However this can fail under some circumstances. - For example, if the user lacks the CAP_SETGID capability. - Search setgroups(2) for EPERM to find more detailed information on this. + For example, if the user lacks the `CAP_SETGID` capability. + Search `setgroups(2)` for `EPERM` to find more detailed information on this. If you encounter such a failure, setting this option to `false` will let you ignore it and continue. But before doing so, you should consider the security implications carefully. From a2acd23466108a1a10ed3b6b874c558bd95e7645 Mon Sep 17 00:00:00 2001 From: Ben Radford <104896700+benradf@users.noreply.github.com> Date: Wed, 12 Jul 2023 12:33:05 +0100 Subject: [PATCH 33/38] Update src/libstore/globals.hh Co-authored-by: Valentin Gagarin --- src/libstore/globals.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index d8fda9707..d4b8fb1f9 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -538,7 +538,7 @@ public: Not dropping supplementary groups means the build sandbox will be less restricted than intended. This option defaults to `true` when the user is root - (since root usually has permissions to call setgroups) + (since `root` usually has permissions to call setgroups) and `false` otherwise. )"}; From e072e18475bc461e522f34d63cc74fc5fbf1640e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 13 Jul 2023 08:03:42 -0400 Subject: [PATCH 34/38] Fix race condition in the language tests When we pipe to `>(...)` like that, we unfortunately don't wait for the process to finish. Better to just substitute the file. Also, use the "unified" diff output that people (including myself) are more familiar with, thanks to Git. --- tests/lang.sh | 5 +++-- tests/lang/framework.sh | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/lang.sh b/tests/lang.sh index bfb942d3d..75dbbc38e 100755 --- a/tests/lang.sh +++ b/tests/lang.sh @@ -52,9 +52,10 @@ for i in lang/parse-okay-*.nix; do i=$(basename "$i" .nix) if expect 0 nix-instantiate --parse - < "lang/$i.nix" \ - 1> >(sed "s!$(pwd)!/pwd!g" > "lang/$i.out") \ - 2> >(sed "s!$(pwd)!/pwd!g" > "lang/$i.err") + 1> "lang/$i.out" \ + 2> "lang/$i.err" then + sed "s!$(pwd)!/pwd!g" "lang/$i.out" "lang/$i.err" diffAndAccept "$i" out exp diffAndAccept "$i" err err.exp else diff --git a/tests/lang/framework.sh b/tests/lang/framework.sh index d6499e0e9..516bff8ad 100644 --- a/tests/lang/framework.sh +++ b/tests/lang/framework.sh @@ -16,7 +16,7 @@ function diffAndAcceptInner() { fi # Diff so we get a nice message - if ! diff "$got" "$expectedOrEmpty"; then + if ! diff --unified "$got" "$expectedOrEmpty"; then echo "FAIL: evaluation result of $testName not as expected" badDiff=1 fi From 9e64f243406ff2b3fbe06adbc762cf965e6825a8 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 13 Jul 2023 15:06:34 -0400 Subject: [PATCH 35/38] Revert "Check _NIX_TEST_NO_SANDBOX when setting _canUseSandbox." This reverts commit c1d39de1fbceebbb6b46abc4a21fb8e34423df99. --- tests/common/vars-and-functions.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/common/vars-and-functions.sh.in b/tests/common/vars-and-functions.sh.in index ad0623871..dc7ce13cc 100644 --- a/tests/common/vars-and-functions.sh.in +++ b/tests/common/vars-and-functions.sh.in @@ -141,7 +141,7 @@ restartDaemon() { startDaemon } -if [[ -z "${_NIX_TEST_NO_SANDBOX:-}" ]] && [[ $(uname) == Linux ]] && [[ -L /proc/self/ns/user ]] && unshare --user true; then +if [[ $(uname) == Linux ]] && [[ -L /proc/self/ns/user ]] && unshare --user true; then _canUseSandbox=1 fi From 84c4e6f0acfc902955d580aa090aa52f20ee82ad Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 13 Jul 2023 15:06:50 -0400 Subject: [PATCH 36/38] Revert "Skip build-remote-trustless unless sandbox is supported." This reverts commit 41412dc4ae0fec2d08335c19724276d99e0c6056. --- tests/build-remote-trustless-should-fail-0.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/build-remote-trustless-should-fail-0.sh b/tests/build-remote-trustless-should-fail-0.sh index b14101f83..fad1def59 100644 --- a/tests/build-remote-trustless-should-fail-0.sh +++ b/tests/build-remote-trustless-should-fail-0.sh @@ -2,7 +2,6 @@ source common.sh enableFeatures "daemon-trust-override" -requireSandboxSupport restartDaemon [[ $busybox =~ busybox ]] || skipTest "no busybox" From 1a13757880479921966adeca839cf182b3ffcbd0 Mon Sep 17 00:00:00 2001 From: cidkidnix Date: Thu, 13 Jul 2023 14:17:22 -0500 Subject: [PATCH 37/38] Add comment regarding the unset of NIX_STORE_DIR in build-remote.sh and supplementary-groups.sh --- tests/build-remote.sh | 1 + tests/supplementary-groups.sh | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/tests/build-remote.sh b/tests/build-remote.sh index 78e12b477..d2a2132c1 100644 --- a/tests/build-remote.sh +++ b/tests/build-remote.sh @@ -1,6 +1,7 @@ requireSandboxSupport [[ $busybox =~ busybox ]] || skipTest "no busybox" +# Avoid store dir being inside sandbox build-dir unset NIX_STORE_DIR unset NIX_STATE_DIR diff --git a/tests/supplementary-groups.sh b/tests/supplementary-groups.sh index 47c6ef605..474ba7503 100644 --- a/tests/supplementary-groups.sh +++ b/tests/supplementary-groups.sh @@ -5,6 +5,10 @@ requireSandboxSupport if ! command -p -v unshare; then skipTest "Need unshare"; fi needLocalStore "The test uses --store always so we would just be bypassing the daemon" +# Avoid store dir being inside sandbox build-dir +unset NIX_STORE_DIR +unset NIX_STATE_DIR + unshare --mount --map-root-user bash < Date: Thu, 13 Jul 2023 14:23:24 -0500 Subject: [PATCH 38/38] move unset NIX_STORE_DIR in supplementary-groups.sh to inside the unshare --- tests/supplementary-groups.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/supplementary-groups.sh b/tests/supplementary-groups.sh index 474ba7503..d18fb2414 100644 --- a/tests/supplementary-groups.sh +++ b/tests/supplementary-groups.sh @@ -5,13 +5,13 @@ requireSandboxSupport if ! command -p -v unshare; then skipTest "Need unshare"; fi needLocalStore "The test uses --store always so we would just be bypassing the daemon" -# Avoid store dir being inside sandbox build-dir -unset NIX_STORE_DIR -unset NIX_STATE_DIR - unshare --mount --map-root-user bash <