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
52 changes: 48 additions & 4 deletions src/install/isolated_install/Installer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,23 @@ impl<'a> Installer<'a> {
),
);
}
TaskError::DependencyBinaries(dep_errs) => {
for dep_err in dep_errs.iter() {
let dep_node_id = entry_node_ids[dep_err.dep_entry_id.get() as usize];
let dep_pkg_id = node_pkg_ids[dep_node_id.get() as usize];
Output::err(
dep_err.err,
"failed to link binaries of dependency {}@{} for package: {}@{}",
(
bstr::BStr::new(pkg_names[dep_pkg_id as usize].slice(string_buf)),
pkg_resolutions[dep_pkg_id as usize]
.fmt(string_buf, bun_core::fmt::PathSep::Auto),
bstr::BStr::new(pkg_name.slice(string_buf)),
pkg_res.fmt(string_buf, bun_core::fmt::PathSep::Auto),
),
);
}
}
TaskError::Download(dl) => {
Output::err_generic(
"failed to download <b>{}@{}<r>: {}\n <d>{}<r>",
Expand Down Expand Up @@ -684,11 +701,24 @@ pub struct DownloadError {
pub(crate) url: Box<[u8]>,
}

/// One dependency whose bins could not be linked into the failing entry's
/// `node_modules/.bin`.
Comment thread
robobun marked this conversation as resolved.
Outdated
#[derive(Clone, Copy)]
pub struct DependencyBinariesError {
pub(crate) dep_entry_id: StoreEntryId,
pub(crate) err: crate::Error,
}

pub enum TaskError {
LinkPackage(sys::Error),
SymlinkDependencies(sys::Error),
RunScripts(crate::Error),
/// The entry's own bins (`Step::Binaries`).
Binaries(crate::Error),
/// Bins of the entry's dependencies (`Step::SymlinkDependencyBinaries`).
/// Every dependency is attempted before the step fails, so this holds
/// one error per dependency that failed; never empty.
Comment thread
robobun marked this conversation as resolved.
Outdated
DependencyBinaries(Box<[DependencyBinariesError]>),
Patching(Log),
Download(DownloadError),
}
Expand All @@ -699,6 +729,7 @@ impl TaskError {
TaskError::LinkPackage(err) => TaskError::LinkPackage(err.clone()),
TaskError::SymlinkDependencies(err) => TaskError::SymlinkDependencies(err.clone()),
TaskError::Binaries(err) => TaskError::Binaries(*err),
TaskError::DependencyBinaries(errs) => TaskError::DependencyBinaries(errs.clone()),
TaskError::RunScripts(err) => TaskError::RunScripts(*err),
TaskError::Patching(_log) => {
// `bun_ast::Log` is non-Clone; the only caller of
Expand Down Expand Up @@ -1555,7 +1586,7 @@ impl Task {
Step::SymlinkDependencyBinaries => {
let current_step = Step::SymlinkDependencyBinaries;
if let Err(err) = installer.link_dependency_bins(self.entry_id) {
return Ok(Yield::failure(TaskError::Binaries(err)));
return Ok(Yield::failure(err));
}

match pkg_res.tag {
Expand Down Expand Up @@ -2304,7 +2335,13 @@ impl<'a> Installer<'a> {
Ok(changed)
}

pub(crate) fn link_dependency_bins(&self, parent_entry_id: StoreEntryId) -> crate::Result<()> {
/// Links the bins of every dependency of `parent_entry_id` into its
/// `node_modules/.bin`. A dependency that fails does not stop the rest from
/// being linked; the failures are returned together afterwards.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) fn link_dependency_bins(
&self,
parent_entry_id: StoreEntryId,
) -> core::result::Result<(), TaskError> {
let lockfile = self.lockfile();
let store = self.store;

Expand All @@ -2331,6 +2368,7 @@ impl<'a> Installer<'a> {
let mut link_rel_buf = paths::path_buffer_pool::get();

let mut seen: StringHashMap<()> = StringHashMap::default();
let mut failed: Vec<DependencyBinariesError> = Vec::new();

let mut node_modules_path = DefaultAbsPath::init_top_level_dir();
self.append_real_store_node_modules_path(
Expand Down Expand Up @@ -2427,11 +2465,17 @@ impl<'a> Installer<'a> {
}

if let Some(err) = bin_linker.err {
return Err(err);
failed.push(DependencyBinariesError {
dep_entry_id: dep.entry_id,
err,
});
}
}

Ok(())
if failed.is_empty() {
return Ok(());
}
Err(TaskError::DependencyBinaries(failed.into_boxed_slice()))
}

/// True when this entry should live in the shared global virtual store
Expand Down
73 changes: 72 additions & 1 deletion test/cli/install/isolated-install.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import { afterAll, beforeAll, describe, expect, test } from "bun:test";
import { existsSync, lstatSync, readlinkSync, statSync } from "fs";
import { mkdir, readlink, rm, symlink } from "fs/promises";
import { VerdaccioRegistry, bunEnv, bunExe, readdirSorted, runBunInstall, tempDir } from "harness";
import { VerdaccioRegistry, bunEnv, bunExe, isWindows, readdirSorted, runBunInstall, tempDir } from "harness";
import { createRequire } from "module";
import { dirname, join } from "path";

Expand Down Expand Up @@ -367,6 +367,77 @@
]);
});

test("a dependency whose bins fail to link does not stop its siblings' bins from being linked", async () => {
// `directories.bin` names a file, so opening it as a directory fails with
// ENOTDIR (a missing directory would be skipped silently).
const badBinDir = (name: string) => JSON.stringify({ name, version: "1.0.0", directories: { bin: "package.json" } });
const { packageDir } = await registry.createTestDir({
bunfigOpts: { linker: "isolated" },
files: {
"package.json": JSON.stringify({
name: "test-pkg-bin-link-errors",
workspaces: ["packages/*"],
dependencies: {
"a-bin": "file:./deps/a-bin",
"bad-bin-dir-1": "file:./deps/bad-bin-dir-1",
"bad-bin-dir-2": "file:./deps/bad-bin-dir-2",
"z-bin": "file:./deps/z-bin",
},
}),
"packages/ws/package.json": JSON.stringify({
name: "ws",
dependencies: {
"bad-bin-dir-1": "file:../../deps/bad-bin-dir-1",
"z-bin": "file:../../deps/z-bin",
},
}),
"deps/a-bin/package.json": JSON.stringify({ name: "a-bin", version: "1.0.0", bin: { "a-cli": "cli.js" } }),
"deps/a-bin/cli.js": "#!/usr/bin/env node\nconsole.log('a');\n",
"deps/z-bin/package.json": JSON.stringify({ name: "z-bin", version: "1.0.0", bin: { "z-cli": "cli.js" } }),
"deps/z-bin/cli.js": "#!/usr/bin/env node\nconsole.log('z');\n",
"deps/bad-bin-dir-1/package.json": badBinDir("bad-bin-dir-1"),
"deps/bad-bin-dir-2/package.json": badBinDir("bad-bin-dir-2"),
},
});

await using proc = spawn({
cmd: [bunExe(), "install"],
cwd: packageDir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);

Check warning on line 410 in test/cli/install/isolated-install.test.ts

View check run for this annotation

Claude / Claude Code Review

Test pipes stdout but never drains it

The test spawns `bun install` with `stdout: "pipe"` but never drains `proc.stdout` — only `proc.stderr.text()` and `proc.exited` are awaited. Per REVIEW.md's subprocess convention, either add `proc.stdout.text()` to the `Promise.all` or change to `stdout: "ignore"`; with 4 file: deps + 1 workspace it won't hit 64KB in practice, so this is a robustness nit rather than a live deadlock.
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

const linkedBin = (dir: string, name: string) =>
existsSync(join(dir, "node_modules", ".bin", isWindows ? `${name}.bunx` : name));
expect({
"root a-cli": linkedBin(packageDir, "a-cli"),
"root z-cli": linkedBin(packageDir, "z-cli"),
"ws z-cli": linkedBin(join(packageDir, "packages", "ws"), "z-cli"),
}).toEqual({
"root a-cli": true,
"root z-cli": true,
"ws z-cli": true,
});

// Every failing dependency is reported, attributed to the package whose
// `.bin` it could not be linked into. Tasks run in parallel, so sort.
const binErrors = stderr
.split("\n")
.filter(line => line.includes("failed to link binaries"))
.map(line => line.replaceAll("\\", "/"))
.sort();
expect(binErrors).toEqual([
"ENOTDIR: failed to link binaries for package: bad-bin-dir-1@deps/bad-bin-dir-1",
"ENOTDIR: failed to link binaries for package: bad-bin-dir-2@deps/bad-bin-dir-2",
"ENOTDIR: failed to link binaries of dependency bad-bin-dir-1@deps/bad-bin-dir-1 for package: test-pkg-bin-link-errors@",
"ENOTDIR: failed to link binaries of dependency bad-bin-dir-1@deps/bad-bin-dir-1 for package: ws@workspace:packages/ws",
"ENOTDIR: failed to link binaries of dependency bad-bin-dir-2@deps/bad-bin-dir-2 for package: test-pkg-bin-link-errors@",
]);
expect(exitCode).toBe(1);
});

describe("isolated workspaces", () => {
test("basic", async () => {
const { packageJson, packageDir } = await registry.createTestDir({ bunfigOpts: { linker: "isolated" } });
Expand Down
Loading