Purify `CanonPath` #9881

Ericson2314 · 2024-01-29T22:15:16Z

Motivation

The core CanonPath constructors were using absPath, but absPath in some situations does IO which is not appropriate. It turns out that these constructors avoided those situations, and thus were pure, but it was far from obvious this was the case.

To remedy the situation, abstract the core algorithm from canonPath to use separately in CanonPath without any IO. No we know by-construction that those constructors are pure.

That leaves CanonPath::fromCWD as the only operation which uses IO / is impure. Add docs on it, and CanonPath as a whole, explaining the situation.

Context

This is also necessary to support Windows paths on windows without messing up CanonPath. But, I think it is good even without that.

In particular, #9767 is on top of this

Priorities and Process

Add 👍 to pull requests you find important.

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

thufschmitt · 2024-01-30T09:19:27Z

src/libutil/file-path-impl.hh

+ */
+typename std::string canonPathInner(
+    std::string_view path,
+    auto && hookComponent)


Can't this be made a std::function<void(std::string &, std::string_view &)> to avoid the need for a template?

Actually, I'd rather make this a flag to follow or not symlinks (unless there's a reason to generalize it). Higher-order functions are nice but add some non-trivial complexity (a consumer would have a pretty hard time re-implementing the followSymlinks lambda used here) so I don't think it's a great tradeoff here

It was a flag to follow symlinks before. The point of this PR is that following the flags was non-obvious. It's very important that CanonPath stuff not secretly use the host file system unless one opts-in with a PosixSourceAccesor.

To quote the PR description

The core CanonPath constructors were using absPath, but absPath in some situations does IO which is not appropriate. It turns out that these constructors avoided those situations, and thus were pure, but it was far from obvious this was the case.

Also fwiw in #9767 this code needs template regardless; I didn't just do it to try to make the function inline.

My interpretation of #9767 (comment) and https://discourse.nixos.org/t/2024-02-05-nix-team-meeting-minutes-121/39340 is that we do want to do that PR, and so this the stepping-stone.

src/libutil/canon-path.cc

src/libutil/file-system.cc

edolstra · 2024-01-30T14:12:08Z

When does absPath() do I/O? AFAIK only when resolveSymlinks is true, but it's false when called from the CanonPath constructor.

Ericson2314 · 2024-01-30T15:20:48Z

Let's talk more about the motivation of this PR, which I think has been lost in this discussion.

I believe that Nix needs "Nix Paths", which are conventionally Unix-style, but could equally be something abstract like std::vector<std::string>, and "Native paths", which may or may not be Unix-style. I arrived at this conclusion thinking about Windows support, but I think it's true either way --- we don't want to e.g. use the current working directory to resolve a relative path that we wish to use on a NAR.

On a conceptual level, it's rather dubious that these two implementations share code at all. For Windows, that conceptual problem becomes practical, as "Nix paths" should not become Windows-like on Windows. I have done this refactor to factor to try to begin separate these things.

If you all don't like templates, and don't like higher-order functions, then I suppose I better just jump straight to using std::filesystem::weakly_canonical in the "Native Path" case, and keep the custom logic just for CanonPath.

Ericson2314 · 2024-01-30T16:05:48Z

Oh we cannot just drop in std::filesystem::weakly_canonical because it still unconditionally follows symlinks when they exist.

Ericson2314 · 2024-02-13T20:21:32Z

This also seems useful for the things we are discussing in #9985.

roberth

Mostly two kinds of things

actionable suggestions, but also
moaning about the (stringly) state of path types in the code. Ignore those?

src/libutil/canon-path.hh

roberth · 2024-02-16T07:20:30Z

src/libutil/file-path-impl.hh

