implement nt path conversion for windows #7664

marler8997 · 2021-01-03T03:03:25Z

Second attempt to fix #6044 (previous attempt #7537)

The first attempt leveraged the functionality in ntdll to perform the Dos to NT path conversion, however, that API requires heap allocation which @andrewrk has indicated we don't want (see #7537 (comment)).

So this PR implements the logic to convert a DOS file path into an NT-compatible path in Zig's std lib.

daurnimator · 2021-01-03T04:15:30Z

lib/std/os/windows.zig

+/// The error type for `removeDotDirsSanitized`
+pub const RemoveDotDirsError = error { TooManyParentDirs };
+
+/// Removes '.' and '..' path components from a "sanitized relative path".


this function looks like it belongs in std.fs.path. it might be a duple of resolveWindows?

It's specific to windows...only works with backslashes. We could also make it work with forward slashes but I think posix has other better ways to perform this operation. resolveWindows does alot more than this function, it uses CWD and touches the filesystem (and I think resolves symlinks). This function does none of that, it just removes the . and .. directories from the path name.

It's specific to windows...only works with backslashes

Os-specific code is all over the standard library: it doesn't need to all get dumped into os/windows.zig.

lib/std/mem.zig

LemonBoy · 2021-01-03T10:31:55Z

lib/std/mem.zig

