From 158280d8e9b9d7fbf1ccee9cef829081859810e6 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 27 Feb 2022 15:25:22 +0100 Subject: [PATCH 1/2] fetchTree: Do not re-fetch paths already present --- src/libexpr/primops/fetchTree.cc | 28 ++++++++++++++++++++++++---- tests/fetchurl.sh | 4 ++++ tests/tarball.sh | 4 ++++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index f3e3e70d8..ea416d2d9 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -185,6 +185,13 @@ static void prim_fetchTree(EvalState & state, const Pos & pos, Value * * args, V // FIXME: document static RegisterPrimOp primop_fetchTree("fetchTree", 1, prim_fetchTree); +static void setStorePath(EvalState &state, const StorePath &storePath, Value &v) { + state.allowPath(storePath); + + auto path = state.store->printStorePath(storePath); + v.mkString(path, PathSet({path})); +} + static void fetch(EvalState & state, const Pos & pos, Value * * args, Value & v, const std::string & who, bool unpack, std::string name) { @@ -203,6 +210,8 @@ static void fetch(EvalState & state, const Pos & pos, Value * * args, Value & v, url = state.forceStringNoCtx(*attr.value, *attr.pos); else if (n == "sha256") expectedHash = newHashAllowEmpty(state.forceStringNoCtx(*attr.value, *attr.pos), htSHA256); + else if (n == "narHash") + expectedHash = newHashAllowEmpty(state.forceStringNoCtx(*attr.value, *attr.pos), htSHA256); else if (n == "name") name = state.forceStringNoCtx(*attr.value, *attr.pos); else @@ -230,6 +239,20 @@ static void fetch(EvalState & state, const Pos & pos, Value * * args, Value & v, if (evalSettings.pureEval && !expectedHash) throw Error("in pure evaluation mode, '%s' requires a 'sha256' argument", who); + // early exit if pinned and already in the store + if (expectedHash && expectedHash->type == htSHA256) { + auto expectedPath = + unpack + ? state.store->makeFixedOutputPath(FileIngestionMethod::Recursive, *expectedHash, name, {}) + : state.store->makeFixedOutputPath(FileIngestionMethod::Flat, *expectedHash, name, {}); + + auto validPaths = state.store->queryValidPaths({expectedPath}, NoSubstitute); + if (!validPaths.empty()) { + setStorePath(state, expectedPath, v); + return; + } + } + auto storePath = unpack ? fetchers::downloadTarball(state.store, *url, name, (bool) expectedHash).first.storePath @@ -244,10 +267,7 @@ static void fetch(EvalState & state, const Pos & pos, Value * * args, Value & v, *url, expectedHash->to_string(Base32, true), hash.to_string(Base32, true)); } - state.allowPath(storePath); - - auto path = state.store->printStorePath(storePath); - v.mkString(path, PathSet({path})); + setStorePath(state, storePath, v); } static void prim_fetchurl(EvalState & state, const Pos & pos, Value * * args, Value & v) diff --git a/tests/fetchurl.sh b/tests/fetchurl.sh index 3d1685f43..b41d8c4b7 100644 --- a/tests/fetchurl.sh +++ b/tests/fetchurl.sh @@ -9,6 +9,10 @@ outPath=$(nix-build -vvvvv --expr 'import ' --argstr url file: cmp $outPath fetchurl.sh +# Do not re-fetch paths already present. +outPath2=$(nix-build -vvvvv --expr 'import ' --argstr url file:///does-not-exist/must-remain-unused/fetchurl.sh --argstr sha256 $hash --no-out-link) +test "$outPath" == "$outPath2" + # Now using a base-64 hash. clearStore diff --git a/tests/tarball.sh b/tests/tarball.sh index 1301922a5..d5cab879c 100644 --- a/tests/tarball.sh +++ b/tests/tarball.sh @@ -26,10 +26,14 @@ test_tarball() { nix-build -o $TEST_ROOT/result '' -I foo=file://$tarball nix-build -o $TEST_ROOT/result -E "import (fetchTarball file://$tarball)" + # Do not re-fetch paths already present + nix-build -o $TEST_ROOT/result -E "import (fetchTarball { url = file:///does-not-exist/must-remain-unused/$tarball; sha256 = \"$hash\"; })" nix-build -o $TEST_ROOT/result -E "import (fetchTree file://$tarball)" nix-build -o $TEST_ROOT/result -E "import (fetchTree { type = \"tarball\"; url = file://$tarball; })" nix-build -o $TEST_ROOT/result -E "import (fetchTree { type = \"tarball\"; url = file://$tarball; narHash = \"$hash\"; })" + # Do not re-fetch paths already present + nix-build -o $TEST_ROOT/result -E "import (fetchTree { type = \"tarball\"; url = file:///does-not-exist/must-remain-unused/$tarball; narHash = \"$hash\"; })" nix-build -o $TEST_ROOT/result -E "import (fetchTree { type = \"tarball\"; url = file://$tarball; narHash = \"sha256-xdKv2pq/IiwLSnBBJXW8hNowI4MrdZfW+SYqDQs7Tzc=\"; })" 2>&1 | grep 'NAR hash mismatch in input' nix-instantiate --strict --eval -E "!((import (fetchTree { type = \"tarball\"; url = file://$tarball; narHash = \"$hash\"; })) ? submodules)" >&2 From ee019d0afce63f5145a043b025a37dfc39444a3d Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 27 Feb 2022 15:59:34 +0100 Subject: [PATCH 2/2] Add EvalState::allowAndSetStorePathString helper This switches addPath from `printStorePath` to `toRealPath`. --- src/libexpr/eval.cc | 8 ++++++++ src/libexpr/eval.hh | 3 +++ src/libexpr/primops.cc | 15 +++++---------- src/libexpr/primops/fetchTree.cc | 11 ++--------- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 60f0bf08c..d94afbb99 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -517,6 +517,14 @@ void EvalState::allowPath(const StorePath & storePath) allowedPaths->insert(store->toRealPath(storePath)); } +void EvalState::allowAndSetStorePathString(const StorePath &storePath, Value &v) +{ + allowPath(storePath); + + auto path = store->printStorePath(storePath); + v.mkString(path, PathSet({path})); +} + Path EvalState::checkSourcePath(const Path & path_) { if (!allowedPaths) return path_; diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 1f0e97b2e..9324961f7 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -161,6 +161,9 @@ public: the real store path if `store` is a chroot store. */ void allowPath(const StorePath & storePath); + /* Allow access to a store path and return it as a string. */ + void allowAndSetStorePathString(const StorePath & storePath, Value &v); + /* Check whether access to a path is allowed and throw an error if not. Otherwise return the canonicalised path. */ Path checkSourcePath(const Path & path); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index aa22c3b61..3124025aa 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1919,20 +1919,15 @@ static void addPath( if (expectedHash) expectedStorePath = state.store->makeFixedOutputPath(method, *expectedHash, name); - Path dstPath; if (!expectedHash || !state.store->isValidPath(*expectedStorePath)) { - dstPath = state.store->printStorePath(settings.readOnlyMode + StorePath dstPath = settings.readOnlyMode ? state.store->computeStorePathForPath(name, path, method, htSHA256, filter).first - : state.store->addToStore(name, path, method, htSHA256, filter, state.repair, refs)); - if (expectedHash && expectedStorePath != state.store->parseStorePath(dstPath)) + : state.store->addToStore(name, path, method, htSHA256, filter, state.repair, refs); + if (expectedHash && expectedStorePath != dstPath) throw Error("store path mismatch in (possibly filtered) path added from '%s'", path); + state.allowAndSetStorePathString(dstPath, v); } else - dstPath = state.store->printStorePath(*expectedStorePath); - - v.mkString(dstPath, {dstPath}); - - state.allowPath(dstPath); - + state.allowAndSetStorePathString(*expectedStorePath, v); } catch (Error & e) { e.addTrace(pos, "while adding path '%s'", path); throw; diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index ea416d2d9..2eeee7173 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -185,13 +185,6 @@ static void prim_fetchTree(EvalState & state, const Pos & pos, Value * * args, V // FIXME: document static RegisterPrimOp primop_fetchTree("fetchTree", 1, prim_fetchTree); -static void setStorePath(EvalState &state, const StorePath &storePath, Value &v) { - state.allowPath(storePath); - - auto path = state.store->printStorePath(storePath); - v.mkString(path, PathSet({path})); -} - static void fetch(EvalState & state, const Pos & pos, Value * * args, Value & v, const std::string & who, bool unpack, std::string name) { @@ -248,7 +241,7 @@ static void fetch(EvalState & state, const Pos & pos, Value * * args, Value & v, auto validPaths = state.store->queryValidPaths({expectedPath}, NoSubstitute); if (!validPaths.empty()) { - setStorePath(state, expectedPath, v); + state.allowAndSetStorePathString(expectedPath, v); return; } } @@ -267,7 +260,7 @@ static void fetch(EvalState & state, const Pos & pos, Value * * args, Value & v, *url, expectedHash->to_string(Base32, true), hash.to_string(Base32, true)); } - setStorePath(state, storePath, v); + state.allowAndSetStorePathString(storePath, v); } static void prim_fetchurl(EvalState & state, const Pos & pos, Value * * args, Value & v)