diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index da5691de7ee9..260555560071 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -11,7 +11,7 @@ use bun_collections::{ HashMap as BunHashMap, IdentityContext, LinearFifo, linear_fifo::DynamicBuffer, }; use bun_core::fmt::PathSep; -use bun_core::{Global, Output}; +use bun_core::{Global, Output, UnwrapOrOom as _}; use bun_paths::{MAX_PATH_BYTES, PathBuffer, SEP, SEP_STR, platform, resolve_path}; // `bun_install` sits above `bun_resolver` in the crate graph (no cycle), so use // the real resolver `FileSystem` directly — same as `PackageManager.rs`. @@ -498,6 +498,21 @@ impl Lockfile { } pub fn load_from_dir<'a, const ATTEMPT_LOADING_FROM_OTHER_LOCKFILE: bool>( + &'a mut self, + dir: Fd, + manager: Option<&mut PackageManager>, + log: &mut bun_ast::Log, + ) -> LoadResult<'a> { + let mut result = + self.read_from_dir::(dir, manager, log); + if let LoadResult::Ok(ok) = &mut result { + ok.lockfile.rebase_folder_dependencies().unwrap_or_oom(); + } + result + } + + /// bun.lock, bun.lockb or, failing both, a foreign lockfile to migrate from. + fn read_from_dir<'a, const ATTEMPT_LOADING_FROM_OTHER_LOCKFILE: bool>( &'a mut self, dir: Fd, mut manager: Option<&mut PackageManager>, @@ -712,6 +727,76 @@ impl Lockfile { }) } + /// A lockfile stores a folder dependency as its literal, relative to the + /// declaring package; give the rows of root, workspace and `file:` packages + /// the top-level relative `value.folder` that `Package::parse` gives them. + /// Idempotent, since the literal is left as is. + pub(crate) fn rebase_folder_dependencies(&mut self) -> Result<(), AllocError> { + let Lockfile { + packages, + buffers, + string_pool, + .. + } = self; + let Buffers { + string_bytes, + dependencies, + .. + } = buffers; + let pkgs = packages.slice(); + let mut path_buf = bun_paths::path_buffer_pool::get(); + + for (pkg_resolution, pkg_dependencies) in pkgs + .items_resolution() + .iter() + .zip(pkgs.items_dependencies()) + { + let pkg_dir: SemverString = match pkg_resolution.tag { + ResolutionTag::Root => SemverString::default(), + ResolutionTag::Workspace => *pkg_resolution.workspace(), + ResolutionTag::Folder => *pkg_resolution.folder(), + _ => continue, + }; + + for dep in pkg_dependencies.mut_(dependencies.as_mut_slice()) { + if dep.version.tag != dependency::Tag::Folder { + continue; + } + let relative = { + let buf = string_bytes.as_slice(); + let literal = dep.version.literal.sliced(buf); + let Some(declared) = dependency::parse_with_tag( + dep.name, + Some(dep.name_hash), + literal.slice, + dependency::Tag::Folder, + &literal, + None, + None, + ) else { + continue; + }; + let declared_folder = *declared.folder(); + package::folder_relative_to_top_level_dir( + pkg_dir.slice(buf), + declared_folder.slice(buf), + &mut path_buf[..], + ) + }; + let Some(relative) = relative else { + continue; + }; + dep.version.value.folder = SemverStringBuf { + bytes: &mut *string_bytes, + pool: &mut *string_pool, + } + .append(relative)?; + } + } + + Ok(()) + } + pub(crate) fn is_resolved_dependency_disabled( &self, dep_id: DependencyID, diff --git a/src/install/lockfile/Package.rs b/src/install/lockfile/Package.rs index fcd5b545bd4d..82f649191952 100644 --- a/src/install/lockfile/Package.rs +++ b/src/install/lockfile/Package.rs @@ -111,6 +111,28 @@ pub(crate) fn value_loc_of(source: &bun_ast::Source, key_loc: bun_ast::Loc) -> b crate::bun_json::property_value_loc(&source.contents, key_loc).unwrap_or(key_loc) } +/// The form `Version.value.folder` has in root, workspace and `file:` packages (`None`: too long for `buf`). +pub(crate) fn folder_relative_to_top_level_dir<'a>( + pkg_dir: &[u8], + folder: &[u8], + buf: &'a mut [u8], +) -> Option<&'a [u8]> { + let top_level_dir = FileSystem::instance().top_level_dir(); + let joined = resolve_path::join_abs_string_buf_checked::( + top_level_dir, + buf, + &[pkg_dir, folder], + )?; + let relative = resolve_path::relative(top_level_dir, joined); + // empty when the package depends on itself + let relative: &[u8] = if relative.is_empty() { b"." } else { relative }; + let len = relative.len(); + buf[..len].copy_from_slice(relative); + #[cfg(windows)] + path::dangerously_convert_path_to_posix_in_place::(&mut buf[..len]); + Some(&buf[..len]) +} + #[cold] fn invalid_trusted_dependencies( log: &mut bun_ast::Log, @@ -1846,10 +1868,10 @@ impl Package { dependency::version::Tag::Folder => { let folder = *dependency_version.folder(); let mut folder_buf = PathBuffer::uninit(); - let Some(joined) = resolve_path::join_abs_string_buf_checked::( - FileSystem::instance().top_level_dir(), + let Some(relative) = folder_relative_to_top_level_dir( + source.path.name().dir, + folder.slice(buf), &mut folder_buf.0, - &[source.path.name().dir, folder.slice(buf)], ) else { log.add_error_fmt( source, @@ -1861,20 +1883,7 @@ impl Package { ); return Err(crate::Error::InstallFailed); }; - let relative: &[u8] = - resolve_path::relative(FileSystem::instance().top_level_dir(), joined); - #[cfg(windows)] - let relative: &[u8] = { - let len = relative.len(); - folder_buf.0[..len].copy_from_slice(relative); - path::dangerously_convert_path_to_posix_in_place::( - &mut folder_buf.0[..len], - ); - &folder_buf.0[..len] - }; - // if relative is empty, we are linking the package to itself - dependency_version.value.folder = string_builder - .append::(if relative.is_empty() { b"." } else { relative }); + dependency_version.value.folder = string_builder.append::(relative); } dependency::version::Tag::Npm => { if let Some(workspace_version) = workspace_version { diff --git a/src/install/pnpm.rs b/src/install/pnpm.rs index e14532accf02..5bf088835ef1 100644 --- a/src/install/pnpm.rs +++ b/src/install/pnpm.rs @@ -1620,6 +1620,7 @@ pub(crate) fn migrate_pnpm_lockfile<'a>( // pnpm records `os`/`cpu` for every `packages:` entry whose manifest // declares them, including `file:` folders, tarballs, and git packages. crate::migration::clear_non_registry_platform_constraints(lockfile); + declare_folder_dependencies_relative_to_their_package(lockfile)?; lockfile.resolve(log)?; @@ -1780,6 +1781,48 @@ fn declared_package_peers( Ok(peers) } +/// pnpm records a folder package's `file:` dependencies relative to the lockfile +/// (`file:sub-dep/child` for a declared `file:./child`), which the resolution +/// pass above keys on; bun.lock stores them relative to the declaring package, +/// as declared. Importers already carry the declared `specifier`. +fn declare_folder_dependencies_relative_to_their_package( + lockfile: &mut Lockfile, +) -> Result<(), AllocError> { + let mut literal: Vec = Vec::new(); + for pkg_id in 0..lockfile.packages.len() { + let pkg_resolution = lockfile.packages.items_resolution()[pkg_id]; + if pkg_resolution.tag != resolution::Tag::Folder { + continue; + } + let pkg_dir = *pkg_resolution.folder(); + let rows = lockfile.packages.items_dependencies()[pkg_id]; + + for dep_id in rows.begin() as usize..rows.end() as usize { + let dep = &lockfile.buffers.dependencies[dep_id]; + if dep.version.tag != dependency::VersionTag::Folder { + continue; + } + let folder = *dep.version.folder(); + + literal.clear(); + literal.extend_from_slice(b"file:"); + let relative = bun_paths::resolve_path::relative( + pkg_dir.slice(string_bytes!(lockfile)), + folder.slice(string_bytes!(lockfile)), + ); + literal.extend_from_slice(if relative.is_empty() { b"." } else { relative }); + #[cfg(windows)] + bun_paths::dangerously_convert_path_to_posix_in_place::( + &mut literal[b"file:".len()..], + ); + + let appended = sbuf!(lockfile).append(&literal)?; + lockfile.buffers.dependencies[dep_id].version.literal = appended; + } + } + Ok(()) +} + fn parse_append_package_dependencies( lockfile: &mut Lockfile, package_obj: &Expr, diff --git a/test/cli/install/bun-lock.test.ts b/test/cli/install/bun-lock.test.ts index c8ba27d17e5f..147bd4d2a56a 100644 --- a/test/cli/install/bun-lock.test.ts +++ b/test/cli/install/bun-lock.test.ts @@ -1826,3 +1826,113 @@ describe.each(["hoisted", "isolated"] as const)("peer no published version satis expect(await file(lockfilePath).text()).toBe(lockfile); }); }); + +// A lockfile stores the `file:` path of a dependency exactly as the declaring +// package.json wrote it, i.e. relative to that package.json, while a fresh parse +// of the same package.json stores it relative to the project. The rows below +// only get resolved again from the loaded lockfile (an override for their name +// went away, or `bun update ` targets them), which is when the two forms +// have to agree. +describe.concurrent("re-resolving file: dependencies declared by a file: package or a workspace", () => { + const pkg = (name: string, version: string, dependencies?: Record) => + JSON.stringify({ name, version, dependencies }); + const app = (overrides?: Record) => + JSON.stringify({ name: "app", dependencies: { a: "file:./vendor/a" }, overrides }); + const installedVersion = async (packageDir: string, ...segments: string[]) => + (await file(join(packageDir, ...segments, "package.json")).json()).version; + + // `a` declares `b` relative to itself; the override redirects `b` elsewhere. + const vendored = { + "vendor/a/package.json": pkg("a", "1.0.0", { b: "file:../b" }), + "vendor/b/package.json": pkg("b", "1.0.0"), + "vendor/b2/package.json": pkg("b", "2.0.0"), + }; + + it.each([ + ["bun.lock", true], + ["bun.lockb", false], + ])("removing an override resolves the file: package's dependency next to it again (%s)", async (_, text) => { + const { packageDir, packageJson } = await registry.createTestDir({ + bunfigOpts: { linker: "hoisted", saveTextLockfile: text }, + files: { ...vendored, "package.json": app({ b: "file:./vendor/b2" }) }, + }); + const run = makeInstallRunner(packageDir); + + await run(["install"]); + expect(await installedVersion(packageDir, "node_modules", "a", "node_modules", "b")).toBe("2.0.0"); + expect(await exists(join(packageDir, text ? "bun.lock" : "bun.lockb"))).toBe(true); + + await write(packageJson, app()); + await run(["install"]); + expect(await installedVersion(packageDir, "node_modules", "a", "node_modules", "b")).toBe("1.0.0"); + await run(["install", "--frozen-lockfile"]); + + await run(["install", "--save-text-lockfile", "--lockfile-only"]); + const lockfile = await file(join(packageDir, "bun.lock")).text(); + expect(lockfile).toContain('"a": ["a@file:vendor/a", { "dependencies": { "b": "file:../b" } }]'); + expect(lockfile).toContain('"a/b": ["b@file:vendor/b", {}]'); + }); + + it("a path without .. is resolved from the declaring package, not from the project root", async () => { + const { packageDir, packageJson } = await registry.createTestDir({ + bunfigOpts: { linker: "hoisted" }, + files: { + "vendor/a/package.json": pkg("a", "1.0.0", { b: "file:./b" }), + "vendor/a/b/package.json": pkg("b", "1.0.0"), + // same relative path from the project root + "b/package.json": pkg("b", "9.9.9"), + "vendor/b2/package.json": pkg("b", "2.0.0"), + "package.json": app({ b: "file:./vendor/b2" }), + }, + }); + const run = makeInstallRunner(packageDir); + + await run(["install"]); + await write(packageJson, app()); + await run(["install"]); + expect(await file(join(packageDir, "bun.lock")).text()).toContain('"a/b": ["b@file:vendor/a/b", {}]'); + expect(await installedVersion(packageDir, "node_modules", "a", "node_modules", "b")).toBe("1.0.0"); + }); + + it("bun update re-resolves the file: package's dependency", async () => { + const { packageDir } = await registry.createTestDir({ + bunfigOpts: { linker: "hoisted" }, + files: { ...vendored, "package.json": app() }, + }); + const run = makeInstallRunner(packageDir); + + await run(["install"]); + const lockfile = await file(join(packageDir, "bun.lock")).text(); + expect(lockfile).toContain('"a/b": ["b@file:vendor/b", {}]'); + + await run(["update", "b"]); + expect(await file(join(packageDir, "bun.lock")).text()).toBe(lockfile); + expect(await installedVersion(packageDir, "node_modules", "a", "node_modules", "b")).toBe("1.0.0"); + }); + + it("removing an override resolves a workspace's file: dependency relative to the workspace again", async () => { + const workspaceRoot = (overrides?: Record) => + JSON.stringify({ name: "app", workspaces: ["packages/*"], overrides }); + const { packageDir, packageJson } = await registry.createTestDir({ + bunfigOpts: { linker: "hoisted" }, + files: { + "packages/ws1/package.json": pkg("ws1", "1.0.0", { b: "file:../../vendor/b" }), + "vendor/b/package.json": pkg("b", "1.0.0"), + "vendor/b2/package.json": pkg("b", "2.0.0"), + "package.json": workspaceRoot({ b: "file:./vendor/b2" }), + }, + }); + const run = makeInstallRunner(packageDir); + + await run(["install"]); + expect(await file(join(packageDir, "bun.lock")).text()).toContain('"ws1/b": ["b@file:vendor/b2", {}]'); + + await write(packageJson, workspaceRoot()); + await run(["install"]); + const lockfile = await file(join(packageDir, "bun.lock")).text(); + expect(lockfile).toContain('"b": "file:../../vendor/b"'); + expect(lockfile).toContain('"ws1/b": ["b@file:vendor/b", {}]'); + expect(await installedVersion(packageDir, "packages", "ws1", "node_modules", "b")).toBe("1.0.0"); + await run(["install", "--frozen-lockfile"]); + }); +}); diff --git a/test/cli/install/migration/__snapshots__/pnpm-lock-v9.test.ts.snap b/test/cli/install/migration/__snapshots__/pnpm-lock-v9.test.ts.snap index 7c9edc41e8a4..8f622d8afc82 100644 --- a/test/cli/install/migration/__snapshots__/pnpm-lock-v9.test.ts.snap +++ b/test/cli/install/migration/__snapshots__/pnpm-lock-v9.test.ts.snap @@ -42,7 +42,7 @@ exports[`pnpm-lock.yaml v9 snapshot alias whose dep-path version is a file: dire "packages": { "fork": ["bar@github:o/bar#aeb6b15f9c9957c8fa56f9731e914c4d8a6d2f2b", {}, ""], - "outer": ["outer@file:outer", { "dependencies": { "config": "file:shared/config", "fork": "https://codeload.github.com/o/bar/tar.gz/aeb6b15f9c9957c8fa56f9731e914c4d8a6d2f2b" } }], + "outer": ["outer@file:outer", { "dependencies": { "config": "file:../shared/config", "fork": "https://codeload.github.com/o/bar/tar.gz/aeb6b15f9c9957c8fa56f9731e914c4d8a6d2f2b" } }], "outer/config": ["hi2@file:shared/config", {}], } diff --git a/test/cli/install/migration/migrate.test.ts b/test/cli/install/migration/migrate.test.ts index 2429afe854aa..4bed40a63f35 100644 --- a/test/cli/install/migration/migrate.test.ts +++ b/test/cli/install/migration/migrate.test.ts @@ -217,6 +217,49 @@ test("npm lockfile migration skips extraneous packages that also declare inBundl expect(fs.existsSync(join(testDir, "bun.lock"))).toBeTrue(); }); +test("bun update re-resolves a file: dependency of a file: package in the same run as the migration", async () => { + // package-lock.json (as `npm install --package-lock-only` writes it) keeps `b` the way + // vendor/a/package.json declares it, relative to vendor/a. Updating `b` resolves that row again + // straight from the migrated lockfile, so it has to be looked up from vendor/a, not the project. + await using testDir = tempDir("migrate-nested-file-dep", { + "package.json": JSON.stringify({ name: "app", dependencies: { a: "file:./vendor/a" } }), + "vendor/a/package.json": JSON.stringify({ name: "a", version: "1.0.0", dependencies: { b: "file:../b" } }), + "vendor/b/package.json": JSON.stringify({ name: "b", version: "1.0.0" }), + "package-lock.json": JSON.stringify({ + name: "app", + lockfileVersion: 3, + requires: true, + packages: { + "": { name: "app", dependencies: { a: "file:./vendor/a" } }, + "node_modules/a": { resolved: "vendor/a", link: true }, + "node_modules/b": { resolved: "vendor/b", link: true }, + "vendor/a": { version: "1.0.0", dependencies: { b: "file:../b" } }, + "vendor/b": { version: "1.0.0" }, + }, + }), + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "update", "b"], + cwd: String(testDir), + env: bunEnv, + stdout: "ignore", + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + expect(stderr).toContain("migrated lockfile from package-lock.json"); + expect(stderr).not.toContain("error:"); + expect(exitCode).toBe(0); + + const bunLock = await Bun.file(join(testDir, "bun.lock")).text(); + expect(bunLock).toContain(`"a": ["a@file:vendor/a", { "dependencies": { "b": "file:../b" } }]`); + expect(bunLock).toContain(`"a/b": ["b@file:vendor/b", {}]`); + expect(await Bun.file(join(testDir, "node_modules", "a", "node_modules", "b", "package.json")).json()).toEqual({ + name: "b", + version: "1.0.0", + }); +}); + test("package-lock.json migration requires integrity for tarball URLs outside the configured registry", async () => { // A package-lock.json entry whose `resolved` tarball URL points outside the configured // registry and that carries no `integrity` field must not be imported as-is. The bun.lock diff --git a/test/cli/install/migration/pnpm-lock-v9.test.ts b/test/cli/install/migration/pnpm-lock-v9.test.ts index b943197ac326..bf43c82e5542 100644 --- a/test/cli/install/migration/pnpm-lock-v9.test.ts +++ b/test/cli/install/migration/pnpm-lock-v9.test.ts @@ -999,7 +999,8 @@ snapshots: expect(exitCode).toBe(0); const bunLock = await bunLockOf(String(dir)); - expect(bunLock).toContain(`"config": "file:shared/config"`); + // as outer/package.json declares it, not pnpm's lockfile-relative `file:shared/config` + expect(bunLock).toContain(`"config": "file:../shared/config"`); expect(bunLock).toContain( `"fork": "https://codeload.github.com/o/bar/tar.gz/aeb6b15f9c9957c8fa56f9731e914c4d8a6d2f2b"`, ); @@ -1081,8 +1082,15 @@ snapshots: const bunLock = await bunLockOf(String(dir)); expect(bunLock).toContain(`"sub-dep": "file:./sub-dep"`); - expect(bunLock).toContain(`["sub-dep@file:sub-dep", { "dependencies": { "nested-child": "file:sub-dep/child" } }]`); + // pnpm-lock.yaml has `nested-child: file:sub-dep/child`; bun.lock stores the path relative to + // sub-dep, the way sub-dep/package.json declares it, and resolves it from there when loaded. + expect(bunLock).toContain(`["sub-dep@file:sub-dep", { "dependencies": { "nested-child": "file:child" } }]`); expect(bunLock).toContain(`["nested-child@file:sub-dep/child", {}]`); + + const update = await run(String(dir), "update", "nested-child"); + expect(update.stderr).not.toContain("error:"); + expect(update.exitCode).toBe(0); + expect(await bunLockOf(String(dir))).toBe(bunLock); }); test.concurrent("codeload tarballs with and without gitHosted: true", async () => {