Skip to content
Closed
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
60 changes: 60 additions & 0 deletions src/install/lockfile/Package/Scripts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,28 @@

const SCRIPT_NAMES_LEN: usize = LockfileScripts::NAMES.len();

/// Build the shell command that installs a git dependency's own
/// `dependencies`/`devDependencies` into its `node_modules/` before the
/// package's prepare scripts run. `None` only when `self_exe_path` fails.
Comment thread
robobun marked this conversation as resolved.
Resolved
fn git_dep_dev_install_command() -> Option<Box<[u8]>> {
let bun_exe = bun_core::self_exe_path().ok()?;
let registry = crate::package_manager_real::PackageManager::get()
.options
.scope
.url
.href();

Check warning on line 31 in src/install/lockfile/Package/Scripts.rs

View check run for this annotation

Claude / Claude Code Review

Nested install only forwards default registry, not scoped registries or project-level auth

The nested install only forwards the default registry (`options.scope.url`) — per-scope registries (`@scope:registry=...`) and auth tokens configured in the consumer's *project-level* `bunfig.toml`/`.npmrc` are not visible to it, since it runs with `cwd=node_modules/<pkg>` and bun does not walk up for config files. This is still a strict improvement (before this PR `prepare` always failed), and `~/.npmrc` is still inherited, so not blocking — but worth a follow-up to forward the consumer's confi
Comment thread
robobun marked this conversation as resolved.
Resolved
let mut cmd: Vec<u8> = Vec::with_capacity(bun_exe.len() + registry.len() + 64);
cmd.push(b'"');
cmd.extend_from_slice(bun_exe.as_bytes());
cmd.extend_from_slice(b"\" install --ignore-scripts --no-summary --no-progress");
if !registry.is_empty() {
cmd.extend_from_slice(b" --registry \"");
cmd.extend_from_slice(registry);
cmd.push(b'"');
}

Check warning on line 40 in src/install/lockfile/Package/Scripts.rs

View check run for this annotation

Claude / Claude Code Review

Nested install command does not shell-escape bun exe path or registry URL

The nested-install command wraps `self_exe_path()` and the registry href in raw `"..."` without shell-escaping, but lifecycle scripts run through `sh -c` (POSIX) / `bun exec` (Windows), which expand `$`/backtick and terminate on unescaped `"` inside double quotes — so a registry like `http://artifactory/$team-npm/` or an install path containing `$` will misparse. `bun_shell_parser::escape_8bit` (already used by `run_command.rs` for exactly this) is the in-tree helper to reach for here instead of
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
Some(cmd.into_boxed_slice())
}

