Skip to content

Commit

Permalink
Fix flaky patch test (#17301)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jarred-Sumner
Jarred-Sumner authored Feb 13, 2025
1 parent 6353fa4 commit baee1c1
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 86 deletions.
3 changes: 2 additions & 1 deletion src/install/install.zig
View file
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
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
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
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
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

0 comments on commit baee1c1

Please sign in to comment.