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.

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

Build Runner: Initial Implementation for File System Watching on Windows #20667

Merged
merged 4 commits into from
Jul 29, 2024

Conversation

jayrod246
Copy link
Contributor

@jayrod246 jayrod246 commented Jul 17, 2024

Closes #20598.

This initial implementation has the limit of 64 directories due to the usage of WaitForMultipleObjects. If merged, a follow up issue should be created to track overcoming that limitation.

Using IO completion ports instead of WaitForMultipleObjects, I am able to observe more than 64 directories at once.

BTW First time contributing, not very experienced with contributing on GitHub in general, but I love Zig. And I saw this issue and thought it would be a good learning opportunity for me.

lib/std/Build/Watch.zig Outdated Show resolved Hide resolved
@jayrod246
Copy link
Contributor Author

windows.kernel32.ReadDirectoryChangesW is recently removed in #19641

Thoughts on where or how I can reintroduce it? Or if there is an Nt way it can be replicated?

Copy link
Collaborator

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

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

Re-adding kernel32.ReadDirectoryChangesW seems fine for now IMO. The Nt implementation can be figured out later (FWIW it looks like the relevant ntdll function is NtNotifyChangeDirectoryFileEx)

lib/std/Build/Watch.zig Outdated Show resolved Hide resolved
lib/std/Build/Watch.zig Outdated Show resolved Hide resolved
@jayrod246
Copy link
Contributor Author

I added an error return to readChanges and some accountability for resources.

I still need to deal with this point though:

Note, though, that it also may be true that the dh_gop.found_existing case is impossible. Opening the same directory twice does not result in the same HANDLE:

const std = @import("std");

pub fn main() !void {
    var dir1 = try std.fs.cwd().openDir(".", .{});
    defer dir1.close();

    var dir2 = try std.fs.cwd().openDir(".", .{});
    defer dir2.close();

    std.debug.print("1: {}\n2: {}\n", .{ dir1.fd, dir2.fd });
}

results in

1: anyopaque@9c
2: anyopaque@98

@jayrod246
Copy link
Contributor Author

jayrod246 commented Jul 20, 2024

Instead of keying on the file handle itself, which is determined to be different each NtCreateFile call, would it be best to key on a FILE_ID_INFORMATION structure returned by NtQueryInformationFile? I believe that it is unique per file and volume. If it already exists, I would just close that handle and keep the existing one open.

edit: Further reading I found that FileId is not supported on Fat32. To determine that, you check if the FileId is zero and if so, the file system doesn’t support it. So, I could hash on the VolumeSerialNumber and, the FileId when non-zero, or a number based on I’m guessing an all lowercase full path?

@squeek502
Copy link
Collaborator

squeek502 commented Jul 21, 2024

I think the way to go might be:

Then hash the combination of those.

Some examples of calling NtQueryInformationFile/NtQueryVolumeInformationFile can be found here:

  • zig/lib/std/fs/File.zig

    Lines 211 to 217 in ff02bf4

    var io_status: windows.IO_STATUS_BLOCK = undefined;
    var device_info: windows.FILE_FS_DEVICE_INFORMATION = undefined;
    const rc = windows.ntdll.NtQueryVolumeInformationFile(handle, &io_status, &device_info, @sizeOf(windows.FILE_FS_DEVICE_INFORMATION), .FileFsDeviceInformation);
    switch (rc) {
    .SUCCESS => {},
    else => return false,
    }
  • zig/lib/std/fs/File.zig

    Lines 457 to 469 in ff02bf4

    var io_status_block: windows.IO_STATUS_BLOCK = undefined;
    var info: windows.FILE_ALL_INFORMATION = undefined;
    const rc = windows.ntdll.NtQueryInformationFile(self.handle, &io_status_block, &info, @sizeOf(windows.FILE_ALL_INFORMATION), .FileAllInformation);
    switch (rc) {
    .SUCCESS => {},
    // Buffer overflow here indicates that there is more information available than was able to be stored in the buffer
    // size provided. This is treated as success because the type of variable-length information that this would be relevant for
    // (name, volume name, etc) we don't care about.
    .BUFFER_OVERFLOW => {},
    .INVALID_PARAMETER => unreachable,
    .ACCESS_DENIED => return error.AccessDenied,
    else => return windows.unexpectedStatus(rc),
    }

(I'd recommend using that BUFFER_OVERFLOW handling for the NtQueryVolumeInformationFile since we don't care about the volume name)

@jayrod246
Copy link
Contributor Author

Forgot that I had to add FILE_FS_VOLUME_INFORMATION in windows.zig, will fix later today.

lib/std/os/windows.zig Outdated Show resolved Hide resolved
lib/std/Build/Watch.zig Outdated Show resolved Hide resolved
lib/std/Build/Watch.zig Outdated Show resolved Hide resolved
lib/std/Build/Watch.zig Outdated Show resolved Hide resolved
lib/std/Build/Watch.zig Outdated Show resolved Hide resolved
@jayrod246 jayrod246 force-pushed the windows-watch branch 3 times, most recently from 8effc49 to 4329052 Compare July 27, 2024 01:22
lib/std/Build/Watch.zig Outdated Show resolved Hide resolved
Copy link
Collaborator

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

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

Looking good IMO, nice work!

Seems to be working as expected from my testing. Tested on NTFS, FAT32, and a networked Samba share and they all worked fine.

lib/std/Build/Watch.zig Outdated Show resolved Hide resolved
@andrewrk andrewrk merged commit 3a0da43 into ziglang:master Jul 29, 2024
10 checks passed
@andrewrk
Copy link
Member

Nice work!

@jayrod246 jayrod246 deleted the windows-watch branch July 29, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build Runner: Implement File System Watching on Windows
4 participants