Convert all dir functions to just use libuv with windows long path support #1780

MasterDuke17 · 2023-11-27T04:52:03Z

This is #1745 + #1776 to see what the combination looks like in CI.

instead of doing our own `#define`s for Windows support. Almost all the logic is the same. However, libuv removed '.' and '..' from readdir results, and since Rakudo (and roast) expects them, this will require a minor change in Rakudo. The JVM also doesn't return those for its readdir implementation, so there's already code that deals with that in Rakudo in `#?if jmv`s, which will just need to be adjusted to be `#?if !js`.

On Windows if you want path parts longer than MAX_PATH then you need to use DOS paths, i.e. absolute paths prefixed with \\?\. Additionally, using DOS paths does not work with / so those slashes need to be normalized when prefixing paths with \\?\. This adds the aforementioned prefix and normalization on Windows, allowing long paths to be used on that OS. Note this is a Proof of Concept and I haven't spent any time thinking about the design or what I may have otherwise broke.

This is more code that was doing its own \\?\ prefixing, which does not work as expected now that we are prefixing the paths before calling the function. This remove the MVM_platform_unlink function that was used on Windows, such that all platforms use the same unlink function.

In a previous commit the windows specific code for mkdir_p was removed such that all platforms would use the same logic. However, the non-Windows logic had a S_ISDIR check that I temporarily removed due to it not being available on Windows. This adds the S_ISDIR macro on Windows, and readds the original S_ISDIR check to our mkdir logic.

Absolute paths of any length can use the \\?\ prefix, so we initially implemented things without checking the length. However, the native nqp and rakudo runners currently use absolute paths that contain `..` as a path part which is literal (i.e. not the parent directory) when used with the \\?\ prefix. This checks the path length to see if it is longer than MAX_PATH so we avoid the issue with the nqp/rakudo runners in most situations while still getting some immediate relief to other problems stemming from Windows path length issues.

MasterDuke17 · 2023-11-27T13:46:55Z

‘Failed to remove the directory 'C:\Users\VSSADM~1\AppData\Local\Temp\rakudo-precompTXX99GWZUNWDWHT3Z9CPGG99\34310A559C756907FABDDC38457C1B3DCE353841': Failed to rmdir: directory not empty
’ @ugexe does this look familiar?

MasterDuke17 · 2023-11-27T19:31:36Z

Oh, and why doesn't that path start with \\?\?

ugexe · 2023-11-28T17:59:36Z

The path doesn't start with \\?\ because of ee63bb3

ugexe · 2023-11-28T18:03:27Z

Failed to remove the directory 'C:\Users\VSSADM~1\AppData\Local\Temp\rakudo-precompTXX99GWZUNWDWHT3Z9CPGG99\34310A559C756907FABDDC38457C1B3DCE353841': Failed to rmdir: directory not empty

Hmm I'm not sure what that error is about. My initial guess would be that a file or directory isn't getting deleted (maybe because a handle is left open?) but I'm sure you already suspected that.

ugexe · 2023-11-30T22:52:12Z

It seems to be caused by https://github.com/rakudo/rakudo/blob/c3e68caa1b84c825b9ba4aff91602168ab8645be/t/02-rakudo/reproducible-builds.t#L35 (i.e. the code in the test itself, not something in rakudo). I wonder if the test would pass if it was rerun a couple of times... and if that works I wonder if we need to add another 'retry filesystem operation on windows' thing we added elsewhere in the past.

edit: hmmm, but the reloc build has the same error which suggest retrying the test probably wouldn't help...

MasterDuke17 · 2023-12-01T00:49:38Z

edit: hmmm, but the reloc build has the same error which suggest retrying the test probably wouldn't help...

Well, maybe it needs a couple retries.... I'll restart those jobs to see if another run works.

ugexe · 2023-12-01T23:17:46Z

I went to try and debug this, but unfortunately my windows VMs are making this impossible. I can't build moar-blead due to it using a newish libuv. It fails with uv.lib(util.obj) : error LNK2001: unresolved external symbol _GetSystemTimePreciseAsFileTime, and I believe it is related to whatever Windows 11 ARM uses to translate (like the windows equivalent of macOS Rosetta). I haven't had much luck finding hard evidence of that though. Regardless, the newest version of MoarVM I can build on this ARM machine using Windows is 2023.04.

