PosixSourceAccessor: always require a root #12494
Conversation
With the introduction of CanonPath, we don't have roots on paths we want to access. For example, /nix/store/abc doesn't say whether we need to use C: or D: on Windows. This commit keeps getFSSourceAccessor() which does NOT have a root specified, so it pulls one from the current working directory. This is silly, since nix.exe could be on Z: but the Nix store could be on C: which means we'll build wrong paths. More commits will have to come which removes getFSSourceAccessor() altogether. This shouldn't change anything on Unix systems, since the root should always resolve to / for every possible path.
github-actions
bot
added
the
store
|
The future goal is to use CanonPath in a store-relative way. Eventually the source accessor root will become the full "C:\nix\store" instead of just "C:" but I think making it mandatory at all is a good first step. |
| @@ -1383,7 +1383,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) | |||
| checkInterrupt(); | |||
| auto name = link.path().filename(); | |||
| printMsg(lvlTalkative, "checking contents of '%s'", name); | |||
| PosixSourceAccessor accessor; | |||
| PosixSourceAccessor accessor(std::filesystem::current_path().root_path()); | |||
There was a problem hiding this comment.
Please wrap all std::filesystem calls that can exceptions that can throw exceptions in a try-catch so that we can throw a nix error instead. Otherwise it breaks our error messages as the error bubbles up without any error context getting added.
There was a problem hiding this comment.
Here it looks like only the current_path call can throw an exception, as defined by implementation. In practice Windows, Linux and macOS seem to always provide a value.
| @@ -139,6 +139,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, | |||
| return; | |||
| } | |||
|
|
|||
| auto root = std::filesystem::path { storeDir }.root_path(); | |||
| @@ -120,6 +120,12 @@ std::string StoreDirConfig::printStorePath(const StorePath & path) const | |||
| return (storeDir + "/").append(path.to_string()); | |||
| } | |||
|
|
|||
| CanonPath StoreDirConfig::canonStorePath(const StorePath & path) const | |||
| { | |||
| auto relativeStoreDir = std::filesystem::path(storeDir).relative_path(); | |||
| // HACK: We need a root, so grab one, even if it has a good chance of being incorrect on Windows. | ||
| // For example, nix.exe could be on Z: but the store could actually be in C: | ||
| // TODO: Remove getFSSourceAccessor() and only use makeFSSourceAccessor(root) | ||
| auto root = std::filesystem::current_path().root_path(); |
|
Are we reaching the point where it is easier to wrap the functions than |
I think it would be easier to have our own wrapper, yes. But we would need to do it per std::filesystem function so we can generate proper error message that contains the arguments of the failed function. The exception from std::filesystem are not very readable unfortunately. |
I'm actually surprised that the filesystem API does not have exceptions everywhere. In this PR, the only call which I think can realistically throw an exception is the It might make more sense to enforce this requirement as an invariant via a wrapper to the constructor. |
|
@puffnfresh sorry a little confused
Did you mean to say?
|
Maybe that's more accurate. Methods like |
With the introduction of CanonPath, we don't have roots on paths we want to access. For example, /nix/store/abc doesn't say whether we need to use C: or D: on Windows.
This commit keeps getFSSourceAccessor() which does NOT have a root specified, so it pulls one from the current working directory. This is silly, since nix.exe could be on Z: but the Nix store could be on C: which means we'll build wrong paths. More commits will have to come which removes getFSSourceAccessor() altogether.
This shouldn't change anything on Unix systems, since the root should always resolve to / for every possible path.
This change makes a lot of path related things work on Windows. For example, things which previously would fail with something like "/nix/store/abc is not in the Nix store" now actually does the right thing, by resolving properly to "C:\nix\store\abc".
Motivation
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.