diff --git a/src/install/PackageManager/install_with_manager.rs b/src/install/PackageManager/install_with_manager.rs index 3c134c0cc864..bdc903b2f6b9 100644 --- a/src/install/PackageManager/install_with_manager.rs +++ b/src/install/PackageManager/install_with_manager.rs @@ -523,70 +523,26 @@ pub fn install_with_manager( pinned_rows = enqueue_transitive(manager, &transitive, invalidates_rows)?; } - // `enqueueDependencyWithMain` can reach `Lockfile.Package.fromNPM`, - // which grows `buffers.dependencies` and may reallocate it. - // Iterate by index against a snapshot of the original length and - // copy each entry to the stack so neither the loop nor the callee - // ever reads through a pointer into the old backing storage. - if manager.summary.overrides_changed && !all_name_hashes.is_empty() { - let dependencies_len = manager.lockfile.buffers.dependencies.len(); - for dependency_i in 0..dependencies_len { - if pinned_rows.is_set_allow_out_of_bound(dependency_i, false) { - continue; - } - let dependency = - manager.lockfile.buffers.dependencies[dependency_i].clone(); - if all_name_hashes.binary_search(&dependency.name_hash).is_ok() { - manager.lockfile.buffers.resolutions[dependency_i] = - invalid_package_id; - if let Err(err) = enqueue_dependency_with_main( - manager, - dependency_i as u32, - &dependency, - invalid_package_id, - false, - ) { - add_dependency_error(manager, &dependency, err); - } - } - } - } - - if manager.summary.catalogs_changed { + if invalidates_rows { + let catalogs_changed = manager.summary.catalogs_changed; let mut catalog_overridden: Vec = Vec::new(); - manager - .lockfile - .overrides - .append_catalog_valued_name_hashes(&mut catalog_overridden); - catalog_overridden.sort_unstable(); - catalog_overridden.dedup(); - let dependencies_len = manager.lockfile.buffers.dependencies.len(); - for _dep_id in 0..dependencies_len { - let dep_id: DependencyID = u32::try_from(_dep_id).expect("int cast"); - if pinned_rows.is_set_allow_out_of_bound(_dep_id, false) { - continue; - } - let dep = - manager.lockfile.buffers.dependencies[dep_id as usize].clone(); - if dep.version.tag != DependencyVersionTag::Catalog - && (catalog_overridden.is_empty() - || catalog_overridden.binary_search(&dep.name_hash).is_err()) - { - continue; - } - - manager.lockfile.buffers.resolutions[dep_id as usize] = - invalid_package_id; - if let Err(err) = enqueue_dependency_with_main( - manager, - dep_id, - &dep, - invalid_package_id, - false, - ) { - add_dependency_error(manager, &dep, err); - } + if catalogs_changed { + manager + .lockfile + .overrides + .append_catalog_valued_name_hashes(&mut catalog_overridden); + catalog_overridden.sort_unstable(); + catalog_overridden.dedup(); } + // `all_name_hashes` is empty unless the overrides changed. + reresolve_owned_rows(manager, &pinned_rows, |dependency| { + all_name_hashes.binary_search(&dependency.name_hash).is_ok() + || (catalogs_changed + && dependency.version.tag == DependencyVersionTag::Catalog) + || catalog_overridden + .binary_search(&dependency.name_hash) + .is_ok() + }); } // Split this into two passes because the below may allocate memory or invalidate pointers @@ -1577,7 +1533,7 @@ fn report_lockfile_load_error( Ok(()) } -/// Returns the rows the plan re-resolved so the overrides/catalogs invalidation loops that follow leave them pinned; only tracked when those loops will run. +/// Returns the rows the plan re-resolved so the overrides/catalogs invalidation pass that follows leaves them pinned; only tracked when that pass will run. fn enqueue_transitive( manager: &mut PackageManager, transitive: &TransitiveUpdate, @@ -1590,6 +1546,42 @@ fn enqueue_transitive( transitive.enqueue_tracked(manager) } +/// Re-resolves the rows `selects`, walking each package's current dependency list in package order. The root's +/// list (just rebuilt by the differ) goes first because a later row dedupes onto what an earlier one appended +/// (`Lockfile::get_package_id`); the root's loaded rows, now in no list, would resolve as nobody's and are not +/// walked, nor are `pinned_rows`, which the update plan just resolved. A workspace the add/update pass is about +/// to re-read still holds its loaded list here and is walked like any other package. +fn reresolve_owned_rows( + manager: &mut PackageManager, + pinned_rows: &DynamicBitSet, + selects: impl Fn(&Dependency) -> bool, +) { + // Resolving appends packages and rows; only the lists present now are walked. + let lists: Vec = + manager.lockfile.packages.items_dependencies().to_vec(); + for list in lists { + for dep_id in list.begin()..list.end() { + if pinned_rows.is_set_allow_out_of_bound(dep_id as usize, false) { + continue; + } + let dependency = manager.lockfile.buffers.dependencies[dep_id as usize].clone(); + if !selects(&dependency) { + continue; + } + manager.lockfile.buffers.resolutions[dep_id as usize] = invalid_package_id; + if let Err(err) = enqueue_dependency_with_main( + manager, + dep_id, + &dependency, + invalid_package_id, + false, + ) { + add_dependency_error(manager, &dependency, err); + } + } + } +} + #[derive(Default)] struct NamedUpdates { /// Invalidated rows paired with the package they resolved to, for redirect_moved_edges. diff --git a/test/cli/install/catalogs.test.ts b/test/cli/install/catalogs.test.ts index 159c6aabe4aa..88fb656a382f 100644 --- a/test/cli/install/catalogs.test.ts +++ b/test/cli/install/catalogs.test.ts @@ -259,6 +259,54 @@ describe("basic", () => { await runBunInstall(bunEnv, packageDir, { savesLockfile: false }); }); + // A file: path outside the project is only accepted on a row the root (or a workspace) owns. A catalog change + // re-resolves every catalog: row; the rows the root was loaded with have been replaced by then and belong to + // nobody, so re-resolving them fails the way an escaping transitive file: dependency does. + test.concurrent("changing the entry of a catalog: dependency pointing outside the project", async () => { + const packageJson = (xPath: string) => + JSON.stringify({ + name: "catalog-file-dep", + workspaces: { packages: [], catalog: { x: xPath } }, + dependencies: { x: "catalog:" }, + }); + using dir = tempDir("catalog-file-dep", { + "x/package.json": JSON.stringify({ name: "x", version: "1.0.0" }), + "x2/package.json": JSON.stringify({ name: "x", version: "2.0.0" }), + "project/package.json": packageJson("file:../x"), + }); + const packageDir = join(String(dir), "project"); + const installedVersion = async () => + (await file(join(packageDir, "node_modules", "x", "package.json")).json()).version; + + await runBunInstall(bunEnv, packageDir); + expect(await installedVersion()).toBe("1.0.0"); + + await write(join(packageDir, "package.json"), packageJson("file:../x2")); + await runBunInstall(bunEnv, packageDir); + expect(await installedVersion()).toBe("2.0.0"); + expect(normalizeBunSnapshot(await file(join(packageDir, "bun.lock")).text(), packageDir)).toMatchInlineSnapshot(` + "{ + "lockfileVersion": 2, + "configVersion": 1, + "workspaces": { + "": { + "name": "catalog-file-dep", + "dependencies": { + "x": "catalog:", + }, + }, + }, + "catalog": { + "x": "file:../x2", + }, + "packages": { + "x": ["x@file:../x2", {}], + } + }" + `); + await runBunInstall(bunEnv, packageDir, { frozenLockfile: true }); + }); + test.concurrent("catalog and catalogs.default may split different packages between them", async () => { const { packageDir } = await registry.createTestDir({ files: { @@ -1274,6 +1322,65 @@ describe("peer dependencies", () => { expect(await packageKeys(dir)).toStrictEqual(dedupedKeys); }); + // When the catalog changes, every row declared through it is re-resolved. The root's rows from bun.lock have + // been replaced by then and belong to no package; re-resolving them too bound the peer a second time and + // repeated its warning. + describe("a root peer whose catalog range stops matching the installed version is checked once", () => { + const peerWarning = 'warn: incorrect peer dependency "no-deps@1.0.0"'; + const peerWarnings = (err: string) => err.split(peerWarning).length - 1; + + function rootWithPeer(peerSpec: string, fields: Record = {}) { + return JSON.stringify({ name: "root", peerDependencies: { "no-deps": peerSpec }, ...fields }); + } + + async function installedAlone(packageJson: string) { + const { packageDir } = await registry.createTestDir({ + bunfigOpts: { linker: "hoisted" }, + files: { "package.json": packageJson }, + }); + const { err } = await install(packageDir, "hoisted"); + expect(peerWarnings(err)).toBe(0); + expect((await Bun.file(join(packageDir, "node_modules", "no-deps", "package.json")).json()).version).toBe( + "1.0.0", + ); + return packageDir; + } + + async function reinstall(dir: string, packageJson: string) { + await Bun.write(join(dir, "package.json"), packageJson); + const { err } = await install(dir, "hoisted"); + expect(err).toContain("Saved lockfile"); + return peerWarnings(err); + } + + // The inline row is the baseline: it changes itself and is only re-enqueued by the add/update pass. + test.concurrent.each([ + [ + "through the catalog", + (range: string) => rootWithPeer("catalog:", { workspaces: { catalog: { "no-deps": range } } }), + ], + ["inline", (range: string) => rootWithPeer(range)], + ])("declared %s", async (_, packageJson) => { + const dir = await installedAlone(packageJson("1.0.0")); + expect(await reinstall(dir, packageJson("^1.0.1"))).toBe(1); + }); + + test.concurrent("overridden to catalog:", async () => { + const packageJson = (range: string) => + rootWithPeer("1.0.0", { overrides: { "no-deps": "catalog:" }, workspaces: { catalog: { "no-deps": range } } }); + const dir = await installedAlone(packageJson("1.0.0")); + expect(await reinstall(dir, packageJson("^1.0.1"))).toBe(1); + }); + + // Selected both as an overridden name and as a catalog: row; one pass handles both. + test.concurrent("declared through the catalog while an override of the same name changes too", async () => { + const packageJson = (range: string) => + rootWithPeer("catalog:", { overrides: { "no-deps": range }, workspaces: { catalog: { "no-deps": range } } }); + const dir = await installedAlone(packageJson("1.0.0")); + expect(await reinstall(dir, packageJson("^1.0.1"))).toBe(1); + }); + }); + // pnpm: deps-installer/test/catalogs.ts "frozen lockfile error is thrown if catalog config changes" test.concurrent("--frozen-lockfile fails when only a peer's catalog range changed", async () => { const dir = await makeRepo({ catalog: { "no-deps": ">=1.0.0" }, peerSpec: "catalog:", linker: "hoisted" }); diff --git a/test/cli/install/nested-overrides.test.ts b/test/cli/install/nested-overrides.test.ts index 9308eb302438..43526a00c5d0 100644 --- a/test/cli/install/nested-overrides.test.ts +++ b/test/cli/install/nested-overrides.test.ts @@ -2,7 +2,7 @@ import { file, write } from "bun"; import { afterAll, beforeAll, describe, expect, test } from "bun:test"; import { existsSync, realpathSync } from "fs"; import { rm } from "fs/promises"; -import { VerdaccioRegistry, bunEnv, bunExe } from "harness"; +import { VerdaccioRegistry, bunEnv, bunExe, normalizeBunSnapshot, tempDir } from "harness"; import { join } from "path"; const registry = new VerdaccioRegistry(); @@ -1290,6 +1290,125 @@ describe.concurrent("lockfile", () => { await installOk(dir, "--frozen-lockfile"); }); + // An overrides change re-resolves every row naming a previously or newly overridden package. By then the root's + // rows from bun.lock have been replaced by freshly parsed ones and belong to no package; they must not be + // re-resolved as well: a range that is no longer in package.json would add a package the current row then dedupes + // onto, and a file: path outside the project is only accepted on a row the root (or a workspace) owns. The + // root's current rows also have to be resolved before everybody else's, as on a fresh install, since a row + // resolved later dedupes onto a same-major package an earlier one added. + describe.concurrent("removing a flat rule re-resolves only the root's current rows", () => { + test("the root's row resolves before a dependency's row of the same name", async () => { + const deps = { "no-deps": "^1.0.0", "one-dep": "1.0.0" }; // one-dep@1.0.0 depends on no-deps@1.0.1 + const [dir, fresh] = await Promise.all([ + project({ dependencies: deps, overrides: { "no-deps": "2.0.0" } }), + project({ dependencies: deps }), + ]); + await Promise.all([installOk(dir), installOk(fresh)]); + expect(await lock(dir)).toContain("no-deps@2.0.0"); + + await write(join(dir, "package.json"), JSON.stringify({ name: "nested-overrides", dependencies: deps })); + const { err } = await installOk(dir); + expect(err).toContain("Saved lockfile"); + expect(await versionSeenBy(dir, undefined, "no-deps")).toBe("1.1.0"); + expect(await versionSeenBy(dir, "one-dep", "no-deps")).toBe("1.0.1"); + // Same packages as installing this package.json from scratch. + expect(await lock(dir)).toBe(await lock(fresh)); + }); + + test("a range changed in the same edit resolves on its own", async () => { + const dir = await project({ dependencies: { "no-deps": "~1.0.0" }, overrides: { "no-deps": "2.0.0" } }); + await installOk(dir); + expect(await versionSeenBy(dir, undefined, "no-deps")).toBe("2.0.0"); + + await write( + join(dir, "package.json"), + JSON.stringify({ name: "nested-overrides", dependencies: { "no-deps": "^1.0.0" } }), + ); + const { err } = await installOk(dir); + expect(err).toContain("Saved lockfile"); + // 1.0.1 is what the dropped ~1.0.0 range would pick. + expect(await versionSeenBy(dir, undefined, "no-deps")).toBe("1.1.0"); + const after = await lock(dir); + expect(after).not.toContain('"overrides"'); + expect(after).not.toContain("no-deps@1.0.1"); + expect(after).not.toContain("no-deps@2.0.0"); + await installOk(dir, "--frozen-lockfile"); + }); + + const outside = { + "x/package.json": JSON.stringify({ name: "x", version: "1.0.0" }), + "x2/package.json": JSON.stringify({ name: "x", version: "2.0.0" }), + "project/y/package.json": JSON.stringify({ name: "y", version: "1.0.0" }), + }; + const rootPackageJson = (pkg: Record) => JSON.stringify({ name: "nested-overrides", ...pkg }); + const before = rootPackageJson({ + dependencies: { x: "file:../x", y: "file:./y" }, + overrides: { x: "file:../x2" }, + }); + + test("a file: dependency outside the project re-resolves to its own path", async () => { + using root = tempDir("override-removed-file-dep", { ...outside, "project/package.json": before }); + const dir = join(String(root), "project"); + await installOk(dir); + expect(await versionSeenBy(dir, undefined, "x")).toBe("2.0.0"); + + await write(join(dir, "package.json"), rootPackageJson({ dependencies: { x: "file:../x", y: "file:./y" } })); + const { err } = await installOk(dir); + expect(err).toContain("Saved lockfile"); + expect(await versionSeenBy(dir, undefined, "x")).toBe("1.0.0"); + expect(normalizeBunSnapshot(await lock(dir), dir)).toMatchInlineSnapshot(` + "{ + "lockfileVersion": 2, + "configVersion": 1, + "workspaces": { + "": { + "name": "nested-overrides", + "dependencies": { + "x": "file:../x", + "y": "file:./y", + }, + }, + }, + "packages": { + "x": ["x@file:../x", {}], + + "y": ["y@file:y", {}], + } + }" + `); + await installOk(dir, "--frozen-lockfile"); + }); + + test("a file: dependency outside the project removed together with its rule", async () => { + using root = tempDir("override-and-file-dep-removed", { ...outside, "project/package.json": before }); + const dir = join(String(root), "project"); + await installOk(dir); + expect(await versionSeenBy(dir, undefined, "x")).toBe("2.0.0"); + + await write(join(dir, "package.json"), rootPackageJson({ dependencies: { y: "file:./y" } })); + const { err } = await installOk(dir); + expect(err).toContain("Saved lockfile"); + expect(normalizeBunSnapshot(await lock(dir), dir)).toMatchInlineSnapshot(` + "{ + "lockfileVersion": 2, + "configVersion": 1, + "workspaces": { + "": { + "name": "nested-overrides", + "dependencies": { + "y": "file:./y", + }, + }, + }, + "packages": { + "y": ["y@file:y", {}], + } + }" + `); + await installOk(dir, "--frozen-lockfile"); + }); + }); + test("changing only the parent's range text is a frozen-lockfile change", async () => { const dir = await project({ dependencies: twoParents, overrides: { "one-fixed-dep@1": { "no-deps": "1.1.0" } } }); await installOk(dir);