diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index db8f7c3058d3..bb4a93e74caf 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -2642,7 +2642,24 @@ fn get_or_put_resolved_package( dependency::version::Tag::Folder => { let folder = *version.folder(); let res: FolderResolutionValue = 'res: { - if this.lockfile.is_workspace_dependency(dependency_id) { + if !this.lockfile.is_workspace_dependency(dependency_id) + && crate::bin::bin_target_escapes_package_dir(this.lockfile.str(&folder)) + { + // overrides/resolutions are only ever parsed from the root + // package.json, so a folder path that reached here via an + // override was written by the user and is trusted the same + // as a direct dependency of the root. + let buf = this.lockfile.buffers.string_bytes.as_slice(); + if !this.lockfile.overrides.contains_name( + dependency.name_hash, + dependency.name.slice(buf), + buf, + ) { + break 'res FolderResolutionValue::Err(crate::Error::MissingPackageJSON); + } + } + + if this.lockfile.is_dependency_of_local_package(dependency_id) { // relative to cwd // reshaped for borrowck — `folder_path` borrows // `string_bytes`; detach the slice lifetime so the @@ -2676,22 +2693,8 @@ fn get_or_put_resolved_package( ); } - // transitive folder dependencies do not have their dependencies resolved - if crate::bin::bin_target_escapes_package_dir(this.lockfile.str(&folder)) { - // overrides/resolutions are only ever parsed from the root - // package.json, so a folder path that reached here via an - // override was written by the user and is trusted the same - // as a direct dependency of the root. - let buf = this.lockfile.buffers.string_bytes.as_slice(); - if !this.lockfile.overrides.contains_name( - dependency.name_hash, - dependency.name.slice(buf), - buf, - ) { - break 'res FolderResolutionValue::Err(crate::Error::MissingPackageJSON); - } - } - + // Declared by a registry package: `Package::from_npm` keeps the path + // relative to that package, which is not on disk until it is installed. let mut package = Package::default(); { diff --git a/src/install/PackageManager/PackageManagerResolution.rs b/src/install/PackageManager/PackageManagerResolution.rs index 1f1b0b65edf0..dede07e3df35 100644 --- a/src/install/PackageManager/PackageManagerResolution.rs +++ b/src/install/PackageManager/PackageManagerResolution.rs @@ -330,11 +330,10 @@ impl PackageManager { continue; } - let features = match pkg_resolutions[parent_id].tag { - ResolutionTag::Root | ResolutionTag::Workspace | ResolutionTag::Folder => { - self.options.local_package_features - } - _ => self.options.remote_package_features, + let features = if pkg_resolutions[parent_id].tag.is_local_package() { + self.options.local_package_features + } else { + self.options.remote_package_features }; // even if optional dependencies are enabled, it's still allowed to fail if failed_dep.behavior.is_optional() || !failed_dep.behavior.is_enabled(features) { diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index 04da1c724225..a8d4f389fd7d 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -802,6 +802,31 @@ impl Lockfile { invalid_package_id } + /// The package whose dependency list contains `id`, or `invalid_package_id` when + /// no package declares it. + pub(crate) fn get_parent_pkg_of_dependency(&self, id: DependencyID) -> PackageID { + self.packages + .items_dependencies() + .iter() + .position(|dependencies| dependencies.contains(id)) + .map_or(invalid_package_id, |pkg_id| { + PackageID::try_from(pkg_id).expect("int cast") + }) + } + + /// Is this a direct dependency of a local package (`resolution::Tag::is_local_package`)? + /// + /// A folder package declared by a registry package gets no dependency list (see the + /// Folder arm of `get_or_put_resolved_package`), so every folder package that + /// declares anything is reached from the root through local packages only. + pub(crate) fn is_dependency_of_local_package(&self, id: DependencyID) -> bool { + let parent_id = self.get_parent_pkg_of_dependency(id); + parent_id != invalid_package_id + && self.packages.items_resolution()[parent_id as usize] + .tag + .is_local_package() + } + /// Does this tree id belong to a workspace (including workspace root)? /// TODO(dylan-conway) fix! pub(crate) fn is_workspace_tree_id(&self, id: tree::Id) -> bool { diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index 66d3f9597a28..866c90e610ce 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -602,11 +602,10 @@ pub(crate) fn is_filtered_dependency_or_workspace( return true; } - let dep_features = match parent_res.tag { - crate::resolution::Tag::Root - | crate::resolution::Tag::Workspace - | crate::resolution::Tag::Folder => manager.options.local_package_features, - _ => manager.options.remote_package_features, + let dep_features = if parent_res.tag.is_local_package() { + manager.options.local_package_features + } else { + manager.options.remote_package_features }; if !dep.behavior.is_enabled(dep_features) { diff --git a/src/install/resolution.rs b/src/install/resolution.rs index 5998e39bc87b..2da2ec593d55 100644 --- a/src/install/resolution.rs +++ b/src/install/resolution.rs @@ -965,6 +965,13 @@ impl Tag { self == Tag::Git || self == Tag::Github } + /// The root, a workspace, or a `file:` folder: a package.json of the project's own, + /// so its dependencies get `local_package_features` and `Package::parse` stored its + /// `file:` paths relative to the top-level dir. + pub(crate) fn is_local_package(self) -> bool { + self == Tag::Root || self == Tag::Workspace || self == Tag::Folder + } + pub(crate) fn can_enqueue_install_task(self) -> bool { self == Tag::Npm || self == Tag::LocalTarball diff --git a/test/cli/install/bun-install-registry.test.ts b/test/cli/install/bun-install-registry.test.ts index 0e6f5e17b6ed..1bdeed884b9e 100644 --- a/test/cli/install/bun-install-registry.test.ts +++ b/test/cli/install/bun-install-registry.test.ts @@ -5243,6 +5243,97 @@ describe("transitive file dependencies", () => { version: "1.1.1", }); }); + + // Unlike the registry packages above, whose file: targets only exist once the + // package is installed, a local file: package's own file: dependency is on + // disk while resolving, so it is read like one declared by the root. + for (const linker of ["hoisted", "isolated"] as const) { + test(`${linker}: a file: dependency of a local file: package is resolved like a root one`, async () => { + const { packageDir } = await registry.createTestDir({ + bunfigOpts: { linker }, + files: { + "package.json": JSON.stringify({ + name: "foo", + dependencies: { + lib: "file:./vendor/lib", + }, + }), + "vendor/lib/package.json": JSON.stringify({ + name: "lib", + version: "1.0.0", + dependencies: { + tool: "file:../tool", + }, + }), + "vendor/lib/index.js": `module.exports = "lib->" + require("tool");`, + "vendor/tool/package.json": JSON.stringify({ + name: "tool", + version: "1.0.0", + bin: { tool: "cli.js" }, + dependencies: { + "no-deps": "1.0.0", + }, + }), + "vendor/tool/cli.js": `#!/usr/bin/env node\nconsole.log("tool");`, + "vendor/tool/index.js": `module.exports = "tool->no-deps@" + require("no-deps").version;`, + }, + }); + const libNodeModules = + linker === "hoisted" + ? join(packageDir, "node_modules", "lib", "node_modules") + : join(packageDir, "node_modules", ".bun", "lib@file+vendor+lib", "node_modules"); + + let { out } = await runBunInstall(env, packageDir); + expect(out).toContain("3 packages installed"); + + const lock = (await file(join(packageDir, "bun.lock")).text()).replaceAll(/localhost:\d+/g, "localhost:1234"); + expect(normalizeBunSnapshot(lock)).toMatchInlineSnapshot(` + "{ + "lockfileVersion": 2, + "configVersion": 1, + "workspaces": { + "": { + "name": "foo", + "dependencies": { + "lib": "file:./vendor/lib", + }, + }, + }, + "packages": { + "lib": ["lib@file:vendor/lib", { "dependencies": { "tool": "file:../tool" } }], + + "no-deps": ["no-deps@1.0.0", "http://localhost:1234/no-deps/-/no-deps-1.0.0.tgz", {}, "sha512-v4w12JRjUGvfHDUP8vFDwu0gUWu04j0cv9hLb1Abf9VdaXu4XcrddYFTMVBVvmldKViGWH7jrb6xPJRF0wq6gw=="], + + "lib/tool": ["tool@file:vendor/tool", { "dependencies": { "no-deps": "1.0.0" }, "bin": { "tool": "cli.js" } }], + } + }" + `); + + // Once from the package.json files, once from the lockfile they produced. + for (const frozenLockfile of [false, true]) { + if (frozenLockfile) { + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + ({ out } = await runBunInstall(env, packageDir, { frozenLockfile })); + expect(out).toContain("3 packages installed"); + } + + expect(await readdirSorted(join(libNodeModules, ".bin"))).toHaveBins(["tool"]); + expect(join(libNodeModules, ".bin", "tool")).toBeValidBin(join("..", "tool", "cli.js")); + + await using proc = spawn({ + cmd: [bunExe(), "-e", `console.log(require("lib"))`], + cwd: packageDir, + env, + stdout: "pipe", + stderr: "pipe", + }); + const [runOut, runErr, runExit] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(runErr).toBe(""); + expect(runOut).toBe("lib->tool->no-deps@1.0.0\n"); + expect(runExit).toBe(0); + } + }); + } }); test("name from manifest is scoped and url encoded", async () => { diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index 279851e5dd27..65e0f68b988e 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -1,7 +1,7 @@ import { file, listen, Socket, spawn, write } from "bun"; import { afterAll, beforeAll, describe, expect, it, jest, setDefaultTimeout, test } from "bun:test"; import { readFileSync, readlinkSync, realpathSync, statSync } from "fs"; -import { access, cp, exists, mkdir, readlink, rm, stat, writeFile } from "fs/promises"; +import { access, chmod, cp, exists, mkdir, readlink, rm, stat, writeFile } from "fs/promises"; import { bunEnv, bunExe, @@ -10104,7 +10104,7 @@ for (const field of ["resolutions", "overrides"]) { "packages": { "pkg-a": ["pkg-a@file:pkg-a", { "dependencies": { "shared": "1.0.0" } }], - "pkg-a/shared": ["shared@file:./vendor/shared", {}], + "pkg-a/shared": ["shared@file:vendor/shared", {}], } }" `); @@ -10171,6 +10171,152 @@ it("installs the transitive file: dependency of a file: dependency", async () => } }); +// Where each linker puts the `node_modules` of the two file: packages that have +// dependencies. Folder packages are never hoisted, so `tool` lives under `lib`. +// (bun-install-registry.test.ts covers requiring a registry dependency of such a +// package at runtime.) +for (const [linker, libNodeModules, toolNodeModules, toolNodeModulesEntries] of [ + [ + "hoisted", + join("node_modules", "lib", "node_modules"), + join("node_modules", "lib", "node_modules", "tool", "node_modules"), + [".bin", "dev-only", "helper"], + ], + [ + "isolated", + join("node_modules", ".bun", "lib@file+vendor+lib", "node_modules"), + join("node_modules", ".bun", "tool@file+vendor+tool", "node_modules"), + // the isolated store also links a package's own bins into its own entry + [".bin", "dev-only", "helper", "tool"], + ], +] as const) { + it.concurrent(`${linker}: nested file: dependencies are installed with their dependencies and bins`, async () => { + using dir = tempDir("nested-file-dep-bins", { + "package.json": JSON.stringify({ + name: "my-app", + version: "1.0.0", + dependencies: { + lib: "file:./vendor/lib", + }, + }), + "vendor/lib/package.json": JSON.stringify({ + name: "lib", + version: "1.0.0", + dependencies: { + tool: "file:../tool", + }, + }), + "vendor/tool/package.json": JSON.stringify({ + name: "tool", + version: "1.0.0", + bin: { tool: "cli.js" }, + dependencies: { + helper: "file:../helper", + }, + devDependencies: { + "dev-only": "file:../dev-only", + }, + }), + "vendor/tool/cli.js": "#!/bin/sh\necho tool\n", + "vendor/helper/package.json": JSON.stringify({ + name: "helper", + version: "1.0.0", + bin: { helper: "cli.js" }, + }), + "vendor/helper/cli.js": "#!/bin/sh\necho helper\n", + "vendor/dev-only/package.json": JSON.stringify({ + name: "dev-only", + version: "1.0.0", + }), + }); + const projectDir = String(dir); + const locks: string[] = []; + + // The hoisted linker installs these packages as per-file symlinks and its bin + // chmod does not reach through them (#38777), so the scripts are made + // executable up front; what is checked below is that each .bin link runs + // the right script. + if (!isWindows) { + await chmod(join(projectDir, "vendor", "tool", "cli.js"), 0o755); + await chmod(join(projectDir, "vendor", "helper", "cli.js"), 0o755); + } + + // The first pass resolves from the package.json files, the second installs + // what the first one recorded in bun.lock. + for (const args of [["install"], ["install", "--frozen-lockfile"]]) { + await rm(join(projectDir, "node_modules"), { recursive: true, force: true }); + + await using proc = spawn({ + cmd: [bunExe(), ...args, `--linker=${linker}`], + cwd: projectDir, + stdout: "pipe", + stdin: "ignore", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]); + + expect(err).not.toContain("error:"); + expect(out).toContain("4 packages installed"); + expect(exitCode).toBe(0); + + locks.push(await file(join(projectDir, "bun.lock")).text()); + + expect(await readdirSorted(join(projectDir, libNodeModules, ".bin"))).toHaveBins(["tool"]); + expect(join(projectDir, libNodeModules, ".bin", "tool")).toBeValidBin(join("..", "tool", "cli.js")); + expect(await readdirSorted(join(projectDir, toolNodeModules))).toEqual(toolNodeModulesEntries); + expect(join(projectDir, toolNodeModules, ".bin", "helper")).toBeValidBin(join("..", "helper", "cli.js")); + + if (!isWindows) { + for (const [bin, expected] of [ + [join(libNodeModules, ".bin", "tool"), "tool\n"], + [join(toolNodeModules, ".bin", "helper"), "helper\n"], + ]) { + await using binProc = spawn({ + cmd: [join(projectDir, bin)], + cwd: projectDir, + stdout: "pipe", + stderr: "pipe", + }); + const [binOut, binErr, binExit] = await Promise.all([ + binProc.stdout.text(), + binProc.stderr.text(), + binProc.exited, + ]); + expect(binErr).toBe(""); + expect(binOut).toBe(expected); + expect(binExit).toBe(0); + } + } + } + + expect(locks[1]).toBe(locks[0]); + expect(normalizeBunSnapshot(locks[0], projectDir)).toMatchInlineSnapshot(` + "{ + "lockfileVersion": 2, + "configVersion": 1, + "workspaces": { + "": { + "name": "my-app", + "dependencies": { + "lib": "file:./vendor/lib", + }, + }, + }, + "packages": { + "lib": ["lib@file:vendor/lib", { "dependencies": { "tool": "file:../tool" } }], + + "lib/tool": ["tool@file:vendor/tool", { "dependencies": { "helper": "file:../helper" }, "devDependencies": { "dev-only": "file:../dev-only" }, "bin": { "tool": "cli.js" } }], + + "lib/tool/dev-only": ["dev-only@file:vendor/dev-only", {}], + + "lib/tool/helper": ["helper@file:vendor/helper", { "bin": { "helper": "cli.js" } }], + } + }" + `); + }); +} + const fileDepCycleFixture = { "package.json": JSON.stringify({ name: "my-app", @@ -10267,9 +10413,9 @@ it("installs file: dependencies that depend on each other", async () => { "b": ["b@file:packages/b", { "dependencies": { "a": "file:../a" } }], - "a/b": ["b@file:packages/b", {}], + "a/b": ["b@file:packages/b", { "dependencies": { "a": "file:../a" } }], - "b/a": ["a@file:packages/a", {}], + "b/a": ["a@file:packages/a", { "dependencies": { "b": "file:../b" } }], } }" `); @@ -10318,33 +10464,71 @@ it("installs file: dependencies that depend on each other from a lockfile that o `); }); -it("fails when a transitive file: dependency's folder does not exist", async () => { - using dir = tempDir("transitive-file-dep-missing", { - "package.json": JSON.stringify({ - name: "my-app", - version: "1.0.0", - dependencies: { - lib: "file:./vendor/lib", +const missingTransitiveFileDepFixture = { + "package.json": JSON.stringify({ + name: "my-app", + version: "1.0.0", + dependencies: { + lib: "file:./vendor/lib", + }, + }), + "vendor/lib/package.json": JSON.stringify({ + name: "lib", + version: "1.0.0", + dependencies: { + nested: "file:../nested", + }, + }), + "vendor/lib/index.js": `module.exports = require("nested");`, +}; + +it.concurrent("fails to resolve when a transitive file: dependency's folder does not exist", async () => { + using dir = tempDir("transitive-file-dep-missing", missingTransitiveFileDepFixture); + + await using proc = spawn({ + cmd: [bunExe(), "install"], + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]); + + // Same failure as a missing file: dependency of the root: the folder is read + // while resolving, so nothing is installed and no lockfile is written. + expect(normalizeBunSnapshot(err, String(dir))).toMatchInlineSnapshot(` + "error: Could not find package.json for "file:vendor/nested" dependency "nested" + error: nested@file:../nested failed to resolve" + `); + expect(out).not.toContain("packages installed"); + expect(await exists(join(String(dir), "bun.lock"))).toBe(false); + expect(await exists(join(String(dir), "node_modules"))).toBe(false); + expect(exitCode).toBe(1); +}); + +it.concurrent("fails to install a lockfile's transitive file: dependency whose folder is missing", async () => { + using dir = tempDir("transitive-file-dep-missing-lock", { + ...missingTransitiveFileDepFixture, + "bun.lock": JSON.stringify({ + lockfileVersion: 1, + workspaces: { + "": { name: "my-app", dependencies: { lib: "file:./vendor/lib" } }, }, - }), - "vendor/lib/package.json": JSON.stringify({ - name: "lib", - version: "1.0.0", - dependencies: { - nested: "file:../nested", + packages: { + "lib": ["lib@file:vendor/lib", { dependencies: { nested: "file:../nested" } }], + "lib/nested": ["nested@file:vendor/nested", {}], }, }), - "vendor/lib/index.js": `module.exports = require("nested");`, }); - const { stdout, stderr, exited } = spawn({ + await using proc = spawn({ cmd: [bunExe(), "install"], cwd: String(dir), stdout: "pipe", stderr: "pipe", env, }); - const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + const [err, out, exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]); // The printed folder path uses the platform separator on Windows. expect(err.replaceAll(sep, "/")).toContain('Could not find folder "file:vendor/nested" for dependency "nested"');