diff --git a/src/install/PackageManager.rs b/src/install/PackageManager.rs index e1524675ffb9..90c25f7f2fbc 100644 --- a/src/install/PackageManager.rs +++ b/src/install/PackageManager.rs @@ -78,6 +78,8 @@ pub mod populate_manifest_cache; pub mod process_dependency_list; #[path = "PackageManager/ProgressStrings.rs"] pub mod progress_strings; +#[path = "PackageManager/remove_stale_workspace_links.rs"] +pub(crate) mod remove_stale_workspace_links; #[path = "PackageManager/runTasks.rs"] pub mod run_tasks; #[path = "PackageManager/security_scanner.rs"] diff --git a/src/install/PackageManager/install_with_manager.rs b/src/install/PackageManager/install_with_manager.rs index 11ed45e6278e..249b266ae881 100644 --- a/src/install/PackageManager/install_with_manager.rs +++ b/src/install/PackageManager/install_with_manager.rs @@ -38,6 +38,7 @@ use bun_install_types::NodeLinker::NodeLinker; // Free-function "methods" on `PackageManager` hosted in sibling modules // to avoid one giant `impl PackageManager` block. +use crate::package_manager_real::remove_stale_workspace_links::remove_stale_workspace_links; use crate::package_manager_real::run_tasks::{RunTasksCallbacks, run_tasks}; use crate::package_manager_real::{ UpdateRequest, enqueue_dependency_list, enqueue_dependency_with_main, enqueue_patch_task_pre, @@ -878,6 +879,11 @@ pub fn install_with_manager( break 'install_summary PackageInstallSummary::default(); } + // Every workspace is a root dependency, so without a diff the workspace set is unchanged. + if had_any_diffs { + remove_stale_workspace_links(&lockfile_before_clean, &manager.lockfile); + } + let mut linker = manager.options.node_linker; loop { match linker { diff --git a/src/install/PackageManager/remove_stale_workspace_links.rs b/src/install/PackageManager/remove_stale_workspace_links.rs new file mode 100644 index 000000000000..3540d66fc4f3 --- /dev/null +++ b/src/install/PackageManager/remove_stale_workspace_links.rs @@ -0,0 +1,80 @@ +use bstr::BStr; +use bun_paths::AutoAbsPathChecked; + +use crate::lockfile::Lockfile; +use crate::lockfile::package::PackageColumns as _; +use crate::package_installer::alias_is_safe_install_target; +use crate::package_manager_real::PackageManager; +use crate::resolution::Tag as ResolutionTag; + +/// Linkers only visit names in the new lockfile; a workspace name that left it is unlinked here. +pub(crate) fn remove_stale_workspace_links(previous: &Lockfile, current: &Lockfile) { + let previous_string_buf = previous.buffers.string_bytes.as_slice(); + let previous_packages = previous.packages.slice(); + let resolutions = previous_packages.items_resolution(); + let name_hashes = previous_packages.items_name_hash(); + let names = previous_packages.items_name(); + + for pkg_id in 0..previous_packages.len() { + if resolutions[pkg_id].tag != ResolutionTag::Workspace + || current.workspace_paths.contains_key(&name_hashes[pkg_id]) + { + continue; + } + + let name = names[pkg_id].slice(previous_string_buf); + if !alias_is_safe_install_target(name) { + continue; + } + + remove_links_named(current, name); + } +} + +#[cold] +#[inline(never)] +fn remove_links_named(current: &Lockfile, name: &[u8]) { + let current_string_buf = current.buffers.string_bytes.as_slice(); + + let mut path = AutoAbsPathChecked::init_top_level_dir(); + let top_level_len = path.len(); + + remove_link(&mut path, b"", name); + for workspace_path in current.workspace_paths.values() { + path.set_length(top_level_len); + remove_link(&mut path, workspace_path.slice(current_string_buf), name); + } +} + +/// `readlink` is the symlink (or junction) check; a real directory or file there is left alone. +fn remove_link(path: &mut AutoAbsPathChecked, package_dir: &[u8], name: &[u8]) { + if path.append(package_dir).is_err() + || path.append(b"node_modules").is_err() + || path.append(name).is_err() + { + return; + } + + let mut link_target = bun_paths::path_buffer_pool::get(); + if bun_sys::readlink(path.slice_z(), &mut link_target).is_err() { + return; + } + + bun_output::scoped_log!( + PackageManager, + "removing stale workspace link {}", + BStr::new(path.slice()) + ); + + #[cfg(windows)] + { + // Directory symlinks and junctions need rmdir; a file symlink needs unlink. + if bun_sys::rmdir(path.slice_z()).is_err() { + let _ = bun_sys::unlink(path.slice_z()); + } + } + #[cfg(not(windows))] + { + let _ = bun_sys::unlink(path.slice_z()); + } +} diff --git a/test/cli/install/bun-workspaces.test.ts b/test/cli/install/bun-workspaces.test.ts index 48903d67ec9f..ab62dbf2158f 100644 --- a/test/cli/install/bun-workspaces.test.ts +++ b/test/cli/install/bun-workspaces.test.ts @@ -2629,3 +2629,179 @@ describe("packages whose version label is longer than 512 bytes", () => { ); }); }); + +// Both linkers only create or repoint the `node_modules/` links the current +// lockfile asks for. When a workspace is renamed or removed, its name leaves the +// lockfile and the links bun had created under that name (in the root's node_modules +// and, with the isolated linker, in the node_modules of the workspaces depending on +// it) used to stay behind, still resolving to the workspace folder. +describe("links of a renamed or removed workspace", () => { + type Linker = "hoisted" | "isolated"; + const linkers: Linker[] = ["hoisted", "isolated"]; + + async function install(ctx: TestCtx, linker: Linker, ...args: string[]): Promise { + await using proc = spawn({ + cmd: [bunExe(), "install", "--linker", linker, ...args], + cwd: ctx.packageDir, + stdout: "pipe", + stderr: "pipe", + env: ctx.env, + }); + const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(err).not.toContain("error:"); + expect(exitCode).toBe(0); + return out; + } + + function writeProject( + packageDir: string, + rootDependencies: Record, + packages: Record>, + ) { + return Promise.all([ + write( + join(packageDir, "package.json"), + JSON.stringify({ name: "foo", workspaces: ["packages/*"], devDependencies: rootDependencies }), + ), + ...Object.entries(packages).map(([dir, packageJson]) => + write(join(packageDir, "packages", dir, "package.json"), JSON.stringify({ version: "1.0.0", ...packageJson })), + ), + ]); + } + + // Package entries of a node_modules directory (no `.bun`/`.bin`), or [] when it does + // not exist. A listing (rather than `exists`) also sees links whose target is gone. + async function entries(nodeModules: string): Promise { + try { + return (await readdirSorted(nodeModules)).filter(name => !name.startsWith(".")); + } catch (err: any) { + if (err.code !== "ENOENT") throw err; + return []; + } + } + + for (const linker of linkers) { + test.concurrent(`${linker}: renaming a workspace removes the links under its old name`, async () => { + using ctx = await setupTest(); + const { packageDir } = ctx; + const rootNodeModules = join(packageDir, "node_modules"); + const bNodeModules = join(packageDir, "packages", "b", "node_modules"); + + await writeProject( + packageDir, + { a: "*", b: "*" }, + { a: { name: "a" }, b: { name: "b", dependencies: { a: "*" } } }, + ); + await install(ctx, linker); + expect(await entries(rootNodeModules)).toEqual(["a", "b"]); + expect(await entries(bNodeModules)).toEqual(linker === "isolated" ? ["a"] : []); + + await writeProject( + packageDir, + { "a-renamed": "*", b: "*" }, + { a: { name: "a-renamed" }, b: { name: "b", dependencies: { "a-renamed": "*" } } }, + ); + await install(ctx, linker); + expect(await entries(rootNodeModules)).toEqual(["a-renamed", "b"]); + expect(await entries(bNodeModules)).toEqual(linker === "isolated" ? ["a-renamed"] : []); + expect(await file(join(rootNodeModules, "a-renamed", "package.json")).json()).toMatchObject({ + name: "a-renamed", + }); + + expect(await install(ctx, linker)).toContain("(no changes)"); + expect(await entries(rootNodeModules)).toEqual(["a-renamed", "b"]); + expect(await entries(bNodeModules)).toEqual(linker === "isolated" ? ["a-renamed"] : []); + }); + + test.concurrent(`${linker}: renaming a scoped workspace removes the link under its old name`, async () => { + using ctx = await setupTest(); + const { packageDir } = ctx; + const scopeDir = join(packageDir, "node_modules", "@repo"); + + await writeProject( + packageDir, + { "@repo/a": "*", "@repo/b": "*" }, + { a: { name: "@repo/a" }, b: { name: "@repo/b" } }, + ); + await install(ctx, linker); + expect(await entries(scopeDir)).toEqual(["a", "b"]); + + await writeProject( + packageDir, + { "@repo/a-renamed": "*", "@repo/b": "*" }, + { a: { name: "@repo/a-renamed" }, b: { name: "@repo/b" } }, + ); + await install(ctx, linker); + expect(await entries(scopeDir)).toEqual(["a-renamed", "b"]); + expect(await file(join(scopeDir, "a-renamed", "package.json")).json()).toMatchObject({ name: "@repo/a-renamed" }); + }); + + test.concurrent(`${linker}: removing a workspace from the project removes its link`, async () => { + using ctx = await setupTest(); + const { packageDir } = ctx; + const rootNodeModules = join(packageDir, "node_modules"); + + await writeProject(packageDir, { a: "*", b: "*" }, { a: { name: "a" }, b: { name: "b" } }); + await install(ctx, linker); + expect(await entries(rootNodeModules)).toEqual(["a", "b"]); + + await rm(join(packageDir, "packages", "a"), { recursive: true }); + await writeProject(packageDir, { b: "*" }, { b: { name: "b" } }); + await install(ctx, linker); + expect(await entries(rootNodeModules)).toEqual(["b"]); + }); + + // The old name is unlinked before the linker runs, so a dependency that still + // wants that name (here an alias of the renamed workspace) is linked again. + test.concurrent(`${linker}: the old name is linked again when a dependency still uses it`, async () => { + using ctx = await setupTest(); + const { packageDir } = ctx; + const rootNodeModules = join(packageDir, "node_modules"); + + await writeProject(packageDir, { a: "*" }, { a: { name: "a" } }); + await install(ctx, linker); + expect(await entries(rootNodeModules)).toEqual(["a"]); + + await writeProject(packageDir, { a: "workspace:a-renamed@*" }, { a: { name: "a-renamed" } }); + await install(ctx, linker); + expect(await entries(rootNodeModules)).toEqual(linker === "isolated" ? ["a"] : ["a", "a-renamed"]); + expect(await file(join(rootNodeModules, "a", "package.json")).json()).toMatchObject({ name: "a-renamed" }); + }); + } + + // The isolated linker only links the workspaces the root depends on, so nothing is + // ever linked for `a` here and the directory under its name is not bun's to remove. + test.concurrent("a directory under the old name is left alone", async () => { + using ctx = await setupTest(); + const { packageDir } = ctx; + const rootNodeModules = join(packageDir, "node_modules"); + const marker = join(rootNodeModules, "a", "marker.txt"); + + await writeProject(packageDir, { b: "*" }, { a: { name: "a" }, b: { name: "b" } }); + await install(ctx, "isolated"); + expect(await entries(rootNodeModules)).toEqual(["b"]); + + await write(marker, "not a workspace link"); + await writeProject(packageDir, { b: "*" }, { a: { name: "a-renamed" }, b: { name: "b" } }); + await install(ctx, "isolated"); + expect(await entries(rootNodeModules)).toEqual(["a", "b"]); + expect(await file(marker).text()).toBe("not a workspace link"); + }); + + test.concurrent("--dry-run does not touch the links", async () => { + using ctx = await setupTest(); + const { packageDir } = ctx; + const rootNodeModules = join(packageDir, "node_modules"); + + await writeProject(packageDir, { a: "*", b: "*" }, { a: { name: "a" }, b: { name: "b" } }); + await install(ctx, "hoisted"); + expect(await entries(rootNodeModules)).toEqual(["a", "b"]); + + await writeProject(packageDir, { "a-renamed": "*", b: "*" }, { a: { name: "a-renamed" }, b: { name: "b" } }); + await install(ctx, "hoisted", "--dry-run"); + expect(await entries(rootNodeModules)).toEqual(["a", "b"]); + + await install(ctx, "hoisted"); + expect(await entries(rootNodeModules)).toEqual(["a-renamed", "b"]); + }); +});