Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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
64 changes: 39 additions & 25 deletions src/install/PackageManager/PackageManagerEnqueue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1903,31 +1903,18 @@ fn enqueue_local_tarball(
// other dependencies (e.g. `appendPackage` / `StringBuilder.allocate`
// in `Package.fromNPM`).
let mut abs_buf = PathBuffer::uninit();
let (tarball_path, normalize): (&[u8], bool) = 'tarball_path: {
let workspace_pkg_id = this
.lockfile
.get_workspace_pkg_if_workspace_dep(dependency_id);
if workspace_pkg_id == invalid_package_id {
break 'tarball_path (path, true);
}

let workspace_res = this.lockfile.packages.items_resolution()[workspace_pkg_id as usize];
if workspace_res.tag != ResolutionTag::Workspace {
break 'tarball_path (path, true);
}

// Construct an absolute path to the tarball.
// Normally tarball paths are always relative to the root directory, but if a
// workspace depends on a tarball path, it should be relative to the workspace.
let workspace_str = *workspace_res.workspace();
let workspace_path = workspace_str.slice(this.lockfile.buffers.string_bytes.as_slice());
let joined = Path::resolve_path::join_abs_string_buf::<Path::platform::Auto>(
FileSystem::instance().top_level_dir(),
&mut abs_buf,
&[workspace_path, path],
);
break 'tarball_path (joined, false);
};
let (tarball_path, normalize): (&[u8], bool) =
match local_tarball_base_dir(&this.lockfile, dependency_id, path) {
None => (path, true),
Some(base_dir) => (
Path::resolve_path::join_abs_string_buf::<Path::platform::Auto>(
FileSystem::instance().top_level_dir(),
&mut abs_buf,
&[base_dir, path],
),
false,
),
};

// Build the `Task` value *before* claiming a hive slot — the `.expect()`s
// below can unwind, and `Task` carries drop glue. See `enqueue_git_clone`.
Expand Down Expand Up @@ -1978,6 +1965,33 @@ fn enqueue_local_tarball(
unsafe { &raw mut (*task).threadpool_task }
}

/// The workspace or `file:` folder directory that `path` is relative to; `None` is the top-level dir.
fn local_tarball_base_dir<'a>(
lockfile: &'a Lockfile::Lockfile,
dependency_id: DependencyID,
path: &[u8],
) -> Option<&'a [u8]> {
let declared = &lockfile.buffers.dependencies[dependency_id as usize].version;
let declared_by_parent = declared.tag == dependency::version::Tag::Tarball
&& matches!(
&declared.tarball().uri,
dependency::tarball::Uri::Local(declared_path) if lockfile.str(declared_path) == path
);
if !declared_by_parent {
// Overrides, resolutions and catalogs are all written in the root package.json.
return None;
}

let declarer = lockfile.get_parent_pkg_of_dependency(dependency_id)?;
let declarer_res = &lockfile.packages.items_resolution()[declarer as usize];
let base_dir = match declarer_res.tag {
ResolutionTag::Workspace => declarer_res.workspace(),
ResolutionTag::Folder => declarer_res.folder(),
_ => return None,
};
Some(lockfile.str(base_dir))
}

