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
101 changes: 101 additions & 0 deletions src/cli/pack_command.zig
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,107 @@ pub const PackCommand = struct {
}
}

pub const PublishablePaths = struct {
arena: std.heap.ArenaAllocator,
/// Relative POSIX subpaths (sentinel-terminated) of files that
/// `bun pm pack` would include in the published tarball. Includes
/// `bin` entries and entries reachable via `package.json#files`
/// (or the entire tree if `files` is absent), and the auto-included
/// `package.json`. Default ignores plus `.npmignore` / `.gitignore`
/// at every depth are honored.
paths: []const [:0]const u8,

pub fn deinit(this: *@This()) void {
this.arena.deinit();
}
};

/// Single source of truth for "what files would `bun pm pack` ship?".
/// Used by the isolated linker (when honoring an active `bun link`) so
/// the contents of `node_modules/.bun/<pkg>/` match what a publish of
/// the producer would contain — including the auto-included
/// `package.json`, `bin` entries outside the `files` whitelist, and
/// the recursive semantics of `files: ["dist/**/*.js"]`-style globs.
/// Without this, callers were forced to mirror the publish rules by
/// hand and inevitably drifted (cf. nested same-name directories).
pub fn collectPublishablePaths(
parent_allocator: std.mem.Allocator,
root_dir: std.fs.Dir,
json_root: Expr,
) OOM!PublishablePaths {
var arena = std.heap.ArenaAllocator.init(parent_allocator);
errdefer arena.deinit();
const allocator = arena.allocator();

var pack_queue = PackQueue.init(allocator, {});

const bins = try getPackageBins(allocator, json_root);

for (bins) |bin| {
switch (bin.type) {
.file => try pack_queue.add(.{ .path = bin.path, .optional = true }),
.dir => {
const bin_dir = root_dir.openDir(bin.path, .{ .iterate = true }) catch continue;
try iterateProjectTree(allocator, &pack_queue, &.{}, .{ bin_dir, bin.path, 2 }, .silent);
},
}
}

if (json_root.get("files")) |files| {
if (files.asArray()) |_files_array| {
var includes: std.ArrayListUnmanaged(Pattern) = .{};
var excludes: std.ArrayListUnmanaged(Pattern) = .{};

var path_buf: PathBuffer = undefined;
var files_array = _files_array;
while (files_array.next()) |files_entry| {
const file_entry_str = files_entry.asString(allocator) orelse continue;
const normalized = bun.path.normalizeBuf(file_entry_str, &path_buf, .posix);
const parsed = try Pattern.fromUTF8(allocator, normalized) orelse continue;
if (parsed.flags.negated) {
try excludes.append(allocator, parsed);
} else {
try includes.append(allocator, parsed);
}
}

try iterateIncludedProjectTree(
allocator,
&pack_queue,
bins,
includes.items,
excludes.items,
root_dir,
.silent,
);
} else {
// `files` not an array → malformed manifest. `bun pm pack`
// crashes here; we can't, so we mirror the no-`files` path
// (publish-default tree) instead of dropping everything.
try iterateProjectTree(allocator, &pack_queue, bins, .{ root_dir, "", 1 }, .silent);
}
} else {
try iterateProjectTree(allocator, &pack_queue, bins, .{ root_dir, "", 1 }, .silent);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// `package.json` is unconditionally included — both iterators skip
// it explicitly because the pack pipeline writes it from a
// normalized AST. We don't have that pipeline; just ship the file.
const pkg_path = try allocator.dupeZ(u8, "package.json");

var paths = try allocator.alloc([:0]const u8, pack_queue.count() + 1);
paths[0] = pkg_path;
var i: usize = 1;
while (pack_queue.removeOrNull()) |item| : (i += 1) {
paths[i] = item.path;
}

return .{
.arena = arena,
.paths = paths,
};
Comment on lines +928 to +1011

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

collectPublishablePaths() still drops bundled dependencies.

This helper is described as the pack-time source of truth, but unlike pack() it never calls iterateBundledDeps(). A linked producer with bundledDependencies/bundleDependencies will therefore materialize without its vendored node_modules/... subtree even though bun pm pack would ship it. Please fold the bundled-deps walk into this helper (or extract a shared collector used by both paths).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli/pack_command.zig` around lines 928 - 1011, collectPublishablePaths
currently never walks vendored bundled dependencies so packages with
bundledDependencies/bundleDependencies are missing their node_modules subtree;
fix by invoking the same bundled-deps collector used by pack()
(iterateBundledDeps) and feeding its results into the PackQueue so they become
part of paths returned by collectPublishablePaths. Locate
collectPublishablePaths and, after parsing json_root (before assembling pkg_path
and paths), detect bundledDependencies/bundleDependencies and call
iterateBundledDeps(allocator, &pack_queue, /*pass same root_dir/json_root/bins
as pack() does*/), or refactor to a shared helper used by both pack() and
collectPublishablePaths; ensure items produced by iterateBundledDeps are added
via PackQueue.add so they are included in the final paths array.

}

fn getBundledDeps(
allocator: std.mem.Allocator,
json: Expr,
Expand Down
12 changes: 12 additions & 0 deletions src/install/PackageManager.zig
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,16 @@ global_link_dir: ?std.fs.Dir = null,
global_dir: ?std.fs.Dir = null,
global_link_dir_path: string = "",

/// Names of packages registered via `bun link`, read from
/// `<globalLinkDir>/` once per install. Populated on the main thread
/// before any install worker starts; after that it's read-only and can
/// be accessed lock-free from worker threads. Lookups return early
/// when the set is empty (no active links) — the common case on dev
/// machines without `bun link` configured, and unconditional on CI.
/// See `populateLinkedNamesCache` / `linkedPackagePath`.
linked_names: bun.StringHashMapUnmanaged(void) = .{},
linked_names_populated: bool = false,

onWake: WakeHandler = .{},
ci_mode: bun.LazyBool(computeIsContinuousIntegration, @This(), "ci_mode") = .{},

Expand Down Expand Up @@ -1177,6 +1187,8 @@ pub const getTemporaryDirectory = directories.getTemporaryDirectory;
pub const globalLinkDir = directories.globalLinkDir;
pub const globalLinkDirAndPath = directories.globalLinkDirAndPath;
pub const globalLinkDirPath = directories.globalLinkDirPath;
pub const linkedPackagePath = directories.linkedPackagePath;
pub const populateLinkedNamesCache = directories.populateLinkedNamesCache;
pub const isFolderInCache = directories.isFolderInCache;
pub const pathForCachedNPMPath = directories.pathForCachedNPMPath;
pub const pathForResolution = directories.pathForResolution;
Expand Down
117 changes: 117 additions & 0 deletions src/install/PackageManager/PackageManagerDirectories.zig
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,123 @@ pub fn globalLinkDirAndPath(this: *PackageManager) struct { std.fs.Dir, []const
return .{ dir, this.global_link_dir_path };
}

/// Read the global link dir once and populate
/// `this.linked_names` with every registered package name (including
/// scoped names as `@scope/name`). Must be called on the main thread
/// before any install worker touches `linkedPackagePath`; after that
/// the map is read-only and lock-free.
///
/// Safe to call repeatedly; subsequent calls are no-ops.
pub fn populateLinkedNamesCache(this: *PackageManager) void {
if (this.linked_names_populated) return;
this.linked_names_populated = true;

// Best-effort: for users who have never run `bun link`, the global
// link dir may not exist (or may be unreadable). `globalLinkDirPath`
// would `Global.exit(1)` on setup failure — treat that same failure
// here as "no links on this machine" instead, and leave the cache
// empty so `linkedPackagePath` short-circuits to null. If a later
// code path really does need the global dir (e.g. `bun link`
// itself), `globalLinkDirPath` will be called on that path and
// surface the error there.
const dir_path = dir_path: {
if (this.global_link_dir_path.len != 0) break :dir_path this.global_link_dir_path;
var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
this.global_dir = global_dir;
this.global_link_dir = link_dir;
var buf: bun.PathBuffer = undefined;
const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return;
this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice));
Comment on lines +434 to +440

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid caching a half-initialized global link dir.

If bun.getFdPath() fails here, populateLinkedNamesCache() returns with this.global_link_dir already set but this.global_link_dir_path still empty. A later globalLinkDirPath() call then skips re-initialization and can build linked-package paths from "". Move the field assignments until after the path lookup succeeds, and close the local dirs on the early-return path.

Suggested fix
-        var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
-        const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
-        this.global_dir = global_dir;
-        this.global_link_dir = link_dir;
+        var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
+        errdefer global_dir.close();
+        const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
+        errdefer link_dir.close();
         var buf: bun.PathBuffer = undefined;
         const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return;
+        this.global_dir = global_dir;
+        this.global_link_dir = link_dir;
         this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
this.global_dir = global_dir;
this.global_link_dir = link_dir;
var buf: bun.PathBuffer = undefined;
const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return;
this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice));
var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
errdefer global_dir.close();
const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
errdefer link_dir.close();
var buf: bun.PathBuffer = undefined;
const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return;
this.global_dir = global_dir;
this.global_link_dir = link_dir;
this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/install/PackageManager/PackageManagerDirectories.zig` around lines 434 -
440, The code sets this.global_dir and this.global_link_dir before calling
bun.getFdPath, which can fail and leave this.global_link_dir_path empty; move
the assignments to this.global_dir and this.global_link_dir until after
bun.getFdPath and the call to Fs.FileSystem.DirnameStore.instance.append(...)
succeed so the object is never left half-initialized; if bun.getFdPath or the
path append fails, close the opened local dir handles (the values returned by
Options.openGlobalDir and global_dir.makeOpenPath) before returning to avoid
resource leaks; ensure populateLinkedNamesCache() and globalLinkDirPath() will
see either all three fields set or none set.

break :dir_path this.global_link_dir_path;
};
const root_fd = switch (bun.openDirForIteration(bun.FD.cwd(), dir_path)) {
.result => |fd| fd,
// Dir missing / unreadable → empty set. Every linkedPackagePath
// lookup will short-circuit to null with no further syscalls.
.err => return,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
};
defer root_fd.close();

var iter = bun.DirIterator.iterate(root_fd, if (bun.Environment.isWindows) .u16 else .u8);
while (iter.next().unwrap() catch null) |entry| {
const name = entry.name.slice();
if (name.len == 0) continue;

// Scope dirs (`@scope`) contain the actual links nested one
// level deeper; flatten to `@scope/name` in the cache.
if (name[0] == '@' and entry.kind == .directory) {
if (comptime bun.Environment.isWindows) {
// WTF-16 name; skip scope flattening on Windows for now.
// Falls through to the lstat path in linkedPackagePath.
continue;
}
const scope_fd = switch (bun.openDirForIteration(root_fd, name)) {
.result => |fd| fd,
.err => continue,
};
defer scope_fd.close();

var scope_iter = bun.DirIterator.iterate(scope_fd, .u8);
while (scope_iter.next().unwrap() catch null) |scope_entry| {
const sub_name = scope_entry.name.slice();
if (sub_name.len == 0) continue;
const full = bun.handleOom(std.fmt.allocPrint(this.allocator, "{s}/{s}", .{ name, sub_name }));
bun.handleOom(this.linked_names.put(this.allocator, full, {}));
}
continue;
}

if (comptime bun.Environment.isWindows) continue;
const dup = bun.handleOom(this.allocator.dupe(u8, name));
bun.handleOom(this.linked_names.put(this.allocator, dup, {}));
}
}

