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

windows: fixes unlinking symlink to dir, rmdir on opened empty dir. #1172

Merged
merged 11 commits into from
Feb 27, 2023
Merged

windows: fixes unlinking symlink to dir, rmdir on opened empty dir. #1172

merged 11 commits into from
Feb 27, 2023

Conversation

mathetake
Copy link
Member

No description provided.

@mathetake mathetake mentioned this pull request Feb 27, 2023
Merged
@mathetake
Copy link
Member Author

not found a fix yet.

mathetake
mathetake commented Feb 27, 2023
View reviewed changes
internal/sysfs/dirfs_test.go
require.EqualErrno(t, syscall.EISDIR, err)

require.NoError(t, os.Remove(realPath))
})

t.Run("target: symlink to dir", func(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this one is the new one and only windows failing

@mathetake
Copy link
Member Author

interesting as I expected this only fails with 1.20.

@mathetake
Copy link
Member Author

looks like this has existed for a while but somehow hasn't been caught until Zig test has started failing with 1.20 upgrade

@mathetake
test
5d22056 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
fix
c46495b 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
fix
fe0fb55 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
fix
c3b212f 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Copy link
Member Author

ok original issue has been fixed, but another comes up:


test "makePath in a directory that no longer exists" {
    if (builtin.os.tag == .windows) return error.SkipZigTest; // Windows returns FileBusy if attempting to remove an open dir

    var tmp = tmpDir(.{});
    defer tmp.cleanup();
    try tmp.parent_dir.deleteTree(&tmp.sub_path);

    try testing.expectError(error.FileNotFound, tmp.dir.makePath("sub-path"));
}

@codefromthecrypt
Copy link
Contributor

@mathetake the other one looks like they are skipping on windows, which I guess is annoying because we can't do that without excluding "makePath in a directory that no longer exists" from all operating systems (due to the filter being a compile option). one way could be to exclude it from zig, but re-add the test in our repo so that it runs when != windows. wdyt?

@mathetake
Copy link
Member Author

mathetake commented Feb 27, 2023 •
edited
Loading

makePath in a directory that no longer exists" this is not the only test in zig skipped on window, and there's tens skipped on windows we previously passing including ^. Plus wasmtime passes so there must be a way to pass on windows. Also, we should be able to provide consistent behavior regardless of the host. So I will figure out this rather than adding skip on windows.

@mathetake
Copy link
Member Author

minimize the case

test "minimized" {
    var tmp = tmpDir(.{});
    tmp.parent_dir.deleteTree(&tmp.sub_path) catch @panic("aaaaaaaa");
}

@mathetake
refine
e77e207 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
this..
1e35027 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Copy link
Member Author

found the repro which only fails on windows 1e35027

@mathetake
Copy link
Member Author

maybe time to resolve https://github.com/tetratelabs/wazero/blame/75aa6b2a6efa86b20e9b4eecd2889c83b714b1ca/internal/platform/open_file_windows.go#L40

@mathetake
make it real
0766741 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
fix?
e4a40d7 
Signed-off-by: Takeshi Yoneda <[email protected]>
mathetake
mathetake commented Feb 27, 2023
View reviewed changes
internal/platform/open_file_windows.go Outdated
@@ -140,6 +140,10 @@ func open(path string, mode int, perm uint32) (fd syscall.Handle, err error) {
}
}
}
if createmode == syscall.OPEN_EXISTING && access == syscall.GENERIC_READ {
Copy link
Member Author

Choose a reason for hiding this comment

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

lifted from the change made in Go 1.20 https://github.com/golang/go/blob/go1.20/src/syscall/syscall_windows.go#L308-L379 and I guess that's why we haven't encountered it before.

@mathetake mathetake changed the title dirfs: ensures no error on unlinking symlink to dir windows: fixes unlinking symlink to dir, rmdir on opened empty dir. Feb 27, 2023
@mathetake
Copy link
Member Author

LOL only windows 1.18 is failing LOL

@mathetake
ok
0ca0dbd 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Copy link
Member Author

🤞

@mathetake mathetake marked this pull request as ready for review February 27, 2023 07:14
@mathetake
variable
1671b14 
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Copy link
Member Author

all went green

codefromthecrypt
codefromthecrypt approved these changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

thanks!

internal/platform/open_file_windows.go
h, e := syscall.CreateFile(pathp, access, sharemode, sa, createmode, attrs, 0)
return h, e
}

var isGo120 = strings.Contains(runtime.Version(), "go1.20")
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break in 1.21 but I can sort it after merge

@mathetake mathetake merged commit d955cd7 into main Feb 27, 2023
@mathetake mathetake deleted the dirfsunlink branch February 27, 2023 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codefromthecrypt codefromthecrypt codefromthecrypt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mathetake @codefromthecrypt