From 9e8bdaba93be49a5eddcde08524a2d710e1cf88a Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 17 Mar 2024 03:42:10 -0700 Subject: [PATCH] Fixes #9433 (#9471) * Fix assertion failure when package-lock.json is out of sync with package.json * Fixes #9433 * Update bun.lockb --------- Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com> --- bun.lockb | Bin 72152 -> 72152 bytes src/install/install.zig | 43 +++- src/install/lockfile.zig | 14 +- .../registry/bun-install-registry.test.ts | 192 ++++++++++++++++++ 4 files changed, 237 insertions(+), 12 deletions(-) diff --git a/bun.lockb b/bun.lockb index 2062b91a58329e4f2b468cc1d58c3acba52a37bf..90d939917cf9f7455f193176ba78ef1b9131515c 100755 GIT binary patch delta 1140 zcmW;ADU6%|07l_#vOzUS>QD_5i6m8QPkJuXliu{EH@)f2Iz%@?jvPsvOoK!sk!00K zBy!}W{h#^r@(z~U2g~h0yE|K3`_azqzcm`i)G;<_9@j~*MeBr4iXGY~bu#SHIi-{1 zfbMCX0$1pTIwg+ipV6ss!r-jV9M>40(^=pKqw_j78W+?tHfUbdNw7t0S0}{|?MNrX z9-Wy^jsv=vbP8OdcUh;z5&bJV6;2pj)tTcO!)rPV++cKFr$!@I$Jn5GLnpx&t(!V2 zc4*(y$*@P~woZ-%x_5L6T%mVYr^FHcdpZ?P7$iD#Tw{1&XMr1x9_Z9)JXFWnp!rBA z!4|E@Iw^K&KhepsN9UPJt`*p6Qf0qW@f{!U=;HI&)lO_)=$q8;oA*)M&g` z$Jn6xMkm1*txPAy4(+!(8TRPB)5&o__q|SmEA&3-lsKaQQK!NQgHJkhTx0lIXMr1x za-ABDFX|W@G{5R3*rN4KC&do!?>ZUw=={*haX|N{PJt`*e(97rqF?A#IAO3i+dq2f L@bd58k>CFSodI2n delta 1140 zcmW;ADU6%|07l_9*`S({)S((A5=m0S_N3=BJ?TwvdefWUtV47Y}(%AuorI6|68GPOdVm3v*S83HfWyENw7uhq)v(*+NX3f z?9n-`ljDGHpi|(8-Wi<|C-l$iEO3p%Ih`eLFg&kQp>aVSVU4qkIx#kA?&u`gq7~|- z*r7ew$*@P~l1`2Tx|ekd9MQX?Q{sgFRhb%Zs}Zs^3=pm|d# z!4|DsIw^K&-`2^nN9T@Cjsv=PbqXBOyQfp)gnq2Ez%>T1}}A%xWVw1PKCy6 zb%Zs}-sr^Gpqc6<*rN4TC&do!cRCsN=)Bj-aX|NjPJttOA9YHc(Ep^fz%>S+b(Xln zFw?2f_@a)m#@Sb$7#lRd=_J^q^<5{$4(%U08TRP>)X8x`_m@tABYL?`i4*#}^Sz^o L4zK?19{K$b`xji$ diff --git a/src/install/install.zig b/src/install/install.zig index 9c7d1cd739..8ca1f839a4 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -5796,9 +5796,15 @@ pub const PackageManager = struct { this.log_level = if (default_disable_progress_bar) LogLevel.default_no_progress else LogLevel.default; PackageManager.verbose_install = false; } + + // If the lockfile is frozen, don't save it to disk. + if (this.enable.frozen_lockfile) { + this.do.save_lockfile = false; + this.enable.force_save_lockfile = false; + } } - pub const Do = struct { + pub const Do = packed struct { save_lockfile: bool = true, load_lockfile: bool = true, install_packages: bool = true, @@ -5812,7 +5818,7 @@ pub const PackageManager = struct { trust_dependencies_from_args: bool = false, }; - pub const Enable = struct { + pub const Enable = packed struct { manifest_cache: bool = true, manifest_cache_control: bool = true, cache: bool = true, @@ -7572,6 +7578,16 @@ pub const PackageManager = struct { cli.positionals = args.positionals(); + if (cli.production and cli.trusted) { + Output.errGeneric("The '--production' and '--trust' flags together are not supported because the --trust flag potentially modifies the lockfile after installing packages\n", .{}); + Global.crash(); + } + + if (cli.frozen_lockfile and cli.trusted) { + Output.errGeneric("The '--frozen-lockfile' and '--trust' flags together are not supported because the --trust flag potentially modifies the lockfile after installing packages\n", .{}); + Global.crash(); + } + return cli; } }; @@ -9958,6 +9974,16 @@ pub const PackageManager = struct { const packages_len_before_install = manager.lockfile.packages.len; + if (manager.options.enable.frozen_lockfile) { + if (manager.lockfile.hasMetaHashChanged(PackageManager.verbose_install or manager.options.do.print_meta_hash_string, packages_len_before_install) catch false) { + if (comptime log_level != .silent) { + Output.prettyErrorln("error: lockfile had changes, but lockfile is frozen", .{}); + Output.note("try re-running without --frozen-lockfile and commit the updated lockfile", .{}); + } + Global.crash(); + } + } + var install_summary = PackageInstall.Summary{}; if (manager.options.do.install_packages) { install_summary = try manager.installPackages( @@ -9966,7 +9992,10 @@ pub const PackageManager = struct { ); } - const did_meta_hash_change = try manager.lockfile.hasMetaHashChanged( + const did_meta_hash_change = + // If the lockfile was frozen, we already checked it + !manager.options.enable.frozen_lockfile and + try manager.lockfile.hasMetaHashChanged( PackageManager.verbose_install or manager.options.do.print_meta_hash_string, @min(packages_len_before_install, manager.lockfile.packages.len), ); @@ -9979,14 +10008,6 @@ pub const PackageManager = struct { manager.package_json_updates.len > 0 or (load_lockfile_result == .ok and load_lockfile_result.ok.serializer_result.packages_need_update); - if (did_meta_hash_change and manager.options.enable.frozen_lockfile) { - if (comptime log_level != .silent) { - Output.prettyErrorln("error: lockfile had changes, but lockfile is frozen", .{}); - Output.note("try re-running without --frozen-lockfile and commit the updated lockfile", .{}); - } - Global.crash(); - } - // It's unnecessary work to re-save the lockfile if there are no changes if (manager.options.do.save_lockfile and (should_save_lockfile or manager.lockfile.isEmpty() or manager.options.enable.force_save_lockfile)) diff --git a/src/install/lockfile.zig b/src/install/lockfile.zig index b555cdc626..fb2f0650bd 100644 --- a/src/install/lockfile.zig +++ b/src/install/lockfile.zig @@ -415,6 +415,7 @@ pub const Tree = struct { queue: Lockfile.TreeFiller, log: *logger.Log, old_lockfile: *Lockfile, + prefer_dev_dependencies: bool = false, pub fn maybeReportError(this: *Builder, comptime fmt: string, args: anytype) void { this.log.addErrorFmt(null, logger.Loc.Empty, this.allocator, fmt, args) catch {}; @@ -559,7 +560,17 @@ pub const Tree = struct { for (this_dependencies) |dep_id| { const dep = builder.dependencies[dep_id]; if (dep.name_hash != dependency.name_hash) continue; - if (builder.resolutions[dep_id] != package_id and !dependency.behavior.isPeer()) { + const mismatch = builder.resolutions[dep_id] != package_id; + + if (mismatch and dep.behavior.isDev() != dependency.behavior.isDev()) { + if (builder.prefer_dev_dependencies and dep.behavior.isDev()) { + return hoisted; // 2 + } + + return dependency_loop; // 3 + } + + if (mismatch and !dependency.behavior.isPeer()) { if (as_defined and !dep.behavior.isPeer()) { builder.maybeReportError("Package \"{}@{}\" has a dependency loop\n Resolution: \"{}@{}\"\n Dependency: \"{}@{}\"", .{ builder.packageName(package_id), @@ -949,6 +960,7 @@ const Cloner = struct { .dependencies = this.lockfile.buffers.dependencies.items, .log = this.log, .old_lockfile = this.old, + .prefer_dev_dependencies = PackageManager.instance.options.local_package_features.dev_dependencies, }; try (Tree{}).processSubtree(Tree.root_dep_id, &builder); diff --git a/test/cli/install/registry/bun-install-registry.test.ts b/test/cli/install/registry/bun-install-registry.test.ts index df78a45ccc..00de84bd17 100644 --- a/test/cli/install/registry/bun-install-registry.test.ts +++ b/test/cli/install/registry/bun-install-registry.test.ts @@ -50,6 +50,198 @@ afterEach(async () => { await rm(packageDir, { force: true, recursive: true }); }); +describe.each(["--production", "without --production"])("%s", flag => { + const prod = flag === "--production"; + const order = ["devDependencies", "dependencies"]; + // const stdio = process.versions.bun.includes("debug") ? "inherit" : "ignore"; + const stdio = "ignore"; + + if (prod) { + test("modifying package.json with --production should not save lockfile", async () => { + await writeFile( + join(packageDir, "package.json"), + JSON.stringify({ + name: "foo", + version: "1.0.0", + dependencies: { + "bin-change-dir": "1.0.0", + }, + devDependencies: { + "bin-change-dir": "1.0.1", + "basic-1": "1.0.0", + }, + }), + ); + + var { exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + stdout: stdio, + stdin: stdio, + stderr: stdio, + env, + }); + + expect(await exited).toBe(0); + + const initialHash = Bun.hash(await file(join(packageDir, "bun.lockb")).arrayBuffer()); + + expect(await file(join(packageDir, "node_modules", "bin-change-dir", "package.json")).json()).toMatchObject({ + name: "bin-change-dir", + version: "1.0.1", + }); + + var { exited } = spawn({ + cmd: [bunExe(), "install", "--production"], + cwd: packageDir, + stdout: stdio, + stdin: stdio, + stderr: stdio, + env, + }); + + expect(await exited).toBe(0); + + expect(await file(join(packageDir, "node_modules", "bin-change-dir", "package.json")).json()).toMatchObject({ + name: "bin-change-dir", + version: "1.0.0", + }); + + var { exited } = spawn({ + cmd: [bunExe(), "install", "--production", "bin-change-dir@1.0.1"], + cwd: packageDir, + stdout: stdio, + stdin: stdio, + stderr: stdio, + env, + }); + + expect(await exited).toBe(1); + + // We should not have saved bun.lockb + expect(Bun.hash(await file(join(packageDir, "bun.lockb")).arrayBuffer())).toBe(initialHash); + + // We should not have installed bin-change-dir@1.0.1 + expect(await file(join(packageDir, "node_modules", "bin-change-dir", "package.json")).json()).toMatchObject({ + name: "bin-change-dir", + version: "1.0.0", + }); + + // This is a no-op. It should work. + var { exited } = spawn({ + cmd: [bunExe(), "install", "--production", "bin-change-dir@1.0.0"], + cwd: packageDir, + stdout: stdio, + stdin: stdio, + stderr: stdio, + env, + }); + + expect(await exited).toBe(0); + + // We should not have saved bun.lockb + expect(Bun.hash(await file(join(packageDir, "bun.lockb")).arrayBuffer())).toBe(initialHash); + + // We should have installed bin-change-dir@1.0.0 + expect(await file(join(packageDir, "node_modules", "bin-change-dir", "package.json")).json()).toMatchObject({ + name: "bin-change-dir", + version: "1.0.0", + }); + }); + } + + test(`should prefer ${order[+prod % 2]} over ${order[1 - (+prod % 2)]}`, async () => { + await writeFile( + join(packageDir, "package.json"), + JSON.stringify({ + name: "foo", + version: "1.0.0", + dependencies: { + "bin-change-dir": "1.0.0", + }, + devDependencies: { + "bin-change-dir": "1.0.1", + "basic-1": "1.0.0", + }, + }), + ); + + let initialLockfileHash; + async function saveWithoutProd() { + var hash; + // First install without --production + // so that the lockfile is up to date + var { exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + stdout: stdio, + stdin: stdio, + stderr: stdio, + env, + }); + expect(await exited).toBe(0); + + await Promise.all([ + (async () => + expect(await file(join(packageDir, "node_modules", "bin-change-dir", "package.json")).json()).toMatchObject({ + name: "bin-change-dir", + version: "1.0.1", + }))(), + (async () => + expect(await file(join(packageDir, "node_modules", "basic-1", "package.json")).json()).toMatchObject({ + name: "basic-1", + version: "1.0.0", + }))().then( + async () => await rm(join(packageDir, "node_modules", "basic-1"), { recursive: true, force: true }), + ), + + (async () => (hash = Bun.hash(await file(join(packageDir, "bun.lockb")).arrayBuffer())))(), + ]); + + return hash; + } + if (prod) { + initialLockfileHash = await saveWithoutProd(); + } + + var { exited } = spawn({ + cmd: [bunExe(), "install", prod ? "--production" : ""].filter(Boolean), + cwd: packageDir, + stdout: stdio, + stdin: stdio, + stderr: stdio, + env, + }); + + expect(await exited).toBe(0); + expect(await file(join(packageDir, "node_modules", "bin-change-dir", "package.json")).json()).toMatchObject({ + name: "bin-change-dir", + version: prod ? "1.0.0" : "1.0.1", + }); + + if (!prod) { + expect(await file(join(packageDir, "node_modules", "basic-1", "package.json")).json()).toMatchObject({ + name: "basic-1", + version: "1.0.0", + }); + } else { + // it should not install devDependencies + expect(await exists(join(packageDir, "node_modules", "basic-1"))).toBeFalse(); + + // it should not mutate the lockfile when there were no changes to begin with. + const newHash = Bun.hash(await file(join(packageDir, "bun.lockb")).arrayBuffer()); + + expect(newHash).toBe(initialLockfileHash!); + } + + if (prod) { + // lets now try to install again without --production + const newHash = await saveWithoutProd(); + expect(newHash).toBe(initialLockfileHash); + } + }); +}); + test("basic 1", async () => { await writeFile( join(packageDir, "package.json"),