Use std::filesystem in canonPath and local store symlink detection
#11871
Conversation
github-actions
bot
added
the
store
| while (!result.has_filename() && result.has_parent_path()) | ||
| { | ||
| // The parent of "D:/" is "D:/" so we need to be careful. | ||
| fs::path parent = result.parent_path(); | ||
| if (parent == result) | ||
| break; | ||
| result = parent; | ||
| } |
There was a problem hiding this comment.
I think a lexically_normal path can have at most one trailing separator, so you probably don't need a while loop here?
There was a problem hiding this comment.
| while (!result.has_filename() && result.has_parent_path()) | |
| { | |
| // The parent of "D:/" is "D:/" so we need to be careful. | |
| fs::path parent = result.parent_path(); | |
| if (parent == result) | |
| break; | |
| result = parent; | |
| } | |
| if (result.has_parent_path()) result = result.parent_path(); |
| // This function used to resolve 1024 symlinks via a custom implementation. | ||
| // The standard filesystem library will behave differently. For example, | ||
| // libstd++ in GCC will only resolve 40 symlinks. | ||
| // I hope that isn't a problem! |
There was a problem hiding this comment.
| // This function used to resolve 1024 symlinks via a custom implementation. | |
| // The standard filesystem library will behave differently. For example, | |
| // libstd++ in GCC will only resolve 40 symlinks. | |
| // I hope that isn't a problem! | |
| // This function used to resolve 1024 symlinks via a custom implementation. | |
| // The standard filesystem library will behave differently. For example, | |
| // libstd++ in GCC will only resolve 40 symlinks. | |
| // I hope that isn't a problem! |
This kind of historical comment is best put in the Git commit message.
It is worth noting what happens if the recursion limit is exceeded. Does it throw an exception?
There was a problem hiding this comment.
https://en.cppreference.com/w/cpp/filesystem/path/lexically_normal says
Exceptions
May throw implementation-defined exceptions.
Great, lol
Ericson2314
changed the title
std::filesystem in canonPath and local store symlink detection
| std::filesystem::path path = realStoreDir.get(); | ||
| std::filesystem::path root = path.root_path(); | ||
| while (path != root) { | ||
| if (std::filesystem::is_symlink(path)) |
There was a problem hiding this comment.
Would be nice to wrap is_symlink in an try-catch and re-throw a nix error instead the original exception that std::filesystem throws because those actually will have an error trace added when the exception is bubbled up.
There was a problem hiding this comment.
The function rethrowExceptionAsError() can be used for that. (Should be moved from local-derivation-goal.cc to libutil, probably.)
There was a problem hiding this comment.
Ok. Just make sure rethrowExceptionAsError() properly also logs paths as well this way so that users have a clue what goes wrong.
|
I have a branch with many changes related to this. I'll be reworking that branch and I'll update this PR soon. |
Motivation
Nix on Windows.
Context
Local store wouldn't work on Windows due to an infinite loop (paths never become "/" on Windows)
Canon path was always prepending the root path (e.g. D:/) which ended up with silly paths (e.g. D:/D:/nix/store) and it was quite a complicated function.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.