It has to be something related to the libuv changes in some way. I wanted to see debug output added to https://github.com/rakudo/rakudo/blob/73470580c9286b8257be9175fd8bff57f3b14bb4/src/core.c/CompUnit/PrecompilationStore/FileSystem.rakumod#L230C1-L237 so we could see what is there initially, what actually does get deleted, etc. That would help pinpoint where in MoarVM to look at harder at least...

MasterDuke17 · 2023-12-02T02:06:07Z

I wanted to see debug output added to rakudo/rakudo@7347058/src/core.c/CompUnit/PrecompilationStore/FileSystem.rakumod#L230C1-L237 so we could see what is there initially, what actually does get deleted, etc. That would help pinpoint where in MoarVM to look at harder at least...

I think it might be possible to get CI to use a different MoarVM and/or Rakudo repo. So create a Rakudo PR that adds debugging info to that function, and then also changes the Rakudo CI workflow to use my MoarVM repo on this branch (or vice versa, add a Rakudo branch that adds the debugging and then add a change to this PR to point there).

…VM#1780

src/io/dirops.c

Windows doesn't always close the filehandle right away, so if we get an ENOTEMPTY, sleep and try again (ignoring ENOENT the second time, since that would just mean the delete/close happened during the sleep).

ugexe · 2023-12-04T18:12:43Z

Hmm, it is definitely confusing that having the sleep in MoarVM seems to not fix the issue that putting the sleep in Rakudo does 🤔

MasterDuke17 · 2023-12-04T19:42:20Z

Hmm, it is definitely confusing that having the sleep in MoarVM seems to not fix the issue that putting the sleep in Rakudo does 🤔

However, the first sleep added in rakudo was before the rmdir, not after the first call to it. I'll see if that changes anything.

ugexe · 2023-12-04T20:49:38Z

One hail mary I can think of would be updating the deletion code to hopefully ensure that .dir isn't still open when deleting:

    method delete-by-compiler(CompUnit::PrecompilationId:D $compiler-id) {
         my $compiler-dir := self.prefix.add($compiler-id);
         for $compiler-dir.dir.list -> $subdir {
             .unlink for $subdir.dir.list;
             $subdir.rmdir;
         }
         $compiler-dir.rmdir;
    }

which changes .dir to .dir.list

MasterDuke17 · 2023-12-04T22:11:47Z

Of course what's crazy is that rakudo/rakudo#5483 is now passing.

ugexe · 2023-12-07T00:17:22Z

fwiw I cut and paste all the changes one by one into my franken-VM on 2023.04 and the reproducible build test passes every time without the need to retry.

MasterDuke17 · 2023-12-07T01:02:47Z

Think it’s because of the libuv bump between then and now?Sent from my iPhoneOn Dec 6, 2023, at 7:17 PM, Nick Logan ***@***.***> wrote: fwiw I cut and paste all the changes one by one into my franken-VM on 2023.04 and the reproducible build test passes every time without the need to retry. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

ugexe · 2023-12-07T01:14:21Z

Im not sure. Maybe in the CI test we can cd to libuv, checkout an older version, and do nmake install to test that. Or maybe @coke can try running that test 😬

This reverts commit 9a6eabf.

ugexe · 2023-12-07T19:28:21Z

Just to be clear: my VM is running libuv v1.44.1, so just reverting v1.47.0 wouldn't put it at the same version. Looking at the changelog though I'm not sure the libuv version would be affecting this.

lizmat · 2024-07-29T16:35:37Z

ping @niner

MasterDuke17 and others added 9 commits November 26, 2023 23:47

Use constant from libuv and fix parens

7c1f286

Don't want to try to read from a closed dirhandle

5ad8b05

Simplify and reuse is_absolute_path utility function

8f65da3

Rename MVM_process_path to MVM_platform_path

20ca3d6

MasterDuke17 added a commit to MasterDuke17/rakudo that referenced this pull request Dec 2, 2023

Attempt at adding debugging to delete-by-compiler to test MoarVM/Moar…