#[repr(C)]
#[derive(Default, Clone, Copy)]
pub struct Scripts {
Expand Down Expand Up @@ -164,6 +186,20 @@
ResolutionTag::Git | ResolutionTag::Github | ResolutionTag::Root => {
let prepare_scripts = [&self.preprepare, &self.prepare, &self.postprepare];
Comment thread
robobun marked this conversation as resolved.
Resolved

// npm installs a git dependency by cloning it, running a full
// `npm install` (dependencies + devDependencies), running
// `prepare`, then packing the result. Run a nested install
// first so `prepare` can find the package's own devDependencies
// without adding them to the consumer's lockfile
// (oven-sh/bun#10297, #6138).
Comment thread
robobun marked this conversation as resolved.
Resolved
let install_deps = if resolution_tag != ResolutionTag::Root
&& prepare_scripts.iter().any(|s| !s.is_empty())
{
git_dep_dev_install_command()
} else {
None
};

for script in prepare_scripts {
if !script.is_empty() {
if first_script_index == -1 {
Expand All @@ -175,6 +211,30 @@
}
script_index += 1;
}

if let Some(install_cmd) = install_deps {
const PREPREPARE: usize = 3;
scripts[PREPREPARE] = Some(match scripts[PREPREPARE].take() {
Some(user_preprepare) => {
let mut chained =
Vec::with_capacity(install_cmd.len() + 4 + user_preprepare.len());
chained.extend_from_slice(&install_cmd);
chained.extend_from_slice(b" && ");
chained.extend_from_slice(&user_preprepare);
chained.into_boxed_slice()
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
}
None => {
counter += 1;
if first_script_index == -1
|| first_script_index > i8::try_from(PREPREPARE).expect("int cast")
{
first_script_index =
i8::try_from(PREPREPARE).expect("int cast");
}
install_cmd
}
});
Comment thread
robobun marked this conversation as resolved.
Resolved
}
}
ResolutionTag::Workspace => {
script_index += 1;
Expand Down
2 changes: 1 addition & 1 deletion src/install/repository.rs
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ impl RepositoryExt for Repository {
// it is only valid until the next use of `TlBufs::final_path_buf()` on this
// thread (another `try_https` call, or `checkout`'s `get_fd_path`).
let final_path_buf = TlBufs::final_path_buf();
if url.starts_with(b"http") {
if url.starts_with(b"http") || url.starts_with(b"file:") {
Comment thread
robobun marked this conversation as resolved.
Resolved
return Some(url);
}

Expand Down
76 changes: 76 additions & 0 deletions test/cli/install/bun-install-lifecycle-scripts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1885,6 +1885,82 @@ for (const forceWaiterThread of isLinux ? [false, true] : [false]) {
expect(await exists(join(packageDir, "node_modules", "lifecycle-install-test", "postinstall.txt"))).toBeTrue();
});

// https://github.com/oven-sh/bun/issues/10297
// https://github.com/oven-sh/bun/issues/6138
test("git dependency devDependencies are installed before `prepare` runs", async () => {
using ctx = await setupTest();
const { packageDir, packageJson, env } = ctx;
const testEnv = forceWaiterThread ? { ...env, BUN_FEATURE_FLAG_FORCE_WAITER_THREAD: "1" } : env;

// A local git repo that needs a devDependency (`what-bin`, served by the
// verdaccio registry) for its `prepare` script. npm clones a git
// dependency, runs a full install including devDependencies, runs
// `prepare`, then installs the packaged result.
const repoDir = join(packageDir, "repo");
await mkdir(repoDir, { recursive: true });
await writeFile(
join(repoDir, "package.json"),
JSON.stringify({
name: "git-dep-needs-dev",
version: "1.0.0",
devDependencies: { "what-bin": "1.0.0" },
scripts: { prepare: "what-bin" },
}),
);

const gitEnv = {
...baseEnv,
GIT_AUTHOR_NAME: "bun",
GIT_AUTHOR_EMAIL: "bun@example.com",
GIT_COMMITTER_NAME: "bun",
GIT_COMMITTER_EMAIL: "bun@example.com",
GIT_CONFIG_NOSYSTEM: "1",
};
for (const args of [["init", "-b", "main"], ["add", "."], ["commit", "-m", "init"]]) {
const r = Bun.spawnSync({ cmd: ["git", ...args], cwd: repoDir, env: gitEnv, stdout: "ignore", stderr: "pipe" });
if (r.exitCode !== 0) throw new Error(`git ${args.join(" ")} failed: ${r.stderr}`);
Comment thread
robobun marked this conversation as resolved.
Resolved
}

await writeFile(
packageJson,
JSON.stringify({
name: "consumer",
version: "1.0.0",
dependencies: {
"git-dep-needs-dev": `git+file://${repoDir.replaceAll("\\", "/")}#main`,
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
},
trustedDependencies: ["git-dep-needs-dev"],
}),
);

const { stdout, stderr, exited } = spawn({
cmd: [bunExe(), "install"],
cwd: packageDir,
stdout: "pipe",
stdin: "ignore",
stderr: "pipe",
env: testEnv,
});

const err = stderrForInstall(await stderr.text());
const out = await stdout.text();
const depDir = join(packageDir, "node_modules", "git-dep-needs-dev");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

expect(err).not.toContain("what-bin: command not found");
expect(err).not.toContain("what-bin: not found");
expect(err).not.toContain("error:");
expect(out).toContain("+ git-dep-needs-dev@git+file://");
expect(await exited).toBe(0);

// `prepare` ran and found the `what-bin` devDependency.
expect(await file(join(depDir, "what-bin.txt")).text()).toBe("what-bin@1.0.0");

// npm does not add the git dependency's devDependencies to the outer
// lockfile or hoist them into the outer node_modules.
expect(await exists(join(packageDir, "node_modules", "what-bin"))).toBeFalse();
expect(await file(join(packageDir, "bun.lock")).text()).not.toContain("what-bin");
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

test("root lifecycle scripts should wait for dependency lifecycle scripts", async () => {
using ctx = await setupTest();
const { packageDir, packageJson, env } = ctx;
Expand Down
Loading