Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flaky patch test #17301

Merged
merged 3 commits into from
Feb 13, 2025
+111 −86
Merged

Fix flaky patch test #17301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c644a1d
Fix flaky patch test
Jarred-Sumner Feb 13, 2025
59a0587
Merge branch 'main' into jarred/fix-patch
Jarred-Sumner Feb 13, 2025
2f04f3e
Update sys.zig
Jarred-Sumner Feb 13, 2025
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
3 changes: 2 additions & 1 deletion src/install/install.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6369,13 +6369,14 @@ pub const PackageManager = struct {
installer.node_modules.path = path;
installer.current_tree_id = ctx.tree_id;
const pkg_id = ptask.callback.apply.pkg_id;
const resolution = &manager.lockfile.packages.items(.resolution)[pkg_id];

installer.installPackageWithNameAndResolution(
ctx.dependency_id,
pkg_id,
log_level,
ptask.callback.apply.pkgname,
ptask.callback.apply.resolution,
resolution,
false,
false,
);
Expand Down
122 changes: 71 additions & 51 deletions src/install/patch_install.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ pub const PatchTask = struct {
pkg_id: PackageID,
patch_hash: u64,
name_and_version_hash: u64,
resolution: *const Resolution,

patchfilepath: []const u8,
pkgname: String,

Expand Down Expand Up @@ -262,14 +262,13 @@ pub const PatchTask = struct {
debug("apply patch task", .{});
bun.assert(this.callback == .apply);

const strbuf: []const u8 = this.manager.lockfile.buffers.string_bytes.items;

const patch: *const ApplyPatch = &this.callback.apply;
const dir = this.project_dir;
const patchfile_path = patch.patchfilepath;

var absolute_patchfile_path_buf: bun.PathBuffer = undefined;
// 1. Parse the patch file
const absolute_patchfile_path = bun.path.joinZ(&[_][]const u8{
const absolute_patchfile_path = bun.path.joinZBuf(&absolute_patchfile_path_buf, &[_][]const u8{
dir,
patchfile_path,
}, .auto);
Expand Down Expand Up @@ -310,14 +309,18 @@ pub const PatchTask = struct {

const pkg_name = this.callback.apply.pkgname;

var resolution_buf: [512]u8 = undefined;
const resolution_label = std.fmt.bufPrint(&resolution_buf, "{}", .{this.callback.apply.resolution.fmt(strbuf, .posix)}) catch unreachable;

const dummy_node_modules: PackageManager.NodeModulesFolder = .{
.path = std.ArrayList(u8).init(this.manager.allocator),
.tree_id = 0,
};

const resolution_label, const resolution_tag = brk: {
// TODO: fix this threadsafety issue.
const resolution = &this.manager.lockfile.packages.items(.resolution)[patch.pkg_id];
break :brk .{ std.fmt.allocPrint(bun.default_allocator, "{}", .{resolution.fmt(this.manager.lockfile.buffers.string_bytes.items, .posix)}) catch bun.outOfMemory(), resolution.tag };
};
defer this.manager.allocator.free(resolution_label);

// 3. copy the unpatched files into temp dir
var pkg_install = PreparePatchPackageInstall{
.allocator = bun.default_allocator,
Expand All @@ -333,7 +336,7 @@ pub const PatchTask = struct {
.lockfile = this.manager.lockfile,
};

switch (pkg_install.installImpl(true, system_tmpdir, .copyfile, this.callback.apply.resolution.tag)) {
switch (pkg_install.installImpl(true, system_tmpdir, .copyfile, resolution_tag)) {
.success => {},
.failure => |reason| {
return try log.addErrorFmtOpts(
Expand All @@ -345,50 +348,62 @@ pub const PatchTask = struct {
},
}

var patch_pkg_dir = system_tmpdir.openDir(tempdir_name, .{}) catch |e| return try log.addErrorFmtOpts(
this.manager.allocator,
"failed trying to open temporary dir to apply patch to package: {s}",
.{@errorName(e)},
.{},
);
defer patch_pkg_dir.close();

// 4. apply patch
if (patchfile.apply(this.manager.allocator, bun.toFD(patch_pkg_dir.fd))) |e| {
return try log.addErrorFmtOpts(
this.manager.allocator,
"failed applying patch file: {}",
.{e},
.{},
);
}
{
const patch_pkg_dir = switch (bun.sys.openat(
bun.toFD(system_tmpdir.fd),
tempdir_name,
bun.O.RDONLY | bun.O.DIRECTORY,
0,
)) {
.result => |fd| fd,
.err => |e| return try log.addSysError(
this.manager.allocator,
e,
"failed trying to open temporary dir to apply patch to package: {s}",
.{resolution_label},
),
};
defer _ = bun.sys.close(patch_pkg_dir);

// 5. Add bun tag
const bun_tag_prefix = bun_hash_tag;
var buntagbuf: BuntagHashBuf = undefined;
@memcpy(buntagbuf[0..bun_tag_prefix.len], bun_tag_prefix);
const hashlen = (std.fmt.bufPrint(buntagbuf[bun_tag_prefix.len..], "{x}", .{this.callback.apply.patch_hash}) catch unreachable).len;
buntagbuf[bun_tag_prefix.len + hashlen] = 0;
const buntagfd = switch (bun.sys.openat(
bun.toFD(patch_pkg_dir.fd),
buntagbuf[0 .. bun_tag_prefix.len + hashlen :0],
bun.O.RDWR | bun.O.CREAT,
0o666,
)) {
.result => |fd| fd,
.err => |e| {
// 4. apply patch
if (patchfile.apply(this.manager.allocator, patch_pkg_dir)) |e| {
return try log.addErrorFmtOpts(
this.manager.allocator,
"failed adding bun tag: {}",
.{e.withPath(buntagbuf[0 .. bun_tag_prefix.len + hashlen :0])},
"failed applying patch file: {}",
.{e.withoutPath()},
.{},
);
},
};
_ = bun.sys.close(buntagfd);
}

// 5. Add bun tag
const bun_tag_prefix = bun_hash_tag;
var buntagbuf: BuntagHashBuf = undefined;
@memcpy(buntagbuf[0..bun_tag_prefix.len], bun_tag_prefix);
const hashlen = (std.fmt.bufPrint(buntagbuf[bun_tag_prefix.len..], "{x}", .{this.callback.apply.patch_hash}) catch unreachable).len;
buntagbuf[bun_tag_prefix.len + hashlen] = 0;
const buntagfd = switch (bun.sys.openat(
patch_pkg_dir,
buntagbuf[0 .. bun_tag_prefix.len + hashlen :0],
bun.O.RDWR | bun.O.CREAT,
0o666,
)) {
.result => |fd| fd,
.err => |e| {
return try log.addErrorFmtOpts(
this.manager.allocator,
"failed adding bun tag: {}",
.{e.withPath(buntagbuf[0 .. bun_tag_prefix.len + hashlen :0])},
.{},
);
},
};
_ = bun.sys.close(buntagfd);
}

// 6. rename to cache dir
const path_in_tmpdir = bun.path.joinZ(
var path_in_tmpdir_buf: bun.PathBuffer = undefined;
const path_in_tmpdir = bun.path.joinZBuf(
&path_in_tmpdir_buf,
&[_][]const u8{
tempdir_name,
// tempdir_name,
Expand Down Expand Up @@ -417,11 +432,16 @@ pub const PatchTask = struct {
const dir = this.project_dir;
const patchfile_path = this.callback.calc_hash.patchfile_path;

var absolute_patchfile_path_buf: bun.PathBuffer = undefined;
// parse the patch file
const absolute_patchfile_path = bun.path.joinZ(&[_][]const u8{
dir,
patchfile_path,
}, .auto);
const absolute_patchfile_path = bun.path.joinZBuf(
&absolute_patchfile_path_buf,
&[_][]const u8{
dir,
patchfile_path,
},
.auto,
);

const stat: bun.Stat = switch (bun.sys.stat(absolute_patchfile_path)) {
.err => |e| {
Expand Down Expand Up @@ -543,7 +563,8 @@ pub const PatchTask = struct {
name_and_version_hash: u64,
) *PatchTask {
const pkg_name = pkg_manager.lockfile.packages.items(.name)[pkg_id];
const resolution: *const Resolution = &pkg_manager.lockfile.packages.items(.resolution)[pkg_id];

const resolution = &pkg_manager.lockfile.packages.items(.resolution)[pkg_id];

var folder_path_buf: bun.PathBuffer = undefined;
const stuff = pkg_manager.computeCacheDirAndSubpath(
Expand All @@ -560,7 +581,6 @@ pub const PatchTask = struct {
.callback = .{
.apply = .{
.pkg_id = pkg_id,
.resolution = resolution,
.patch_hash = patch_hash,
.name_and_version_hash = name_and_version_hash,
.cache_dir = stuff.cache_dir,
Expand Down
48 changes: 16 additions & 32 deletions src/patch.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,25 +35,6 @@ pub const PatchFilePart = union(enum) {
pub const PatchFile = struct {
parts: List(PatchFilePart) = .{},

const ScratchBuffer = struct {
buf: std.ArrayList(u8),

fn deinit(scratch: *@This()) void {
scratch.buf.deinit();
}

fn clear(scratch: *@This()) void {
scratch.buf.clearRetainingCapacity();
}

fn dupeZ(scratch: *@This(), path: []const u8) [:0]const u8 {
const start = scratch.buf.items.len;
scratch.buf.appendSlice(path) catch unreachable;
scratch.buf.append(0) catch unreachable;
return scratch.buf.items[start .. start + path.len :0];
}
};

pub fn deinit(this: *PatchFile, allocator: Allocator) void {
for (this.parts.items) |*part| part.deinit(allocator);
this.parts.deinit(allocator);
Expand All @@ -79,6 +60,7 @@ pub const PatchFile = struct {
var state: ApplyState = .{};
var sfb = std.heap.stackFallback(1024, allocator);
var arena = bun.ArenaAllocator.init(sfb.get());
defer arena.deinit();

for (this.parts.items) |*part| {
defer _ = arena.reset(.retain_capacity);
Expand All @@ -87,7 +69,7 @@ pub const PatchFile = struct {
const pathz = arena.allocator().dupeZ(u8, part.file_deletion.path) catch bun.outOfMemory();

if (bun.sys.unlinkat(patch_dir, pathz).asErr()) |e| {
return e.withPath(pathz);
return e.withoutPath();
}
},
.file_rename => {
Expand All @@ -97,7 +79,7 @@ pub const PatchFile = struct {
if (std.fs.path.dirname(to_path)) |todir| {
const abs_patch_dir = switch (state.patchDirAbsPath(patch_dir)) {
.result => |p| p,
.err => |e| return e,
.err => |e| return e.withoutPath(),
};
const path_to_make = bun.path.joinZ(&[_][]const u8{
abs_patch_dir,
Expand All @@ -108,11 +90,11 @@ pub const PatchFile = struct {
.path = .{ .string = bun.PathString.init(path_to_make) },
.recursive = true,
.mode = 0o755,
}).asErr()) |e| return e;
}).asErr()) |e| return e.withoutPath();
}

if (bun.sys.renameat(patch_dir, from_path, patch_dir, to_path).asErr()) |e| {
return e;
return e.withoutPath();
}
},
.file_creation => {
Expand All @@ -126,7 +108,7 @@ pub const PatchFile = struct {
.path = .{ .string = bun.PathString.init(filedir) },
.recursive = true,
.mode = @intCast(@intFromEnum(mode)),
}).asErr()) |e| return e;
}).asErr()) |e| return e.withoutPath();
}

const newfile_fd = switch (bun.sys.openat(
Expand All @@ -136,7 +118,7 @@ pub const PatchFile = struct {
mode.toBunMode(),
)) {
.result => |fd| fd,
.err => |e| return e.withPath(filepath.slice()),
.err => |e| return e.withoutPath(),
};
defer _ = bun.sys.close(newfile_fd);

Expand Down Expand Up @@ -180,37 +162,39 @@ pub const PatchFile = struct {
while (written < file_contents.len) {
switch (bun.sys.write(newfile_fd, file_contents[written..])) {
.result => |bytes| written += bytes,
.err => |e| return e.withPath(filepath.slice()),
.err => |e| return e.withoutPath(),
}
}
},
.file_patch => {
// TODO: should we compute the hash of the original file and check it against the on in the patch?
if (applyPatch(part.file_patch, &arena, patch_dir, &state).asErr()) |e| {
return e;
return e.withoutPath();
}
},
.file_mode_change => {
const newmode = part.file_mode_change.new_mode;
const filepath = arena.allocator().dupeZ(u8, part.file_mode_change.path) catch bun.outOfMemory();
if (comptime bun.Environment.isPosix) {
if (bun.sys.fchmodat(patch_dir, filepath, newmode.toBunMode(), 0).asErr()) |e| {
return e;
return e.withoutPath();
}
}

if (comptime bun.Environment.isWindows) {
const absfilepath = switch (state.patchDirAbsPath(patch_dir)) {
.result => |p| p,
.err => |e| return e,
.err => |e| return e.withoutPath(),
};
const fd = switch (bun.sys.open(bun.path.joinZ(&[_][]const u8{ absfilepath, filepath }, .auto), bun.O.RDWR, 0)) {
.err => |e| return e,
var buf: bun.PathBuffer = undefined;
const joined_absfilepath = bun.path.joinZBuf(&buf, &[_][]const u8{ absfilepath, filepath }, .auto);
const fd = switch (bun.sys.open(joined_absfilepath, bun.O.RDWR, 0)) {
.err => |e| return e.withoutPath(),
.result => |f| f,
};
defer _ = bun.sys.close(fd);
if (bun.sys.fchmod(fd, newmode.toBunMode()).asErr()) |e| {
return e;
return e.withoutPath();
}
}
},
Expand Down
22 changes: 21 additions & 1 deletion src/sys.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,19 @@ pub const Error = struct {
}

pub fn format(self: Error, comptime fmt: []const u8, opts: std.fmt.FormatOptions, writer: anytype) !void {
try self.toShellSystemError().format(fmt, opts, writer);
// We want to reuse the code from SystemError for formatting.
// But, we do not want to call String.createUTF8 on the path/dest strings
// because we're intending to pass them to writer.print()
// which will convert them back into UTF*.
var that = self.withoutPath().toShellSystemError();
bun.debugAssert(that.path.tag != .WTFStringImpl);
bun.debugAssert(that.dest.tag != .WTFStringImpl);
that.path = bun.String.fromUTF8(self.path);
that.dest = bun.String.fromUTF8(self.dest);
bun.debugAssert(that.path.tag != .WTFStringImpl);
bun.debugAssert(that.dest.tag != .WTFStringImpl);

return that.format(fmt, opts, writer);
}

pub inline fn getErrno(this: Error) E {
Expand Down Expand Up @@ -421,6 +433,14 @@ pub const Error = struct {
};
}

/// When the memory of the path/dest buffer is unsafe to use, call this function to clone the error without the path/dest.
pub fn withoutPath(this: *const Error) Error {
var copy = this.*;
copy.path = "";
copy.dest = "";
return copy;
}

pub fn name(this: *const Error) []const u8 {
if (comptime Environment.isWindows) {
const system_errno = brk: {
Expand Down
2 changes: 1 addition & 1 deletion test/cli/install/bun-install-patch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ index 832d92223a9ec491364ee10dcbe3ad495446ab80..7e079a817825de4b8c3d01898490dc7e
},
"index.ts": /* ts */ `import isEven from 'is-even'; console.log(isEven())`,
});

console.log(filedir);
await $`${bunExe()} i`.env(bunEnv).cwd(filedir);

const pkgjsonWithPatch = {
Expand Down