From 7c5056878818e67f5b64c903c43d06847acfe315 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 7 Oct 2021 13:15:01 +0200 Subject: [PATCH 1/4] Remove unnecessary call to queryMissing() Worker::run() already does this. --- src/libexpr/primops.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 1b79785ff..77b27799f 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -56,13 +56,9 @@ void EvalState::realiseContext(const PathSet & context) "cannot build '%1%' during evaluation because the option 'allow-import-from-derivation' is disabled", store->printStorePath(drvs.begin()->drvPath)); - /* For performance, prefetch all substitute info. */ - StorePathSet willBuild, willSubstitute, unknown; - uint64_t downloadSize, narSize; + /* Build/substitute the context. */ std::vector buildReqs; for (auto & d : drvs) buildReqs.emplace_back(DerivedPath { d }); - store->queryMissing(buildReqs, willBuild, willSubstitute, unknown, downloadSize, narSize); - store->buildPaths(buildReqs); /* Add the output of this derivations to the allowed From 66c4b20d8bdfb9434329439a7314942b25c7ba50 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 7 Oct 2021 13:34:04 +0200 Subject: [PATCH 2/4] Typo --- src/libstore/build/worker.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index a7a6b92a6..55afb5cca 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -239,7 +239,7 @@ void Worker::run(const Goals & _topGoals) } } - /* Call queryMissing() efficiently query substitutes. */ + /* Call queryMissing() to efficiently query substitutes. */ StorePathSet willBuild, willSubstitute, unknown; uint64_t downloadSize, narSize; store.queryMissing(topPaths, willBuild, willSubstitute, unknown, downloadSize, narSize); From 4806f2f6b0fd2cae401b89fe19d8c528ffd88b5f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 7 Oct 2021 13:43:17 +0200 Subject: [PATCH 3/4] Allow builtins.{path,filterSource} on paths with a context We now build the context (so this has the side-effect of making builtins.{path,filterSource} work on derivations outputs, if IFD is enabled) and then check that the path has no references (which is what we really care about). --- src/libexpr/primops.cc | 62 ++++++++++++++++++++++++++++-------------- tests/flakes.sh | 1 + 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 77b27799f..67d670fb4 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1843,12 +1843,43 @@ static RegisterPrimOp primop_toFile({ .fun = prim_toFile, }); -static void addPath(EvalState & state, const Pos & pos, const string & name, const Path & path_, - Value * filterFun, FileIngestionMethod method, const std::optional expectedHash, Value & v) +static void addPath( + EvalState & state, + const Pos & pos, + const string & name, + const Path & path_, + Value * filterFun, + FileIngestionMethod method, + const std::optional expectedHash, + Value & v, + const PathSet & context) { - const auto path = evalSettings.pureEval && expectedHash ? - path_ : - state.checkSourcePath(path_); + try { + // FIXME: handle CA derivation outputs (where path_ needs to + // be rewritten to the actual output). + state.realiseContext(context); + } catch (InvalidPathError & e) { + throw EvalError({ + .msg = hintfmt("cannot add path '%s', since path '%s' is not valid", path_, e.path), + .errPos = pos + }); + } catch (Error & e) { + e.addTrace(pos, "while adding path '%s'", path_); + throw; + } + + const auto path = evalSettings.pureEval && expectedHash + ? path_ + : state.checkSourcePath(path_); + + if (state.store->isInStore(path)) { + auto storePath = state.store->toStorePath(path).first; + auto info = state.store->queryPathInfo(storePath); + if (!info->references.empty()) + throw EvalError("store path '%s' is not allowed to have references", + state.store->printStorePath(storePath)); + } + PathFilter filter = filterFun ? ([&](const Path & path) { auto st = lstat(path); @@ -1876,6 +1907,7 @@ static void addPath(EvalState & state, const Pos & pos, const string & name, con std::optional expectedStorePath; if (expectedHash) expectedStorePath = state.store->makeFixedOutputPath(method, *expectedHash, name); + Path dstPath; if (!expectedHash || !state.store->isValidPath(*expectedStorePath)) { dstPath = state.store->printStorePath(settings.readOnlyMode @@ -1896,11 +1928,6 @@ static void prim_filterSource(EvalState & state, const Pos & pos, Value * * args { PathSet context; Path path = state.coerceToPath(pos, *args[1], context); - if (!context.empty()) - throw EvalError({ - .msg = hintfmt("string '%1%' cannot refer to other paths", path), - .errPos = pos - }); state.forceValue(*args[0], pos); if (args[0]->type() != nFunction) @@ -1911,7 +1938,7 @@ static void prim_filterSource(EvalState & state, const Pos & pos, Value * * args .errPos = pos }); - addPath(state, pos, std::string(baseNameOf(path)), path, args[0], FileIngestionMethod::Recursive, std::nullopt, v); + addPath(state, pos, std::string(baseNameOf(path)), path, args[0], FileIngestionMethod::Recursive, std::nullopt, v, context); } static RegisterPrimOp primop_filterSource({ @@ -1977,18 +2004,13 @@ static void prim_path(EvalState & state, const Pos & pos, Value * * args, Value Value * filterFun = nullptr; auto method = FileIngestionMethod::Recursive; std::optional expectedHash; + PathSet context; for (auto & attr : *args[0]->attrs) { const string & n(attr.name); - if (n == "path") { - PathSet context; + if (n == "path") path = state.coerceToPath(*attr.pos, *attr.value, context); - if (!context.empty()) - throw EvalError({ - .msg = hintfmt("string '%1%' cannot refer to other paths", path), - .errPos = *attr.pos - }); - } else if (attr.name == state.sName) + else if (attr.name == state.sName) name = state.forceStringNoCtx(*attr.value, *attr.pos); else if (n == "filter") { state.forceValue(*attr.value, pos); @@ -2011,7 +2033,7 @@ static void prim_path(EvalState & state, const Pos & pos, Value * * args, Value if (name.empty()) name = baseNameOf(path); - addPath(state, pos, name, path, filterFun, method, expectedHash, v); + addPath(state, pos, name, path, filterFun, method, expectedHash, v, context); } static RegisterPrimOp primop_path({ diff --git a/tests/flakes.sh b/tests/flakes.sh index f71c52f11..57d1b9aad 100644 --- a/tests/flakes.sh +++ b/tests/flakes.sh @@ -262,6 +262,7 @@ cat > $flake3Dir/flake.nix < \$out ''; From c4dcf3cf2578cb4d1a7105b32e27c5c6dd2ad9ba Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 7 Oct 2021 13:47:15 +0200 Subject: [PATCH 4/4] Add a trace to all errors in addPath() --- src/libexpr/primops.cc | 120 ++++++++++++++++++++--------------------- 1 file changed, 58 insertions(+), 62 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 67d670fb4..94e196d62 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1847,7 +1847,7 @@ static void addPath( EvalState & state, const Pos & pos, const string & name, - const Path & path_, + Path path, Value * filterFun, FileIngestionMethod method, const std::optional expectedHash, @@ -1855,72 +1855,68 @@ static void addPath( const PathSet & context) { try { - // FIXME: handle CA derivation outputs (where path_ needs to + // FIXME: handle CA derivation outputs (where path needs to // be rewritten to the actual output). state.realiseContext(context); - } catch (InvalidPathError & e) { - throw EvalError({ - .msg = hintfmt("cannot add path '%s', since path '%s' is not valid", path_, e.path), - .errPos = pos - }); + + path = evalSettings.pureEval && expectedHash + ? path + : state.checkSourcePath(path); + + if (state.store->isInStore(path)) { + auto storePath = state.store->toStorePath(path).first; + auto info = state.store->queryPathInfo(storePath); + if (!info->references.empty()) + throw EvalError("store path '%s' is not allowed to have references", + state.store->printStorePath(storePath)); + } + + PathFilter filter = filterFun ? ([&](const Path & path) { + auto st = lstat(path); + + /* Call the filter function. The first argument is the path, + the second is a string indicating the type of the file. */ + Value arg1; + mkString(arg1, path); + + Value fun2; + state.callFunction(*filterFun, arg1, fun2, noPos); + + Value arg2; + mkString(arg2, + S_ISREG(st.st_mode) ? "regular" : + S_ISDIR(st.st_mode) ? "directory" : + S_ISLNK(st.st_mode) ? "symlink" : + "unknown" /* not supported, will fail! */); + + Value res; + state.callFunction(fun2, arg2, res, noPos); + + return state.forceBool(res, pos); + }) : defaultPathFilter; + + std::optional expectedStorePath; + if (expectedHash) + expectedStorePath = state.store->makeFixedOutputPath(method, *expectedHash, name); + + Path dstPath; + if (!expectedHash || !state.store->isValidPath(*expectedStorePath)) { + dstPath = state.store->printStorePath(settings.readOnlyMode + ? state.store->computeStorePathForPath(name, path, method, htSHA256, filter).first + : state.store->addToStore(name, path, method, htSHA256, filter, state.repair)); + if (expectedHash && expectedStorePath != state.store->parseStorePath(dstPath)) + throw Error("store path mismatch in (possibly filtered) path added from '%s'", path); + } else + dstPath = state.store->printStorePath(*expectedStorePath); + + mkString(v, dstPath, {dstPath}); + + state.allowPath(v.string.s); + } catch (Error & e) { - e.addTrace(pos, "while adding path '%s'", path_); + e.addTrace(pos, "while adding path '%s'", path); throw; } - - const auto path = evalSettings.pureEval && expectedHash - ? path_ - : state.checkSourcePath(path_); - - if (state.store->isInStore(path)) { - auto storePath = state.store->toStorePath(path).first; - auto info = state.store->queryPathInfo(storePath); - if (!info->references.empty()) - throw EvalError("store path '%s' is not allowed to have references", - state.store->printStorePath(storePath)); - } - - PathFilter filter = filterFun ? ([&](const Path & path) { - auto st = lstat(path); - - /* Call the filter function. The first argument is the path, - the second is a string indicating the type of the file. */ - Value arg1; - mkString(arg1, path); - - Value fun2; - state.callFunction(*filterFun, arg1, fun2, noPos); - - Value arg2; - mkString(arg2, - S_ISREG(st.st_mode) ? "regular" : - S_ISDIR(st.st_mode) ? "directory" : - S_ISLNK(st.st_mode) ? "symlink" : - "unknown" /* not supported, will fail! */); - - Value res; - state.callFunction(fun2, arg2, res, noPos); - - return state.forceBool(res, pos); - }) : defaultPathFilter; - - std::optional expectedStorePath; - if (expectedHash) - expectedStorePath = state.store->makeFixedOutputPath(method, *expectedHash, name); - - Path dstPath; - if (!expectedHash || !state.store->isValidPath(*expectedStorePath)) { - dstPath = state.store->printStorePath(settings.readOnlyMode - ? state.store->computeStorePathForPath(name, path, method, htSHA256, filter).first - : state.store->addToStore(name, path, method, htSHA256, filter, state.repair)); - if (expectedHash && expectedStorePath != state.store->parseStorePath(dstPath)) - throw Error("store path mismatch in (possibly filtered) path added from '%s'", path); - } else - dstPath = state.store->printStorePath(*expectedStorePath); - - mkString(v, dstPath, {dstPath}); - - state.allowPath(v.string.s); }