/// If `<globalLinkDir>/<pkg_name>` exists (typically a symlink created by
/// `bun link` from the producer dir), write its absolute path into `buf` and
/// return it. Otherwise `null`. Scoped names (`@scope/name`) are handled
/// because `joinAbsStringBufZ` preserves the `/`.
///
/// Performance: when `linked_names` has been populated (via
/// `populateLinkedNamesCache` at install start), this is a single
/// hashmap check with no syscalls. When the cache is empty (no active
/// links on this machine), it returns immediately. Falls back to the
/// per-call `lstat` when the cache has not been populated (e.g. on
/// Windows, or if a caller runs outside the isolated-install flow).
pub fn linkedPackagePath(
this: *PackageManager,
pkg_name: []const u8,
buf: *bun.PathBuffer,
) ?[:0]const u8 {
if (pkg_name.len == 0) return null;

const use_cache = this.linked_names_populated and !bun.Environment.isWindows;
if (use_cache) {
if (this.linked_names.count() == 0) return null;
if (!this.linked_names.contains(pkg_name)) return null;
const dir_path = this.globalLinkDirPath();
return bun.path.joinAbsStringBufZ(dir_path, buf, &.{pkg_name}, .auto);
}

const dir_path = this.globalLinkDirPath();
const joined = bun.path.joinAbsStringBufZ(dir_path, buf, &.{pkg_name}, .auto);
if (comptime bun.Environment.isWindows) {
const attrs = bun.sys.getFileAttributes(joined) orelse return null;
return if (attrs.is_directory or attrs.is_reparse_point) joined else null;
}
return switch (bun.sys.lstat(joined)) {
.result => |st| brk: {
const mode: u32 = @intCast(st.mode);
if (std.posix.S.ISDIR(mode) or std.posix.S.ISLNK(mode)) {
break :brk joined;
}
break :brk null;
},
.err => null,
};
}

