Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add assumptions for resolveWindows, resolvePosixand realpath #10988

Closed
wants to merge 1 commit into from
Closed

Add assumptions for resolveWindows, resolvePosixand realpath #10988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/std/fs/path.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,10 @@ fn asciiEqlIgnoreCase(s1: []const u8, s2: []const u8) bool {
}

/// On Windows, this calls `resolveWindows` and on POSIX it calls `resolvePosix`.
/// Note: Symlinks currently dont work and should not be used,
/// because Windows and Posix having different behavior.
/// On Windows, symlinks are resolved after `..` is resolved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "Note: does not expand symlinks" is sufficient for the note here

The details on symlink resolution between platforms have to do with realpath and the other std.os calls, not this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "Note: does not expand symlinks" is sufficient for the note here

agreed.

The details on symlink resolution between platforms have to do with realpath and the other std.os calls, not this function

Why does this not belong into the description of the portable method, if it is a portability restriction?
I would expect that users tend to do tests on one platform with the most portable method and extrapolate to the others.

/// On POSIX, symlinks are resolved eagerly.
pub fn resolve(allocator: Allocator, paths: []const []const u8) ![]u8 {
if (native_os == .windows) {
return resolveWindows(allocator, paths);
Expand Down
8 changes: 6 additions & 2 deletions lib/std/os.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1438,7 +1438,7 @@ var wasi_cwd = if (builtin.os.tag == .wasi and !builtin.link_libc) struct {
/// Note that `cwd_init` corresponds to a Preopen directory, not necessarily
/// a POSIX path. For example, "." matches a Preopen provided with `--dir=.`
///
/// This must be called before using any relative or absolute paths with `std.os`
/// This must be called before using any relative or absolute paths with `std.os`
/// functions, if you are on WASI without linking libc.
///
/// `alloc` must not be a temporary or leak-detecting allocator, since `std.os`
Expand Down Expand Up @@ -1472,7 +1472,7 @@ pub fn initPreopensWasi(alloc: Allocator, cwd_init: ?[]const u8) !void {

/// Resolve a relative or absolute path to an handle (`fd_t`) and a relative subpath.
///
/// For absolute paths, this automatically searches among available Preopens to find
/// For absolute paths, this automatically searches among available Preopens to find
/// a match. For relative paths, it uses the "emulated" CWD.
pub fn resolvePathWasi(path: []const u8, out_buffer: *[MAX_PATH_BYTES]u8) !RelativePathWasi {
// Note: Due to WASI's "sandboxed" file handles, operations with this RelativePathWasi
Expand Down Expand Up @@ -5091,6 +5091,10 @@ pub const RealPathError = error{
/// extra `/` characters in `pathname`.
/// The return value is a slice of `out_buffer`, but not necessarily from the beginning.
/// See also `realpathZ` and `realpathW`.
/// Note: All /-separated components of the supplied path must resolve to a
/// directory, or a symlink to a directory, except for the last component
/// Use `fs.path.resolve` or `fs.path.relative` to get dir of file (`file/..`).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realpath should fully resolve any path containing relative . and .. components on any POSIX-compliant system as far as I remember. It is only Windows that is the problem where the NT kernel call we issue is unable to deal with those which implies it is up to the user to resolve the relative components first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non-working semantics (pathtofolder/file/..) of fs.path.relative can be seen here matu3ba/chepa@f34084c#diff-d924ca21d81d7d5a59eb9e10d3e2689c4c58a7e28e6ecce6a064701666f5a730L173

It is only Windows that is the problem where the NT kernel call we issue is unable to deal with those which implies it is up to the user to resolve the relative components first.

Could we on Windows work around the problem with calling relative on the subpath and then call resolve for each extending subpath? If you can point me to the things I need for testing, I can play abit to see if that could work.
Kind related is #4812.

/// assume: if relative path used, then they are relative to `getcwd()`
pub fn realpath(pathname: []const u8, out_buffer: *[MAX_PATH_BYTES]u8) RealPathError![]u8 {
if (builtin.os.tag == .windows) {
const pathname_w = try windows.sliceToPrefixedFileW(pathname);
Expand Down