Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 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
67 changes: 42 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,36 @@ fn enqueue_local_tarball(
unsafe { &raw mut (*task).threadpool_task }
}

/// The directory `path` is relative to when that is not the top-level dir: the workspace or
/// `file:` folder package whose package.json wrote it. An edge whose own specifier is not
/// `path` got it from the root's overrides / resolutions / catalogs, so it stays relative
/// to the top-level dir.
Comment thread
robobun marked this conversation as resolved.
Outdated
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;
if declared.tag != dependency::version::Tag::Tarball {
return None;
}
let dependency::tarball::Uri::Local(declared_path) = &declared.tarball().uri else {
return None;
};
if lockfile.str(declared_path) != path {
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
10 changes: 4 additions & 6 deletions src/install/PackageManagerTask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -699,12 +699,10 @@ 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.
/// `tarball.url` itself (`normalize` set) or the absolute path it was joined
/// into (`local_tarball_base_dir`). Computed on the main thread: resolving it
/// reads `lockfile.packages` / `string_bytes`, which the main thread may
/// reallocate while this task runs on a ThreadPool worker.
Comment thread
robobun marked this conversation as resolved.
Outdated
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