From a62a62f3a4476518433ef686794bdc1e27754038 Mon Sep 17 00:00:00 2001 From: Zack Radisic <56137411+zackradisic@users.noreply.github.com> Date: Sat, 8 Jun 2024 18:09:17 -0700 Subject: [PATCH] fix #11681 and some other stuff --- src/cli.zig | 8 ++ src/install/install.zig | 82 +++++++++++----- src/install/lockfile.zig | 4 + test/cli/install/bun-install-patch.test.ts | 104 +++++++++++++++++++++ 4 files changed, 174 insertions(+), 24 deletions(-) diff --git a/src/cli.zig b/src/cli.zig index 6f7b39726ec022..af883346a78210 100644 --- a/src/cli.zig +++ b/src/cli.zig @@ -1005,6 +1005,8 @@ pub const HelpCommand = struct { \\ update {s:<16} Update outdated dependencies \\ link [\] Register or link a local npm package \\ unlink Unregister a local npm package + \\ patch \ Prepare a package for patching + \\ patch-commit \ Install a modified package \\ pm \ Additional package management utilities \\ \\ build ./a.ts ./b.jsx Bundle TypeScript & JavaScript into a single file @@ -2293,6 +2295,12 @@ pub const Command = struct { Output.pretty("Usage: bun completions", .{}); Output.flush(); }, + Command.Tag.PatchCommand => { + Install.PackageManager.CommandLineArguments.printHelp(.patch); + }, + Command.Tag.PatchCommitCommand => { + Install.PackageManager.CommandLineArguments.printHelp(.@"patch-commit"); + }, Command.Tag.ExecCommand => { Output.pretty( \\Usage: bun exec \ diff --git a/src/install/install.zig b/src/install/install.zig index 34c8ae93ff111b..4004f028939103 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -1087,7 +1087,6 @@ pub fn NewPackageInstall(comptime kind: PkgInstallKind) type { return strings.eqlLong(repo.resolved.slice(buf), bun_tag_file, true); } - // TODO: patched dependencies pub fn verify( this: *@This(), resolution: *const Resolution, @@ -2717,6 +2716,8 @@ pub const PackageManager = struct { // dependency name -> original version information updating_packages: bun.StringArrayHashMapUnmanaged(PackageUpdateInfo) = .{}, + patched_dependencies_to_remove: std.ArrayHashMapUnmanaged(PackageNameAndVersionHash, void, ArrayIdentityContext.U64, false) = .{}, + pub const PackageUpdateInfo = struct { original_version_literal: string, is_alias: bool, @@ -5569,7 +5570,6 @@ pub const PackageManager = struct { manager.network_resolve_batch = .{}; manager.patch_apply_batch = .{}; manager.patch_calc_hash_batch = .{}; - // TODO probably have to put patch tasks here return count; } @@ -7195,10 +7195,10 @@ pub const PackageManager = struct { if (subcommand == .patch) { // TODO args - } else if (subcommand == .patch_commit) { + } else if (subcommand == .@"patch-commit") { this.patch_features = .{ .commit = .{ - .patches_dir = cli.patch_commit.patches_dir, + .patches_dir = cli.@"patch-commit".patches_dir, }, }; } @@ -8145,7 +8145,7 @@ pub const PackageManager = struct { link, unlink, patch, - patch_commit, + @"patch-commit", }; pub fn init(ctx: Command.Context, comptime subcommand: Subcommand) !*PackageManager { @@ -8634,7 +8634,7 @@ pub const PackageManager = struct { } pub inline fn patchCommit(ctx: Command.Context) !void { - try updatePackageJSONAndInstallCatchError(ctx, .patch_commit); + try updatePackageJSONAndInstallCatchError(ctx, .@"patch-commit"); } pub inline fn update(ctx: Command.Context) !void { @@ -9068,7 +9068,7 @@ pub const PackageManager = struct { concurrent_scripts: ?usize = null, patch: PatchOpts = .{}, - patch_commit: PatchCommitOpts = .{}, + @"patch-commit": PatchCommitOpts = .{}, const PatchOpts = struct { edit_dir: ?[]const u8 = null, @@ -9163,7 +9163,7 @@ pub const PackageManager = struct { // Output.pretty("\n\n" ++ outro_text ++ "\n", .{}); Output.flush(); }, - Subcommand.patch_commit => { + Subcommand.@"patch-commit" => { const intro_text = \\Usage: bun patch-commit \ \\ @@ -9300,7 +9300,7 @@ pub const PackageManager = struct { .link => link_params, .unlink => unlink_params, .patch => patch_params, - .patch_commit => patch_commit_params, + .@"patch-commit" => patch_commit_params, }; var diag = clap.Diagnostic{}; @@ -9338,7 +9338,6 @@ pub const PackageManager = struct { // link and unlink default to not saving, all others default to // saving. - // TODO: I think `bun patch` command goes here if (comptime subcommand == .link or subcommand == .unlink) { cli.no_save = !args.flag("--save"); } else { @@ -9349,8 +9348,8 @@ pub const PackageManager = struct { cli.patch = .{}; } - if (comptime subcommand == .patch_commit) { - cli.patch_commit = .{ + if (comptime subcommand == .@"patch-commit") { + cli.@"patch-commit" = .{ .patches_dir = args.option("--patches-dir") orelse "patches", }; } @@ -9430,7 +9429,7 @@ pub const PackageManager = struct { Global.crash(); } - if (subcommand == .patch_commit and cli.positionals.len < 2) { + if (subcommand == .@"patch-commit" and cli.positionals.len < 2) { Output.errGeneric("Missing pkg folder to patch\n", .{}); Global.crash(); } @@ -9802,7 +9801,7 @@ pub const PackageManager = struct { } } }, - .patch_commit => { + .@"patch-commit" => { _ = manager.lockfile.loadFromDisk( manager, manager.allocator, @@ -10037,10 +10036,10 @@ pub const PackageManager = struct { fn preparePatch(manager: *PackageManager) !void { const @"pkg + maybe version to patch" = manager.options.positionals[1]; const name: []const u8, const version: ?[]const u8 = brk: { - if (std.mem.indexOfScalar(u8, @"pkg + maybe version to patch", '@')) |version_delimiter| { + if (std.mem.indexOfScalar(u8, @"pkg + maybe version to patch"[1..], '@')) |version_delimiter| { break :brk .{ @"pkg + maybe version to patch"[0..version_delimiter], - @"pkg + maybe version to patch"[version_delimiter + 1 ..], + @"pkg + maybe version to patch"[1..][version_delimiter + 1 ..], }; } break :brk .{ @@ -11056,14 +11055,29 @@ pub const PackageManager = struct { break :brk ""; } else std.fmt.bufPrint(&resolution_buf, "{}", .{resolution.fmt(buf, .posix)}) catch unreachable; - const patch_patch, const patch_contents_hash, const patch_name_and_version_hash = brk: { - if (this.manager.lockfile.patched_dependencies.entries.len == 0) break :brk .{ null, null, null }; + if (std.mem.eql(u8, "is-even", name)) { + std.debug.print("HELLO!\n", .{}); + } + + const patch_patch, const patch_contents_hash, const patch_name_and_version_hash, const remove_patch = brk: { + if (this.manager.lockfile.patched_dependencies.entries.len == 0 and this.manager.patched_dependencies_to_remove.entries.len) break :brk .{ null, null, null, false }; var sfb = std.heap.stackFallback(1024, this.lockfile.allocator); const name_and_version = std.fmt.allocPrint(sfb.get(), "{s}@{s}", .{ name, package_version }) catch unreachable; defer sfb.get().free(name_and_version); const name_and_version_hash = String.Builder.stringHash(name_and_version); - const patchdep = this.lockfile.patched_dependencies.get(name_and_version_hash) orelse break :brk .{ null, null, null }; + const patchdep = this.lockfile.patched_dependencies.get(name_and_version_hash) orelse { + const to_remove = this.manager.patched_dependencies_to_remove.contains(name_and_version_hash); + if (to_remove) { + break :brk .{ + null, + null, + name_and_version_hash, + true, + }; + } + break :brk .{ null, null, null, false }; + }; bun.assert(!patchdep.patchfile_hash_is_null); // if (!patchdep.patchfile_hash_is_null) { // this.manager.enqueuePatchTask(PatchTask.newCalcPatchHash(this, package_id, name_and_version_hash, dependency_id, url: string)) @@ -11072,6 +11086,7 @@ pub const PackageManager = struct { patchdep.path.slice(this.lockfile.buffers.string_bytes.items), patchdep.patchfileHash().?, name_and_version_hash, + false, }; }; @@ -11200,7 +11215,7 @@ pub const PackageManager = struct { }, } - const needs_install = this.force_install or this.skip_verify_installed_version_number or !needs_verify or !installer.verify( + const needs_install = this.force_install or this.skip_verify_installed_version_number or !needs_verify or remove_patch or !installer.verify( resolution, buf, this.root_node_modules_folder, @@ -11208,7 +11223,7 @@ pub const PackageManager = struct { this.summary.skipped += @intFromBool(!needs_install); if (needs_install) { - if (resolution.tag.canEnqueueInstallTask() and installer.packageMissingFromCache(this.manager, package_id)) { + if (!remove_patch and resolution.tag.canEnqueueInstallTask() and installer.packageMissingFromCache(this.manager, package_id)) { if (comptime Environment.allow_assert) { bun.assert(resolution.canEnqueueInstallTask()); } @@ -11292,7 +11307,7 @@ pub const PackageManager = struct { // above checks if unpatched package is in cache, if not null apply patch in temp directory, copy // into cache, then install into node_modules if (!installer.patch.isNull()) { - if (installer.patchedPackageMissingFromCache(this.manager, package_id, installer.patch.patch_contents_hash)) { + if (installer.patchedPackageMissingFromCache(this.manager, package_id)) { const task = PatchTask.newApplyPatchHash( this.manager, package_id, @@ -11337,7 +11352,7 @@ pub const PackageManager = struct { const install_result = switch (resolution.tag) { .symlink, .workspace => installer.installFromLink(this.skip_delete, destination_dir), - else => installer.install(this.skip_delete, destination_dir), + else => installer.install(this.skip_delete and (!remove_patch), destination_dir), }; switch (install_result) { @@ -12568,7 +12583,6 @@ pub const PackageManager = struct { // Update patched dependencies { var iter = lockfile.patched_dependencies.iterator(); - // TODO: if one key is present in manager.lockfile and not present in lockfile we should get rid of it while (iter.next()) |entry| { const pkg_name_and_version_hash = entry.key_ptr.*; bun.debugAssert(entry.value_ptr.patchfile_hash_is_null); @@ -12587,6 +12601,26 @@ pub const PackageManager = struct { gop.value_ptr.setPatchfileHash(null); } } + + var count: usize = 0; + iter = manager.lockfile.patched_dependencies.iterator(); + while (iter.next()) |entry| { + if (!lockfile.patched_dependencies.contains(entry.key_ptr.*)) { + count += 1; + } + } + if (count > 0) { + try manager.patched_dependencies_to_remove.ensureTotalCapacity(manager.allocator, count); + iter = manager.lockfile.patched_dependencies.iterator(); + while (iter.next()) |entry| { + if (!lockfile.patched_dependencies.contains(entry.key_ptr.*)) { + try manager.patched_dependencies_to_remove.put(manager.allocator, entry.key_ptr.*, {}); + } + } + for (manager.patched_dependencies_to_remove.keys()) |hash| { + _ = manager.lockfile.patched_dependencies.orderedRemove(hash); + } + } } builder.clamp(); diff --git a/src/install/lockfile.zig b/src/install/lockfile.zig index 4a48b3e03d6e01..bc93559701d0ee 100644 --- a/src/install/lockfile.zig +++ b/src/install/lockfile.zig @@ -3775,6 +3775,10 @@ pub const Package = extern struct { )) break :patched_dependencies_changed true; } else break :patched_dependencies_changed true; } + iter = from_lockfile.patched_dependencies.iterator(); + while (iter.next()) |entry| { + if (!to_lockfile.patched_dependencies.contains(entry.key_ptr.*)) break :patched_dependencies_changed true; + } break :patched_dependencies_changed false; }; diff --git a/test/cli/install/bun-install-patch.test.ts b/test/cli/install/bun-install-patch.test.ts index da972c13b73b0c..398e5929b6def4 100644 --- a/test/cli/install/bun-install-patch.test.ts +++ b/test/cli/install/bun-install-patch.test.ts @@ -351,6 +351,58 @@ index c8950c17b265104bcf27f8c345df1a1b13a78950..7ce57ab96400ab0ff4fac7e06f6e02c2 } }); + describe("should work when patches are removed", async () => { + for (const [version, patchVersion_] of versions) { + const patchFilename = filepathEscape(`is-even@${version}.patch`); + const patchVersion = patchVersion_ ?? version; + test(version, async () => { + const filedir = tempDirWithFiles("patch1", { + "package.json": JSON.stringify({ + "name": "bun-patch-test", + "module": "index.ts", + "type": "module", + "patchedDependencies": { + [`is-even@${patchVersion}`]: `patches/${patchFilename}`, + }, + "dependencies": { + "is-even": version, + }, + }), + patches: { + [patchFilename]: is_even_patch2, + }, + "index.ts": /* ts */ `import isEven from 'is-even'; isEven(2); console.log('lol')`, + }); + + console.log("FILEDIR", filedir); + + await $`${bunExe()} i`.env(bunEnv).cwd(filedir); + throw new Error("woopsie!"); + + await $`echo ${JSON.stringify({ + "name": "bun-patch-test", + "module": "index.ts", + "type": "module", + "patchedDependencies": { + [`is-odd@0.1.2`]: `patches/is-odd@0.1.2.patch`, + }, + "dependencies": { + "is-even": version, + }, + })} > package.json` + .env(bunEnv) + .cwd(filedir); + + await $`echo ${is_odd_patch2} > patches/is-odd@0.1.2.patch; ${bunExe()} i`.env(bunEnv).cwd(filedir); + + const { stdout, stderr } = await $`${bunExe()} run index.ts`.env(bunEnv).cwd(filedir); + expect(stderr.toString()).toBe(""); + expect(stdout.toString()).toContain("Hi from isOdd!\n"); + expect(stdout.toString()).not.toContain("lmao\n"); + }); + } + }); + it("should update a transitive dependency when the patchfile changes", async () => { $.throws(true); const filedir = tempDirWithFiles("patch1", { @@ -379,4 +431,56 @@ index c8950c17b265104bcf27f8c345df1a1b13a78950..7ce57ab96400ab0ff4fac7e06f6e02c2 expect(stderr.toString()).toBe(""); expect(stdout.toString()).toContain("lmao\n"); }); + + it("should update a scoped package", async () => { + const patchfile = /* patch */ `diff --git a/private/var/folders/wy/3969rv2x63g63jf8jwlcb2x40000gn/T/.b7f7d77b9ffdd3ee-00000000.tmp/index.js b/index.js +new file mode 100644 +index 0000000000000000000000000000000000000000..6edc0598a84632c41d9c770cfbbad7d99e2ab624 +--- /dev/null ++++ b/index.js +@@ -0,0 +1,4 @@ ++ ++module.exports = () => { ++ return 'PATCHED!' ++} +diff --git a/package.json b/package.json +index aa7c7012cda790676032d1b01d78c0b69ec06360..6048e7cb462b3f9f6ac4dc21aacf9a09397cd4be 100644 +--- a/package.json ++++ b/package.json +@@ -2,7 +2,7 @@ + "name": "@zackradisic/hls-dl", + "version": "0.0.1", + "description": "", +- "main": "dist/hls-dl.commonjs2.js", ++ "main": "./index.js", + "dependencies": { + "m3u8-parser": "^4.5.0", + "typescript": "^4.0.5" +`; + + $.throws(true); + const filedir = tempDirWithFiles("patch1", { + "package.json": JSON.stringify({ + "name": "bun-patch-test", + "module": "index.ts", + "type": "module", + "patchedDependencies": { + "@zackradisic/hls-dl@0.0.1": "patches/thepatch.patch", + }, + "dependencies": { + "@zackradisic/hls-dl": "0.0.1", + }, + }), + patches: { + ["thepatch.patch"]: patchfile, + }, + "index.ts": /* ts */ `import hlsDl from '@zackradisic/hls-dl'; console.log(hlsDl())`, + }); + + await $`${bunExe()} i`.env(bunEnv).cwd(filedir); + + const { stdout, stderr } = await $`${bunExe()} run index.ts`.env(bunEnv).cwd(filedir); + expect(stderr.toString()).toBe(""); + expect(stdout.toString()).toContain("PATCHED!\n"); + }); });