InstallableFlake::getCursors(): Force re-evaluation in case of cached failure

[edolstra]

Motivation

This fixes the "cached failure of attribute ..." message in case the flake output fails at top-level (i.e. in getCursors()).

Issue #3872.

Test case borrowed from #10368.

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[Ma27]

Can you explain why this was re-implemented here when 3dfb4bf already exists?

As I mentioned before, that part of #10368. So far I didn't get an answer.

Again, to me it seems as if forceErrors was added just where it was needed. This PR doesn't address that. This risks that in another case the , true is forgotten again in the future (and if we have especially bad luck that won't even be caught by this testcase).

Finally, I'd highly appreciate it if you could cc people if you develop alternative patches for discussion.

[edolstra]

@Ma27 To be honest I don't understand the motivation of 3dfb4bf. If cached failures don't cause re-evaluation, that's a bug that we should fix (like this PR does). We shouldn't force the user to work around it with some ad hoc CLI flag (and we already have --no-eval-cache in any case).

[Ma27]

As described in the commit message (3dfb4bf) and my comment right above yours (#10513 (review)), there's an argument forceErrors = false in AttrCursor::maybeGetAttr which is added in a very ad-hoc way to a few methods where it was needed.

As soon as e.g. AttrCursor::maybeGetAttr(std::string_view) or AttrCursor::getAttr(std::string_view) are used, you'll have the same issue again because these set forceErrors to false. That's why I wanted to make sure there's a single way to either force errors or not. The idea is to let commands like nix search set the global setting evalSettings.forceErrorsInEvalCache to false so that errors aren't forced when searching. For now, I'd set the setting to true by default, but what I'd like to see is that the eval cache actually caches errors so that you don't have to re-run (potentially expensive) evaluations just to get a full error message (or a trace).

and we already have --no-eval-cache in any case

This is not the same though: I want caching, but (for now - as described above) not for errors. With that reasoning you wouldn't need any fix for this bug because all you have to do is to bypass the eval cache.

We shouldn't force the user to work around

The default is true, so by default the user doesn't have to set anything.
And as soon as we can set it to false because - as described above - error messages are cached this may still be useful for debugging purposes. Or it can be removed then. I think I'm fine with both.

If cached failures don't cause re-evaluation, that's a bug that we should fix (like this PR does)

If that's the case then why does this code-path exist in the first place?!

[edolstra]

Closing in preference for #10564.