d34c7d8

…VM#1780

MasterDuke17 mentioned this pull request Dec 2, 2023

Attempt at adding debugging to delete-by-compiler to test https://git… rakudo/rakudo#5483

Draft

ugexe reviewed Dec 2, 2023

View reviewed changes

src/io/dirops.c Outdated Show resolved Hide resolved

Use uv_strerror_r instead of (uv_)?sterror

1c06c27

MasterDuke17 force-pushed the use_more_libuv_dir_functions_with_windows-long-path-support_from_ugexe branch from 3ee5dcf to ba4128d Compare December 3, 2023 22:43

ugexe reviewed Dec 3, 2023

View reviewed changes

src/io/dirops.c Outdated Show resolved Hide resolved

ugexe reviewed Dec 3, 2023

View reviewed changes

src/io/dirops.c Show resolved Hide resolved

Retry filesytem deletes on Windows for ENOTEMPTY

c75d40e

Windows doesn't always close the filehandle right away, so if we get an ENOTEMPTY, sleep and try again (ignoring ENOENT the second time, since that would just mean the delete/close happened during the sleep).

MasterDuke17 force-pushed the use_more_libuv_dir_functions_with_windows-long-path-support_from_ugexe branch from ba4128d to c75d40e Compare December 3, 2023 23:44

MasterDuke17 added 3 commits December 3, 2023 20:32

Use UV_* constants and check for more to retry

9a10f1f

Let's see what's happening on windows

4e153c7

How about sleeping longer?

4ae5717

MasterDuke17 force-pushed the use_more_libuv_dir_functions_with_windows-long-path-support_from_ugexe branch from d704be1 to 4ae5717 Compare December 4, 2023 22:23

Revert "Update libuv to version 1.47.0"

2ad5d62

This reverts commit 9a6eabf.

AntonOks mentioned this pull request Feb 25, 2024

uv.lib(util.obj) : error LNK2001: unresolved external symbol _GetSystemTimePreciseAsFileTime #1789

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert all dir functions to just use libuv with windows long path support #1780

Convert all dir functions to just use libuv with windows long path support #1780

MasterDuke17 commented Nov 27, 2023

MasterDuke17 commented Nov 27, 2023 •

edited

Loading

MasterDuke17 commented Nov 27, 2023

ugexe commented Nov 28, 2023

ugexe commented Nov 28, 2023

ugexe commented Nov 30, 2023 •

edited

Loading

MasterDuke17 commented Dec 1, 2023

ugexe commented Dec 1, 2023

MasterDuke17 commented Dec 2, 2023

ugexe commented Dec 4, 2023

MasterDuke17 commented Dec 4, 2023

ugexe commented Dec 4, 2023

MasterDuke17 commented Dec 4, 2023

ugexe commented Dec 7, 2023

MasterDuke17 commented Dec 7, 2023 via email

ugexe commented Dec 7, 2023

ugexe commented Dec 7, 2023

lizmat commented Jul 29, 2024

Convert all dir functions to just use libuv with windows long path support #1780

Are you sure you want to change the base?

Convert all dir functions to just use libuv with windows long path support #1780

Conversation

MasterDuke17 commented Nov 27, 2023

MasterDuke17 commented Nov 27, 2023 • edited Loading

MasterDuke17 commented Nov 27, 2023

ugexe commented Nov 28, 2023

ugexe commented Nov 28, 2023

ugexe commented Nov 30, 2023 • edited Loading

MasterDuke17 commented Dec 1, 2023

ugexe commented Dec 1, 2023

MasterDuke17 commented Dec 2, 2023

ugexe commented Dec 4, 2023

MasterDuke17 commented Dec 4, 2023

ugexe commented Dec 4, 2023

MasterDuke17 commented Dec 4, 2023

ugexe commented Dec 7, 2023

MasterDuke17 commented Dec 7, 2023 via email

ugexe commented Dec 7, 2023

ugexe commented Dec 7, 2023

lizmat commented Jul 29, 2024

MasterDuke17 commented Nov 27, 2023 •

edited

Loading

ugexe commented Nov 30, 2023 •

edited

Loading