From 3dfb4bf1cc8d06f45b5a0a7796fbbde1700ba831 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Fri, 29 Mar 2024 20:17:40 +0100 Subject: [PATCH] eval cache: guard behind an eval setting whether or not to cache errors When I run `nix build` on a broken attribute in a flake twice, I get the dreaded error: cached failure of attribute 'snens' This also happens when you re-run the previous command with `--show-trace`. While trying to fix that, Linus and I discovered that the eval cache was broken for use on local flakes (i.e. `nix build .#`) was broken which got fixed in the commit before. We decided to use an eval setting for that because `forceError` seems to have been added somewhat arbitrarily as argument to the functions of AttrCursor where this was needed at the time it was developed. The effect was that it was hard for us to understand when an invocation can force errors and when it doesn't. For instance, `AttrCursor::isDerivation()` never forces re-evaluation of errors, so if `maybeGetAttr("type")` is the first thing that breaks eval, you'd always run into a cached failure. For now, this is `true` by default because I think the current behavior with a properly working eval cache is pretty painful while hacking on (or with) a flake. Potential future improvements are * Setting `evalSettings.forceErrorsInEvalCache` to `false` for commands like `nix search`. * Allowing the eval cache to cache the full failure (right now, `failed_t` is empty). That way, you could get errors when re-running `nix build` without having to re-evaluate everything, perhaps even with traced. That's the solution Linus and I would prefer. Co-authored-by: Linus Heckemann --- src/libexpr/eval-cache.cc | 17 +++++++++-------- src/libexpr/eval-cache.hh | 6 +++--- src/libexpr/eval-settings.hh | 3 +++ tests/functional/flakes/eval-cache.sh | 8 ++++++-- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 1538eb05679..2cccbb25e5c 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -3,6 +3,7 @@ #include "sqlite.hh" #include "eval.hh" #include "eval-inline.hh" +#include "eval-settings.hh" #include "store-api.hh" namespace nix::eval_cache { @@ -470,7 +471,7 @@ Suggestions AttrCursor::getSuggestionsForAttr(Symbol name) return Suggestions::bestMatches(strAttrNames, root->state.symbols[name]); } -std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErrors) +std::shared_ptr AttrCursor::maybeGetAttr(Symbol name) { if (root->db) { if (!cachedValue) @@ -488,7 +489,7 @@ std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErro if (std::get_if(&attr->second)) return nullptr; else if (std::get_if(&attr->second)) { - if (forceErrors) + if (evalSettings.forceErrorsInEvalCache) debug("reevaluating failed cached attribute '%s'", getAttrPathStr(name)); else throw CachedEvalError(root->state, "cached failure of attribute '%s'", getAttrPathStr(name)); @@ -537,9 +538,9 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name) return maybeGetAttr(root->state.symbols.create(name)); } -ref AttrCursor::getAttr(Symbol name, bool forceErrors) +ref AttrCursor::getAttr(Symbol name) { - auto p = maybeGetAttr(name, forceErrors); + auto p = maybeGetAttr(name); if (!p) throw Error("attribute '%s' does not exist", getAttrPathStr(name)); return ref(p); @@ -550,11 +551,11 @@ ref AttrCursor::getAttr(std::string_view name) return getAttr(root->state.symbols.create(name)); } -OrSuggestions> AttrCursor::findAlongAttrPath(const std::vector & attrPath, bool force) +OrSuggestions> AttrCursor::findAlongAttrPath(const std::vector & attrPath) { auto res = shared_from_this(); for (auto & attr : attrPath) { - auto child = res->maybeGetAttr(attr, force); + auto child = res->maybeGetAttr(attr); if (!child) { auto suggestions = res->getSuggestionsForAttr(attr); return OrSuggestions>::failed(suggestions); @@ -691,7 +692,7 @@ std::vector AttrCursor::getListOfStrings() } } - debug("evaluating uncached attribute '%s'", getAttrPathStr()); + debug("[AttrCursor::getListOfStrings] evaluating uncached attribute '%s'", getAttrPathStr()); auto & v = getValue(); root->state.forceValue(v, noPos); @@ -751,7 +752,7 @@ bool AttrCursor::isDerivation() StorePath AttrCursor::forceDerivation() { - auto aDrvPath = getAttr(root->state.sDrvPath, true); + auto aDrvPath = getAttr(root->state.sDrvPath); auto drvPath = root->state.store->parseStorePath(aDrvPath->getString()); if (!root->state.store->isValidPath(drvPath) && !settings.readOnlyMode) { /* The eval cache contains 'drvPath', but the actual path has diff --git a/src/libexpr/eval-cache.hh b/src/libexpr/eval-cache.hh index 46c4999c84c..efecf8232ec 100644 --- a/src/libexpr/eval-cache.hh +++ b/src/libexpr/eval-cache.hh @@ -102,11 +102,11 @@ public: Suggestions getSuggestionsForAttr(Symbol name); - std::shared_ptr maybeGetAttr(Symbol name, bool forceErrors = false); + std::shared_ptr maybeGetAttr(Symbol name); std::shared_ptr maybeGetAttr(std::string_view name); - ref getAttr(Symbol name, bool forceErrors = false); + ref getAttr(Symbol name); ref getAttr(std::string_view name); @@ -114,7 +114,7 @@ public: * Get an attribute along a chain of attrsets. Note that this does * not auto-call functors or functions. */ - OrSuggestions> findAlongAttrPath(const std::vector & attrPath, bool force = false); + OrSuggestions> findAlongAttrPath(const std::vector & attrPath); std::string getString(); diff --git a/src/libexpr/eval-settings.hh b/src/libexpr/eval-settings.hh index 60d3a6f25dd..d17667f0b67 100644 --- a/src/libexpr/eval-settings.hh +++ b/src/libexpr/eval-settings.hh @@ -128,6 +128,9 @@ struct EvalSettings : Config Setting useEvalCache{this, true, "eval-cache", "Whether to use the flake evaluation cache."}; + Setting forceErrorsInEvalCache{this, true, "force-errors-in-eval-cache", + "Whether to force reevaluation of cached failures."}; + Setting ignoreExceptionsDuringTry{this, false, "ignore-try", R"( If set to true, ignore exceptions inside 'tryEval' calls when evaluating nix expressions in diff --git a/tests/functional/flakes/eval-cache.sh b/tests/functional/flakes/eval-cache.sh index a12cf4267e9..21e6cc3d8a1 100644 --- a/tests/functional/flakes/eval-cache.sh +++ b/tests/functional/flakes/eval-cache.sh @@ -30,11 +30,15 @@ nix flake lock "$flake1Dir" # FIXME `packages.$system.foo` requires three invocations of `nix build`, # for `foo = throw` only it'll never be cached. expect 1 nix build "$flake1Dir#foo.bar" 2>&1 | grepQuiet 'error: breaks' -expect 1 nix build "$flake1Dir#foo.bar" 2>&1 | grepQuiet 'error: cached failure of attribute '"'foo.bar'" +expect 1 nix build "$flake1Dir#foo.bar" --no-force-errors-in-eval-cache 2>&1 | \ + grepQuiet 'error: cached failure of attribute '"'foo.bar'" +expect 1 nix build "$flake1Dir#foo.bar" 2>&1 | grepQuiet 'error: breaks' git -C "$flake1Dir" add flake.lock git -C "$flake1Dir" commit -m "Init" # Cache is invalidated with a new git revision expect 1 nix build "$flake1Dir#foo.bar" 2>&1 | grepQuiet 'error: breaks' -expect 1 nix build "$flake1Dir#foo.bar" 2>&1 | grepQuiet 'error: cached failure of attribute '"'foo.bar'" +expect 1 nix build "$flake1Dir#foo.bar" --no-force-errors-in-eval-cache 2>&1 | \ + grepQuiet 'error: cached failure of attribute '"'foo.bar'" +expect 1 nix build "$flake1Dir#foo.bar" 2>&1 | grepQuiet 'error: breaks'