+/**
+ * Core pure path canonicalization algorithm.
+ *
+ * @param hookComponent A chance to modify `s` and `path` in an


What do s and path refer to? These look undefined.

Oh probably local variables in the implementation. Can't do that in api docs.

Oh wait, no they're implicitly parameters in the auto type. Oh C++...

Please describe the supposed signature that auto represents.

I don't know how we're supposed to do this, but maybe

Suggested change

* @param hookComponent A chance to modify `s` and `path` in an

* @param hookComponent(string s, string_view path) A callback to modify `s` and `path` in an

Yet that gives me no clue as to what I my callback is actually supposed to be able to do.

If the answer is "it kinda just works out", that would be rather disappointing, but on brand for template generics.
We might just live with that, but it would be nice to know more about why it works out or how.

I have done another thing to fix the documentation, I hope, without avoiding what I think is syntax Doxygen won't accept.

src/libutil/file-system.cc

roberth · 2024-02-16T07:46:08Z

src/libutil/file-path-impl.hh

+    if (s.empty())
+        s = "/";


Not sure I get this behavior. I know it was already there, so we should probably keep it?
I suppose the story about relative paths doesn't apply here?

This is probably not the right PR yet for such a change. Need better path types.

Agreed (a) I am not sure how we get this either (b) better to modify it later.

fricklerhandwerk · 2024-02-16T08:47:23Z

@Ericson2314 #9881 (comment) agreed that for the long-term we need our own abstraction for paths (or more generally the file system) that allows lossless projection onto existing implementations. A classic example is Python's pathlib, so no need to reinvent the wheel from first principles here.

Ericson2314 · 2024-02-16T14:41:53Z

@fricklerhandwerk indeed we've discussed moving to CanonPath + std::filesystem::path as the only two options with clearly separated purposes :).

The core `CanonPath` constructors were using `absPath`, but `absPath` in some situations does IO which is not appropriate. It turns out that these constructors avoided those situations, and thus were pure, but it was far from obvious this was the case. To remedy the situation, abstract the core algorithm from `canonPath` to use separately in `CanonPath` without any IO. No we know by-construction that those constructors are pure. That leaves `CanonPath::fromCWD` as the only operation which uses IO / is impure. Add docs on it, and `CanonPath` as a whole, explaining the situation. This is also necessary to support Windows paths on windows without messing up `CanonPath`. But, I think it is good even without that. Co-authored-by: Eelco Dolstra <[email protected]> Co-authored-by: Robert Hensing <[email protected]>

Ericson2314 · 2024-02-16T15:25:53Z

@alois31 now that this is merged, making that new version of the pathExists fix should work :)

Ericson2314 requested a review from edolstra as a code owner January 29, 2024 22:15

Ericson2314 mentioned this pull request Jan 29, 2024

Support Windows paths in canonPath and absPath #9767

Merged

thufschmitt reviewed Jan 30, 2024

View reviewed changes

edolstra reviewed Jan 30, 2024

View reviewed changes

src/libutil/canon-path.cc Outdated Show resolved Hide resolved

edolstra reviewed Jan 30, 2024

View reviewed changes

src/libutil/file-system.cc Outdated Show resolved Hide resolved

Ericson2314 force-pushed the purify-canon-path branch 3 times, most recently from f425a50 to 5aec28c Compare February 5, 2024 14:48

Ericson2314 assigned edolstra Feb 5, 2024

Ericson2314 force-pushed the purify-canon-path branch from 5aec28c to 5037c82 Compare February 12, 2024 13:13

Ericson2314 mentioned this pull request Feb 13, 2024

Restore builtins.pathExists behavior on broken symlinks #9985

Merged

roberth requested changes Feb 16, 2024

View reviewed changes

Ericson2314 force-pushed the purify-canon-path branch 3 times, most recently from d27d828 to 5a8604b Compare February 16, 2024 14:29

Ericson2314 force-pushed the purify-canon-path branch from 5a8604b to d17e1d9 Compare February 16, 2024 14:47

roberth approved these changes Feb 16, 2024

View reviewed changes

Ericson2314 enabled auto-merge February 16, 2024 15:08

Ericson2314 merged commit 60936f2 into NixOS:master Feb 16, 2024
8 checks passed

Ericson2314 deleted the purify-canon-path branch February 16, 2024 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Purify `CanonPath` #9881

Purify `CanonPath` #9881

Ericson2314 commented Jan 29, 2024 •

edited

Loading

thufschmitt Jan 30, 2024

thufschmitt Jan 30, 2024

Ericson2314 Jan 30, 2024

Ericson2314 Jan 30, 2024

Ericson2314 Jan 30, 2024

Ericson2314 Feb 13, 2024

edolstra commented Jan 30, 2024

Ericson2314 commented Jan 30, 2024 •

edited

Loading

Ericson2314 commented Jan 30, 2024

Ericson2314 commented Feb 13, 2024

roberth left a comment

roberth Feb 16, 2024

roberth Feb 16, 2024

roberth Feb 16, 2024

Ericson2314 Feb 16, 2024

roberth Feb 16, 2024

Ericson2314 Feb 16, 2024 •

edited

Loading

fricklerhandwerk commented Feb 16, 2024 •

edited

Loading

Ericson2314 commented Feb 16, 2024

Ericson2314 commented Feb 16, 2024

	* @param hookComponent A chance to modify `s` and `path` in an
	* @param hookComponent(string s, string_view path) A callback to modify `s` and `path` in an

Purify CanonPath #9881

Purify CanonPath #9881

Conversation

Ericson2314 commented Jan 29, 2024 • edited Loading

Motivation

Context

Priorities and Process

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edolstra commented Jan 30, 2024

Ericson2314 commented Jan 30, 2024 • edited Loading

Ericson2314 commented Jan 30, 2024

Ericson2314 commented Feb 13, 2024

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

fricklerhandwerk commented Feb 16, 2024 • edited Loading

Ericson2314 commented Feb 16, 2024

Ericson2314 commented Feb 16, 2024

Purify `CanonPath` #9881

Purify `CanonPath` #9881

Ericson2314 commented Jan 29, 2024 •

edited

Loading

Ericson2314 commented Jan 30, 2024 •

edited

Loading

Ericson2314 Feb 16, 2024 •

edited

Loading

fricklerhandwerk commented Feb 16, 2024 •

edited

Loading