Add win32 path.toNamespacedPath and align rest of node:path with Node #8469

jdalton · 2024-01-25T04:54:59Z

What does this PR do?

Add win32 path.toNamespacedPath and align rest of node:path with Node.

Port of Node's node:path 22+ methods to Zig 🎉
Added Node's node:path unit tests (80 node:path tests now) 🗒️
Verified on Windows (test\\js\\node\\path\\*

This is a very close port of Node's methods. I like this approach as it avoids gotchas that creep in with reworking code. Even things like how JavaScript allows -1 for String#slice plays its part in Node's implementation.

After this lands I'll do a follow-up PR to move these into resolve_path.zig.

github-actions · 2024-01-25T05:18:19Z

❌ @jdalton 4 files with test failures on bun-darwin-aarch64:

test/cli/test/bun-test.test.ts 2 failing
test/js/bun/shell/leak.test.ts 1 failing
test/js/deno/crypto/webcrypto.test.ts 2 failing
test/js/node/watch/fs.watchFile.test.ts 2 failing

View test output

^{#62f76d0785fe52b271a0e2d9040816f4783b54c8}

github-actions · 2024-01-25T05:25:41Z

❌ @jdalton 5 files with test failures on linux-x64:

test/cli/test/bun-test.test.ts 1 failing
test/integration/next/default-pages-dir/test/next-build.test.ts 1 failing
test/js/bun/shell/leak.test.ts 3 failing
test/js/deno/crypto/webcrypto.test.ts 2 failing
test/js/node/http2/node-http2.test.js 1 failing

View test output

^{#62f76d0785fe52b271a0e2d9040816f4783b54c8}

github-actions · 2024-01-25T05:26:16Z

❌ @jdalton 4 files with test failures on linux-x64-baseline:

test/cli/test/bun-test.test.ts 1 failing
test/integration/next/default-pages-dir/test/next-build.test.ts 1 failing
test/js/bun/shell/leak.test.ts 3 failing
test/js/deno/crypto/webcrypto.test.ts 2 failing

View test output

^{#62f76d0785fe52b271a0e2d9040816f4783b54c8}

github-actions · 2024-01-25T05:30:37Z

❌ @jdalton 4 files with test failures on bun-darwin-x64:

test/js/bun/shell/leak.test.ts 1 failing
test/js/bun/util/which.test.ts 1 failing
test/js/deno/crypto/webcrypto.test.ts 2 failing
test/js/node/fs/fs.test.ts 1 failing

View test output

^{#62f76d0785fe52b271a0e2d9040816f4783b54c8}

github-actions · 2024-01-25T06:07:46Z

❌🪟 @jdalton, there are 11 test regressions on Windows x86_64

test\cli\hot\hot.test.ts
test\cli\run\require-cache.test.ts
test\cli\run\transpiler-cache.test.ts
test\js\bun\console\console-iterator.test.ts
test\js\bun\util\bun-file-windows.test.ts
test\js\deno\crypto\webcrypto.test.ts
test\js\bun\http\bun-server.test.ts
test\js\node\process\process-args.test.js
test\js\third_party\es-module-lexer\es-module-lexer.test.ts
test\js\web\timers\setTimeout.test.js
test\js\web\workers\worker.test.ts

Full Test Output

Revised

* windows: fix module.paths getter causing a crash * use the buffer that was there before

paperclover

the approach of copying node 1:1 is great i think, though i did not verify all code was ported over properly. might be worth double checking if tests are failing, as i did spot one single place where something was misported.

regards to performance, i see:

the obvious complexity of what node:path does. this is unavoidable and i think it isn't actually going to be that huge of a deal. path normalization has to happen, and this will be a big win in accuracy (bun is not accurate in many regards)
the environment variable lookup in resolve is problematic i think. i really wish it didnt have to happen, see comments at that point in the review
the allocation stategy is alright but i think the getBufAlloc function seems very problematic. ~~i simply think we should just not support paths longer than the max. though i may be wrong in the need for this.~~ i am incorrect

after this pr, it would also be nice to reintroduce bun.String.map, a function did not ever commit: it lets us actually apply the generic versions of these functions to the javascript strings. some advances by dylan and georgijs in our string code make this a more useful api now (at the time i couldnt actually use the function or anything)

also, maybe we rename the Impl functions to have T as the suffix, as that was the pattern taken by the other code doing a similar pattern of generic string manipulation

src/bun.js/node/types.zig

paperclover · 2024-02-02T21:00:10Z

src/bun.js/node/types.zig


-    pub fn create(globalObject: *JSC.JSGlobalObject, isWindows: bool) callconv(.C) JSC.JSValue {
-        return shim.cppFn("create", .{ globalObject, isWindows });
+    /// Taken from Zig 0.11.0 zig/src/resinator/rc.zig


i wish this was in the standard library

I can be with a PR to zig! It's used internally to them already.

src/bun.js/node/types.zig

jdalton · 2024-02-12T17:45:16Z

test/js/node/fs/fs.test.ts

      }

      const results = await Promise.allSettled(pending);
      for (let i = 0; i < iterCount; i++) {
        expect(results[i].status).toBe("rejected");
        expect(results[i].reason!.code).toBe("ENOENT");
-        expect(results[i].reason!.path).toBe(join(notfound, i));
+        expect(results[i].reason!.path).toBe(join(notfound, `${i}`));


☝️ Ensure path.join gets string values.

jdalton · 2024-02-12T17:46:20Z

test/js/node/path/parse-format.test.js

+            //
+            // While Node's error message is:
+            //   The "pathObject" argument must be of type object. Received nullå
+            message: `"pathObject" property must be of type object, got ${typeof pathObject}`,


👆 TODO for error message compliance. Node provides a bit more-helpful messages around data types.

jdalton · 2024-02-12T17:46:47Z

test/js/node/path/path.test.js

@@ -1,900 +1,62 @@
-const { file } = import.meta;
-


Renamed to test/js/node/path/browserify.test.js

jdalton · 2024-02-12T17:47:52Z

test/js/node/path/resolve.test.js

+    //   const spawnResult = child.spawnSync(process.argv[0], [relativeFixture, currentDriveLetter]);
+    //   const resolvedPath = spawnResult.stdout.toString().trim();
+    //   assert.strictEqual(resolvedPath.toLowerCase(), process.cwd().toLowerCase());
+    // }


☝️ TODO for once our spawnSync().stdout result works on Windows.

jdalton · 2024-02-12T17:55:25Z

src/bun.js/node/types.zig

+
+inline fn MaybeSlice(comptime T: type) type {
+    return Maybe([]const T, Syscall.Error);
+}


MaybeBuf and MaybeSlice use Syscall.Error for now.

jdalton · 2024-02-12T17:57:28Z

src/bun.js/node/types.zig

+        };
+        const pathZStr = path_ptr.getZigString(globalObject);
+        const len = pathZStr.len;
+        if (len == 0) return toUTF8JSString(globalObject, CHAR_STR_DOT);


☝️ We don't even allocate if len is0, exiting early

jdalton · 2024-02-12T17:58:56Z

src/bun.js/node/types.zig

+        );
+        const allocator = stack_fallback.get();
+
+        const pathZSlice = pathZStr.toSlice(allocator);


Naming pathZSlice to distinguish between the ZigString.Slice and the standard buffer slice. Since we will be doing pathZSlice.slice() later and reading it from a glance can be a head scratcher.

jdalton · 2024-02-12T17:59:51Z

src/bun.js/node/types.zig

+            else
+                toJSString(globalObject, normalizePosix(pathZSlice.slice(), buf));
+        }
+        var buf: PathBuffer = undefined;


☝️ We only use the allocator when needed else we use the cheap stack var buf: PathBuffer = undefined;

jdalton · 2024-02-12T18:07:28Z

src/bun.js/node/types.zig

+            if (len == 0) {
+                // Skip work for empty entries
+                paths[i] = "";
+            } else {


☝️ Skip work for empty paths (defer allocator.free(paths) is still happy)

jdalton · 2024-02-12T18:14:48Z

src/bun.js/node/types.zig

+        );
+        const allocator = stack_fallback.get();
+
+        const pathZSlice = path_ptr.toSlice(globalObject, allocator);


☝️ The only place we jump straight to pathZSlice instead of going to pathZStr.

jdalton · 2024-02-12T18:16:33Z

src/bun.js/node/types.zig

@@ -2431,6 +4930,9 @@ pub const Path = struct {
            @export(Path.resolve, .{
                .name = Export[9].symbol_name,
            });
+            @export(Path.toNamespacedPath, .{
+                .name = Export[10].symbol_name,
+            });


☝️ toNamespacePath wiring

paperclover · 2024-02-12T18:57:19Z

src/bun.zig

@@ -173,7 +173,7 @@ pub const RefCount = @import("./ref_count.zig").RefCount;

 pub const MAX_PATH_BYTES: usize = if (Environment.isWasm) 1024 else std.fs.MAX_PATH_BYTES;
 pub const PathBuffer = [MAX_PATH_BYTES]u8;
-pub const WPathBuffer = [MAX_PATH_BYTES / 2]u16;
+pub const WPathBuffer = [windows.PATH_MAX_WIDE]u16;


src/bun.js/node/types.zig

…h Node (#8469)

* Fix some related to paths and string encoding * Fix relative paths with glob * Fix scan tests * Fix glob scan test * [autofix.ci] apply automated fixes * Fix leak test * clean up post merge * [autofix.ci] apply automated fixes * clean up glob getcwd * remove old struct * fix open on posix * feat: Add win32 path.toNamespacedPath and align rest of node:path with Node (#8469) * restore zls file change * [autofix.ci] apply automated fixes * switch to using fs.top_level_dir in glob --------- Co-authored-by: Jarred Sumner <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Georgijs Vilums <[email protected]> Co-authored-by: Georgijs Vilums <[email protected]> Co-authored-by: John-David Dalton <[email protected]> Co-authored-by: Georgijs <[email protected]>

This comment was marked as outdated.

Sign in to view

jdalton force-pushed the jdalton/to-namespaced-path-win branch 2 times, most recently from 2a52ec0 to 529197d Compare January 26, 2024 02:56

jdalton force-pushed the jdalton/to-namespaced-path-win branch from 529197d to 546932d Compare February 2, 2024 08:07

jdalton changed the title ~~Add Windows implementation of path.toNamespacedPath~~ Add win32 path.toNamespacedPath and align rest of node:path with Node Feb 2, 2024

jdalton force-pushed the jdalton/to-namespaced-path-win branch from 546932d to c30180a Compare February 2, 2024 08:17

jdalton requested a review from paperclover February 2, 2024 08:18

jdalton marked this pull request as ready for review February 2, 2024 08:19

jdalton force-pushed the jdalton/to-namespaced-path-win branch from c30180a to 394730b Compare February 2, 2024 08:32

jdalton force-pushed the jdalton/to-namespaced-path-win branch 4 times, most recently from f84b876 to dd3112e Compare February 2, 2024 11:00

jdalton referenced this pull request Feb 2, 2024

windows: fix module.paths getter causing a crash (#8633)

3a6318b

* windows: fix module.paths getter causing a crash * use the buffer that was there before

jdalton force-pushed the jdalton/to-namespaced-path-win branch from dd3112e to e3ca9de Compare February 2, 2024 19:57

paperclover previously requested changes Feb 2, 2024

View reviewed changes

jdalton force-pushed the jdalton/to-namespaced-path-win branch 8 times, most recently from 0bac691 to 5e4a05c Compare February 3, 2024 18:41

jdalton commented Feb 12, 2024

View reviewed changes

jdalton force-pushed the jdalton/to-namespaced-path-win branch from 8dcafa5 to 767b62b Compare February 12, 2024 17:50

jdalton commented Feb 12, 2024

View reviewed changes

jdalton force-pushed the jdalton/to-namespaced-path-win branch from 767b62b to f4e4fe9 Compare February 12, 2024 18:04

jdalton commented Feb 12, 2024

View reviewed changes

jdalton force-pushed the jdalton/to-namespaced-path-win branch from f4e4fe9 to 769c79a Compare February 12, 2024 18:18

paperclover self-requested a review February 12, 2024 18:46

paperclover reviewed Feb 12, 2024

View reviewed changes

paperclover approved these changes Feb 12, 2024

View reviewed changes

jdalton force-pushed the jdalton/to-namespaced-path-win branch from 769c79a to 8e272e5 Compare February 12, 2024 19:43

dylan-conway reviewed Feb 12, 2024

View reviewed changes

src/bun.js/node/types.zig Show resolved Hide resolved

jdalton force-pushed the jdalton/to-namespaced-path-win branch 2 times, most recently from d54f2cc to 46ec84d Compare February 12, 2024 20:13

Add win32 path.toNamespacedPath and align rest of node:path with Node

fc31407

jdalton force-pushed the jdalton/to-namespaced-path-win branch from 46ec84d to fc31407 Compare February 12, 2024 20:21

dylan-conway approved these changes Feb 12, 2024

View reviewed changes

paperclover merged commit 96e7227 into main Feb 12, 2024
26 of 31 checks passed

paperclover deleted the jdalton/to-namespaced-path-win branch February 12, 2024 22:27

gvilums pushed a commit that referenced this pull request Feb 12, 2024

feat: Add win32 path.toNamespacedPath and align rest of node:path wit…

fa756a1

…h Node (#8469)

This was referenced Feb 13, 2024

[win][fs] The second part of a UNC path should be a directory (path.resolve) #8882

Closed

[win][fs] path.toNamespacedPath does nothing #8247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add win32 path.toNamespacedPath and align rest of node:path with Node #8469

Add win32 path.toNamespacedPath and align rest of node:path with Node #8469

jdalton commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

This comment was marked as outdated.

paperclover left a comment •

edited

Loading

paperclover Feb 2, 2024

jdalton Feb 3, 2024

jdalton Feb 12, 2024

jdalton Feb 12, 2024

jdalton Feb 12, 2024

jdalton Feb 12, 2024

jdalton Feb 12, 2024

jdalton Feb 12, 2024

jdalton Feb 12, 2024

jdalton Feb 12, 2024

jdalton Feb 12, 2024

jdalton Feb 12, 2024

jdalton Feb 12, 2024

paperclover Feb 12, 2024

Add win32 path.toNamespacedPath and align rest of node:path with Node #8469

Add win32 path.toNamespacedPath and align rest of node:path with Node #8469

Conversation

jdalton commented Jan 25, 2024 • edited Loading

What does this PR do?

github-actions bot commented Jan 25, 2024 • edited Loading

github-actions bot commented Jan 25, 2024 • edited Loading

github-actions bot commented Jan 25, 2024 • edited Loading

github-actions bot commented Jan 25, 2024 • edited Loading

github-actions bot commented Jan 25, 2024 • edited Loading

❌🪟 @jdalton, there are 11 test regressions on Windows x86_64

This comment was marked as outdated.

paperclover left a comment • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

paperclover left a comment •

edited

Loading