diff --git a/src/install/hoisted_install.rs b/src/install/hoisted_install.rs index 45bc4f7c2c06..f53ac9a724db 100644 --- a/src/install/hoisted_install.rs +++ b/src/install/hoisted_install.rs @@ -76,7 +76,7 @@ pub(crate) fn install_hoisted_packages( .hoisted_dependencies .clone_from(&original_tree_dep_ids); - { + let tree_required_packages = { // `lockfile` is `Box` so the heap object // is disjoint from the `PackageManager` struct; snapshot raw `*mut // Lockfile` and `*mut Log` first so `filter` can hold `&mut Lockfile` @@ -95,9 +95,9 @@ pub(crate) fn install_hoisted_packages( install_root_dependencies, workspace_filters, packages_to_install, - )?; + )? } - } + }; // Re-derive after `filter()` so every subsequent `this` use (progress // setup through the install loop) is a fresh child of `mgr_ptr` under // Stacked Borrows — `&mut *mgr_ptr` inside the block above popped the @@ -384,7 +384,8 @@ pub(crate) fn install_hoisted_packages( summary: &mut summary, force_install, successfully_installed: Bitset::init_empty(pkg_len)?, - required_packages: tree::RequiredPackages::new( + required_packages: tree::RequiredPackages::from_tree( + tree_required_packages, workspace_filters, install_root_dependencies, packages_to_install, diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index e753842367a5..42edaa2b29b0 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -1375,10 +1375,14 @@ impl<'a> Cloner<'a> { impl Lockfile { /// Re-hoists while a pass bound an optional peer late; a reload has that binding up front. pub(crate) fn resolve(&mut self, log: &mut bun_ast::Log) -> Result<(), tree::SubtreeError> { - while self.hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)? {} + while self + .hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)? + .late_bound_optional_peer + {} Ok(()) } + /// Returns the packages that the install tree binds a non-optional dependency to. pub(crate) fn filter( &mut self, log: &mut bun_ast::Log, @@ -1386,18 +1390,19 @@ impl Lockfile { install_root_dependencies: bool, workspace_filters: &[WorkspaceFilter], packages_to_install: Option<&[PackageID]>, - ) -> Result<(), tree::SubtreeError> { - self.hoist::<{ tree::BuilderMethod::Filter }>( - log, - Some(manager), - install_root_dependencies, - workspace_filters, - packages_to_install, - )?; - Ok(()) - } - - /// Sets `buffers.trees`/`hoisted_dependencies`; returns `Builder::late_bound_optional_peer`. + ) -> Result { + Ok(self + .hoist::<{ tree::BuilderMethod::Filter }>( + log, + Some(manager), + install_root_dependencies, + workspace_filters, + packages_to_install, + )? + .required_packages) + } + + /// Sets `buffers.trees`/`hoisted_dependencies`. pub(crate) fn hoist( &mut self, log: &mut bun_ast::Log, @@ -1407,7 +1412,7 @@ impl Lockfile { install_root_dependencies: bool, workspace_filters: &[WorkspaceFilter], packages_to_install: Option<&[PackageID]>, - ) -> Result { + ) -> Result { let slice = self.packages.slice(); // Only the install applies the barrier, so the saved tree does not depend on it. @@ -1440,6 +1445,11 @@ impl Lockfile { late_bound_optional_peer: false, list: Default::default(), sort_buf: Default::default(), + required_packages: if METHOD == tree::BuilderMethod::Filter { + DynamicBitSet::init_empty(slice.len())? + } else { + DynamicBitSet::default() + }, }; Tree::default().process_subtree(tree::ROOT_DEP_ID, tree::INVALID_ID, &mut builder)?; @@ -1459,10 +1469,19 @@ impl Lockfile { let late_bound_optional_peer = builder.late_bound_optional_peer; self.buffers.trees = cleaned.trees; self.buffers.hoisted_dependencies = cleaned.dep_ids; - Ok(late_bound_optional_peer) + Ok(HoistResult { + late_bound_optional_peer, + required_packages: cleaned.required_packages, + }) } } +pub(crate) struct HoistResult { + pub(crate) late_bound_optional_peer: bool, + /// `Builder::required_packages`. Empty unless `METHOD` is `Filter`. + pub(crate) required_packages: DynamicBitSet, +} + #[derive(Clone, Copy)] pub struct PendingResolution { pub(crate) old_resolution: PackageID, diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index 177bcedb8727..83be799e7495 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -123,7 +123,8 @@ impl Tree { enum HoistDependencyResult { DependencyLoop, - Hoisted, + /// Deduplicated onto a placed dependency on this package. + Hoisted(PackageID), Resolve(PackageID), ResolveReplace(ResolveReplace), ResolveLater, @@ -443,6 +444,8 @@ pub struct Builder<'a, const METHOD: BuilderMethod> { pub(crate) packages_to_install: Option<&'a [PackageID]>, /// Workspace package ids that are hoisting barriers (self-contained node_modules). pub(crate) self_contained: Vec, + /// The packages that a dependency without `Behavior::OPTIONAL` is bound to. Only `Filter`. + pub(crate) required_packages: DynamicBitSet, } pub struct BuilderEntry { @@ -460,6 +463,7 @@ bun_collections::multi_array_columns! { pub(crate) struct CleanResult { pub trees: Vec, pub dep_ids: Vec, + pub required_packages: DynamicBitSet, } impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> { @@ -487,6 +491,32 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> { self.lockfile().buffers.string_bytes.as_slice() } + /// `dependency`, one of `parent_range`, is linked, bound to `pkg_id`. + fn mark_required( + &mut self, + dependency: &Dependency, + parent_range: DependencyIDSlice, + pkg_id: PackageID, + ) { + if METHOD != BuilderMethod::Filter + || dependency + .behavior + .contains(crate::dependency::Behavior::OPTIONAL) + || (pkg_id as usize) >= self.required_packages.bit_length() + { + return; + } + // A peer whose parent also lists the name in `optionalDependencies` is an optional peer. + if dependency.behavior.is_peer() + && self.dependencies[parent_range.begin() as usize..parent_range.end() as usize] + .iter() + .any(|dep| dep.name_hash == dependency.name_hash && dep.behavior.is_optional()) + { + return; + } + self.required_packages.set(pkg_id as usize); + } + /// Flatten the multi-dimensional ArrayList of package IDs into a single easily serializable array pub(crate) fn clean(&mut self) -> Result { let mut total: u32 = 0; @@ -529,7 +559,11 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> { slice.deinit_owned(); - Ok(CleanResult { trees, dep_ids }) + Ok(CleanResult { + trees, + dep_ids, + required_packages: core::mem::take(&mut self.required_packages), + }) } } @@ -620,6 +654,8 @@ pub(crate) struct RequiredPackages<'a> { workspace_filters: &'a [WorkspaceFilter], install_root_dependencies: bool, packages_to_install: Option<&'a [PackageID]>, + /// `Builder::required_packages` of the hoisted install tree. Peers are bound there. + tree: DynamicBitSet, /// Walked on the first question that the asking dependency cannot answer. packages: Option, } @@ -629,11 +665,27 @@ impl<'a> RequiredPackages<'a> { workspace_filters: &'a [WorkspaceFilter], install_root_dependencies: bool, packages_to_install: Option<&'a [PackageID]>, + ) -> Self { + Self::from_tree( + DynamicBitSet::default(), + workspace_filters, + install_root_dependencies, + packages_to_install, + ) + } + + /// `tree` is what `Lockfile::filter` returns. + pub(crate) fn from_tree( + tree: DynamicBitSet, + workspace_filters: &'a [WorkspaceFilter], + install_root_dependencies: bool, + packages_to_install: Option<&'a [PackageID]>, ) -> Self { Self { workspace_filters, install_root_dependencies, packages_to_install, + tree, packages: None, } } @@ -652,6 +704,9 @@ impl<'a> RequiredPackages<'a> { { return true; } + if (package_id as usize) < self.tree.bit_length() { + return self.tree.is_set(package_id as usize); + } // Walk again if the install appended a package since. let packages = match &mut self.packages { Some(packages) if packages.bit_length() == lockfile.packages.len() => packages, @@ -894,20 +949,18 @@ impl Tree { if pkg_resolutions[pkg_id as usize].tag == crate::resolution::Tag::Folder { // A peer an ancestor edge already provides dedupes instead of nesting // a second copy of the folder (#40561). - if dependency.behavior.is_peer() - && matches!( - Tree::hoist_dependency::( - next_id, - hoist_root_id, - pkg_id, - dep_id, - resolution_list, - builder, - ), - HoistDependencyResult::Hoisted - ) - { - break 'hoisted HoistDependencyResult::Hoisted; + if dependency.behavior.is_peer() { + let hoisted = Tree::hoist_dependency::( + next_id, + hoist_root_id, + pkg_id, + dep_id, + resolution_list, + builder, + ); + if matches!(hoisted, HoistDependencyResult::Hoisted(_)) { + break 'hoisted hoisted; + } } // Folder packages never hoist, so a cycle between them would nest forever. @@ -941,7 +994,11 @@ impl Tree { }; match hoisted { - HoistDependencyResult::DependencyLoop | HoistDependencyResult::Hoisted => continue, + HoistDependencyResult::DependencyLoop => continue, + HoistDependencyResult::Hoisted(bound_pkg_id) => { + builder.mark_required(dependency, resolution_list, bound_pkg_id); + continue; + } HoistDependencyResult::Resolve(res_id) => { debug_assert!(pkg_id == invalid_package_id); @@ -972,6 +1029,7 @@ impl Tree { } HoistDependencyResult::ResolveReplace(replace) => { debug_assert!(pkg_id != invalid_package_id); + builder.mark_required(dependency, resolution_list, pkg_id); builder.late_bound_optional_peer = true; builder.resolutions[replace.dep_id as usize] = pkg_id; if let Some(entry) = builder @@ -1026,6 +1084,7 @@ impl Tree { entry.value_ptr.put(dep_id, ())?; } HoistDependencyResult::Placement(dest) => { + builder.mark_required(dependency, resolution_list, pkg_id); { // Go through ListExt // accessors sequentially so the &mut borrows do not overlap. @@ -1124,12 +1183,12 @@ impl Tree { if res_id == package_id { // this dependency is the same package as the other, hoist - return HoistDependencyResult::Hoisted; // 1 + return HoistDependencyResult::Hoisted(res_id); // 1 } if input_dep_range.contains(dep_id) { // same package lists this name in another dependency group - return HoistDependencyResult::Hoisted; // 1 + return HoistDependencyResult::Hoisted(res_id); // 1 } // now we either keep the dependency at this place in the tree, @@ -1142,7 +1201,7 @@ impl Tree { { HoistDependencyResult::Rebind(res_id) } else { - HoistDependencyResult::Hoisted + HoistDependencyResult::Hoisted(res_id) } }; diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index e15ebc04c56a..93426831a9d3 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -751,3 +751,176 @@ describe.each(["hoisted", "isolated"])("linker=%s", linker => { }); }); }); + +// The hoisted linker places a package once, under one of the dependencies on it, +// and the failed download of that package is an error only if that dependency is +// required. The result must not depend on which dependency owns the slot. +describe.concurrent("hoisted linker: a failed download of a package that a peer needs", () => { + // The registry serves `packages`. Once `fail()` is called, that tarball answers 404. + async function registry(packages: ({ name: string; version: string } & Record)[]) { + const ctx = await createTestContext({ linker: "hoisted" }); + const registry = ctx.registry_url.slice(0, -1); + const failing = new Set(); + const tarballs = new Map(); + for (const pkg of packages) { + tarballs.set( + `/${pkg.name}-${pkg.version}.tgz`, + await new Bun.Archive({ "package/package.json": JSON.stringify(pkg) }, { compress: "gzip" }).bytes(), + ); + } + setContextHandler(ctx, request => { + const pathname = new URL(request.url).pathname.slice(`/${ctx.id}`.length); + if (tarballs.has(pathname)) { + if (failing.has(pathname)) return new Response("no", { status: 404 }); + return new Response(tarballs.get(pathname)); + } + const name = pathname.slice(1); + const versions = packages.filter(pkg => pkg.name === name); + if (versions.length === 0) return new Response("unexpected", { status: 404 }); + return Response.json({ + name, + "dist-tags": { latest: versions[0].version }, + versions: Object.fromEntries( + versions.map(pkg => [pkg.version, { ...pkg, dist: { tarball: `${registry}/${name}-${pkg.version}.tgz` } }]), + ), + }); + }); + return { + dir: ctx.package_dir, + tarball: (name: string, version: string) => `${registry}/${name}-${version}.tgz`, + fail: (name: string, version: string) => failing.add(`/${name}-${version}.tgz`), + [Symbol.dispose]: () => destroyTestContext(ctx), + }; + } + + async function install(dir: string) { + await using proc = spawn({ + cmd: [bunExe(), "install", "--no-progress", "--ignore-scripts"], + cwd: dir, + stdout: "ignore", + stderr: "pipe", + env, + }); + const [err, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + const lines = err.split(/\r?\n/); + return { + warnLines: lines.filter(l => l.startsWith("warn:")), + errorLines: lines.filter(l => l.startsWith("error:")), + exitCode, + }; + } + + // Dependencies sort by name, so the parent named `aaa` owns node_modules/baz. + it.each([ + ["the parent with the peer", "aaa", "zzz"], + ["the parent with the optional dependency", "zzz", "aaa"], + ])("is an error when %s sorts first", async (_, peerHost, optionalHost) => { + using t = await registry([ + { name: peerHost, version: "1.0.0", peerDependencies: { baz: "1.0.0" } }, + { name: optionalHost, version: "1.0.0", optionalDependencies: { baz: "1.0.0" } }, + { name: "baz", version: "1.0.0" }, + ]); + await writeFile( + join(t.dir, "package.json"), + JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { [peerHost]: "1.0.0", [optionalHost]: "1.0.0" } }), + ); + expect(await install(t.dir)).toEqual({ warnLines: [], errorLines: [], exitCode: 0 }); + + // `cache: false` keeps the cache in node_modules/.cache, so this also empties the cache. + await rm(join(t.dir, "node_modules"), { recursive: true, force: true }); + t.fail("baz", "1.0.0"); + expect(await install(t.dir)).toEqual({ + warnLines: [], + errorLines: [`error: GET ${t.tarball("baz", "1.0.0")} - 404`], + exitCode: 1, + }); + }); + + // A root dependency provides a peer whatever its version. The peer resolves to + // baz@1.0.0 in the lockfile, but the install binds it to the root's baz@2.0.0. + it("is an error when the peer is bound to the root's optional version of the package", async () => { + using t = await registry([ + { name: "aaa", version: "1.0.0", peerDependencies: { baz: "1.0.0" } }, + { name: "baz", version: "1.0.0" }, + { name: "baz", version: "2.0.0" }, + ]); + await writeFile( + join(t.dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { aaa: "1.0.0" }, + optionalDependencies: { baz: "2.0.0" }, + }), + ); + const incorrectPeer = 'warn: incorrect peer dependency "baz@2.0.0"'; + expect(await install(t.dir)).toEqual({ warnLines: [incorrectPeer], errorLines: [], exitCode: 0 }); + expect(await file(join(t.dir, "node_modules", "baz", "package.json")).json()).toMatchObject({ version: "2.0.0" }); + + await rm(join(t.dir, "node_modules"), { recursive: true, force: true }); + t.fail("baz", "2.0.0"); + expect(await install(t.dir)).toEqual({ + warnLines: [], + errorLines: [`error: GET ${t.tarball("baz", "2.0.0")} - 404`], + exitCode: 1, + }); + }); + + // A name in both groups of one package.json is an optional peer. The + // optionalDependencies entry owns the slot and decides. + it("is a warning when the same package.json lists the peer in optionalDependencies", async () => { + using t = await registry([{ name: "baz", version: "1.0.0" }]); + await writeFile( + join(t.dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + optionalDependencies: { baz: "1.0.0" }, + peerDependencies: { baz: "1.0.0" }, + }), + ); + expect(await install(t.dir)).toEqual({ warnLines: [], errorLines: [], exitCode: 0 }); + + await rm(join(t.dir, "node_modules"), { recursive: true, force: true }); + t.fail("baz", "1.0.0"); + expect(await install(t.dir)).toEqual({ + warnLines: [`warn: GET ${t.tarball("baz", "1.0.0")} - 404`], + errorLines: [], + exitCode: 0, + }); + }); + + // The root's optional entry owns the slot, so both of the workspace's entries + // are deduplicated onto it. The workspace's own optional entry still decides. + it("is a warning when a workspace lists the peer in optionalDependencies and the root owns the slot", async () => { + using t = await registry([{ name: "baz", version: "1.0.0" }]); + await writeFile( + join(t.dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + workspaces: ["packages/*"], + optionalDependencies: { baz: "1.0.0" }, + }), + ); + await mkdir(join(t.dir, "packages", "w"), { recursive: true }); + await writeFile( + join(t.dir, "packages", "w", "package.json"), + JSON.stringify({ + name: "w", + version: "0.0.1", + optionalDependencies: { baz: "1.0.0" }, + peerDependencies: { baz: "1.0.0" }, + }), + ); + expect(await install(t.dir)).toEqual({ warnLines: [], errorLines: [], exitCode: 0 }); + + await rm(join(t.dir, "node_modules"), { recursive: true, force: true }); + t.fail("baz", "1.0.0"); + expect(await install(t.dir)).toEqual({ + warnLines: [`warn: GET ${t.tarball("baz", "1.0.0")} - 404`], + errorLines: [], + exitCode: 0, + }); + }); +});