fn update_name_and_name_hash_from_version_replacement(
lockfile: &Lockfile::Lockfile,
original_name: SemverString,
Expand Down
7 changes: 1 addition & 6 deletions src/install/PackageManagerTask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -699,12 +699,7 @@ pub struct GitCheckoutRequest {

pub struct LocalTarballRequest {
pub(crate) tarball: ExtractTarball,
/// Path to read the tarball from. May be the same as `tarball.url` (when
/// `normalize` is true) or an absolute path joined with a workspace
/// directory. Computed on the main thread in `enqueueLocalTarball` because
/// resolving it requires reading `lockfile.packages` / `string_bytes`,
/// which can be reallocated concurrently by the main thread while this
/// task runs on a ThreadPool worker.
/// Resolved by `enqueue_local_tarball` on the main thread; the worker must not read the lockfile.
pub(crate) tarball_path: StringOrTinyString,
/// When true, `tarball_path` is a user-provided path resolved relative to
/// cwd. When false, it is already an absolute path.
Expand Down
10 changes: 10 additions & 0 deletions src/install/lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,16 @@ impl Lockfile {
self.get_workspace_pkg_if_workspace_dep(id) != invalid_package_id
}

/// `None` for the edges `enqueue_dependency_to_root` appends outside of any package.
pub(crate) fn get_parent_pkg_of_dependency(&self, id: DependencyID) -> Option<PackageID> {
for (pkg_id, dependencies) in self.packages.items_dependencies().iter().enumerate() {
if dependencies.contains(id) {
return Some(PackageID::try_from(pkg_id).expect("int cast"));
}
}
None
}

pub(crate) fn get_workspace_pkg_if_workspace_dep(&self, id: DependencyID) -> PackageID {
let packages = self.packages.slice();
let resolutions = packages.items_resolution();
Expand Down
105 changes: 105 additions & 0 deletions test/cli/install/bun-install.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10571,6 +10571,111 @@ it("fails when a transitive file: dependency's folder does not exist", async ()
expect(exitCode).toBe(1);
});

describe.concurrent("file: tarball declared by a file: folder dependency", () => {
// `bar-0.0.2.tgz` is planted at the path the declaration means and
// `baz-0.0.3.tgz` at the other candidate path, so reading the tarball
// relative to the wrong directory installs `baz` instead of failing with ENOENT.
const expected = readFileSync(join(import.meta.dir, "bar-0.0.2.tgz"));
const decoy = readFileSync(join(import.meta.dir, "baz-0.0.3.tgz"));

const fixture = (root: object, lib: object, tarballs: Record<string, Buffer>) => ({
"package.json": JSON.stringify({
name: "my-app",
version: "1.0.0",
dependencies: { lib: "file:./vendor/lib" },
...root,
}),
"vendor/lib/package.json": JSON.stringify({ name: "lib", version: "1.0.0", main: "index.js", ...lib }),
"vendor/lib/index.js": `const pkg = require("tool/package.json"); module.exports = pkg.name + "@" + pkg.version;`,
...tarballs,
});

// The first install resolves `tool` from vendor/lib/package.json and reads
// the tarball in the process. The second one starts from the lockfile with
// an empty cache, so it has to read the tarball again from the path recorded
// there; both have to pick the same file.
async function installAndRequireLib(projectDir: string, linker: "hoisted" | "isolated") {
const cacheDir = join(projectDir, ".bun-cache");
const installed: string[] = [];

for (const args of [["install"], ["install", "--frozen-lockfile"]]) {
await Promise.all([
rm(join(projectDir, "node_modules"), { recursive: true, force: true }),
rm(cacheDir, { recursive: true, force: true }),
]);

await using install = spawn({
cmd: [bunExe(), ...args, `--linker=${linker}`],
cwd: projectDir,
stdout: "pipe",
stderr: "pipe",
env: { ...env, BUN_INSTALL_CACHE_DIR: cacheDir },
});
const [installErr, installOut, installExit] = await Promise.all([
install.stderr.text(),
install.stdout.text(),
install.exited,
]);
expect(installErr).not.toContain("error:");
expect(installOut).toContain("2 packages installed");
expect(installExit).toBe(0);

await using run = spawn({
cmd: [bunExe(), "-e", `console.log(require("lib"))`],
cwd: projectDir,
stdout: "pipe",
stderr: "pipe",
env,
});
const [runErr, runOut, runExit] = await Promise.all([run.stderr.text(), run.stdout.text(), run.exited]);
expect(runErr).toBe("");
expect(runExit).toBe(0);
installed.push(runOut.trim());
}

return { installed, lockfile: await file(join(projectDir, "bun.lock")).text() };
}

for (const linker of ["hoisted", "isolated"] as const) {
it(`is read relative to the folder (${linker} linker)`, async () => {
using dir = tempDir(
"folder-dep-tarball",
fixture(
{},
{ dependencies: { tool: "file:./tool.tgz" } },
{ "vendor/lib/tool.tgz": expected, "tool.tgz": decoy },
),
);

const { installed, lockfile } = await installAndRequireLib(String(dir), linker);
expect(installed).toEqual(["bar@0.0.2", "bar@0.0.2"]);
// The lockfile records the path as declared; the name in front of it is
// read from the tarball that was extracted.
expect(lockfile).toContain('"lib": ["lib@file:vendor/lib", { "dependencies": { "tool": "file:./tool.tgz" } }]');
expect(lockfile).toContain('"tool": ["bar@./tool.tgz", {}, "sha512-');
});
}

it("is read relative to the project when a root override supplies the path", async () => {
// `overrides` can only be written in the root package.json, so the path it
// contains means the project directory even though the dependency it is
// applied to is declared by vendor/lib/package.json.
using dir = tempDir(
"folder-dep-tarball-override",
fixture(
{ overrides: { tool: "file:./tool.tgz" } },
{ dependencies: { tool: "^1.0.0" } },
{ "tool.tgz": expected, "vendor/lib/tool.tgz": decoy },
),
);

const { installed, lockfile } = await installAndRequireLib(String(dir), "hoisted");
expect(installed).toEqual(["bar@0.0.2", "bar@0.0.2"]);
expect(lockfile).toContain('"lib": ["lib@file:vendor/lib", { "dependencies": { "tool": "^1.0.0" } }]');
expect(lockfile).toContain('"tool": ["bar@./tool.tgz", {}, "sha512-');
});
});

it("does not extract a local file: tarball outside the temp dir for a dependency alias containing '..' path segments", async () => {
// For `file:` tarball dependencies, the dependency alias (the key in
// `dependencies`) is used to derive the temporary extraction folder name.
Expand Down
41 changes: 41 additions & 0 deletions test/cli/install/bun-workspaces.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,47 @@ describe("relative tarballs", async () => {
},
});
});
// The tarball path is written in the root package.json, so it is relative to
// the root even though the dependency it ends up satisfying is declared by
// the workspace (#25835 for overrides, #25752 for catalogs). The workspace
// gets a different tarball at the same relative path, so reading it relative
// to the workspace installs `baz` instead of failing.
for (const [source, root, specifier] of [
["override", { overrides: { bar: "file:./bar.tgz" } }, "^0.0.2"],
["catalog entry", { catalogs: { vendored: { bar: "file:./bar.tgz" } } }, "catalog:vendored"],
] as const) {
test.concurrent(`from a root ${source} applied to a workspace dependency`, async () => {
using ctx = await setupTest();
const { packageDir, env } = ctx;
await Promise.all([
write(join(packageDir, "package.json"), JSON.stringify({ name: "foo", workspaces: ["pkgs/*"], ...root })),
write(
join(packageDir, "pkgs", "pkg1", "package.json"),
JSON.stringify({ name: "pkg1", dependencies: { bar: specifier } }),
),
cp(join(import.meta.dir, "bar-0.0.2.tgz"), join(packageDir, "bar.tgz")),
]);
await cp(join(import.meta.dir, "baz-0.0.3.tgz"), join(packageDir, "pkgs", "pkg1", "bar.tgz"));

// The second install starts from the lockfile and an empty cache, so it
// reads the tarball again from the path recorded there.
for (const frozenLockfile of [false, true]) {
await Promise.all([
rm(join(packageDir, "node_modules"), { recursive: true, force: true }),
rm(env.BUN_INSTALL_CACHE_DIR, { recursive: true, force: true }),
]);

await runBunInstall(env, packageDir, { frozenLockfile });

expect(await file(join(packageDir, "node_modules", "bar", "package.json")).json()).toEqual({
name: "bar",
version: "0.0.2",
});
}

expect(await file(join(packageDir, "bun.lock")).text()).toContain('"bar": ["bar@./bar.tgz", {}, "sha512-');
});
}

// Regression test for a data race where the `.local_tarball` task callback
// (running on a ThreadPool worker) read `lockfile.packages` and
Expand Down
Loading