From fdff887c0a99c9bb71f8a1b8fd4830dde46bb270 Mon Sep 17 00:00:00 2001 From: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> Date: Wed, 24 Apr 2024 18:55:13 -0700 Subject: [PATCH] file protocol and abs path (#10493) --- src/install/dependency.zig | 100 ++++++++++++++++++++++++++++--- src/install/install.zig | 18 +++--- src/install/lockfile.zig | 28 ++++----- src/string_immutable.zig | 2 +- test/cli/install/bun-add.test.ts | 91 +++++++++++++++------------- 5 files changed, 168 insertions(+), 71 deletions(-) diff --git a/src/install/dependency.zig b/src/install/dependency.zig index aeb48cbe04..44f0fc0663 100644 --- a/src/install/dependency.zig +++ b/src/install/dependency.zig @@ -396,6 +396,12 @@ pub const Version = struct { pub fn infer(dependency: string) Tag { // empty string means `latest` if (dependency.len == 0) return .dist_tag; + + if (strings.startsWithWindowsDriveLetter(dependency) and (std.fs.path.isSep(dependency[2]))) { + if (isTarball(dependency)) return .tarball; + return .folder; + } + switch (dependency[0]) { // =1 // >1.2 @@ -688,6 +694,23 @@ pub fn eql( return a.name_hash == b.name_hash and a.name.len() == b.name.len() and a.version.eql(&b.version, lhs_buf, rhs_buf); } +pub fn isWindowsAbsPathWithLeadingSlashes(dep: string) ?string { + var i: usize = 0; + if (dep.len > 2 and dep[i] == '/') { + while (dep[i] == '/') { + i += 1; + + // not possible to have windows drive letter and colon + if (i > dep.len - 3) return null; + } + if (strings.startsWithWindowsDriveLetter(dep[i..])) { + return dep[i..]; + } + } + + return null; +} + pub inline fn parse( allocator: std.mem.Allocator, alias: String, @@ -967,18 +990,81 @@ pub fn parseWithTag( .folder => { if (strings.indexOfChar(dependency, ':')) |protocol| { if (strings.eqlComptime(dependency[0..protocol], "file")) { - const folder = brk: { - if (dependency.len > protocol + 1 and dependency[protocol + 1] == '/') { - if (dependency.len > protocol + 2 and dependency[protocol + 2] == '/') { - break :brk dependency[protocol + 3 ..]; + const folder = folder: { + + // from npm: + // + // turn file://../foo into file:../foo + // https://github.com/npm/cli/blob/fc6e291e9c2154c2e76636cb7ebf0a17be307585/node_modules/npm-package-arg/lib/npa.js#L269 + // + // something like this won't behave the same + // file://bar/../../foo + const maybe_dot_dot = maybe_dot_dot: { + if (dependency.len > protocol + 1 and dependency[protocol + 1] == '/') { + if (dependency.len > protocol + 2 and dependency[protocol + 2] == '/') { + if (dependency.len > protocol + 3 and dependency[protocol + 3] == '/') { + break :maybe_dot_dot dependency[protocol + 4 ..]; + } + break :maybe_dot_dot dependency[protocol + 3 ..]; + } + break :maybe_dot_dot dependency[protocol + 2 ..]; } - break :brk dependency[protocol + 2 ..]; + break :folder dependency[protocol + 1 ..]; + }; + + if (maybe_dot_dot.len > 1 and maybe_dot_dot[0] == '.' and maybe_dot_dot[1] == '.') { + return .{ + .literal = sliced.value(), + .value = .{ .folder = sliced.sub(maybe_dot_dot).value() }, + .tag = .folder, + }; } - break :brk dependency[protocol + 1 ..]; + break :folder dependency[protocol + 1 ..]; }; - return .{ .literal = sliced.value(), .value = .{ .folder = sliced.sub(folder).value() }, .tag = .folder }; + // from npm: + // + // turn /C:/blah info just C:/blah on windows + // https://github.com/npm/cli/blob/fc6e291e9c2154c2e76636cb7ebf0a17be307585/node_modules/npm-package-arg/lib/npa.js#L277 + if (comptime Environment.isWindows) { + if (isWindowsAbsPathWithLeadingSlashes(folder)) |dep| { + return .{ + .literal = sliced.value(), + .value = .{ .folder = sliced.sub(dep).value() }, + .tag = .folder, + }; + } + } + + return .{ + .literal = sliced.value(), + .value = .{ .folder = sliced.sub(folder).value() }, + .tag = .folder, + }; + } + + // check for absolute windows paths + if (comptime Environment.isWindows) { + if (protocol == 1 and strings.startsWithWindowsDriveLetter(dependency)) { + return .{ + .literal = sliced.value(), + .value = .{ .folder = sliced.sub(dependency).value() }, + .tag = .folder, + }; + } + + // from npm: + // + // turn /C:/blah info just C:/blah on windows + // https://github.com/npm/cli/blob/fc6e291e9c2154c2e76636cb7ebf0a17be307585/node_modules/npm-package-arg/lib/npa.js#L277 + if (isWindowsAbsPathWithLeadingSlashes(dependency)) |dep| { + return .{ + .literal = sliced.value(), + .value = .{ .folder = sliced.sub(dep).value() }, + .tag = .folder, + }; + } } if (log_) |log| log.addErrorFmt(null, logger.Loc.Empty, allocator, "Unsupported protocol {s}", .{dependency}) catch unreachable; diff --git a/src/install/install.zig b/src/install/install.zig index 26eaf41a92..49c1e43a58 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -1139,6 +1139,7 @@ pub const PackageInstall = struct { opening_dest_dir, copying_files, linking, + linking_dependency, pub fn name(this: Step) []const u8 { return switch (this) { @@ -1146,6 +1147,7 @@ pub const PackageInstall = struct { .opening_cache_dir => "opening cache/package/version dir", .opening_dest_dir => "opening node_modules/package dir", .linking => "linking bins", + .linking_dependency => "linking dependency/workspace to node_modules", }; } }; @@ -2040,7 +2042,7 @@ pub const PackageInstall = struct { const to_path = this.cache_dir.realpath(symlinked_path, &to_buf) catch |err| return Result{ .fail = .{ .err = err, - .step = .linking, + .step = .linking_dependency, }, }; @@ -2052,7 +2054,7 @@ pub const PackageInstall = struct { if (dest_path_length == 0) { const e = bun.windows.Win32Error.get(); const err = if (e.toSystemErrno()) |sys_err| bun.errnoToZigErr(sys_err) else error.Unexpected; - return Result.fail(err, .linking); + return Result.fail(err, .linking_dependency); } var i: usize = dest_path_length; @@ -2069,7 +2071,7 @@ pub const PackageInstall = struct { const fullpath = wbuf[0..i :0]; _ = node_fs_for_package_installer.mkdirRecursiveOSPathImpl(void, {}, fullpath, 0, false).unwrap() catch |err| { - return Result.fail(err, .linking); + return Result.fail(err, .linking_dependency); }; } @@ -2104,7 +2106,7 @@ pub const PackageInstall = struct { return Result{ .fail = .{ .err = bun.errnoToZigErr(err.errno), - .step = .linking, + .step = .linking_dependency, }, }; }, @@ -2115,7 +2117,7 @@ pub const PackageInstall = struct { break :brk bun.MakePath.makeOpenPath(destination_dir, dir, .{}) catch |err| return Result{ .fail = .{ .err = err, - .step = .linking, + .step = .linking_dependency, }, }; } else destination_dir; @@ -2126,7 +2128,7 @@ pub const PackageInstall = struct { const dest_dir_path = bun.getFdPath(dest_dir.fd, &dest_buf) catch |err| return Result{ .fail = .{ .err = err, - .step = .linking, + .step = .linking_dependency, }, }; @@ -2134,7 +2136,7 @@ pub const PackageInstall = struct { std.os.symlinkat(target, dest_dir.fd, dest) catch |err| return Result{ .fail = .{ .err = err, - .step = .linking, + .step = .linking_dependency, }, }; } @@ -2142,7 +2144,7 @@ pub const PackageInstall = struct { if (isDanglingSymlink(symlinked_path)) return Result{ .fail = .{ .err = error.DanglingSymlink, - .step = .linking, + .step = .linking_dependency, }, }; diff --git a/src/install/lockfile.zig b/src/install/lockfile.zig index f487e03892..27191a78e2 100644 --- a/src/install/lockfile.zig +++ b/src/install/lockfile.zig @@ -409,7 +409,7 @@ pub const Tree = struct { resolution_lists: []const Lockfile.DependencyIDSlice, queue: Lockfile.TreeFiller, log: *logger.Log, - old_lockfile: *Lockfile, + lockfile: *Lockfile, prefer_dev_dependencies: bool = false, pub fn maybeReportError(this: *Builder, comptime fmt: string, args: anytype) void { @@ -417,15 +417,15 @@ pub const Tree = struct { } pub fn buf(this: *const Builder) []const u8 { - return this.old_lockfile.buffers.string_bytes.items; + return this.lockfile.buffers.string_bytes.items; } pub fn packageName(this: *Builder, id: PackageID) String.Formatter { - return this.old_lockfile.packages.items(.name)[id].fmt(this.old_lockfile.buffers.string_bytes.items); + return this.lockfile.packages.items(.name)[id].fmt(this.lockfile.buffers.string_bytes.items); } pub fn packageVersion(this: *Builder, id: PackageID) Resolution.Formatter { - return this.old_lockfile.packages.items(.resolution)[id].fmt(this.old_lockfile.buffers.string_bytes.items, .auto); + return this.lockfile.packages.items(.resolution)[id].fmt(this.lockfile.buffers.string_bytes.items, .auto); } pub const Entry = struct { @@ -957,7 +957,7 @@ const Cloner = struct { } if (this.lockfile.buffers.dependencies.items.len > 0) - try this.hoist(); + try this.hoist(this.lockfile); // capacity is used for calculating byte size // so we need to make sure it's exact @@ -965,20 +965,20 @@ const Cloner = struct { this.lockfile.packages.shrinkAndFree(this.lockfile.allocator, this.lockfile.packages.len); } - fn hoist(this: *Cloner) anyerror!void { - if (this.lockfile.packages.len == 0) return; + fn hoist(this: *Cloner, lockfile: *Lockfile) anyerror!void { + if (lockfile.packages.len == 0) return; - const allocator = this.lockfile.allocator; - var slice = this.lockfile.packages.slice(); + const allocator = lockfile.allocator; + var slice = lockfile.packages.slice(); var builder = Tree.Builder{ .name_hashes = slice.items(.name_hash), .queue = TreeFiller.init(allocator), .resolution_lists = slice.items(.resolutions), - .resolutions = this.lockfile.buffers.resolutions.items, + .resolutions = lockfile.buffers.resolutions.items, .allocator = allocator, - .dependencies = this.lockfile.buffers.dependencies.items, + .dependencies = lockfile.buffers.dependencies.items, .log = this.log, - .old_lockfile = this.old, + .lockfile = lockfile, .prefer_dev_dependencies = PackageManager.instance.options.local_package_features.dev_dependencies, }; @@ -988,10 +988,10 @@ const Cloner = struct { try builder.list.items(.tree)[item.tree_id].processSubtree(item.dependency_id, &builder); } - this.lockfile.buffers.hoisted_dependencies = try builder.clean(); + lockfile.buffers.hoisted_dependencies = try builder.clean(); { const final = builder.list.items(.tree); - this.lockfile.buffers.trees = .{ + lockfile.buffers.trees = .{ .items = final, .capacity = final.len, }; diff --git a/src/string_immutable.zig b/src/string_immutable.zig index 5cc94802f2..0d114024c8 100644 --- a/src/string_immutable.zig +++ b/src/string_immutable.zig @@ -5084,7 +5084,7 @@ pub inline fn charIsAnySlash(char: u8) bool { } pub inline fn startsWithWindowsDriveLetter(s: []const u8) bool { - return s.len >= 2 and s[0] == ':' and switch (s[1]) { + return s.len > 2 and s[1] == ':' and switch (s[0]) { 'a'...'z', 'A'...'Z' => true, else => false, }; diff --git a/test/cli/install/bun-add.test.ts b/test/cli/install/bun-add.test.ts index 3f46fc84a1..f6e4a7d88a 100644 --- a/test/cli/install/bun-add.test.ts +++ b/test/cli/install/bun-add.test.ts @@ -131,49 +131,58 @@ it("should reject missing package", async () => { ); }); -it.each(["file://", "file:/", "file:"])("should accept file protocol with prefix %s", async protocolPrefix => { - await writeFile( - join(add_dir, "package.json"), - JSON.stringify({ - name: "foo", - version: "1.2.3", - }), - ); - await writeFile( - join(package_dir, "package.json"), - JSON.stringify({ - name: "bar", - version: "2.3.4", - }), - ); - const add_path = relative(package_dir, add_dir); - const dep = `${protocolPrefix}${add_path.replace(/\\/g, "/")}`; - const { stdout, stderr, exited } = spawn({ - cmd: [bunExe(), "add", dep], - cwd: package_dir, - stdout: "pipe", - stdin: "pipe", - stderr: "pipe", - env, - }); +for (const pathType of ["absolute", "relative"]) { + it.each(["file:///", "file://", "file:/", "file:", "", "//////"])( + `should accept ${pathType} file protocol with prefix "%s"`, + async protocolPrefix => { + await writeFile( + join(add_dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "1.2.3", + }), + ); + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ + name: "bar", + version: "2.3.4", + }), + ); - expect(stderr).toBeDefined(); - const err = await new Response(stderr).text(); - expect(err).not.toContain("error:"); - expect(err).not.toContain("panic:"); - expect(err).toContain("Saved lockfile"); - expect(stdout).toBeDefined(); - const out = await new Response(stdout).text(); - expect(out.replace(/\s*\[[0-9\.]+m?s\]\s*$/, "").split(/\r?\n/)).toEqual([ - "", - ` installed foo@${add_path.replace(/\\/g, "/")}`, - "", - " 1 package installed", - ]); - expect(await exited).toBe(0); -}); + const add_path_rel = relative(package_dir, add_dir); + const add_path_abs = add_dir; -it.each(["fileblah://", "file:///"])("should reject invalid path without segfault: %s", async protocolPrefix => { + const add_dep = `${protocolPrefix}${pathType == "relative" && protocolPrefix != "//////" ? add_path_rel : add_path_abs}`; + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "add", add_dep], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + + expect(stderr).toBeDefined(); + const err = await new Response(stderr).text(); + expect(err).not.toContain("error:"); + expect(err).not.toContain("panic:"); + expect(err).toContain("Saved lockfile"); + expect(stdout).toBeDefined(); + const out = await new Response(stdout).text(); + expect(out.replace(/\s*\[[0-9\.]+m?s\]\s*$/, "").split(/\r?\n/)).toEqual([ + "", + ` installed foo@${add_path_rel.replace(/\\/g, "/")}`, + "", + " 1 package installed", + ]); + expect(await exited).toBe(0); + }, + ); +} + +it.each(["fileblah://"])("should reject invalid path without segfault: %s", async protocolPrefix => { await writeFile( join(add_dir, "package.json"), JSON.stringify({