From 2272b852ba4d4b9510b37e90cfd955ca0d1195fb Mon Sep 17 00:00:00 2001 From: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> Date: Tue, 17 Dec 2024 19:59:23 -0800 Subject: [PATCH] fix(install): npm version to git resolution `package-lock.json` migration (#15810) --- src/install/bun.lock.zig | 4 +- src/install/install.zig | 128 ++++++++++---- src/install/lockfile.zig | 5 +- src/install/migration.zig | 163 +++++++----------- src/install/semver.zig | 37 ++-- src/string_immutable.zig | 5 +- .../__snapshots__/bun-install.test.ts.snap | 2 + test/cli/install/bun-install.test.ts | 76 ++++++-- test/cli/install/bun-link.test.ts | 42 ++--- test/cli/install/migration/migrate.test.ts | 18 ++ .../npm-version-to-git-resolution/.gitignore | 1 + .../package-lock.json | 20 +++ .../package.json | 7 + 13 files changed, 315 insertions(+), 193 deletions(-) create mode 100644 test/cli/install/migration/npm-version-to-git-resolution/.gitignore create mode 100644 test/cli/install/migration/npm-version-to-git-resolution/package-lock.json create mode 100644 test/cli/install/migration/npm-version-to-git-resolution/package.json diff --git a/src/install/bun.lock.zig b/src/install/bun.lock.zig index 3d24d716f6..4a18b12e64 100644 --- a/src/install/bun.lock.zig +++ b/src/install/bun.lock.zig @@ -1053,7 +1053,7 @@ pub fn parseIntoBinaryLockfile( return error.InvalidLockfileVersion; }; - var string_buf = String.Buf.init(allocator); + var string_buf = lockfile.stringBuf(); if (root.get("trustedDependencies")) |trusted_dependencies_expr| { var trusted_dependencies: BinaryLockfile.TrustedDependenciesSet = .{}; @@ -1477,8 +1477,6 @@ pub fn parseIntoBinaryLockfile( lockfile.buffers.resolutions.expandToCapacity(); @memset(lockfile.buffers.resolutions.items, invalid_package_id); - string_buf.apply(lockfile); - const pkgs = lockfile.packages.slice(); const pkg_deps = pkgs.items(.dependencies); var pkg_metas = pkgs.items(.meta); diff --git a/src/install/install.zig b/src/install/install.zig index dd04181106..efbe502056 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -946,6 +946,8 @@ pub const Task = struct { name: strings.StringOrTinyString, url: strings.StringOrTinyString, env: DotEnv.Map, + dep_id: DependencyID, + res: Resolution, }, git_checkout: struct { repo_dir: bun.FileDescriptor, @@ -4838,7 +4840,9 @@ pub const PackageManager = struct { task_id: u64, name: string, repository: *const Repository, + dep_id: DependencyID, dependency: *const Dependency, + res: *const Resolution, /// if patched then we need to do apply step after network task is done patch_name_and_version_hash: ?u64, ) *ThreadPool.Task { @@ -4860,6 +4864,8 @@ pub const PackageManager = struct { FileSystem.FilenameStore.instance, ) catch unreachable, .env = Repository.shared_env.get(this.allocator, this.env), + .dep_id = dep_id, + .res = res.*, }, }, .id = task_id, @@ -5452,7 +5458,7 @@ pub const PackageManager = struct { if (this.hasCreatedNetworkTask(clone_id, dependency.behavior.isRequired())) return; - this.task_batch.push(ThreadPool.Batch.from(this.enqueueGitClone(clone_id, alias, dep, dependency, null))); + this.task_batch.push(ThreadPool.Batch.from(this.enqueueGitClone(clone_id, alias, dep, id, dependency, &res, null))); } }, .github => { @@ -6754,13 +6760,20 @@ pub const PackageManager = struct { manager.extracted_count += 1; bun.Analytics.Features.extracted_packages += 1; - // GitHub and tarball URL dependencies are not fully resolved until after the tarball is downloaded & extracted. - if (manager.processExtractedTarballPackage(&package_id, dependency_id, resolution, &task.data.extract, comptime log_level)) |pkg| brk: { + if (comptime @TypeOf(callbacks.onExtract) != void and ExtractCompletionContext == *PackageInstaller) { + extract_ctx.fixCachedLockfilePackageSlices(); + callbacks.onExtract( + extract_ctx, + dependency_id, + &task.data.extract, + log_level, + ); + } else if (manager.processExtractedTarballPackage(&package_id, dependency_id, resolution, &task.data.extract, log_level)) |pkg| handle_pkg: { // In the middle of an install, you could end up needing to downlaod the github tarball for a dependency // We need to make sure we resolve the dependencies first before calling the onExtract callback // TODO: move this into a separate function var any_root = false; - var dependency_list_entry = manager.task_queue.getEntry(task.id) orelse break :brk; + var dependency_list_entry = manager.task_queue.getEntry(task.id) orelse break :handle_pkg; var dependency_list = dependency_list_entry.value_ptr.*; dependency_list_entry.value_ptr.* = .{}; @@ -6812,13 +6825,8 @@ pub const PackageManager = struct { manager.setPreinstallState(package_id, manager.lockfile, .done); - // if (task.tag == .extract and task.request.extract.network.apply_patch_task != null) { - // manager.enqueuePatchTask(task.request.extract.network.apply_patch_task.?); - // } else - if (comptime @TypeOf(callbacks.onExtract) != void) { - if (ExtractCompletionContext == *PackageInstaller) { - extract_ctx.fixCachedLockfilePackageSlices(); - } + if (comptime @TypeOf(callbacks.onExtract) != void and ExtractCompletionContext != *PackageInstaller) { + // handled *PackageInstaller above callbacks.onExtract(extract_ctx, dependency_id, &task.data.extract, comptime log_level); } @@ -6831,10 +6839,11 @@ pub const PackageManager = struct { }, .git_clone => { const clone = &task.request.git_clone; + const repo_fd = task.data.git_clone; const name = clone.name.slice(); const url = clone.url.slice(); - manager.git_repositories.put(manager.allocator, task.id, task.data.git_clone) catch unreachable; + manager.git_repositories.put(manager.allocator, task.id, repo_fd) catch unreachable; if (task.status == .fail) { const err = task.err orelse error.Failed; @@ -6861,11 +6870,49 @@ pub const PackageManager = struct { continue; } - const dependency_list_entry = manager.task_queue.getEntry(task.id).?; - const dependency_list = dependency_list_entry.value_ptr.*; - dependency_list_entry.value_ptr.* = .{}; + if (comptime @TypeOf(callbacks.onExtract) != void and ExtractCompletionContext == *PackageInstaller) { + // Installing! + // this dependency might be something other than a git dependency! only need the name and + // behavior, use the resolution from the task. + const dep_id = clone.dep_id; + const dep = manager.lockfile.buffers.dependencies.items[dep_id]; + const dep_name = dep.name.slice(manager.lockfile.buffers.string_bytes.items); - try manager.processDependencyList(dependency_list, ExtractCompletionContext, extract_ctx, callbacks, install_peer); + const git = clone.res.value.git; + const committish = git.committish.slice(manager.lockfile.buffers.string_bytes.items); + const repo = git.repo.slice(manager.lockfile.buffers.string_bytes.items); + + const resolved = try Repository.findCommit( + manager.allocator, + manager.env, + manager.log, + task.data.git_clone.asDir(), + dep_name, + committish, + task.id, + ); + + const checkout_id = Task.Id.forGitCheckout(repo, resolved); + + if (manager.hasCreatedNetworkTask(checkout_id, dep.behavior.isRequired())) continue; + + manager.task_batch.push(ThreadPool.Batch.from(manager.enqueueGitCheckout( + checkout_id, + repo_fd, + dep_id, + dep_name, + clone.res, + resolved, + null, + ))); + } else { + // Resolving! + const dependency_list_entry = manager.task_queue.getEntry(task.id).?; + const dependency_list = dependency_list_entry.value_ptr.*; + dependency_list_entry.value_ptr.* = .{}; + + try manager.processDependencyList(dependency_list, ExtractCompletionContext, extract_ctx, callbacks, install_peer); + } if (comptime log_level.showProgress()) { if (!has_updated_this_run) { @@ -6897,15 +6944,29 @@ pub const PackageManager = struct { continue; } - if (manager.processExtractedTarballPackage( + if (comptime @TypeOf(callbacks.onExtract) != void and ExtractCompletionContext == *PackageInstaller) { + // We've populated the cache, package already exists in memory. Call the package installer callback + // and don't enqueue dependencies + + // TODO(dylan-conway) most likely don't need to call this now that the package isn't appended, but + // keeping just in case for now + extract_ctx.fixCachedLockfilePackageSlices(); + + callbacks.onExtract( + extract_ctx, + git_checkout.dependency_id, + &task.data.git_checkout, + log_level, + ); + } else if (manager.processExtractedTarballPackage( &package_id, git_checkout.dependency_id, resolution, &task.data.git_checkout, - comptime log_level, - )) |pkg| brk: { + log_level, + )) |pkg| handle_pkg: { var any_root = false; - var dependency_list_entry = manager.task_queue.getEntry(task.id) orelse break :brk; + var dependency_list_entry = manager.task_queue.getEntry(task.id) orelse break :handle_pkg; var dependency_list = dependency_list_entry.value_ptr.*; dependency_list_entry.value_ptr.* = .{}; @@ -6932,18 +6993,15 @@ pub const PackageManager = struct { }, } } - } - if (comptime @TypeOf(callbacks.onExtract) != void) { - if (ExtractCompletionContext == *PackageInstaller) { - extract_ctx.fixCachedLockfilePackageSlices(); + if (comptime @TypeOf(callbacks.onExtract) != void) { + callbacks.onExtract( + extract_ctx, + git_checkout.dependency_id, + &task.data.git_checkout, + comptime log_level, + ); } - callbacks.onExtract( - extract_ctx, - git_checkout.dependency_id, - &task.data.git_checkout, - comptime log_level, - ); } if (comptime log_level.showProgress()) { @@ -13517,7 +13575,15 @@ pub const PackageManager = struct { if (clone_queue.found_existing) return; - this.task_batch.push(ThreadPool.Batch.from(this.enqueueGitClone(clone_id, alias, repository, &this.lockfile.buffers.dependencies.items[dependency_id], null))); + this.task_batch.push(ThreadPool.Batch.from(this.enqueueGitClone( + clone_id, + alias, + repository, + dependency_id, + &this.lockfile.buffers.dependencies.items[dependency_id], + resolution, + null, + ))); } } diff --git a/src/install/lockfile.zig b/src/install/lockfile.zig index 311411eadf..ae36f49184 100644 --- a/src/install/lockfile.zig +++ b/src/install/lockfile.zig @@ -2538,8 +2538,9 @@ pub inline fn stringBuilder(this: *Lockfile) Lockfile.StringBuilder { pub fn stringBuf(this: *Lockfile) String.Buf { return .{ - .bytes = this.buffers.string_bytes.toManaged(this.allocator), - .pool = this.string_pool, + .bytes = &this.buffers.string_bytes, + .allocator = this.allocator, + .pool = &this.string_pool, }; } diff --git a/src/install/migration.zig b/src/install/migration.zig index 41c8f606dd..5e3688865c 100644 --- a/src/install/migration.zig +++ b/src/install/migration.zig @@ -151,10 +151,6 @@ pub fn migrateNPMLockfile( bun.Analytics.Features.lockfile_migration_from_package_lock += 1; // Count pass - var builder_ = this.stringBuilder(); - var builder = &builder_; - const name = (if (json.get("name")) |expr| expr.asString(allocator) else null) orelse ""; - builder.count(name); var root_package: *E.Object = undefined; var packages_properties = brk: { @@ -198,7 +194,7 @@ pub fn migrateNPMLockfile( json_array, &json_src, wksp.loc, - builder, + null, ); debug("found {d} workspace packages", .{workspace_packages_count}); num_deps += workspace_packages_count; @@ -270,20 +266,6 @@ pub fn migrateNPMLockfile( return error.InvalidNPMLockfile; } num_deps +|= @as(u32, deps.data.e_object.properties.len); - - for (deps.data.e_object.properties.slice()) |dep| { - const dep_name = dep.key.?.asString(allocator).?; - const version_string = dep.value.?.asString(allocator) orelse return error.InvalidNPMLockfile; - - builder.count(dep_name); - builder.count(version_string); - - // If it's a folder or workspace, pessimistically assume we will need a maximum path - switch (Dependency.Version.Tag.infer(version_string)) { - .folder, .workspace => builder.cap += bun.MAX_PATH_BYTES, - else => {}, - } - } } } @@ -291,51 +273,14 @@ pub fn migrateNPMLockfile( if (bin.data != .e_object) return error.InvalidNPMLockfile; switch (bin.data.e_object.properties.len) { 0 => return error.InvalidNPMLockfile, - 1 => { - const first_bin = bin.data.e_object.properties.at(0); - const key = first_bin.key.?.asString(allocator).?; - - const workspace_entry = if (workspace_map) |map| map.map.get(pkg_path) else null; - const is_workspace = workspace_entry != null; - - const pkg_name = if (is_workspace) - workspace_entry.?.name - else if (entry.value.?.get("name")) |set_name| - (set_name.asString(this.allocator) orelse return error.InvalidNPMLockfile) - else - packageNameFromPath(pkg_path); - - if (!strings.eql(key, pkg_name)) { - builder.count(key); - } - builder.count(first_bin.value.?.asString(allocator) orelse return error.InvalidNPMLockfile); - }, + 1 => {}, else => { - for (bin.data.e_object.properties.slice()) |bin_entry| { - builder.count(bin_entry.key.?.asString(allocator).?); - builder.count(bin_entry.value.?.asString(allocator) orelse return error.InvalidNPMLockfile); - } num_extern_strings += @truncate(bin.data.e_object.properties.len * 2); }, } } - if (pkg.get("resolved")) |resolved_expr| { - const resolved = resolved_expr.asString(allocator) orelse return error.InvalidNPMLockfile; - if (strings.hasPrefixComptime(resolved, "file:")) { - builder.count(resolved[5..]); - } else if (strings.hasPrefixComptime(resolved, "git+")) { - builder.count(resolved[4..]); - } else { - builder.count(resolved); - - // this is over-counting but whatever. it would be too hard to determine if the case here - // is an `npm`/`dist_tag` version (the only times this is actually used) - if (pkg.get("version")) |v| if (v.asString(allocator)) |s| { - builder.count(s); - }; - } - } else { + if (pkg.get("resolved") == null) { const version_prop = pkg.get("version"); const pkg_name = packageNameFromPath(pkg_path); if (version_prop != null and pkg_name.len > 0) { @@ -376,10 +321,7 @@ pub fn migrateNPMLockfile( remain = remain[version_str.len..]; remain[0..".tgz".len].* = ".tgz".*; - builder.count(resolved_url); try resolved_urls.put(allocator, pkg_path, resolved_url); - } else { - builder.count(pkg_path); } } } @@ -395,7 +337,10 @@ pub fn migrateNPMLockfile( try this.packages.ensureTotalCapacity(allocator, package_idx); // The package index is overallocated, but we know the upper bound try this.package_index.ensureTotalCapacity(package_idx); - try builder.allocate(); + + // dependency on `resolved`, a dependencies version tag might change, requiring + // new strings to be allocated. + var string_buf = this.stringBuf(); if (workspace_map) |wksp| { try this.workspace_paths.ensureTotalCapacity(allocator, wksp.map.unmanaged.entries.len); @@ -408,7 +353,7 @@ pub fn migrateNPMLockfile( bun.assert(!strings.containsChar(k, '\\')); } - this.workspace_paths.putAssumeCapacity(name_hash, builder.append(String, k)); + this.workspace_paths.putAssumeCapacity(name_hash, try string_buf.append(k)); if (v.version) |version_string| { const sliced_version = Semver.SlicedString.init(version_string, version_string); @@ -446,7 +391,7 @@ pub fn migrateNPMLockfile( // the package name is different. This package doesn't exist // in node_modules, but we still allow packages to resolve to it's // resolution. - path_entry.value_ptr.* = builder.append(String, resolved_str); + path_entry.value_ptr.* = try string_buf.append(resolved_str); if (wksp_entry.version) |version_string| { const sliced_version = Semver.SlicedString.init(version_string, version_string); @@ -489,15 +434,14 @@ pub fn migrateNPMLockfile( // Instead of calling this.appendPackage, manually append // the other function has some checks that will fail since we have not set resolution+dependencies yet. this.packages.appendAssumeCapacity(Lockfile.Package{ - .name = builder.appendWithHash(String, pkg_name, name_hash), + .name = try string_buf.appendWithHash(pkg_name, name_hash), .name_hash = name_hash, // For non workspace packages these are set to .uninitialized, then in the third phase // they are resolved. This is because the resolution uses the dependant's version // specifier as a "hint" to resolve the dependency. .resolution = if (is_workspace) Resolution.init(.{ - // This string is counted by `processWorkspaceNamesArray` - .workspace = builder.append(String, pkg_path), + .workspace = try string_buf.append(pkg_path), }) else Resolution{}, // we fill this data in later @@ -571,7 +515,7 @@ pub fn migrateNPMLockfile( break :bin .{ .tag = .file, .value = Bin.Value.init(.{ - .file = builder.append(String, script_value), + .file = try string_buf.append(script_value), }), }; } @@ -580,8 +524,8 @@ pub fn migrateNPMLockfile( .tag = .named_file, .value = Bin.Value.init(.{ .named_file = .{ - builder.append(String, key), - builder.append(String, script_value), + try string_buf.append(key), + try string_buf.append(script_value), }, }), }; @@ -595,8 +539,8 @@ pub fn migrateNPMLockfile( for (bin.data.e_object.properties.slice()) |bin_entry| { const key = bin_entry.key.?.asString(this.allocator) orelse return error.InvalidNPMLockfile; const script_value = bin_entry.value.?.asString(this.allocator) orelse return error.InvalidNPMLockfile; - this.buffers.extern_strings.appendAssumeCapacity(builder.append(ExternalString, key)); - this.buffers.extern_strings.appendAssumeCapacity(builder.append(ExternalString, script_value)); + this.buffers.extern_strings.appendAssumeCapacity(try string_buf.appendExternal(key)); + this.buffers.extern_strings.appendAssumeCapacity(try string_buf.appendExternal(script_value)); } if (Environment.allow_assert) { @@ -719,8 +663,8 @@ pub fn migrateNPMLockfile( for (wksp.keys(), wksp.values()) |key, value| { const entry1 = id_map.get(key) orelse return error.InvalidNPMLockfile; const name_hash = stringHash(value.name); - const wksp_name = builder.append(String, value.name); - const wksp_path = builder.append(String, key); + const wksp_name = try string_buf.append(value.name); + const wksp_path = try string_buf.append(key); dependencies_buf[0] = Dependency{ .name = wksp_name, .name_hash = name_hash, @@ -764,10 +708,10 @@ pub fn migrateNPMLockfile( const version_bytes = prop.value.?.asString(this.allocator) orelse return error.InvalidNPMLockfile; const name_hash = stringHash(name_bytes); - const dep_name = builder.appendWithHash(String, name_bytes, name_hash); + const dep_name = try string_buf.appendWithHash(name_bytes, name_hash); - const dep_version = builder.append(String, version_bytes); - const sliced = dep_version.sliced(this.buffers.string_bytes.items); + const dep_version = try string_buf.append(version_bytes); + const sliced = dep_version.sliced(string_buf.bytes.items); debug("parsing {s}, {s}\n", .{ name_bytes, version_bytes }); const version = Dependency.parse( @@ -841,11 +785,33 @@ pub fn migrateNPMLockfile( if (resolutions[id].tag == .uninitialized) { debug("resolving '{s}'", .{name_bytes}); + var res_version = version; + const res = resolved: { const dep_pkg = packages_properties.at(found.old_json_index).value.?.data.e_object; const dep_resolved: string = dep_resolved: { if (dep_pkg.get("resolved")) |resolved| { - break :dep_resolved resolved.asString(this.allocator) orelse return error.InvalidNPMLockfile; + const dep_resolved = resolved.asString(this.allocator) orelse return error.InvalidNPMLockfile; + switch (Dependency.Version.Tag.infer(dep_resolved)) { + .git, .github => |tag| { + const dep_resolved_str = try string_buf.append(dep_resolved); + const dep_resolved_sliced = dep_resolved_str.sliced(string_buf.bytes.items); + res_version = Dependency.parseWithTag( + this.allocator, + dep_name, + name_hash, + dep_resolved_sliced.slice, + tag, + &dep_resolved_sliced, + log, + manager, + ) orelse return error.InvalidNPMLockfile; + + break :dep_resolved dep_resolved; + }, + // TODO(dylan-conway): might need to handle more cases + else => break :dep_resolved dep_resolved, + } } if (version.tag == .npm) { @@ -855,14 +821,11 @@ pub fn migrateNPMLockfile( } break :resolved Resolution.init(.{ - .folder = builder.append( - String, - packages_properties.at(found.old_json_index).key.?.asString(allocator).?, - ), + .folder = try string_buf.append(packages_properties.at(found.old_json_index).key.?.asString(allocator).?), }); }; - break :resolved switch (version.tag) { + break :resolved switch (res_version.tag) { .uninitialized => std.debug.panic("Version string {s} resolved to `.uninitialized`", .{version_bytes}), .npm, .dist_tag => res: { // It is theoretically possible to hit this in a case where the resolved dependency is NOT @@ -875,25 +838,25 @@ pub fn migrateNPMLockfile( const dep_actual_version = (dep_pkg.get("version") orelse return error.InvalidNPMLockfile) .asString(this.allocator) orelse return error.InvalidNPMLockfile; - const dep_actual_version_str = builder.append(String, dep_actual_version); - const dep_actual_version_sliced = dep_actual_version_str.sliced(this.buffers.string_bytes.items); + const dep_actual_version_str = try string_buf.append(dep_actual_version); + const dep_actual_version_sliced = dep_actual_version_str.sliced(string_buf.bytes.items); break :res Resolution.init(.{ .npm = .{ - .url = builder.append(String, dep_resolved), + .url = try string_buf.append(dep_resolved), .version = Semver.Version.parse(dep_actual_version_sliced).version.min(), }, }); }, .tarball => if (strings.hasPrefixComptime(dep_resolved, "file:")) - Resolution.init(.{ .local_tarball = builder.append(String, dep_resolved[5..]) }) + Resolution.init(.{ .local_tarball = try string_buf.append(dep_resolved[5..]) }) else - Resolution.init(.{ .remote_tarball = builder.append(String, dep_resolved) }), - .folder => Resolution.init(.{ .folder = builder.append(String, dep_resolved) }), + Resolution.init(.{ .remote_tarball = try string_buf.append(dep_resolved) }), + .folder => Resolution.init(.{ .folder = try string_buf.append(dep_resolved) }), // not sure if this is possible to hit - .symlink => Resolution.init(.{ .folder = builder.append(String, dep_resolved) }), + .symlink => Resolution.init(.{ .folder = try string_buf.append(dep_resolved) }), .workspace => workspace: { - var input = builder.append(String, dep_resolved).sliced(this.buffers.string_bytes.items); + var input = (try string_buf.append(dep_resolved)).sliced(string_buf.bytes.items); if (strings.hasPrefixComptime(input.slice, "workspace:")) { input = input.sub(input.slice["workspace:".len..]); } @@ -903,17 +866,17 @@ pub fn migrateNPMLockfile( }, .git => res: { const str = (if (strings.hasPrefixComptime(dep_resolved, "git+")) - builder.append(String, dep_resolved[4..]) + try string_buf.append(dep_resolved[4..]) else - builder.append(String, dep_resolved)) - .sliced(this.buffers.string_bytes.items); + try string_buf.append(dep_resolved)) + .sliced(string_buf.bytes.items); const hash_index = strings.lastIndexOfChar(str.slice, '#') orelse return error.InvalidNPMLockfile; const commit = str.sub(str.slice[hash_index + 1 ..]).value(); break :res Resolution.init(.{ .git = .{ - .owner = version.value.git.owner, + .owner = res_version.value.git.owner, .repo = str.sub(str.slice[0..hash_index]).value(), .committish = commit, .resolved = commit, @@ -923,17 +886,17 @@ pub fn migrateNPMLockfile( }, .github => res: { const str = (if (strings.hasPrefixComptime(dep_resolved, "git+")) - builder.append(String, dep_resolved[4..]) + try string_buf.append(dep_resolved[4..]) else - builder.append(String, dep_resolved)) - .sliced(this.buffers.string_bytes.items); + try string_buf.append(dep_resolved)) + .sliced(string_buf.bytes.items); const hash_index = strings.lastIndexOfChar(str.slice, '#') orelse return error.InvalidNPMLockfile; const commit = str.sub(str.slice[hash_index + 1 ..]).value(); break :res Resolution.init(.{ .git = .{ - .owner = version.value.github.owner, + .owner = res_version.value.github.owner, .repo = str.sub(str.slice[0..hash_index]).value(), .committish = commit, .resolved = commit, @@ -943,7 +906,7 @@ pub fn migrateNPMLockfile( }, }; }; - debug("-> {}", .{res.fmtForDebug(this.buffers.string_bytes.items)}); + debug("-> {}", .{res.fmtForDebug(string_buf.bytes.items)}); resolutions[id] = res; metas[id].origin = switch (res.tag) { diff --git a/src/install/semver.zig b/src/install/semver.zig index 2dbc1a8a6c..e371344c66 100644 --- a/src/install/semver.zig +++ b/src/install/semver.zig @@ -39,21 +39,18 @@ pub const String = extern struct { } pub const Buf = struct { - bytes: std.ArrayList(u8), - pool: Builder.StringPool, + bytes: *std.ArrayListUnmanaged(u8), + allocator: std.mem.Allocator, + pool: *Builder.StringPool, - pub fn init(allocator: std.mem.Allocator) Buf { + pub fn init(lockfile: *const Lockfile) Buf { return .{ - .bytes = std.ArrayList(u8).init(allocator), - .pool = Builder.StringPool.init(allocator), + .bytes = &lockfile.buffers.string_bytes, + .allocator = lockfile.allocator, + .pool = &lockfile.string_pool, }; } - pub fn apply(this: *Buf, lockfile: *Lockfile) void { - lockfile.buffers.string_bytes = this.bytes.moveToUnmanaged(); - lockfile.string_pool = this.pool; - } - pub fn append(this: *Buf, str: string) OOM!String { if (canInline(str)) { return String.initInline(str); @@ -66,7 +63,7 @@ pub const String = extern struct { } // new entry - const new = try String.initAppend(&this.bytes, str); + const new = try String.initAppend(this.allocator, this.bytes, str); entry.value_ptr.* = new; return new; } @@ -82,7 +79,7 @@ pub const String = extern struct { } // new entry - const new = try String.initAppend(&this.bytes, str); + const new = try String.initAppend(this.allocator, this.bytes, str); entry.value_ptr.* = new; return new; } @@ -105,7 +102,7 @@ pub const String = extern struct { }; } - const new = try String.initAppend(&this.bytes, str); + const new = try String.initAppend(this.allocator, this.bytes, str); entry.value_ptr.* = new; return .{ .value = new, @@ -129,7 +126,7 @@ pub const String = extern struct { }; } - const new = try String.initAppend(&this.bytes, str); + const new = try String.initAppend(this.allocator, this.bytes, str); entry.value_ptr.* = new; return .{ .value = new, @@ -332,7 +329,8 @@ pub const String = extern struct { } pub fn initAppendIfNeeded( - buf: *std.ArrayList(u8), + allocator: std.mem.Allocator, + buf: *std.ArrayListUnmanaged(u8), in: string, ) OOM!String { return switch (in.len) { @@ -350,19 +348,20 @@ pub const String = extern struct { // This should only happen for non-ascii strings that are exactly 8 bytes. // so that's an edge-case if ((in[max_inline_len - 1]) >= 128) - try initAppend(buf, in) + try initAppend(allocator, buf, in) else .{ .bytes = .{ in[0], in[1], in[2], in[3], in[4], in[5], in[6], in[7] } }, - else => try initAppend(buf, in), + else => try initAppend(allocator, buf, in), }; } pub fn initAppend( - buf: *std.ArrayList(u8), + allocator: std.mem.Allocator, + buf: *std.ArrayListUnmanaged(u8), in: string, ) OOM!String { - try buf.appendSlice(in); + try buf.appendSlice(allocator, in); const in_buf = buf.items[buf.items.len - in.len ..]; return @bitCast((@as(u64, 0) | @as(u64, @as(max_addressable_space, @truncate(@as(u64, @bitCast(Pointer.init(buf.items, in_buf))))))) | 1 << 63); } diff --git a/src/string_immutable.zig b/src/string_immutable.zig index 286ac906b7..2517480e98 100644 --- a/src/string_immutable.zig +++ b/src/string_immutable.zig @@ -9,6 +9,7 @@ const log = bun.Output.scoped(.STR, true); const js_lexer = @import("./js_lexer.zig"); const grapheme = @import("./grapheme.zig"); const JSC = bun.JSC; +const OOM = bun.OOM; pub const Encoding = enum { ascii, @@ -640,7 +641,7 @@ pub const StringOrTinyString = struct { // allocator.free(slice_); } - pub fn initAppendIfNeeded(stringy: string, comptime Appender: type, appendy: Appender) !StringOrTinyString { + pub fn initAppendIfNeeded(stringy: string, comptime Appender: type, appendy: Appender) OOM!StringOrTinyString { if (stringy.len <= StringOrTinyString.Max) { return StringOrTinyString.init(stringy); } @@ -648,7 +649,7 @@ pub const StringOrTinyString = struct { return StringOrTinyString.init(try appendy.append(string, stringy)); } - pub fn initLowerCaseAppendIfNeeded(stringy: string, comptime Appender: type, appendy: Appender) !StringOrTinyString { + pub fn initLowerCaseAppendIfNeeded(stringy: string, comptime Appender: type, appendy: Appender) OOM!StringOrTinyString { if (stringy.len <= StringOrTinyString.Max) { return StringOrTinyString.initLowerCase(stringy); } diff --git a/test/cli/install/__snapshots__/bun-install.test.ts.snap b/test/cli/install/__snapshots__/bun-install.test.ts.snap index d7119c6524..96d0b00550 100644 --- a/test/cli/install/__snapshots__/bun-install.test.ts.snap +++ b/test/cli/install/__snapshots__/bun-install.test.ts.snap @@ -55,6 +55,8 @@ note: Package name is also declared here " `; +exports[`should handle modified git resolutions in bun.lock 1`] = `"{"lockfileVersion":0,"workspaces":{"":{"dependencies":{"jquery":"3.7.1"}}},"packages":{"jquery":["jquery@git+ssh://git@github.com/dylan-conway/install-test-8.git#3a1288830817d13da39e9231302261896f8721ea",{},"3a1288830817d13da39e9231302261896f8721ea"]}}"`; + exports[`should read install.saveTextLockfile from bunfig.toml 1`] = ` "{ "lockfileVersion": 0, diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index 1b6a44287d..9097867d4f 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -5709,14 +5709,8 @@ it("should handle tarball URL with existing lockfile", async () => { "3 packages installed", ]); expect(await exited2).toBe(0); - expect(urls.sort()).toEqual([ - `${root_url}/bar`, - `${root_url}/bar-0.0.2.tgz`, - `${root_url}/baz`, - `${root_url}/baz-0.0.3.tgz`, - `${root_url}/moo-0.1.0.tgz`, - ]); - expect(requested).toBe(10); + expect(urls.sort()).toEqual([`${root_url}/bar-0.0.2.tgz`, `${root_url}/baz-0.0.3.tgz`, `${root_url}/moo-0.1.0.tgz`]); + expect(requested).toBe(8); expect(await readdirSorted(join(package_dir, "node_modules"))).toEqual([".bin", ".cache", "@barn", "bar", "baz"]); expect(await readdirSorted(join(package_dir, "node_modules", ".bin"))).toHaveBins(["baz-run"]); expect(join(package_dir, "node_modules", ".bin", "baz-run")).toBeValidBin(join("..", "baz", "index.js")); @@ -5851,13 +5845,8 @@ it("should handle tarball path with existing lockfile", async () => { "3 packages installed", ]); expect(await exited2).toBe(0); - expect(urls.sort()).toEqual([ - `${root_url}/bar`, - `${root_url}/bar-0.0.2.tgz`, - `${root_url}/baz`, - `${root_url}/baz-0.0.3.tgz`, - ]); - expect(requested).toBe(8); + expect(urls.sort()).toEqual([`${root_url}/bar-0.0.2.tgz`, `${root_url}/baz-0.0.3.tgz`]); + expect(requested).toBe(6); expect(await readdirSorted(join(package_dir, "node_modules"))).toEqual([".bin", ".cache", "@barn", "bar", "baz"]); expect(await readdirSorted(join(package_dir, "node_modules", ".bin"))).toHaveBins(["baz-run"]); expect(join(package_dir, "node_modules", ".bin", "baz-run")).toBeValidBin(join("..", "baz", "index.js")); @@ -8363,6 +8352,63 @@ cache = false expect(await exited).toBe(0); }); +it("should handle modified git resolutions in bun.lock", async () => { + // install-test-8 has a dependency but because it's not in the lockfile + // it won't be included in the install. + await Promise.all([ + write( + join(package_dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { + "jquery": "3.7.1", + }, + }), + ), + write( + join(package_dir, "bun.lock"), + JSON.stringify({ + "lockfileVersion": 0, + "workspaces": { + "": { + "dependencies": { + "jquery": "3.7.1", + }, + }, + }, + "packages": { + "jquery": [ + "jquery@git+ssh://git@github.com/dylan-conway/install-test-8.git#3a1288830817d13da39e9231302261896f8721ea", + {}, + "3a1288830817d13da39e9231302261896f8721ea", + ], + }, + }), + ), + ]); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: package_dir, + stdout: "pipe", + stderr: "pipe", + env, + }); + + const err = await Bun.readableStreamToText(stderr); + const out = await Bun.readableStreamToText(stdout); + expect(err).not.toContain("Saved lockfile"); + expect(err).not.toContain("error:"); + + expect(out).toContain("1 package installed"); + expect(await exited).toBe(0); + + expect( + (await file(join(package_dir, "bun.lock")).text()).replaceAll(/localhost:\d+/g, "localhost:1234"), + ).toMatchSnapshot(); +}); + it("should read install.saveTextLockfile from bunfig.toml", async () => { await Promise.all([ write( diff --git a/test/cli/install/bun-link.test.ts b/test/cli/install/bun-link.test.ts index 8cdeaa4413..20d2be6439 100644 --- a/test/cli/install/bun-link.test.ts +++ b/test/cli/install/bun-link.test.ts @@ -1,7 +1,7 @@ import { file, spawn } from "bun"; import { afterAll, afterEach, beforeAll, beforeEach, expect, it } from "bun:test"; import { access, mkdir, writeFile } from "fs/promises"; -import { bunExe, bunEnv as env, runBunInstall, tmpdirSync, toBeValidBin, toHaveBins } from "harness"; +import { bunExe, bunEnv as env, runBunInstall, tmpdirSync, toBeValidBin, toHaveBins, stderrForInstall } from "harness"; import { basename, join } from "path"; import { dummyAfterAll, @@ -56,7 +56,7 @@ it("should link and unlink workspace package", async () => { }), ); let { out, err } = await runBunInstall(env, link_dir); - expect(err.split(/\r?\n/)).toEqual(["Saved lockfile", ""]); + expect(err.split(/\r?\n/).slice(-2)).toEqual(["Saved lockfile", ""]); expect(out.replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([ expect.stringContaining("bun install v1."), "", @@ -72,7 +72,7 @@ it("should link and unlink workspace package", async () => { env, }); - err = await new Response(stderr).text(); + err = stderrForInstall(await new Response(stderr).text()); expect(err.split(/\r?\n/)).toEqual([""]); expect(await new Response(stdout).text()).toContain(`Success! Registered "moo"`); expect(await exited).toBe(0); @@ -86,7 +86,7 @@ it("should link and unlink workspace package", async () => { env, })); - err = await new Response(stderr).text(); + err = stderrForInstall(await new Response(stderr).text()); expect(err.split(/\r?\n/)).toEqual([""]); expect((await new Response(stdout).text()).replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([ expect.stringContaining("bun link v1."), @@ -110,7 +110,7 @@ it("should link and unlink workspace package", async () => { env, })); - err = await new Response(stderr).text(); + err = stderrForInstall(await new Response(stderr).text()); expect(err.split(/\r?\n/)).toEqual([""]); expect(await new Response(stdout).text()).toContain(`success: unlinked package "moo"`); expect(await exited).toBe(0); @@ -125,7 +125,7 @@ it("should link and unlink workspace package", async () => { env, })); - err = await new Response(stderr).text(); + err = stderrForInstall(await new Response(stderr).text()); expect(err.split(/\r?\n/)).toEqual([""]); expect(await new Response(stdout).text()).toContain(`Success! Registered "foo"`); expect(await exited).toBe(0); @@ -139,7 +139,7 @@ it("should link and unlink workspace package", async () => { env, })); - err = await new Response(stderr).text(); + err = stderrForInstall(await new Response(stderr).text()); expect(err.split(/\r?\n/)).toEqual([""]); expect((await new Response(stdout).text()).replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([ expect.stringContaining("bun link v1."), @@ -164,7 +164,7 @@ it("should link and unlink workspace package", async () => { env, })); - err = await new Response(stderr).text(); + err = stderrForInstall(await new Response(stderr).text()); expect(err.split(/\r?\n/)).toEqual([""]); expect(await new Response(stdout).text()).toContain(`success: unlinked package "foo"`); expect(await exited).toBe(0); @@ -199,7 +199,7 @@ it("should link package", async () => { stderr: "pipe", env, }); - const err1 = await new Response(stderr1).text(); + const err1 = stderrForInstall(await new Response(stderr1).text()); expect(err1.split(/\r?\n/)).toEqual([""]); expect(await new Response(stdout1).text()).toContain(`Success! Registered "${link_name}"`); expect(await exited1).toBe(0); @@ -216,7 +216,7 @@ it("should link package", async () => { stderr: "pipe", env, }); - const err2 = await new Response(stderr2).text(); + const err2 = stderrForInstall(await new Response(stderr2).text()); expect(err2.split(/\r?\n/)).toEqual([""]); const out2 = await new Response(stdout2).text(); expect(out2.replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([ @@ -240,7 +240,7 @@ it("should link package", async () => { stderr: "pipe", env, }); - const err3 = await new Response(stderr3).text(); + const err3 = stderrForInstall(await new Response(stderr3).text()); expect(err3.split(/\r?\n/)).toEqual([""]); expect(await new Response(stdout3).text()).toContain(`success: unlinked package "${link_name}"`); expect(await exited3).toBe(0); @@ -257,7 +257,7 @@ it("should link package", async () => { stderr: "pipe", env, }); - const err4 = await new Response(stderr4).text(); + const err4 = stderrForInstall(await new Response(stderr4).text()); expect(err4).toContain(`error: Package "${link_name}" is not linked`); expect(await new Response(stdout4).text()).toEqual(expect.stringContaining("bun link v1.")); expect(await exited4).toBe(1); @@ -292,7 +292,7 @@ it("should link scoped package", async () => { stderr: "pipe", env, }); - const err1 = await new Response(stderr1).text(); + const err1 = stderrForInstall(await new Response(stderr1).text()); expect(err1.split(/\r?\n/)).toEqual([""]); expect(await new Response(stdout1).text()).toContain(`Success! Registered "${link_name}"`); expect(await exited1).toBe(0); @@ -309,7 +309,7 @@ it("should link scoped package", async () => { stderr: "pipe", env, }); - const err2 = await new Response(stderr2).text(); + const err2 = stderrForInstall(await new Response(stderr2).text()); expect(err2.split(/\r?\n/)).toEqual([""]); const out2 = await new Response(stdout2).text(); expect(out2.replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([ @@ -333,7 +333,7 @@ it("should link scoped package", async () => { stderr: "pipe", env, }); - const err3 = await new Response(stderr3).text(); + const err3 = stderrForInstall(await new Response(stderr3).text()); expect(err3.split(/\r?\n/)).toEqual([""]); expect(await new Response(stdout3).text()).toContain(`success: unlinked package "${link_name}"`); expect(await exited3).toBe(0); @@ -350,7 +350,7 @@ it("should link scoped package", async () => { stderr: "pipe", env, }); - const err4 = await new Response(stderr4).text(); + const err4 = stderrForInstall(await new Response(stderr4).text()); expect(err4).toContain(`error: Package "${link_name}" is not linked`); expect((await new Response(stdout4).text()).split(/\r?\n/)).toEqual([expect.stringContaining("bun link v1."), ""]); expect(await exited4).toBe(1); @@ -392,14 +392,14 @@ it("should link dependency without crashing", async () => { stderr: "pipe", env, }); - const err1 = await new Response(stderr1).text(); + const err1 = stderrForInstall(await new Response(stderr1).text()); expect(err1.split(/\r?\n/)).toEqual([""]); expect(await new Response(stdout1).text()).toContain(`Success! Registered "${link_name}"`); expect(await exited1).toBe(0); const { out: stdout2, err: stderr2, exited: exited2 } = await runBunInstall(env, package_dir); - const err2 = await new Response(stderr2).text(); - expect(err2.split(/\r?\n/)).toEqual(["Saved lockfile", ""]); + const err2 = stderrForInstall(await new Response(stderr2).text()); + expect(err2.split(/\r?\n/).slice(-2)).toEqual(["Saved lockfile", ""]); const out2 = await new Response(stdout2).text(); expect(out2.replace(/\s*\[[0-9\.]+ms\]\s*$/, "").split(/\r?\n/)).toEqual([ expect.stringContaining("bun install v1."), @@ -429,7 +429,7 @@ it("should link dependency without crashing", async () => { stderr: "pipe", env, }); - const err3 = await new Response(stderr3).text(); + const err3 = stderrForInstall(await new Response(stderr3).text()); expect(err3.split(/\r?\n/)).toEqual([""]); expect(await new Response(stdout3).text()).toContain(`success: unlinked package "${link_name}"`); expect(await exited3).toBe(0); @@ -446,7 +446,7 @@ it("should link dependency without crashing", async () => { stderr: "pipe", env, }); - const err4 = await new Response(stderr4).text(); + const err4 = stderrForInstall(await new Response(stderr4).text()); expect(err4).toContain(`error: FileNotFound installing ${link_name}`); const out4 = await new Response(stdout4).text(); expect(out4.replace(/\[[0-9\.]+m?s\]/, "[]").split(/\r?\n/)).toEqual([ diff --git a/test/cli/install/migration/migrate.test.ts b/test/cli/install/migration/migrate.test.ts index ccd379af61..ea083297a3 100644 --- a/test/cli/install/migration/migrate.test.ts +++ b/test/cli/install/migration/migrate.test.ts @@ -78,6 +78,24 @@ test("migrate package with dependency on root package", async () => { expect(fs.existsSync(join(testDir, "node_modules", "test-pkg", "package.json"))).toBeTrue(); }); +test("migrate package with npm dependency that resolves to a git package", async () => { + const testDir = tmpdirSync(); + + fs.cpSync(join(import.meta.dir, "npm-version-to-git-resolution"), testDir, { recursive: true }); + + const { exitCode } = Bun.spawnSync([bunExe(), "install"], { + env: bunEnv, + cwd: testDir, + stdout: "pipe", + }); + + expect(exitCode).toBe(0); + expect(await Bun.file(join(testDir, "node_modules", "jquery", "package.json")).json()).toHaveProperty( + "name", + "install-test", + ); +}); + test("migrate from npm lockfile that is missing `resolved` properties", async () => { const testDir = tmpdirSync(); diff --git a/test/cli/install/migration/npm-version-to-git-resolution/.gitignore b/test/cli/install/migration/npm-version-to-git-resolution/.gitignore new file mode 100644 index 0000000000..2fe28d55d5 --- /dev/null +++ b/test/cli/install/migration/npm-version-to-git-resolution/.gitignore @@ -0,0 +1 @@ +!package-lock.json \ No newline at end of file diff --git a/test/cli/install/migration/npm-version-to-git-resolution/package-lock.json b/test/cli/install/migration/npm-version-to-git-resolution/package-lock.json new file mode 100644 index 0000000000..dc1ba33747 --- /dev/null +++ b/test/cli/install/migration/npm-version-to-git-resolution/package-lock.json @@ -0,0 +1,20 @@ +{ + "name": "lock", + "version": "0.0.1", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "lock", + "version": "0.0.1", + "dependencies": { + "jquery": "3.7.1" + } + }, + "node_modules/jquery": { + "version": "3.7.1", + "resolved": "git+ssh://git@github.com/dylan-conway/install-test.git#596234dab30564f37adae1e5c4d7123bcffce537", + "license": "MIT" + } + } +} diff --git a/test/cli/install/migration/npm-version-to-git-resolution/package.json b/test/cli/install/migration/npm-version-to-git-resolution/package.json new file mode 100644 index 0000000000..fdcf09b244 --- /dev/null +++ b/test/cli/install/migration/npm-version-to-git-resolution/package.json @@ -0,0 +1,7 @@ +{ + "name": "npm-version-to-git-resolution", + "version": "1.1.1", + "dependencies": { + "jquery": "3.7.1" + } +}