All std::filesystem related errors need to be re-thrown as nix errors

[Mic92]

Describe the bug

Since @siddhantk232 has introduced std::filesystem in many places
we no longer generate nix traces i.e. when a directory does not exit.
We need to catch all fs::filesystem_error related exception and throw a proper nix error instead,
because only nix errors are catched in eval.cc and get nix error context added.

[DavHau]

example log:

> nix build .#devShells.x86_64-linux.default --show-trace
error: filesystem error: directory iterator cannot open directory: No such file or directory [/nix/store/sl3888lrvzwn0n61vs83zap7hcfypyv3-source/nix/machines]

[Mic92]

@DavHau Fixed this particular issue:

$ ./src/nix/nix eval --impure --expr 'let f = builtins.readDir "/nix/store/39z5zpb72qrnxl832nwphcd4ihfhix3j-hello-2.12.1/foobar"; in f' --show-trace
error:
       … while calling the 'readDir' builtin
         at «string»:1:9:
            1| let f = builtins.readDir "/nix/store/39z5zpb72qrnxl832nwphcd4ihfhix3j-hello-2.12.1/foobar"; in f
             |         ^

       error: failed to read directory '/nix/store/39z5zpb72qrnxl832nwphcd4ihfhix3j-hello-2.12.1/foobar': filesystem error: directory iterator cannot open directory: No such file or directory [/nix/store/39z5zpb72qrnxl832nwphcd4ihfhix3j-hello-2.12.1/foobar]

Do you have more examples?

[Mic92]

Fixed some of them here: #11378

[Ericson2314]

@Mic92 Do you think we could replace all SysError with a wrapper around std::filesystem::filesystem_error? SysError vs WindowsErorr isn't so good for windows porting; I think this could be more robust in the end.

[Mic92]

Ah right. SysError is only good when we have errno set.

[Ericson2314]

I am not sure how std::filesystem::filesystem_error works on Windows, since https://en.cppreference.com/w/cpp/error/errc seems very POSIX-y, but it does.

[Ericson2314]

In any event, more than bug fixes, it would be fantastic to somehow have tests for the regressions we find, as we're not done reworking the file system stuff, and it would be easy to introduce more bugs.

[Mic92]

I am not so sure how to achieve this, except for putting all filesystem functions that can throw in its wrapper and change the exception type to match what we expect.
Each of these functions can fail in its own unique way. Sure testing will uncover some of these, but it won't be 100% complete.

[Ericson2314]

Sure testing will uncover some of these, but it won't be 100% complete.

Well there are plenty of misc CLI file accesses that will basically result in a failure either way. The ones we are really care about are in the evaluator, and that strikes me as more achievable.

Following up with #11378 (comment) I think we can make a few observations:

• The evaluator should never be doing its own IO; all IO should go through SourceAccessor implementations. It should just add context to exceptions thrown elsewhere

• SourceAccessor should have a uniform contract in terms of what exceptions are possible to through regardless of implementation --- e.g. "file not found" is a general concept across implementations (in-memory and host OS FS, for two examples) and so we should use the same exception type in both cases. This would be a good spot to wrap filesystem_errors into a Nix error type.

• file-system.hh functions are fine to continue to use filesystem_error, because they are a single implementation/interface (unlike SourceAccessor with multiple implementations). It is the users responsibility to make sure those exceptions are properly handled, which should only require manual intervention in a few cases.

For testing, I think we can write unit tests using the in-memory source accessor. #10915 (just needs an easy rebase) can also help with things like write-file. We can-do tryEval style tests with the customized evaluator to prove that exceptions are wrapped, right? If so, that is good, because that is directly testing language semantics rather than mere error messages.

[Mic92]

What happens if does use import-from-derviation? Does it not than also leak other code into the evaluator that can throw file system exceptions again?

[roberth]

• file-system.hh functions are fine to continue to use filesystem_error, because they are a single implementation/interface (unlike SourceAccessor with multiple implementations). It is the users responsibility to make sure those exceptions are properly handled, which should only require manual intervention in a few cases.

This means that non-evaluator usages of files, such as store operations, profiles, channel manipulation, and the configuration system will all throw overly verbose errors, or they need to come up with their own prettifying exception handlers.

• The evaluator should never be doing its own IO; all IO should go through SourceAccessor

I agree, but don't take this too literally. It shouldn't read nix.conf through a SourceAccessor for example, mostly because configuration is mostly not part of the evaluator.
It is the right ambition though. All IO should happen through the right indirections, and often that's a SourceAccessor.

What happens if does use import-from-derviation?

More files become available, and they'll be accessed through SourceAccessor as usual.
Only the from derivation part of IFD is somewhat of a "feature"; import just works as it always does.

So IFD is not import, and actually not derivation either, because readFile pkg.drvPath requires no realisation and therefore isn't IFD for any useful definition of IFD. "Read from output" would be more accurate, but that's competing with other alternative terms, so we'll keep calling it IFD until there's wide agreement on a new name, probably RFC-level agreement; otherwise it's even more confusing.

[Ericson2314]

This means that non-evaluator usages of files, such as store operations, profiles, channel manipulation, and the configuration system will all throw overly verbose errors, or they need to come up with their own prettifying exception handlers.

I am cautiously optimistic the "root" try-catch in near main could do this.

I agree, but don't take this too literally.

True, but also I think the stakes are a bit lower for that, since it takes place prior to the evaluation proper and thus doesn't affect builtins.tryCatch.

What happens if does use import-from-derviation?

To add one more thing to what @roberth said, we shouldn't need to worry about misc build errors bubbling back into the evaluator. the procedures to build something should already do a big try-catch (or equivalent), messaging everything into a single "build failed" sort of exception, and then the evaluator just needs to deal with that.

[roberth]

@Mic92 do you want to take another stab at this? Otherwise we could merge #11378 as an incremental fix despite the tech debt of having to fix it differently.