pub fn pathForCachedNPMPath(
this: *PackageManager,
buf: *bun.PathBuffer,
Expand Down
9 changes: 9 additions & 0 deletions src/install/PackageManager/WorkspacePackageJSONCache.zig
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@ pub const GetResult = union(enum) {
};

map: Map = .{},
/// `getWithPath` / `getWithSource` mutate `map` (which invalidates
/// previously-returned `*MapEntry` pointers on grow) and call
/// `initializeStore()` + the JSON parser — none of which are
/// thread-safe. Multi-threaded callers (currently only the isolated
/// installer's `Task.run`) must hold this mutex across both the call
/// *and* any use of the returned entry pointer, since a concurrent
/// call would otherwise grow the map and invalidate the pointer.
/// Single-threaded callers can ignore it.
lock: bun.Mutex = .{},

/// Given an absolute path to a workspace package.json, return the AST
/// and contents of the file. If the package.json is not present in the
Expand Down
1 change: 1 addition & 0 deletions src/install/PackageManager/patchPackage.zig
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,7 @@ fn overwritePackageInNodeModulesFolder(
.fromStdDir(cached_package_folder),
src_path,
dest_subpath,
&.{},
ignore_directories,
);
defer copier.deinit();
Expand Down
30 changes: 30 additions & 0 deletions src/install/isolated_install.zig
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ pub fn installIsolatedPackages(
) OOM!PackageInstall.Summary {
bun.analytics.Features.isolated_bun_install += 1;

// Populate the linked-names cache once, on the main thread, before
// any install worker calls `manager.linkedPackagePath()`. Replaces a
// per-dependency `lstat` with a single readdir of the global link
// dir; short-circuits every subsequent lookup when nothing is
// linked (the common case on CI and dev machines without active
// links). See populateLinkedNamesCache in
// PackageManager/PackageManagerDirectories.zig.
manager.populateLinkedNamesCache();

const lockfile = manager.lockfile;

const store: Store = store: {
Expand Down Expand Up @@ -1577,6 +1586,17 @@ pub fn installIsolatedPackages(
// write the new project-local tree through the link into
// the shared cache). Treat the stale link as
// needs-install so `link_package` detaches and rebuilds.
// An active `bun link` for this package name means the
// producer dir is the source of truth — override the
// cache-based materialization regardless of whether the
// store dir already exists. The existence check below
// would otherwise short-circuit and leave the body stale.
// Only fires when the user didn't opt into the
// symlink-only backend.
var link_buf: bun.PathBuffer = undefined;
const has_active_link = PackageInstall.supported_method != .symlink and
manager.linkedPackagePath(pkg_name.slice(string_buf), &link_buf) != null;

const has_stale_gvs_link = !uses_global_store and stale: {
if (installer.global_store_path == null) break :stale false;
var local: bun.Path(.{ .sep = .auto }) = .initTopLevelDir();
Expand All @@ -1599,6 +1619,7 @@ pub fn installIsolatedPackages(
// should still take the cheap symlink-only path.
(is_new_bun_modules and !uses_global_store) or
has_stale_gvs_link or
has_active_link or
patch_info == .remove or
needs_install: {
var store_path: bun.AbsPath(.{}) = .initTopLevelDir();
Expand Down Expand Up @@ -1657,6 +1678,15 @@ pub fn installIsolatedPackages(
continue;
}

// `link_package` will source from the producer dir via
// `linkedPackagePath`; skip the cache-fetch dance entirely
// (mirrors how `.folder` is handled — no registry traffic
// needed when the body comes from an on-disk producer).
if (has_active_link) {
installer.startTask(entry_id);
continue;
}

var pkg_cache_dir_subpath: bun.RelPath(.{ .sep = .auto }) = .from(switch (pkg_res_tag) {
.npm => manager.cachedNPMPackageFolderName(pkg_name.slice(string_buf), pkg_res.value.npm.version, patch_info.contentsHash()),
.git => manager.cachedGitFolderName(&pkg_res.value.git, patch_info.contentsHash()),
Expand Down
3 changes: 2 additions & 1 deletion src/install/isolated_install/FileCopier.zig
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pub const FileCopier = struct {
src_dir: FD,
src_path: bun.AbsPath(.{ .sep = .auto, .unit = .os }),
dest_subpath: bun.Path(.{ .sep = .auto, .unit = .os }),
skip_filenames: []const bun.OSPathSlice,
skip_dirnames: []const bun.OSPathSlice,
) OOM!FileCopier {
return .{
Expand All @@ -16,7 +17,7 @@ pub const FileCopier = struct {
var w = try Walker.walk(
src_dir,
bun.default_allocator,
&.{},
skip_filenames,
skip_dirnames,
);
w.resolve_unknown_entry_types = true;
Expand Down
3 changes: 2 additions & 1 deletion src/install/isolated_install/Hardlinker.zig
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ pub fn init(
folder_dir: FD,
src: bun.AbsPath(.{ .sep = .auto, .unit = .os }),
dest: bun.Path(.{ .sep = .auto, .unit = .os }),
skip_filenames: []const bun.OSPathSlice,
skip_dirnames: []const bun.OSPathSlice,
) OOM!Hardlinker {
return .{
Expand All @@ -19,7 +20,7 @@ pub fn init(
var w = try Walker.walk(
folder_dir,
bun.default_allocator,
&.{},
skip_filenames,
skip_dirnames,
);
w.resolve_unknown_entry_types = true;
Expand Down
Loading