diff --git a/src/install/PackageInstaller.rs b/src/install/PackageInstaller.rs index ca441264d8ab..084146bb1041 100644 --- a/src/install/PackageInstaller.rs +++ b/src/install/PackageInstaller.rs @@ -1929,7 +1929,6 @@ impl<'a> PackageInstaller<'a> { let dep = &self.lockfile().buffers.dependencies.as_slice()[dependency_id as usize]; - let dep_behavior = dep.behavior; let truncated_dep_name_hash: TruncatedPackageNameHash = dep.name_hash as TruncatedPackageNameHash; let (is_trusted, is_trusted_through_update_request) = 'brk: { @@ -1990,12 +1989,17 @@ impl<'a> PackageInstaller<'a> { break 'enqueue_lifecycle_scripts; } + let optional = !self.required_packages.contains( + self.manager(), + dependency_id, + package_id, + ); if self.enqueue_lifecycle_scripts( alias.slice(string_buf!()), log_level, &mut folder_path, package_id, - dep_behavior.contains(crate::dependency::Behavior::OPTIONAL), + optional, resolution, ) { if is_trusted_through_update_request { @@ -2230,7 +2234,6 @@ impl<'a> PackageInstaller<'a> { } let dep = &self.lockfile().buffers.dependencies.as_slice()[dependency_id as usize]; - let dep_behavior = dep.behavior; let truncated_dep_name_hash: TruncatedPackageNameHash = dep.name_hash as TruncatedPackageNameHash; let (is_trusted, is_trusted_through_update_request, add_to_lockfile) = 'brk: { @@ -2299,12 +2302,16 @@ impl<'a> PackageInstaller<'a> { break 'enqueue_lifecycle_scripts; } + let optional = + !self + .required_packages + .contains(self.manager(), dependency_id, package_id); if self.enqueue_lifecycle_scripts( alias.slice(string_buf!()), log_level, &mut folder_path, package_id, - dep_behavior.contains(crate::dependency::Behavior::OPTIONAL), + optional, resolution, ) { let (trusted_name, trusted_name_hash) = diff --git a/src/install/PackageManager/runTasks.rs b/src/install/PackageManager/runTasks.rs index 4282cf698fc8..c4a76286c03a 100644 --- a/src/install/PackageManager/runTasks.rs +++ b/src/install/PackageManager/runTasks.rs @@ -22,7 +22,6 @@ use super::{ Command, PackageInstaller, PackageManager, ProgressStrings, Subcommand, TaskCallbackList, }; use super::{directories, enqueue}; -use crate::dependency::Behavior; use crate::isolated_install::installer as store_installer; use crate::isolated_install::store::{EntryColumns as _, NodeColumns as _}; use crate::lifecycle_script_runner::InstallCtx; @@ -381,8 +380,11 @@ fn run_tasks_erased( let entry_id = task.entry_id; let node_id = installer.store.entries.items_node_id()[entry_id.get() as usize]; let dep_id = installer.store.nodes.items_dep_id()[node_id.get() as usize]; - let dep = &installer.lockfile().buffers.dependencies[dep_id as usize]; - let optional = dep.behavior.contains(Behavior::OPTIONAL); + let pkg_id = installer.store.nodes.items_pkg_id()[node_id.get() as usize]; + let optional = + !installer + .required_packages + .contains(installer.manager(), dep_id, pkg_id); // SAFETY: `list` is the per-entry scripts slot owned by // `store.entries.items_scripts()[entry_id]`; this Task is // its sole consumer (see Installer.rs Yield::RunScripts). diff --git a/src/install/isolated_install.rs b/src/install/isolated_install.rs index 55c02f20153e..ff288b8e498b 100644 --- a/src/install/isolated_install.rs +++ b/src/install/isolated_install.rs @@ -2047,6 +2047,11 @@ pub(crate) fn install_isolated_packages( None }, store: &store, + required_packages: RequiredPackages::new( + workspace_filters, + install_root_dependencies, + packages_to_install, + ), tasks, waiters_head: vec![store::entry::Id::INVALID; store.entries.len()].into_boxed_slice(), next_waiter: vec![store::entry::Id::INVALID; store.entries.len()].into_boxed_slice(), @@ -2100,12 +2105,6 @@ pub(crate) fn install_isolated_packages( ); } - let mut required_packages = RequiredPackages::new( - workspace_filters, - install_root_dependencies, - packages_to_install, - ); - // add the pending task count upfront installer .manager_mut() @@ -2416,7 +2415,9 @@ pub(crate) fn install_isolated_packages( let dep = &lockfile_ro.buffers.dependencies[dep_id as usize]; let is_required = - required_packages.contains(installer.manager(), dep_id, pkg_id); + installer + .required_packages + .contains(installer.manager(), dep_id, pkg_id); match pkg_res_tag { ResolutionTag::Npm => { diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index 1ee1715dbf53..2c2b10245947 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -14,6 +14,7 @@ use bun_sys::{FdDirExt as _, FdExt as _}; use crate::bin_real; use crate::lockfile::package; +use crate::lockfile::tree::RequiredPackages; use crate::lockfile_real::PackageIDSlice; use crate::package_install::{Method as InstallMethod, Summary as InstallSummary}; use crate::package_manager_real::Command; @@ -88,6 +89,9 @@ pub struct Installer<'a> { pub(crate) store: &'a Store, + /// Main-thread only. + pub(crate) required_packages: RequiredPackages<'a>, + pub(crate) task_queue: UnboundedQueue, // intrusive via .next pub(crate) tasks: Box<[Task]>, diff --git a/test/cli/install/bun-install-lifecycle-scripts.test.ts b/test/cli/install/bun-install-lifecycle-scripts.test.ts index 3ccbfb3b4c3f..49b2ffb3c141 100644 --- a/test/cli/install/bun-install-lifecycle-scripts.test.ts +++ b/test/cli/install/bun-install-lifecycle-scripts.test.ts @@ -210,6 +210,124 @@ test.concurrent("trustedDependencies matches the resolved package name, not the expect(await exited).toBe(0); }); +// `no-deps-scripted-to-fail` has an `install` script that exits 1. The linker places it once, +// under the root's optional dependency. `no-deps-scripted-to-deeply-fail` requires it, so the +// failed script fails the install. +for (const linker of ["hoisted", "isolated"] as const) { + test.concurrent( + `a failed script of a package that an optional and a required dependency share (${linker})`, + async () => { + using ctx = await setupTest(); + const { packageDir, packageJson, env } = ctx; + + await writeFile( + packageJson, + JSON.stringify({ + name: "foo", + version: "1.0.0", + dependencies: { "no-deps-scripted-to-deeply-fail": "1.0.0" }, + optionalDependencies: { "no-deps-scripted-to-fail": "1.0.0" }, + trustedDependencies: ["no-deps-scripted-to-fail"], + }), + ); + + await using proc = spawn({ + cmd: [bunExe(), "install", `--linker=${linker}`], + cwd: packageDir, + stdout: "pipe", + stdin: "ignore", + stderr: "pipe", + env, + }); + const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(out).toContain("bun install v1."); + expect(splitErrLines(err).filter(line => line.startsWith("error:"))).toEqual([ + 'error: install script from "no-deps-scripted-to-fail" exited with 1', + ]); + expect(exitCode).toBe(1); + }, + ); + + test.concurrent(`a failed script of a package that only an optional dependency needs (${linker})`, async () => { + using ctx = await setupTest(); + const { packageDir, packageJson, env } = ctx; + + await writeFile( + packageJson, + JSON.stringify({ + name: "foo", + version: "1.0.0", + dependencies: { "no-deps": "1.0.0" }, + optionalDependencies: { "no-deps-scripted-to-fail": "1.0.0" }, + trustedDependencies: ["no-deps-scripted-to-fail"], + }), + ); + + await using proc = spawn({ + cmd: [bunExe(), "install", `--linker=${linker}`], + cwd: packageDir, + stdout: "pipe", + stdin: "ignore", + stderr: "pipe", + env, + }); + const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(err).not.toContain("error:"); + expect(out).not.toContain("Blocked"); + expect(exitCode).toBe(0); + expect(await exists(join(packageDir, "node_modules", "no-deps", "package.json"))).toBeTrue(); + expect(await exists(join(packageDir, "node_modules", "no-deps-scripted-to-fail"))).toBeFalse(); + }); +} + +// The package is already installed and blocked. The second install trusts it and runs its script. +// Only the hoisted linker runs the scripts of a package that a later install trusts. +test.concurrent( + "a failed script of an installed package that an optional and a required dependency share (hoisted)", + async () => { + using ctx = await setupTest(); + const { packageDir, packageJson, env } = ctx; + + const pkg = { + name: "foo", + version: "1.0.0", + dependencies: { "no-deps-scripted-to-deeply-fail": "1.0.0" }, + optionalDependencies: { "no-deps-scripted-to-fail": "1.0.0" }, + }; + await writeFile(packageJson, JSON.stringify(pkg)); + { + await using proc = spawn({ + cmd: [bunExe(), "install", "--linker=hoisted"], + cwd: packageDir, + stdout: "pipe", + stdin: "ignore", + stderr: "pipe", + env, + }); + const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(err).not.toContain("error:"); + expect(out).toContain("Blocked 1 postinstall"); + expect(exitCode).toBe(0); + } + + await writeFile(packageJson, JSON.stringify({ ...pkg, trustedDependencies: ["no-deps-scripted-to-fail"] })); + await using proc = spawn({ + cmd: [bunExe(), "install", "--linker=hoisted"], + cwd: packageDir, + stdout: "pipe", + stdin: "ignore", + stderr: "pipe", + env, + }); + const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(out).toContain("bun install v1."); + expect(splitErrLines(err).filter(line => line.startsWith("error:"))).toEqual([ + 'error: install script from "no-deps-scripted-to-fail" exited with 1', + ]); + expect(exitCode).toBe(1); + }, +); + test.concurrent( "trustedDependencies added on a later install still matches the resolved package name, not the dependency alias", async () => {