From df83028546bbeea5c37a4ae2701a7d5b123d3241 Mon Sep 17 00:00:00 2001 From: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> Date: Fri, 24 May 2024 00:19:56 -0700 Subject: [PATCH] fix(install): npm lockfile migration bugfix (#11311) --- src/install/install.zig | 17 ------- src/install/lockfile.zig | 47 +++++++++---------- .../lockfile-with-workspaces/.gitignore | 1 + .../package-lock.json | 36 ++++++++++++++ .../lockfile-with-workspaces/package.json | 9 ++++ .../packages/pkg0/package.json | 3 ++ .../packages/pkg1/package.json | 3 ++ .../packages/pkg2/package.json | 3 ++ .../packages/pkg3/package.json | 3 ++ test/cli/install/migration/migrate.test.ts | 20 ++++++++ 10 files changed, 100 insertions(+), 42 deletions(-) create mode 100644 test/cli/install/migration/lockfile-with-workspaces/.gitignore create mode 100644 test/cli/install/migration/lockfile-with-workspaces/package-lock.json create mode 100644 test/cli/install/migration/lockfile-with-workspaces/package.json create mode 100644 test/cli/install/migration/lockfile-with-workspaces/packages/pkg0/package.json create mode 100644 test/cli/install/migration/lockfile-with-workspaces/packages/pkg1/package.json create mode 100644 test/cli/install/migration/lockfile-with-workspaces/packages/pkg2/package.json create mode 100644 test/cli/install/migration/lockfile-with-workspaces/packages/pkg3/package.json diff --git a/src/install/install.zig b/src/install/install.zig index 0e9afecb4820bd..7c08f32d628046 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -2432,9 +2432,6 @@ pub const PackageManager = struct { cpu_count: u32 = 0, package_json_updates: []UpdateRequest = &[_]UpdateRequest{}, - // used for looking up workspaces that aren't loaded into Lockfile.workspace_paths - workspaces: std.StringArrayHashMap(Semver.Version), - // progress bar stuff when not stack allocated root_progress_node: *std.Progress.Node = undefined, @@ -7443,18 +7440,6 @@ pub const PackageManager = struct { Output.flush(); } - var workspaces = std.StringArrayHashMap(Semver.Version).init(ctx.allocator); - for (workspace_names.values()) |entry| { - if (entry.version) |version_string| { - const sliced_version = SlicedString.init(version_string, version_string); - const result = Semver.Version.parse(sliced_version); - if (result.valid and result.wildcard == .none) { - try workspaces.put(entry.name, result.version.min()); - continue; - } - } - } - workspace_names.map.deinit(); var manager = &instance; @@ -7474,7 +7459,6 @@ pub const PackageManager = struct { .resolve_tasks = .{}, .lockfile = undefined, .root_package_json_file = root_package_json_file, - .workspaces = workspaces, // .progress .event_loop = .{ .mini = JSC.MiniEventLoop.init(bun.default_allocator), @@ -7578,7 +7562,6 @@ pub const PackageManager = struct { .event_loop = .{ .js = JSC.VirtualMachine.get().eventLoop(), }, - .workspaces = std.StringArrayHashMap(Semver.Version).init(allocator), .original_package_json_path = original_package_json_path[0..original_package_json_path.len :0], }; manager.lockfile = try allocator.create(Lockfile); diff --git a/src/install/lockfile.zig b/src/install/lockfile.zig index f3560caab04142..d84b253b0c2e2d 100644 --- a/src/install/lockfile.zig +++ b/src/install/lockfile.zig @@ -3433,7 +3433,6 @@ pub const Package = extern struct { const from_deps = from.dependencies.get(from_lockfile.buffers.dependencies.items); const from_resolutions = from.resolutions.get(from_lockfile.buffers.resolutions.items); var to_i: usize = 0; - var skipped_workspaces: usize = 0; if (from_lockfile.overrides.map.count() != to_lockfile.overrides.map.count()) { summary.overrides_changed = true; @@ -3576,11 +3575,6 @@ pub const Package = extern struct { if (from_dep.name_hash == to_deps[to_i].name_hash) break :found; } - if (PackageManager.instance.workspaces.contains(from_lockfile.str(&from_dep.name))) { - skipped_workspaces += 1; - continue; - } - // We found a removed dependency! // We don't need to remove it // It will be cleaned up later @@ -3675,7 +3669,7 @@ pub const Package = extern struct { // Use saturating arithmetic here because a migrated // package-lock.json could be out of sync with the package.json, so the // number of from_deps could be greater than to_deps. - summary.add = @truncate((to_deps.len + skipped_workspaces) -| (from_deps.len -| summary.remove)); + summary.add = @truncate((to_deps.len) -| (from_deps.len -| summary.remove)); inline for (Lockfile.Scripts.names) |hook| { if (!@field(to.scripts, hook).eql( @@ -3807,13 +3801,6 @@ pub const Package = extern struct { if (comptime tag == null) { workspace_path = lockfile.workspace_paths.get(name_hash); workspace_version = lockfile.workspace_versions.get(name_hash); - - if (workspace_path == null or workspace_version == null) { - if (PackageManager.instance.workspaces.get(lockfile.str(&external_alias.value))) |_workspace_version| { - workspace_path = external_alias.value; - workspace_version = _workspace_version; - } - } } if (comptime tag != null) { @@ -3911,7 +3898,7 @@ pub const Package = extern struct { dependency_version.value.workspace = path; const workspace_entry = try lockfile.workspace_paths.getOrPut(allocator, name_hash); - const found_matching_workspace = workspace_entry.found_existing or PackageManager.instance.workspaces.contains(lockfile.str(&external_alias.value)); + const found_matching_workspace = workspace_entry.found_existing; if (workspace_version) |ver| { try lockfile.workspace_versions.put(allocator, name_hash, ver); @@ -4130,17 +4117,13 @@ pub const Package = extern struct { continue; } - var abs_package_json_path: stringZ = Path.joinAbsStringBufZ( + const abs_package_json_path: stringZ = Path.joinAbsStringBufZ( source.path.name.dir, filepath_buf, &.{ input_path, "package.json" }, .auto, ); - if (comptime Environment.isWindows) { - abs_package_json_path = Path.normalizeStringZ(abs_package_json_path, true, .posix); - } - const workspace_entry = processWorkspaceName( allocator, json_cache, @@ -4181,16 +4164,26 @@ pub const Package = extern struct { if (workspace_entry.name.len == 0) continue; + const rel_input_path = Path.relativePlatform( + source.path.name.dir, + strings.withoutSuffixComptime(abs_package_json_path, std.fs.path.sep_str ++ "package.json"), + .auto, + true, + ); + if (comptime Environment.isWindows) { + Path.dangerouslyConvertPathToPosixInPlace(u8, @constCast(rel_input_path)); + } + if (string_builder) |builder| { builder.count(workspace_entry.name); - builder.count(input_path); + builder.count(rel_input_path); builder.cap += bun.MAX_PATH_BYTES; if (workspace_entry.version) |version_string| { builder.count(version_string); } } - try workspace_names.insert(input_path, .{ + try workspace_names.insert(rel_input_path, .{ .name = workspace_entry.name, .name_loc = workspace_entry.name_loc, .version = workspace_entry.version, @@ -4299,16 +4292,20 @@ pub const Package = extern struct { const workspace_path: string = Path.relativePlatform( source.path.name.dir, abs_workspace_dir_path, - .posix, + .auto, true, ); - - Path.dangerouslyConvertPathToPosixInPlace(u8, @constCast(workspace_path)); + if (comptime Environment.isWindows) { + Path.dangerouslyConvertPathToPosixInPlace(u8, @constCast(workspace_path)); + } if (string_builder) |builder| { builder.count(workspace_entry.name); builder.count(workspace_path); builder.cap += bun.MAX_PATH_BYTES; + if (workspace_entry.version) |version| { + builder.count(version); + } } try workspace_names.insert(workspace_path, .{ diff --git a/test/cli/install/migration/lockfile-with-workspaces/.gitignore b/test/cli/install/migration/lockfile-with-workspaces/.gitignore new file mode 100644 index 00000000000000..2fe28d55d55ba7 --- /dev/null +++ b/test/cli/install/migration/lockfile-with-workspaces/.gitignore @@ -0,0 +1 @@ +!package-lock.json \ No newline at end of file diff --git a/test/cli/install/migration/lockfile-with-workspaces/package-lock.json b/test/cli/install/migration/lockfile-with-workspaces/package-lock.json new file mode 100644 index 00000000000000..7071305aeef733 --- /dev/null +++ b/test/cli/install/migration/lockfile-with-workspaces/package-lock.json @@ -0,0 +1,36 @@ +{ + "name": "npm-lockfile-test", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "npm-lockfile-test", + "workspaces": [ + "./packages/pkg0", + "packages\\pkg1", + ".//packages/pkg2", + "packages/../packages/pkg3" + ] + }, + "node_modules/pkg0": { + "resolved": "packages/pkg0", + "link": true + }, + "node_modules/pkg1": { + "resolved": "packages/pkg1", + "link": true + }, + "node_modules/pkg2": { + "resolved": "packages/pkg2", + "link": true + }, + "node_modules/pkg3": { + "resolved": "packages/pkg3", + "link": true + }, + "packages/pkg0": {}, + "packages/pkg1": {}, + "packages/pkg2": {}, + "packages/pkg3": {} + } +} diff --git a/test/cli/install/migration/lockfile-with-workspaces/package.json b/test/cli/install/migration/lockfile-with-workspaces/package.json new file mode 100644 index 00000000000000..bf1531b8b22636 --- /dev/null +++ b/test/cli/install/migration/lockfile-with-workspaces/package.json @@ -0,0 +1,9 @@ +{ + "name": "npm-lockfile-test", + "workspaces": [ + "./packages/pkg0", + "packages\\pkg1", + ".//packages/pkg2", + "packages/../packages/pkg3" + ] +} \ No newline at end of file diff --git a/test/cli/install/migration/lockfile-with-workspaces/packages/pkg0/package.json b/test/cli/install/migration/lockfile-with-workspaces/packages/pkg0/package.json new file mode 100644 index 00000000000000..ec5d48b2708002 --- /dev/null +++ b/test/cli/install/migration/lockfile-with-workspaces/packages/pkg0/package.json @@ -0,0 +1,3 @@ +{ + "name": "pkg0" +} \ No newline at end of file diff --git a/test/cli/install/migration/lockfile-with-workspaces/packages/pkg1/package.json b/test/cli/install/migration/lockfile-with-workspaces/packages/pkg1/package.json new file mode 100644 index 00000000000000..6236a51c1b3bee --- /dev/null +++ b/test/cli/install/migration/lockfile-with-workspaces/packages/pkg1/package.json @@ -0,0 +1,3 @@ +{ + "name": "pkg1" +} \ No newline at end of file diff --git a/test/cli/install/migration/lockfile-with-workspaces/packages/pkg2/package.json b/test/cli/install/migration/lockfile-with-workspaces/packages/pkg2/package.json new file mode 100644 index 00000000000000..217c59c83396f1 --- /dev/null +++ b/test/cli/install/migration/lockfile-with-workspaces/packages/pkg2/package.json @@ -0,0 +1,3 @@ +{ + "name": "pkg2" +} \ No newline at end of file diff --git a/test/cli/install/migration/lockfile-with-workspaces/packages/pkg3/package.json b/test/cli/install/migration/lockfile-with-workspaces/packages/pkg3/package.json new file mode 100644 index 00000000000000..a8c735d91072b2 --- /dev/null +++ b/test/cli/install/migration/lockfile-with-workspaces/packages/pkg3/package.json @@ -0,0 +1,3 @@ +{ + "name": "pkg3" +} \ No newline at end of file diff --git a/test/cli/install/migration/migrate.test.ts b/test/cli/install/migration/migrate.test.ts index 96a436f67d2f46..536311c9816671 100644 --- a/test/cli/install/migration/migrate.test.ts +++ b/test/cli/install/migration/migrate.test.ts @@ -77,3 +77,23 @@ test("migrate from npm lockfile that is missing `resolved` properties", async () expect(await Bun.file(join(testDir, "node_modules/lodash/package.json")).json()).toHaveProperty("version", "4.17.21"); expect(exitCode).toBe(0); }); + +test("npm lockfile with relative workspaces", async () => { + const testDir = tmpdirSync(); + fs.cpSync(join(import.meta.dir, "lockfile-with-workspaces"), testDir, { recursive: true }); + const { exitCode, stderr } = Bun.spawnSync([bunExe(), "install"], { + env: bunEnv, + cwd: testDir, + }); + const err = stderr.toString(); + expect(err).toContain("migrated lockfile from package-lock.json"); + + expect(err).not.toContain("InvalidNPMLockfile"); + for (let i = 0; i < 4; i++) { + expect(await Bun.file(join(testDir, "node_modules", "pkg" + i, "package.json")).json()).toEqual({ + "name": "pkg" + i, + }); + } + + expect(exitCode).toBe(0); +});