Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/install/isolated_install/Installer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,29 @@
};
let _folder_dir_guard = sys::CloseOnDrop::new(folder_dir);

// The hardlink/copy walk below only visits names
// present in the source folder, so rebuilding in
// place merges: a file deleted from the folder
// dependency would survive in the store entry
// forever. Delete to replace, matching the hoisted
// linker's uninstall-before-install. (Lifecycle
// scripts re-run on every install for folder
// entries, so artifacts they write are recreated.)
Comment thread
robobun marked this conversation as resolved.
Outdated
//
// Root entries only reach LinkPackage as a
// dependency on the root package (`"x": "file:."`);
// for those `append_store_path` yields the store
// entry's package dir, never the project dir.
debug_assert!(
pkg_res.tag != ResolutionTag::Root
|| dep_id != invalid_dependency_id
);
let mut prev_build = AutoPath::init_top_level_dir();
installer.append_store_path(&mut prev_build, self.entry_id);
if let Some(e) = Fd::cwd().delete_tree(prev_build.slice()).err() {
return Ok(Yield::failure(TaskError::LinkPackage(e)));
}

Check warning on line 926 in src/install/isolated_install/Installer.rs

View check run for this annotation

Claude / Claude Code Review

debug_assert-only guard on a path that can degenerate to the project root

The `debug_assert!` here compiles out in release builds, so the only thing preventing `delete_tree` from being called on the project root is the scheduling logic in `isolated_install.rs:2142-2150` (a different file). That invariant holds today, so this isn't a live bug — but if it's ever broken by a future refactor, the release binary would silently `rm -rf` the user's project instead of panicking. Consider upgrading to `assert!`, or explicitly skipping the delete when `append_store_path` append
Comment thread
robobun marked this conversation as resolved.
Outdated

let mut backend = InstallMethod::Hardlink;
'backend: loop {
match backend {
Expand Down
58 changes: 58 additions & 0 deletions test/cli/install/isolated-install.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,64 @@ test("can install folder dependencies", async () => {
).toBe("module.exports = 'hello from pkg-1';");
});

test("file deleted from a folder dependency is removed on reinstall", async () => {
const { packageJson, packageDir } = await registry.createTestDir({ bunfigOpts: { linker: "isolated" } });

await Promise.all([
write(
packageJson,
JSON.stringify({
name: "test-pkg-folder-dep-prune",
dependencies: {
"folder-dep": "file:./pkg-1",
},
}),
),
write(
join(packageDir, "pkg-1", "package.json"),
JSON.stringify({
name: "folder-dep",
version: "1.0.0",
dependencies: { "no-deps": "1.0.0" },
}),
),
write(join(packageDir, "pkg-1", "index.js"), "module.exports = 1;"),
write(join(packageDir, "pkg-1", "extra.js"), "module.exports = 2;"),
write(join(packageDir, "pkg-1", "nested", "inner.js"), "module.exports = 3;"),
]);

await runBunInstall(bunEnv, packageDir);

const installedDep = join(packageDir, "node_modules", "folder-dep");
expect(await file(join(installedDep, "extra.js")).exists()).toBe(true);
expect(await file(join(installedDep, "nested", "inner.js")).exists()).toBe(true);

// The store entry rebuilds from the source folder on every install, so a
// file or directory deleted from the source must not survive the rebuild.
await Promise.all([
rm(join(packageDir, "pkg-1", "extra.js")),
rm(join(packageDir, "pkg-1", "nested"), { recursive: true }),
write(join(packageDir, "pkg-1", "index.js"), "module.exports = 'updated';"),
]);

await runBunInstall(bunEnv, packageDir, { savesLockfile: false });

expect(await file(join(installedDep, "extra.js")).exists()).toBe(false);
expect(existsSync(join(installedDep, "nested"))).toBe(false);
expect(await file(join(installedDep, "index.js")).text()).toBe("module.exports = 'updated';");

// The rebuild replaces only the package dir inside the store entry; the
// entry's dependency symlinks still resolve.
expect(readlinkSync(join(packageDir, "node_modules", "folder-dep"))).toBe(
join(".bun", "folder-dep@file+pkg-1", "node_modules", "folder-dep"),
);
expect(
await file(
join(packageDir, "node_modules", ".bun", "folder-dep@file+pkg-1", "node_modules", "no-deps", "package.json"),
).json(),
).toMatchObject({ name: "no-deps", version: "1.0.0" });
});

test("can install folder dependencies on root package", async () => {
const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { linker: "isolated" } });

Expand Down