+}
+
+/// Collapse consecutive duplicate elements into one entry.
+pub fn collapseRepeats(comptime T: type, slice: []T, needle: T) usize {


Not much of a needle, it's more a element no?
And given it squishes slice contents it makes sense for this to return a slice of it.

I've changed needle to elem.

As for returning a slice, such an interface could be supported with a wrapper function at no runtime cost. In the current uses, only the resulting size is used and this same interface is propagated in a handful of places which makes the code simpler.

You can get the old behaviour by simply adding a .len at the end.
Returning a slice makes the API much easier to use as f(collapseRepeats(..)), throwing indices around is error-prone.

Returning a slice makes the API much easier to use

In some cases yes. If such a use case comes up we can add a wrapper function that does this.

It should be the other way around, returning a slice returns a pointer and the adjusted length.

It should be the other way around, returning a slice returns a pointer and the adjusted length.

My philosphy is to do less, and then only do more if you need to. Going the other way around means always doing more even if you don't need to, and then undoing what you don't need.

I believe my pattern here is also already established in other functions in this codebase, so I don't think I'm alone in this.

Another example of an API that uses this methodology is the Allocator resizeFn function.

If you're adding new public-facing APIs you can't just think in terms of "do less" or think of your single use-case.
Returning the length makes using this function with sliced inputs hard to use, consider the following example:

const x = collapseRepeats(foo[bar..Baz], ...)

If you return a sub-slice the user is able to quickly use the modified chunk of input, while if the length is returned instead another slicing operation is required.

But given that opinions are like assholes I'll rest my case, I'm getting too old for this shit.

Ok the fact that this is a public facing "general" function is a good point. How about creating a collapseRepeatsLen that only returns the length and collapseRepeats that wraps it?

lib/std/mem.zig

andrewrk

Thanks for the patch. This solution involves manipulating path strings, however Windows has symlinks, and on other platforms, when opening files with std.fs, ".." path components respect symlinks, so we need to do it here, too.

andrewrk · 2021-04-02T17:33:36Z

lib/std/fs.zig

-            // If you're looking to contribute to zig and fix this, see here for an example of how to
-            // implement this: https://git.midipix.org/ntapi/tree/src/fs/ntapi_tt_open_physical_parent_directory.c


I don't see this addressed in the PR description - did you look into using this technique to resolve .. in a way that respects following symlinks?

Related: NtQueryObject #1840

On windows symlinks are resolved after resolving ".." (#7751)

These tests check that . and .. are able to be handled by the std.fs functions. Before ziglang#7664, these tests would have failed on Windows. Closes ziglang#4659

Added in ziglang#4655/ziglang#5187, obsoleted by ziglang#7664

squeek502 · 2021-05-23T23:56:30Z

Nice work @marler8997! This hugely improves std.fs on Windows.

andrewrk · 2021-05-23T23:57:26Z

Thanks for the clean-up PRs @squeek502!

Added in #4655/#5187, obsoleted by #7664

marler8997 marked this pull request as draft January 3, 2021 03:31

marler8997 force-pushed the fixWindowsPaths branch 3 times, most recently from ba5677a to 344553f Compare January 3, 2021 04:05

daurnimator reviewed Jan 3, 2021

View reviewed changes

daurnimator added os-windows standard library This issue involves writing Zig code for the standard library. labels Jan 3, 2021

marler8997 force-pushed the fixWindowsPaths branch from 344553f to 1a0cd6c Compare January 3, 2021 07:12

marler8997 marked this pull request as ready for review January 3, 2021 08:12

marler8997 force-pushed the fixWindowsPaths branch from 1a0cd6c to 03fc47a Compare January 3, 2021 08:44

LemonBoy reviewed Jan 3, 2021

View reviewed changes

lib/std/mem.zig Outdated Show resolved Hide resolved

LemonBoy reviewed Jan 3, 2021

View reviewed changes

lib/std/mem.zig Outdated Show resolved Hide resolved

marler8997 force-pushed the fixWindowsPaths branch 4 times, most recently from f90bc87 to cd84d96 Compare January 7, 2021 18:35

rohlem mentioned this pull request Jan 12, 2021

failed to run zig fmt on windows #7654

Closed

andrewrk requested changes Apr 2, 2021

View reviewed changes

marler8997 force-pushed the fixWindowsPaths branch 2 times, most recently from 11162fa to b11bb6e Compare April 3, 2021 17:01

marler8997 added 3 commits May 20, 2021 14:00

implement nt path conversion for windows

6665840

have collapseRepeats return slice intead of just len

a72ad61

add the openDir cwd parent test

59de5d0

marler8997 force-pushed the fixWindowsPaths branch 2 times, most recently from 813ce22 to d68ea4a Compare May 21, 2021 05:38

handle relative paths with too many ".."

b0116af

marler8997 force-pushed the fixWindowsPaths branch from d3a81ba to b0116af Compare May 21, 2021 07:29

andrewrk merged commit 7cd9b30 into ziglang:master May 22, 2021

squeek502 mentioned this pull request May 22, 2021

Relative paths on Windows: when to resolve . and .. ? #4659

Closed

marler8997 deleted the fixWindowsPaths branch May 22, 2021 23:31

squeek502 mentioned this pull request May 23, 2021

Add . and .. tests for std.fs functions #8879

Merged

squeek502 added a commit to squeek502/zig that referenced this pull request May 23, 2021

zig fmt: Remove workaround for . and .. path handling on Windows

e07d291

Added in ziglang#4655/ziglang#5187, obsoleted by ziglang#7664

squeek502 mentioned this pull request May 23, 2021

zig fmt: Remove workaround for . and .. path handling on Windows #8881

Merged

andrewrk pushed a commit that referenced this pull request May 24, 2021

zig fmt: Remove workaround for . and .. path handling on Windows

6426519

Added in #4655/#5187, obsoleted by #7664

marler8997 mentioned this pull request Sep 15, 2021

implement ".." (parent directory) for std.fs.Dir.openDir on Windows #3501

Closed

squeek502 mentioned this pull request May 18, 2023

windows: support UNC paths #15283

Closed

matklad mentioned this pull request Aug 11, 2023

Parent directory (..) does not work on windows #16779

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement nt path conversion for windows #7664

implement nt path conversion for windows #7664

marler8997 commented Jan 3, 2021 •

edited

Loading

daurnimator Jan 3, 2021

marler8997 Jan 3, 2021 •

edited

Loading

daurnimator Jan 3, 2021 •

edited

Loading

LemonBoy Jan 3, 2021

marler8997 Jan 5, 2021

LemonBoy Jan 6, 2021

marler8997 Jan 6, 2021

LemonBoy Jan 6, 2021

marler8997 Jan 6, 2021 •

edited

Loading

marler8997 Jan 6, 2021

LemonBoy Jan 6, 2021

marler8997 Jan 7, 2021 •

edited

Loading

andrewrk left a comment

andrewrk Apr 2, 2021 •

edited

Loading

marler8997 Apr 2, 2021

squeek502 commented May 23, 2021

andrewrk commented May 23, 2021

		// If you're looking to contribute to zig and fix this, see here for an example of how to
		// implement this: https://git.midipix.org/ntapi/tree/src/fs/ntapi_tt_open_physical_parent_directory.c

implement nt path conversion for windows #7664

implement nt path conversion for windows #7664

Conversation

marler8997 commented Jan 3, 2021 • edited Loading

Choose a reason for hiding this comment

marler8997 Jan 3, 2021 • edited Loading

Choose a reason for hiding this comment

daurnimator Jan 3, 2021 • edited Loading

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

marler8997 Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

andrewrk left a comment

Choose a reason for hiding this comment

andrewrk Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squeek502 commented May 23, 2021

andrewrk commented May 23, 2021

marler8997 commented Jan 3, 2021 •

edited

Loading

marler8997 Jan 3, 2021 •

edited

Loading

daurnimator Jan 3, 2021 •

edited

Loading

marler8997 Jan 6, 2021 •

edited

Loading

marler8997 Jan 7, 2021 •

edited

Loading

andrewrk Apr 2, 2021 •

edited

Loading