From 9e47857ae9340ecd77f73c3c22ae04c938d28ca7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Sun, 7 Apr 2024 01:02:37 +0200 Subject: [PATCH 01/18] fetch: make failing test --- src/Package/Fetch.zig | 132 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 131 insertions(+), 1 deletion(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 9aeaaffea35b..5dfa4245bf73 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -1735,7 +1735,7 @@ const ThreadPool = std.Thread.Pool; const WaitGroup = std.Thread.WaitGroup; const Fetch = @This(); const git = @import("Fetch/git.zig"); -const Package = @import("../Package.zig"); +//const Package = @import("../Package.zig"); const Manifest = Package.Manifest; const ErrorBundle = std.zig.ErrorBundle; const native_os = builtin.os.tag; @@ -1778,3 +1778,133 @@ test FileHeader { h.update(FileHeader.elf_magic[2..4]); try std.testing.expect(h.isExecutable()); } + +// Removing dependencies +const Package = struct { + const build_zig_basename = "build.zig"; + const Module = struct {}; + const Manifest = @import("Manifest.zig"); +}; + +test "fetch tarball" { + const testing = std.testing; + const gpa = testing.allocator; + + var cache_tmp = std.testing.tmpDir(.{}); + defer cache_tmp.cleanup(); + const global_cache_directory_path = try cache_tmp.dir.realpathAlloc(gpa, "."); + defer gpa.free(global_cache_directory_path); + + const paths: []const []const u8 = &.{ + "main.zig", + "src/root.zig", + // duplicate file paths + "dir/file", + "dir1/file1", + "dir/file", + "dir1/file1", + }; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const file = try tmp.dir.createFile("package.tar", .{}); + try createTarball(file, "package", paths); + file.close(); + + const tmp_path = try tmp.dir.realpathAlloc(gpa, "."); + const path_or_url = try std.fmt.allocPrint( + gpa, + "file://{s}/package.tar", + .{tmp_path}, + ); + gpa.free(tmp_path); + defer gpa.free(path_or_url); + + var thread_pool: ThreadPool = undefined; + try thread_pool.init(.{ .allocator = gpa }); + defer thread_pool.deinit(); + + var http_client: std.http.Client = .{ .allocator = gpa }; + defer http_client.deinit(); + + var global_cache_directory: Cache.Directory = .{ + .handle = try fs.cwd().makeOpenPath(global_cache_directory_path, .{}), + .path = global_cache_directory_path, + }; + defer global_cache_directory.handle.close(); + + var progress: std.Progress = .{ .dont_print_on_dumb = true }; + const root_prog_node = progress.start("Fetch", 0); + defer root_prog_node.end(); + + var job_queue: Fetch.JobQueue = .{ + .http_client = &http_client, + .thread_pool = &thread_pool, + .global_cache = global_cache_directory, + .recursive = false, + .read_only = false, + .debug_hash = true, + .work_around_btrfs_bug = false, + }; + defer job_queue.deinit(); + + var fetch: Fetch = .{ + .arena = std.heap.ArenaAllocator.init(gpa), + .location = .{ .path_or_url = path_or_url }, + .location_tok = 0, + .hash_tok = 0, + .name_tok = 0, + .lazy_status = .eager, + .parent_package_root = Cache.Path{ .root_dir = undefined }, + .parent_manifest_ast = null, + .prog_node = root_prog_node, + .job_queue = &job_queue, + .omit_missing_hash_error = true, + .allow_missing_paths_field = false, + + .package_root = undefined, + .error_bundle = undefined, + .manifest = null, + .manifest_ast = undefined, + .actual_hash = undefined, + .has_build_zig = false, + .oom_flag = false, + + .module = null, + }; + defer fetch.deinit(); + + try testing.expectError(error.FetchFailed, fetch.run()); + + try testing.expectEqual(1, fetch.error_bundle.root_list.items.len); + var errors = try fetch.error_bundle.toOwnedBundle(""); + defer errors.deinit(gpa); + + const em = errors.getErrorMessage(errors.getMessages()[0]); + try testing.expectEqual(2, em.notes_len); + + var al = std.ArrayList(u8).init(gpa); + defer al.deinit(); + try errors.renderToWriter(.{ .ttyconf = .no_color }, al.writer()); + try testing.expectEqualStrings( + \\error: unable to unpack tarball + \\ note: unable to create file 'dir/file': PathAlreadyExists + \\ note: unable to create file 'dir1/file1': PathAlreadyExists + \\ + , al.items); +} + +const TarHeader = std.tar.output.Header; + +fn createTarball(file: fs.File, prefix: []const u8, paths: []const []const u8) !void { + for (paths) |path| { + var hdr = TarHeader.init(); + hdr.typeflag = .regular; + //if (prefix.len > 0) { + try hdr.setPath(prefix, path); + // } else { + // hdr.setName(path); + // } + try hdr.updateChecksum(); + try file.writeAll(std.mem.asBytes(&hdr)); + } +} From b3339f3cd5b1eeb3419ee136e378dc3e348e6a83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Sun, 7 Apr 2024 01:04:38 +0200 Subject: [PATCH 02/18] tar: store diagnostic errors into UnpackResult Test that UnpackResult prints same error output as diagnostic. --- src/Package/Fetch.zig | 193 ++++++++++++++++++++++++++++++++---------- 1 file changed, 149 insertions(+), 44 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 5dfa4245bf73..31a3b598ce9a 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -1189,38 +1189,17 @@ fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!?[]const )); if (diagnostics.errors.items.len > 0) { - const notes_len: u32 = @intCast(diagnostics.errors.items.len); - try eb.addRootErrorMessage(.{ - .msg = try eb.addString("unable to unpack tarball"), - .src_loc = try f.srcLoc(f.location_tok), - .notes_len = notes_len, - }); - const notes_start = try eb.reserveNotes(notes_len); - for (diagnostics.errors.items, notes_start..) |item, note_i| { + var res = UnpackResult.init(gpa); + defer res.deinit(); + + for (diagnostics.errors.items) |item| { switch (item) { - .unable_to_create_sym_link => |info| { - eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ - .msg = try eb.printString("unable to create symlink from '{s}' to '{s}': {s}", .{ - info.file_name, info.link_name, @errorName(info.code), - }), - })); - }, - .unable_to_create_file => |info| { - eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ - .msg = try eb.printString("unable to create file '{s}': {s}", .{ - info.file_name, @errorName(info.code), - }), - })); - }, - .unsupported_file_type => |info| { - eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ - .msg = try eb.printString("file '{s}' has unsupported type '{c}'", .{ - info.file_name, @intFromEnum(info.file_type), - }), - })); - }, + .unable_to_create_file => |i| try res.createFile(i.file_name, i.code), + .unable_to_create_sym_link => |i| try res.symLink(i.file_name, i.link_name, i.code), + .unsupported_file_type => |i| try res.unsupportedFileType(i.file_name, @intFromEnum(i.file_type)), } } + try res.bundleErrors(eb, "unable to unpack tarball", try f.srcLoc(f.location_tok)); return error.FetchFailed; } @@ -1270,24 +1249,16 @@ fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!void try repository.checkout(out_dir, want_oid, &diagnostics); if (diagnostics.errors.items.len > 0) { - const notes_len: u32 = @intCast(diagnostics.errors.items.len); - try eb.addRootErrorMessage(.{ - .msg = try eb.addString("unable to unpack packfile"), - .src_loc = try f.srcLoc(f.location_tok), - .notes_len = notes_len, - }); - const notes_start = try eb.reserveNotes(notes_len); - for (diagnostics.errors.items, notes_start..) |item, note_i| { + var res = UnpackResult.init(gpa); + defer res.deinit(); + + for (diagnostics.errors.items) |item| { switch (item) { - .unable_to_create_sym_link => |info| { - eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ - .msg = try eb.printString("unable to create symlink from '{s}' to '{s}': {s}", .{ - info.file_name, info.link_name, @errorName(info.code), - }), - })); - }, + .unable_to_create_sym_link => |i| try res.symLink(i.file_name, i.link_name, i.code), } } + try res.bundleErrors(eb, "unable to unpack packfile", try f.srcLoc(f.location_tok)); + return error.InvalidGitPack; } } @@ -1779,6 +1750,140 @@ test FileHeader { try std.testing.expect(h.isExecutable()); } +const UnpackResult = struct { + allocator: std.mem.Allocator, + errors: std.ArrayListUnmanaged(Error) = .{}, + + const Error = union(enum) { + unable_to_create_sym_link: struct { + code: anyerror, + file_name: []const u8, + link_name: []const u8, + }, + unable_to_create_file: struct { + code: anyerror, + file_name: []const u8, + }, + unsupported_file_type: struct { + file_name: []const u8, + file_type: u8, + }, + + fn excluded(self: Error, filter: Filter) bool { + switch (self) { + .unable_to_create_file => |info| return !filter.includePath(info.file_name), + .unable_to_create_sym_link => |info| return !filter.includePath(info.file_name), + .unsupported_file_type => |info| return !filter.includePath(info.file_name), + } + } + + fn free(self: Error, allocator: std.mem.Allocator) void { + switch (self) { + .unable_to_create_sym_link => |info| { + allocator.free(info.file_name); + allocator.free(info.link_name); + }, + .unable_to_create_file => |info| { + allocator.free(info.file_name); + }, + .unsupported_file_type => |info| { + allocator.free(info.file_name); + }, + } + } + }; + + fn init(allocator: std.mem.Allocator) UnpackResult { + return .{ .allocator = allocator }; + } + + fn deinit(self: *UnpackResult) void { + for (self.errors.items) |item| { + item.free(self.allocator); + } + self.errors.deinit(self.allocator); + self.* = undefined; + } + + fn hasErrors(self: *UnpackResult) bool { + return self.errors.items.len > 0; + } + + fn createFile(self: *UnpackResult, file_name: []const u8, err: anyerror) !void { + try self.errors.append(self.allocator, .{ .unable_to_create_file = .{ + .code = err, + .file_name = try self.allocator.dupe(u8, file_name), + } }); + } + + fn symLink(self: *UnpackResult, file_name: []const u8, link_name: []const u8, err: anyerror) !void { + try self.errors.append(self.allocator, .{ .unable_to_create_sym_link = .{ + .code = err, + .file_name = try self.allocator.dupe(u8, file_name), + .link_name = try self.allocator.dupe(u8, link_name), + } }); + } + + fn unsupportedFileType(self: *UnpackResult, file_name: []const u8, file_type: u8) !void { + try self.errors.append(self.allocator, .{ .unsupported_file_type = .{ + .file_name = try self.allocator.dupe(u8, file_name), + .file_type = file_type, + } }); + } + + fn filterErrors(self: *UnpackResult, filter: Filter) !void { + var i = self.errors.items.len; + while (i > 0) { + i -= 1; + const item = self.errors.items[i]; + if (item.excluded(filter)) { + _ = self.errors.swapRemove(i); + item.free(self.allocator); + } + } + } + + fn bundleErrors( + self: *UnpackResult, + eb: *ErrorBundle.Wip, + msg: []const u8, + src_loc: ErrorBundle.SourceLocationIndex, + ) !void { + const notes_len: u32 = @intCast(self.errors.items.len); + try eb.addRootErrorMessage(.{ + .msg = try eb.addString(msg), + .src_loc = src_loc, + .notes_len = notes_len, + }); + const notes_start = try eb.reserveNotes(notes_len); + for (self.errors.items, notes_start..) |item, note_i| { + switch (item) { + .unable_to_create_sym_link => |info| { + eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ + .msg = try eb.printString("unable to create symlink from '{s}' to '{s}': {s}", .{ + info.file_name, info.link_name, @errorName(info.code), + }), + })); + }, + .unable_to_create_file => |info| { + eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ + .msg = try eb.printString("unable to create file '{s}': {s}", .{ + info.file_name, @errorName(info.code), + }), + })); + }, + .unsupported_file_type => |info| { + eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ + .msg = try eb.printString("file '{s}' has unsupported type '{c}'", .{ + info.file_name, info.file_type, + }), + })); + }, + } + } + } +}; + // Removing dependencies const Package = struct { const build_zig_basename = "build.zig"; From a0790914b4e0123e36432e2c0bcbf2a517fc410a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Thu, 28 Mar 2024 21:06:26 +0100 Subject: [PATCH 03/18] fetch: return UnpackResult from unpackResource Test that we are still outputing same errors. --- src/Package/Fetch.zig | 80 +++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 38 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 31a3b598ce9a..042b318c49b9 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -461,13 +461,16 @@ fn runResource( }; defer tmp_directory.handle.close(); - // Unpack resource into tmp_directory. A non-null return value means - // that the package contents are inside a `pkg_dir` sub-directory. - const pkg_dir = try unpackResource(f, resource, uri_path, tmp_directory); + var unpack_result = try unpackResource(f, resource, uri_path, tmp_directory); + defer unpack_result.deinit(); + if (unpack_result.hasErrors()) { + try unpack_result.bundleErrors(eb, try f.srcLoc(f.location_tok)); + return error.FetchFailed; + } var pkg_path: Cache.Path = .{ .root_dir = tmp_directory, - .sub_path = if (pkg_dir) |pkg_dir_name| pkg_dir_name else "", + .sub_path = if (unpack_result.root_dir) |root_dir| root_dir else "", }; // Apply btrfs workaround if needed. Reopen tmp_directory. @@ -500,8 +503,8 @@ fn runResource( // directory. f.actual_hash = try computeHash(f, pkg_path, filter); - break :blk if (pkg_dir) |pkg_dir_name| - try fs.path.join(arena, &.{ tmp_dir_sub_path, pkg_dir_name }) + break :blk if (unpack_result.root_dir) |root_dir| + try fs.path.join(arena, &.{ tmp_dir_sub_path, root_dir }) else tmp_dir_sub_path; }; @@ -1053,7 +1056,7 @@ fn unpackResource( resource: *Resource, uri_path: []const u8, tmp_directory: Cache.Directory, -) RunError!?[]const u8 { +) RunError!UnpackResult { const eb = &f.error_bundle; const file_type = switch (resource.*) { .file => FileType.fromPath(uri_path) orelse @@ -1121,7 +1124,8 @@ fn unpackResource( .{ uri_path, @errorName(err) }, )); }; - return null; + const gpa = f.arena.child_allocator; + return UnpackResult.init(gpa); }, }; @@ -1156,23 +1160,19 @@ fn unpackResource( }); return try unpackTarball(f, tmp_directory.handle, dcp.reader()); }, - .git_pack => { - unpackGitPack(f, tmp_directory.handle, resource) catch |err| switch (err) { - error.FetchFailed => return error.FetchFailed, - error.OutOfMemory => return error.OutOfMemory, - else => |e| return f.fail(f.location_tok, try eb.printString( - "unable to unpack git files: {s}", - .{@errorName(e)}, - )), - }; - return null; + .git_pack => return unpackGitPack(f, tmp_directory.handle, resource) catch |err| switch (err) { + error.FetchFailed => return error.FetchFailed, + error.OutOfMemory => return error.OutOfMemory, + else => |e| return f.fail(f.location_tok, try eb.printString( + "unable to unpack git files: {s}", + .{@errorName(e)}, + )), }, } } -fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!?[]const u8 { +fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!UnpackResult { const eb = &f.error_bundle; - const arena = f.arena.allocator(); const gpa = f.arena.child_allocator; var diagnostics: std.tar.Diagnostics = .{ .allocator = gpa }; @@ -1188,10 +1188,11 @@ fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!?[]const .{@errorName(err)}, )); + var res = UnpackResult.init(gpa); + if (diagnostics.root_dir) |root_dir| { + res.root_dir = try gpa.dupe(u8, root_dir); + } if (diagnostics.errors.items.len > 0) { - var res = UnpackResult.init(gpa); - defer res.deinit(); - for (diagnostics.errors.items) |item| { switch (item) { .unable_to_create_file => |i| try res.createFile(i.file_name, i.code), @@ -1199,21 +1200,17 @@ fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!?[]const .unsupported_file_type => |i| try res.unsupportedFileType(i.file_name, @intFromEnum(i.file_type)), } } - try res.bundleErrors(eb, "unable to unpack tarball", try f.srcLoc(f.location_tok)); - return error.FetchFailed; + try res.rootErrorMessage("unable to unpack tarball"); } - - return if (diagnostics.root_dir) |root_dir| - return try arena.dupe(u8, root_dir) - else - null; + return res; } -fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!void { - const eb = &f.error_bundle; +fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!UnpackResult { const gpa = f.arena.child_allocator; const want_oid = resource.git.want_oid; const reader = resource.git.fetch_stream.reader(); + + var res = UnpackResult.init(gpa); // The .git directory is used to store the packfile and associated index, but // we do not attempt to replicate the exact structure of a real .git // directory, since that isn't relevant for fetching a package. @@ -1249,7 +1246,6 @@ fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!void try repository.checkout(out_dir, want_oid, &diagnostics); if (diagnostics.errors.items.len > 0) { - var res = UnpackResult.init(gpa); defer res.deinit(); for (diagnostics.errors.items) |item| { @@ -1257,14 +1253,13 @@ fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!void .unable_to_create_sym_link => |i| try res.symLink(i.file_name, i.link_name, i.code), } } - try res.bundleErrors(eb, "unable to unpack packfile", try f.srcLoc(f.location_tok)); - - return error.InvalidGitPack; + try res.rootErrorMessage("unable to unpack packfile"); } } } try out_dir.deleteTree(".git"); + return res; } fn recursiveDirectoryCopy(f: *Fetch, dir: fs.Dir, tmp_dir: fs.Dir) anyerror!void { @@ -1753,6 +1748,8 @@ test FileHeader { const UnpackResult = struct { allocator: std.mem.Allocator, errors: std.ArrayListUnmanaged(Error) = .{}, + root_error_message: []const u8 = "", + root_dir: ?[]const u8 = null, const Error = union(enum) { unable_to_create_sym_link: struct { @@ -1802,6 +1799,10 @@ const UnpackResult = struct { item.free(self.allocator); } self.errors.deinit(self.allocator); + self.allocator.free(self.root_error_message); + if (self.root_dir) |root_dir| { + self.allocator.free(root_dir); + } self.* = undefined; } @@ -1843,15 +1844,18 @@ const UnpackResult = struct { } } + fn rootErrorMessage(self: *UnpackResult, msg: []const u8) !void { + self.root_error_message = try self.allocator.dupe(u8, msg); + } + fn bundleErrors( self: *UnpackResult, eb: *ErrorBundle.Wip, - msg: []const u8, src_loc: ErrorBundle.SourceLocationIndex, ) !void { const notes_len: u32 = @intCast(self.errors.items.len); try eb.addRootErrorMessage(.{ - .msg = try eb.addString(msg), + .msg = try eb.addString(self.root_error_message), .src_loc = src_loc, .notes_len = notes_len, }); From 4d6a7e074bf79e35a58a4f4bc4199359a57ad0e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Fri, 29 Mar 2024 01:25:57 +0100 Subject: [PATCH 04/18] fetch: filter unpack errors Report only errors which are not filtered by paths in build.zig.zon. --- src/Package.zig | 4 + src/Package/Fetch.zig | 355 +++++++++++++++++++++++++++--------------- 2 files changed, 235 insertions(+), 124 deletions(-) diff --git a/src/Package.zig b/src/Package.zig index e173665e1192..61f90727f342 100644 --- a/src/Package.zig +++ b/src/Package.zig @@ -2,3 +2,7 @@ pub const Module = @import("Package/Module.zig"); pub const Fetch = @import("Package/Fetch.zig"); pub const build_zig_basename = "build.zig"; pub const Manifest = @import("Package/Manifest.zig"); + +test { + _ = Fetch; +} diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 042b318c49b9..cf27265b394f 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -463,10 +463,6 @@ fn runResource( var unpack_result = try unpackResource(f, resource, uri_path, tmp_directory); defer unpack_result.deinit(); - if (unpack_result.hasErrors()) { - try unpack_result.bundleErrors(eb, try f.srcLoc(f.location_tok)); - return error.FetchFailed; - } var pkg_path: Cache.Path = .{ .root_dir = tmp_directory, @@ -491,10 +487,14 @@ fn runResource( .include_paths = if (f.manifest) |m| m.paths else .{}, }; - // TODO: // If any error occurred for files that were ultimately excluded, those // errors should be ignored, such as failure to create symlinks that // weren't supposed to be included anyway. + try unpack_result.filterErrors(filter); + if (unpack_result.hasErrors()) { + try unpack_result.bundleErrors(eb, try f.srcLoc(f.location_tok)); + return error.FetchFailed; + } // Apply the manifest's inclusion rules to the temporary directory by // deleting excluded files. @@ -1701,7 +1701,7 @@ const ThreadPool = std.Thread.Pool; const WaitGroup = std.Thread.WaitGroup; const Fetch = @This(); const git = @import("Fetch/git.zig"); -//const Package = @import("../Package.zig"); +const Package = @import("../Package.zig"); const Manifest = Package.Manifest; const ErrorBundle = std.zig.ErrorBundle; const native_os = builtin.os.tag; @@ -1766,12 +1766,14 @@ const UnpackResult = struct { file_type: u8, }, - fn excluded(self: Error, filter: Filter) bool { - switch (self) { - .unable_to_create_file => |info| return !filter.includePath(info.file_name), - .unable_to_create_sym_link => |info| return !filter.includePath(info.file_name), - .unsupported_file_type => |info| return !filter.includePath(info.file_name), - } + fn excluded(self: Error, filter: Filter, root_dir: []const u8) bool { + const file_name = switch (self) { + .unable_to_create_file => |info| info.file_name, + .unable_to_create_sym_link => |info| info.file_name, + .unsupported_file_type => |info| info.file_name, + }; + + return !filter.includePath(stripRoot(file_name, root_dir)); } fn free(self: Error, allocator: std.mem.Allocator) void { @@ -1834,10 +1836,11 @@ const UnpackResult = struct { fn filterErrors(self: *UnpackResult, filter: Filter) !void { var i = self.errors.items.len; + const root_dir: []const u8 = if (self.root_dir) |root_dir| root_dir else ""; while (i > 0) { i -= 1; const item = self.errors.items[i]; - if (item.excluded(filter)) { + if (item.excluded(filter, root_dir)) { _ = self.errors.swapRemove(i); item.free(self.allocator); } @@ -1888,132 +1891,236 @@ const UnpackResult = struct { } }; -// Removing dependencies -const Package = struct { - const build_zig_basename = "build.zig"; - const Module = struct {}; - const Manifest = @import("Manifest.zig"); -}; - -test "fetch tarball" { +test "fetch tarball: fail with unable to create file" { const testing = std.testing; - const gpa = testing.allocator; + var buf: [4096]u8 = undefined; + var buf_pos: usize = 0; - var cache_tmp = std.testing.tmpDir(.{}); - defer cache_tmp.cleanup(); - const global_cache_directory_path = try cache_tmp.dir.realpathAlloc(gpa, "."); - defer gpa.free(global_cache_directory_path); - - const paths: []const []const u8 = &.{ - "main.zig", - "src/root.zig", - // duplicate file paths - "dir/file", - "dir1/file1", - "dir/file", - "dir1/file1", - }; + // Create tmp dir var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); - const file = try tmp.dir.createFile("package.tar", .{}); - try createTarball(file, "package", paths); - file.close(); - - const tmp_path = try tmp.dir.realpathAlloc(gpa, "."); - const path_or_url = try std.fmt.allocPrint( - gpa, - "file://{s}/package.tar", - .{tmp_path}, - ); - gpa.free(tmp_path); - defer gpa.free(path_or_url); + const tmp_path = try tmp.dir.realpath(".", &buf); + buf_pos += tmp_path.len; - var thread_pool: ThreadPool = undefined; - try thread_pool.init(.{ .allocator = gpa }); - defer thread_pool.deinit(); + // Create tarball in tmp dir without build.zig.zon + const tarball_name = "package.tar"; + try createTestTarball(tmp.dir, tarball_name, false); - var http_client: std.http.Client = .{ .allocator = gpa }; - defer http_client.deinit(); + // Get path to the tarball + const path_or_url = try std.fmt.bufPrint(buf[buf_pos..], "file://{s}/{s}", .{ tmp_path, tarball_name }); + buf_pos += path_or_url.len; - var global_cache_directory: Cache.Directory = .{ - .handle = try fs.cwd().makeOpenPath(global_cache_directory_path, .{}), - .path = global_cache_directory_path, - }; - defer global_cache_directory.handle.close(); - - var progress: std.Progress = .{ .dont_print_on_dumb = true }; - const root_prog_node = progress.start("Fetch", 0); - defer root_prog_node.end(); - - var job_queue: Fetch.JobQueue = .{ - .http_client = &http_client, - .thread_pool = &thread_pool, - .global_cache = global_cache_directory, - .recursive = false, - .read_only = false, - .debug_hash = true, - .work_around_btrfs_bug = false, - }; - defer job_queue.deinit(); - - var fetch: Fetch = .{ - .arena = std.heap.ArenaAllocator.init(gpa), - .location = .{ .path_or_url = path_or_url }, - .location_tok = 0, - .hash_tok = 0, - .name_tok = 0, - .lazy_status = .eager, - .parent_package_root = Cache.Path{ .root_dir = undefined }, - .parent_manifest_ast = null, - .prog_node = root_prog_node, - .job_queue = &job_queue, - .omit_missing_hash_error = true, - .allow_missing_paths_field = false, - - .package_root = undefined, - .error_bundle = undefined, - .manifest = null, - .manifest_ast = undefined, - .actual_hash = undefined, - .has_build_zig = false, - .oom_flag = false, - - .module = null, + // Global cache directory in tmp + const cache_path = try std.fmt.bufPrint(buf[buf_pos..], "{s}/{s}", .{ tmp_path, "global_cache" }); + buf_pos += cache_path.len; + + // Run tarball fetch, expect to fail + var tf: TestFetch = undefined; + try tf.init(testing.allocator, cache_path, path_or_url); + defer tf.deinit(); + try testing.expectError(error.FetchFailed, tf.fetch.run()); + + // Expect fetch errors + { + var errors = try tf.fetch.error_bundle.toOwnedBundle(""); + defer errors.deinit(testing.allocator); + + const em = errors.getErrorMessage(errors.getMessages()[0]); + try testing.expectEqual(1, em.count); + try testing.expectEqual(2, em.notes_len); + + var al = std.ArrayList(u8).init(testing.allocator); + defer al.deinit(); + try errors.renderToWriter(.{ .ttyconf = .no_color }, al.writer()); + try testing.expectEqualStrings( + \\error: unable to unpack tarball + \\ note: unable to create file 'dir/file': PathAlreadyExists + \\ note: unable to create file 'dir1/file1': PathAlreadyExists + \\ + , al.items); + } +} + +test "fetch tarball: error path are excluded" { + const testing = std.testing; + var buf: [4096]u8 = undefined; + var buf_pos: usize = 0; + + // Create tmp dir + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const tmp_path = try tmp.dir.realpath(".", &buf); + buf_pos += tmp_path.len; + + // Create tarball in tmp dir + const tarball_name = "package.tar"; + try createTestTarball(tmp.dir, tarball_name, true); + + // Get path to the tarball + const path_or_url = try std.fmt.bufPrint(buf[buf_pos..], "file://{s}/{s}", .{ tmp_path, tarball_name }); + buf_pos += path_or_url.len; + + // Global cache directory in tmp + const cache_path = try std.fmt.bufPrint(buf[buf_pos..], "{s}/{s}", .{ tmp_path, "global_cache" }); + buf_pos += cache_path.len; + + // Run tarball fetch + var tf: TestFetch = undefined; + try tf.init(testing.allocator, cache_path, path_or_url); + defer tf.deinit(); + try tf.fetch.run(); + + const hex_digest = Package.Manifest.hexDigest(tf.fetch.actual_hash); + try testing.expectEqualStrings("122022afac878639d5ea6fcca14a123e21fd0395c1f2ef2c89017fa71390f73024af", &hex_digest); + + const expected_files: []const []const u8 = &.{ + "build.zig", + "build.zig.zon", + "src/main.zig", }; - defer fetch.deinit(); - - try testing.expectError(error.FetchFailed, fetch.run()); - - try testing.expectEqual(1, fetch.error_bundle.root_list.items.len); - var errors = try fetch.error_bundle.toOwnedBundle(""); - defer errors.deinit(gpa); - - const em = errors.getErrorMessage(errors.getMessages()[0]); - try testing.expectEqual(2, em.notes_len); - - var al = std.ArrayList(u8).init(gpa); - defer al.deinit(); - try errors.renderToWriter(.{ .ttyconf = .no_color }, al.writer()); - try testing.expectEqualStrings( - \\error: unable to unpack tarball - \\ note: unable to create file 'dir/file': PathAlreadyExists - \\ note: unable to create file 'dir1/file1': PathAlreadyExists - \\ - , al.items); + // Unpacked package contains expected files + { + const package_path = try std.fmt.bufPrint(buf[buf_pos..], "global_cache/p/{s}", .{hex_digest}); + buf_pos += package_path.len; + var package_dir = try tmp.dir.openDir(package_path, .{ .iterate = true }); + + var actual_files: std.ArrayListUnmanaged([]u8) = .{}; + defer actual_files.deinit(testing.allocator); + defer for (actual_files.items) |file| testing.allocator.free(file); + var walker = try package_dir.walk(testing.allocator); + defer walker.deinit(); + while (try walker.next()) |entry| { + if (entry.kind != .file) continue; + //std.debug.print("{s}\n", .{entry.path}); + const path = try testing.allocator.dupe(u8, entry.path); + errdefer testing.allocator.free(path); + std.mem.replaceScalar(u8, path, std.fs.path.sep, '/'); + try actual_files.append(testing.allocator, path); + } + std.mem.sortUnstable([]u8, actual_files.items, {}, struct { + fn lessThan(_: void, a: []u8, b: []u8) bool { + return std.mem.lessThan(u8, a, b); + } + }.lessThan); + try testing.expectEqualDeep(expected_files, actual_files.items); + } } -const TarHeader = std.tar.output.Header; +const TestFetch = struct { + thread_pool: ThreadPool, + http_client: std.http.Client, + global_cache_directory: Cache.Directory, + progress: std.Progress, + root_prog_node: *std.Progress.Node, + job_queue: Fetch.JobQueue, + fetch: Fetch, + gpa: std.mem.Allocator, + + fn init( + tf: *TestFetch, + gpa: std.mem.Allocator, + global_cache_directory_path: []const u8, + path_or_url: []const u8, + ) !void { + try tf.thread_pool.init(.{ .allocator = gpa }); + tf.http_client = .{ .allocator = gpa }; + tf.global_cache_directory = .{ + .handle = try fs.cwd().makeOpenPath(global_cache_directory_path, .{}), + .path = global_cache_directory_path, + }; + + tf.progress = .{ .dont_print_on_dumb = true }; + tf.root_prog_node = tf.progress.start("Fetch", 0); + + tf.job_queue = .{ + .http_client = &tf.http_client, + .thread_pool = &tf.thread_pool, + .global_cache = tf.global_cache_directory, + .recursive = false, + .read_only = false, + .debug_hash = false, + .work_around_btrfs_bug = false, + }; + + tf.fetch = .{ + .arena = std.heap.ArenaAllocator.init(gpa), + .location = .{ .path_or_url = path_or_url }, + .location_tok = 0, + .hash_tok = 0, + .name_tok = 0, + .lazy_status = .eager, + .parent_package_root = Cache.Path{ .root_dir = undefined }, + .parent_manifest_ast = null, + .prog_node = tf.root_prog_node, + .job_queue = &tf.job_queue, + .omit_missing_hash_error = true, + .allow_missing_paths_field = false, + + .package_root = undefined, + .error_bundle = undefined, + .manifest = null, + .manifest_ast = undefined, + .actual_hash = undefined, + .has_build_zig = false, + .oom_flag = false, + + .module = null, + }; + } -fn createTarball(file: fs.File, prefix: []const u8, paths: []const []const u8) !void { - for (paths) |path| { + fn deinit(self: *TestFetch) void { + self.fetch.deinit(); + self.job_queue.deinit(); + self.root_prog_node.end(); + self.global_cache_directory.handle.close(); + self.http_client.deinit(); + self.thread_pool.deinit(); + } +}; + +fn createTestTarball(dir: fs.Dir, tarball_name: []const u8, with_manifest: bool) !void { + const file = try dir.createFile(tarball_name, .{}); + defer file.close(); + + const TarHeader = std.tar.output.Header; + const prefix = tarball_name; + + const files: []const []const u8 = &.{ + "build.zig", + "src/main.zig", + // duplicate file paths + "dir/file", + "dir1/file1", + "dir/file", + "dir1/file1", + }; + for (files) |path| { var hdr = TarHeader.init(); hdr.typeflag = .regular; - //if (prefix.len > 0) { try hdr.setPath(prefix, path); - // } else { - // hdr.setName(path); - // } try hdr.updateChecksum(); try file.writeAll(std.mem.asBytes(&hdr)); } + + if (with_manifest) { + const build_zig_zon = + \\ .{ + \\ .name = "fetch", + \\ .version = "0.0.0", + \\ .paths = .{ + \\ "src", + \\ "build.zig", + \\ "build.zig.zon" + \\ }, + \\ } + ; + var hdr = TarHeader.init(); + hdr.typeflag = .regular; + try hdr.setPath(prefix, "build.zig.zon"); + try hdr.setSize(build_zig_zon.len); + try hdr.updateChecksum(); + try file.writeAll(std.mem.asBytes(&hdr)); + try file.writeAll(build_zig_zon); + try file.writeAll(&[_]u8{0} ** (512 - build_zig_zon.len)); + } } From dfec4918a3730df142bc7b58bfc0a6242cb3ca3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Fri, 29 Mar 2024 13:31:43 +0100 Subject: [PATCH 05/18] fetch: remove absolute path from tests --- src/Package/Fetch.zig | 219 ++++++++++++++++++------------------------ 1 file changed, 96 insertions(+), 123 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index cf27265b394f..cfb8706ec51c 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -1193,14 +1193,14 @@ fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!UnpackRes res.root_dir = try gpa.dupe(u8, root_dir); } if (diagnostics.errors.items.len > 0) { + try res.rootErrorMessage("unable to unpack tarball"); for (diagnostics.errors.items) |item| { switch (item) { - .unable_to_create_file => |i| try res.createFile(i.file_name, i.code), - .unable_to_create_sym_link => |i| try res.symLink(i.file_name, i.link_name, i.code), + .unable_to_create_file => |i| try res.unableToCreateFile(i.file_name, i.code), + .unable_to_create_sym_link => |i| try res.unableToCreateSymLink(i.file_name, i.link_name, i.code), .unsupported_file_type => |i| try res.unsupportedFileType(i.file_name, @intFromEnum(i.file_type)), } } - try res.rootErrorMessage("unable to unpack tarball"); } return res; } @@ -1246,14 +1246,12 @@ fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!Unpac try repository.checkout(out_dir, want_oid, &diagnostics); if (diagnostics.errors.items.len > 0) { - defer res.deinit(); - + try res.rootErrorMessage("unable to unpack packfile"); for (diagnostics.errors.items) |item| { switch (item) { - .unable_to_create_sym_link => |i| try res.symLink(i.file_name, i.link_name, i.code), + .unable_to_create_sym_link => |i| try res.unableToCreateSymLink(i.file_name, i.link_name, i.code), } } - try res.rootErrorMessage("unable to unpack packfile"); } } } @@ -1812,14 +1810,14 @@ const UnpackResult = struct { return self.errors.items.len > 0; } - fn createFile(self: *UnpackResult, file_name: []const u8, err: anyerror) !void { + fn unableToCreateFile(self: *UnpackResult, file_name: []const u8, err: anyerror) !void { try self.errors.append(self.allocator, .{ .unable_to_create_file = .{ .code = err, .file_name = try self.allocator.dupe(u8, file_name), } }); } - fn symLink(self: *UnpackResult, file_name: []const u8, link_name: []const u8, err: anyerror) !void { + fn unableToCreateSymLink(self: *UnpackResult, file_name: []const u8, link_name: []const u8, err: anyerror) !void { try self.errors.append(self.allocator, .{ .unable_to_create_sym_link = .{ .code = err, .file_name = try self.allocator.dupe(u8, file_name), @@ -1892,121 +1890,51 @@ const UnpackResult = struct { }; test "fetch tarball: fail with unable to create file" { - const testing = std.testing; - var buf: [4096]u8 = undefined; - var buf_pos: usize = 0; - - // Create tmp dir var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); - const tmp_path = try tmp.dir.realpath(".", &buf); - buf_pos += tmp_path.len; - // Create tarball in tmp dir without build.zig.zon const tarball_name = "package.tar"; try createTestTarball(tmp.dir, tarball_name, false); - // Get path to the tarball - const path_or_url = try std.fmt.bufPrint(buf[buf_pos..], "file://{s}/{s}", .{ tmp_path, tarball_name }); - buf_pos += path_or_url.len; - - // Global cache directory in tmp - const cache_path = try std.fmt.bufPrint(buf[buf_pos..], "{s}/{s}", .{ tmp_path, "global_cache" }); - buf_pos += cache_path.len; - // Run tarball fetch, expect to fail - var tf: TestFetch = undefined; - try tf.init(testing.allocator, cache_path, path_or_url); - defer tf.deinit(); - try testing.expectError(error.FetchFailed, tf.fetch.run()); - - // Expect fetch errors - { - var errors = try tf.fetch.error_bundle.toOwnedBundle(""); - defer errors.deinit(testing.allocator); - - const em = errors.getErrorMessage(errors.getMessages()[0]); - try testing.expectEqual(1, em.count); - try testing.expectEqual(2, em.notes_len); - - var al = std.ArrayList(u8).init(testing.allocator); - defer al.deinit(); - try errors.renderToWriter(.{ .ttyconf = .no_color }, al.writer()); - try testing.expectEqualStrings( - \\error: unable to unpack tarball - \\ note: unable to create file 'dir/file': PathAlreadyExists - \\ note: unable to create file 'dir1/file1': PathAlreadyExists - \\ - , al.items); - } + var fb: TestFetchBuilder = undefined; + var fetch = try fb.build(std.testing.allocator, tmp, tarball_name); + defer fb.deinit(); + try std.testing.expectError(error.FetchFailed, fetch.run()); + + try fb.expectFetchErrors(2, + \\error: unable to unpack tarball + \\ note: unable to create file 'dir/file': PathAlreadyExists + \\ note: unable to create file 'dir1/file1': PathAlreadyExists + \\ + ); } test "fetch tarball: error path are excluded" { - const testing = std.testing; - var buf: [4096]u8 = undefined; - var buf_pos: usize = 0; - - // Create tmp dir var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); - const tmp_path = try tmp.dir.realpath(".", &buf); - buf_pos += tmp_path.len; - // Create tarball in tmp dir const tarball_name = "package.tar"; try createTestTarball(tmp.dir, tarball_name, true); - // Get path to the tarball - const path_or_url = try std.fmt.bufPrint(buf[buf_pos..], "file://{s}/{s}", .{ tmp_path, tarball_name }); - buf_pos += path_or_url.len; - - // Global cache directory in tmp - const cache_path = try std.fmt.bufPrint(buf[buf_pos..], "{s}/{s}", .{ tmp_path, "global_cache" }); - buf_pos += cache_path.len; + // Run tarball fetch, should succeed + var fb: TestFetchBuilder = undefined; + var fetch = try fb.build(std.testing.allocator, tmp, tarball_name); + defer fb.deinit(); + try fetch.run(); - // Run tarball fetch - var tf: TestFetch = undefined; - try tf.init(testing.allocator, cache_path, path_or_url); - defer tf.deinit(); - try tf.fetch.run(); - - const hex_digest = Package.Manifest.hexDigest(tf.fetch.actual_hash); - try testing.expectEqualStrings("122022afac878639d5ea6fcca14a123e21fd0395c1f2ef2c89017fa71390f73024af", &hex_digest); + const hex_digest = Package.Manifest.hexDigest(fetch.actual_hash); + try std.testing.expectEqualStrings("122022afac878639d5ea6fcca14a123e21fd0395c1f2ef2c89017fa71390f73024af", &hex_digest); const expected_files: []const []const u8 = &.{ "build.zig", "build.zig.zon", "src/main.zig", }; - // Unpacked package contains expected files - { - const package_path = try std.fmt.bufPrint(buf[buf_pos..], "global_cache/p/{s}", .{hex_digest}); - buf_pos += package_path.len; - var package_dir = try tmp.dir.openDir(package_path, .{ .iterate = true }); - - var actual_files: std.ArrayListUnmanaged([]u8) = .{}; - defer actual_files.deinit(testing.allocator); - defer for (actual_files.items) |file| testing.allocator.free(file); - var walker = try package_dir.walk(testing.allocator); - defer walker.deinit(); - while (try walker.next()) |entry| { - if (entry.kind != .file) continue; - //std.debug.print("{s}\n", .{entry.path}); - const path = try testing.allocator.dupe(u8, entry.path); - errdefer testing.allocator.free(path); - std.mem.replaceScalar(u8, path, std.fs.path.sep, '/'); - try actual_files.append(testing.allocator, path); - } - std.mem.sortUnstable([]u8, actual_files.items, {}, struct { - fn lessThan(_: void, a: []u8, b: []u8) bool { - return std.mem.lessThan(u8, a, b); - } - }.lessThan); - try testing.expectEqualDeep(expected_files, actual_files.items); - } + try fb.expectPackageFiles(expected_files); } -const TestFetch = struct { +const TestFetchBuilder = struct { thread_pool: ThreadPool, http_client: std.http.Client, global_cache_directory: Cache.Directory, @@ -2014,36 +1942,30 @@ const TestFetch = struct { root_prog_node: *std.Progress.Node, job_queue: Fetch.JobQueue, fetch: Fetch, - gpa: std.mem.Allocator, - fn init( - tf: *TestFetch, - gpa: std.mem.Allocator, - global_cache_directory_path: []const u8, - path_or_url: []const u8, - ) !void { - try tf.thread_pool.init(.{ .allocator = gpa }); - tf.http_client = .{ .allocator = gpa }; - tf.global_cache_directory = .{ - .handle = try fs.cwd().makeOpenPath(global_cache_directory_path, .{}), - .path = global_cache_directory_path, - }; + fn build(self: *TestFetchBuilder, allocator: std.mem.Allocator, tmp: std.testing.TmpDir, tarball_name: []const u8) !*Fetch { + const cache_dir = try tmp.dir.makeOpenPath("zig-global-cache", .{}); + const path_or_url = try std.fmt.allocPrint(allocator, "zig-cache/tmp/{s}/{s}", .{ tmp.sub_path, tarball_name }); + + try self.thread_pool.init(.{ .allocator = allocator }); + self.http_client = .{ .allocator = allocator }; + self.global_cache_directory = .{ .handle = cache_dir, .path = null }; - tf.progress = .{ .dont_print_on_dumb = true }; - tf.root_prog_node = tf.progress.start("Fetch", 0); + self.progress = .{ .dont_print_on_dumb = true }; + self.root_prog_node = self.progress.start("Fetch", 0); - tf.job_queue = .{ - .http_client = &tf.http_client, - .thread_pool = &tf.thread_pool, - .global_cache = tf.global_cache_directory, + self.job_queue = .{ + .http_client = &self.http_client, + .thread_pool = &self.thread_pool, + .global_cache = self.global_cache_directory, .recursive = false, .read_only = false, .debug_hash = false, .work_around_btrfs_bug = false, }; - tf.fetch = .{ - .arena = std.heap.ArenaAllocator.init(gpa), + self.fetch = .{ + .arena = std.heap.ArenaAllocator.init(allocator), .location = .{ .path_or_url = path_or_url }, .location_tok = 0, .hash_tok = 0, @@ -2051,8 +1973,8 @@ const TestFetch = struct { .lazy_status = .eager, .parent_package_root = Cache.Path{ .root_dir = undefined }, .parent_manifest_ast = null, - .prog_node = tf.root_prog_node, - .job_queue = &tf.job_queue, + .prog_node = self.root_prog_node, + .job_queue = &self.job_queue, .omit_missing_hash_error = true, .allow_missing_paths_field = false, @@ -2066,9 +1988,11 @@ const TestFetch = struct { .module = null, }; + return &self.fetch; } - fn deinit(self: *TestFetch) void { + fn deinit(self: *TestFetchBuilder) void { + self.fetch.arena.child_allocator.free(self.fetch.location.path_or_url); self.fetch.deinit(); self.job_queue.deinit(); self.root_prog_node.end(); @@ -2076,6 +2000,55 @@ const TestFetch = struct { self.http_client.deinit(); self.thread_pool.deinit(); } + + fn packageDir(self: *TestFetchBuilder) !fs.Dir { + const root = self.fetch.package_root; + return try root.root_dir.handle.openDir(root.sub_path, .{ .iterate = true }); + } + + fn expectPackageFiles(self: *TestFetchBuilder, expected_files: []const []const u8) !void { + var package_dir = try self.packageDir(); + defer package_dir.close(); + + var actual_files: std.ArrayListUnmanaged([]u8) = .{}; + defer actual_files.deinit(std.testing.allocator); + defer for (actual_files.items) |file| std.testing.allocator.free(file); + var walker = try package_dir.walk(std.testing.allocator); + defer walker.deinit(); + while (try walker.next()) |entry| { + if (entry.kind != .file) continue; + // std.debug.print("{s}\n", .{entry.path}); + const path = try std.testing.allocator.dupe(u8, entry.path); + errdefer std.testing.allocator.free(path); + std.mem.replaceScalar(u8, path, std.fs.path.sep, '/'); + try actual_files.append(std.testing.allocator, path); + } + std.mem.sortUnstable([]u8, actual_files.items, {}, struct { + fn lessThan(_: void, a: []u8, b: []u8) bool { + return std.mem.lessThan(u8, a, b); + } + }.lessThan); + + try std.testing.expectEqual(expected_files.len, actual_files.items.len); + for (expected_files, 0..) |file_name, i| { + try std.testing.expectEqualStrings(file_name, actual_files.items[i]); + } + try std.testing.expectEqualDeep(expected_files, actual_files.items); + } + + fn expectFetchErrors(self: *TestFetchBuilder, notes_len: usize, msg: []const u8) !void { + var errors = try self.fetch.error_bundle.toOwnedBundle(""); + defer errors.deinit(std.testing.allocator); + + const em = errors.getErrorMessage(errors.getMessages()[0]); + try std.testing.expectEqual(1, em.count); + try std.testing.expectEqual(notes_len, em.notes_len); + + var al = std.ArrayList(u8).init(std.testing.allocator); + defer al.deinit(); + try errors.renderToWriter(.{ .ttyconf = .no_color }, al.writer()); + try std.testing.expectEqualStrings(msg, al.items); + } }; fn createTestTarball(dir: fs.Dir, tarball_name: []const u8, with_manifest: bool) !void { From ad60b6c1edfc9ee30cf137fb1d56269e0af0941c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Fri, 29 Mar 2024 20:48:51 +0100 Subject: [PATCH 06/18] fetch: update comments --- src/Package/Fetch.zig | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index cfb8706ec51c..a7f146ad0fad 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -461,6 +461,7 @@ fn runResource( }; defer tmp_directory.handle.close(); + // Fetch and unpack a resource into a temporary directory. var unpack_result = try unpackResource(f, resource, uri_path, tmp_directory); defer unpack_result.deinit(); @@ -487,9 +488,8 @@ fn runResource( .include_paths = if (f.manifest) |m| m.paths else .{}, }; - // If any error occurred for files that were ultimately excluded, those - // errors should be ignored, such as failure to create symlinks that - // weren't supposed to be included anyway. + // Ignore errors that were excluded by manifest, such as failure to + // create symlinks that weren't supposed to be included anyway. try unpack_result.filterErrors(filter); if (unpack_result.hasErrors()) { try unpack_result.bundleErrors(eb, try f.srcLoc(f.location_tok)); From 5a38924a7d41fee0ac5544d9ddb85ce82bc92030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Sat, 30 Mar 2024 22:43:27 +0100 Subject: [PATCH 07/18] fetch.git: collect file create diagnostic errors On case insensitive file systems, don't overwrite files with same name in different casing. Add diagnostic error so caller could decide what to do. --- src/Package/Fetch.zig | 1 + src/Package/Fetch/git.zig | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index a7f146ad0fad..83bc2753433e 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -1249,6 +1249,7 @@ fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!Unpac try res.rootErrorMessage("unable to unpack packfile"); for (diagnostics.errors.items) |item| { switch (item) { + .unable_to_create_file => |i| try res.unableToCreateFile(i.file_name, i.code), .unable_to_create_sym_link => |i| try res.unableToCreateSymLink(i.file_name, i.link_name, i.code), } } diff --git a/src/Package/Fetch/git.zig b/src/Package/Fetch/git.zig index 36652bd88c55..d7cdd8483c8e 100644 --- a/src/Package/Fetch/git.zig +++ b/src/Package/Fetch/git.zig @@ -46,6 +46,10 @@ pub const Diagnostics = struct { file_name: []const u8, link_name: []const u8, }, + unable_to_create_file: struct { + code: anyerror, + file_name: []const u8, + }, }; pub fn deinit(d: *Diagnostics) void { @@ -55,6 +59,9 @@ pub const Diagnostics = struct { d.allocator.free(info.file_name); d.allocator.free(info.link_name); }, + .unable_to_create_file => |info| { + d.allocator.free(info.file_name); + }, } } d.errors.deinit(d.allocator); @@ -119,11 +126,19 @@ pub const Repository = struct { try repository.checkoutTree(subdir, entry.oid, sub_path, diagnostics); }, .file => { - var file = try dir.createFile(entry.name, .{}); - defer file.close(); try repository.odb.seekOid(entry.oid); const file_object = try repository.odb.readObject(); if (file_object.type != .blob) return error.InvalidFile; + var file = dir.createFile(entry.name, .{ .exclusive = true }) catch |e| { + const file_name = try std.fs.path.join(diagnostics.allocator, &.{ current_path, entry.name }); + errdefer diagnostics.allocator.free(file_name); + try diagnostics.errors.append(diagnostics.allocator, .{ .unable_to_create_file = .{ + .code = e, + .file_name = file_name, + } }); + continue; + }; + defer file.close(); try file.writeAll(file_object.data); try file.sync(); }, From 373d48212f91ab1fd345da49f8a3c737075274d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Sat, 30 Mar 2024 23:41:13 +0100 Subject: [PATCH 08/18] fetch: add pathological packages test Using test cases from: https://github.com/ianprime0509/pathological-packages repository. Depends on existence of the FAT32 file system. Folder is in FAT32 file system because it is case insensitive and and does not support symlinks. It is complicated test case requires internet connection, depends on existence of FAT32 in the specific location. But it is so valuable for development. Running `zig test Package.zig` is so much faster than building zig binary and running `zig fetch URL`. Committing it here although it should probably be removed. --- src/Package/Fetch.zig | 141 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 131 insertions(+), 10 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 83bc2753433e..bf52c9cef219 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -1890,16 +1890,19 @@ const UnpackResult = struct { } }; -test "fetch tarball: fail with unable to create file" { +test "tarball with duplicate file names" { + const gpa = std.testing.allocator; var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); const tarball_name = "package.tar"; try createTestTarball(tmp.dir, tarball_name, false); + const tarball_path = try std.fmt.allocPrint(gpa, "zig-cache/tmp/{s}/{s}", .{ tmp.sub_path, tarball_name }); + defer gpa.free(tarball_path); // Run tarball fetch, expect to fail var fb: TestFetchBuilder = undefined; - var fetch = try fb.build(std.testing.allocator, tmp, tarball_name); + var fetch = try fb.build(gpa, tmp.dir, tarball_path); defer fb.deinit(); try std.testing.expectError(error.FetchFailed, fetch.run()); @@ -1911,16 +1914,19 @@ test "fetch tarball: fail with unable to create file" { ); } -test "fetch tarball: error path are excluded" { +test "tarball with error paths excluded" { + const gpa = std.testing.allocator; var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); const tarball_name = "package.tar"; try createTestTarball(tmp.dir, tarball_name, true); + const tarball_path = try std.fmt.allocPrint(gpa, "zig-cache/tmp/{s}/{s}", .{ tmp.sub_path, tarball_name }); + defer gpa.free(tarball_path); // Run tarball fetch, should succeed var fb: TestFetchBuilder = undefined; - var fetch = try fb.build(std.testing.allocator, tmp, tarball_name); + var fetch = try fb.build(std.testing.allocator, tmp.dir, tarball_path); defer fb.deinit(); try fetch.run(); @@ -1944,9 +1950,8 @@ const TestFetchBuilder = struct { job_queue: Fetch.JobQueue, fetch: Fetch, - fn build(self: *TestFetchBuilder, allocator: std.mem.Allocator, tmp: std.testing.TmpDir, tarball_name: []const u8) !*Fetch { - const cache_dir = try tmp.dir.makeOpenPath("zig-global-cache", .{}); - const path_or_url = try std.fmt.allocPrint(allocator, "zig-cache/tmp/{s}/{s}", .{ tmp.sub_path, tarball_name }); + fn build(self: *TestFetchBuilder, allocator: std.mem.Allocator, cache_parent_dir: std.fs.Dir, path_or_url: []const u8) !*Fetch { + const cache_dir = try cache_parent_dir.makeOpenPath("zig-global-cache", .{}); try self.thread_pool.init(.{ .allocator = allocator }); self.http_client = .{ .allocator = allocator }; @@ -1993,7 +1998,6 @@ const TestFetchBuilder = struct { } fn deinit(self: *TestFetchBuilder) void { - self.fetch.arena.child_allocator.free(self.fetch.location.path_or_url); self.fetch.deinit(); self.job_queue.deinit(); self.root_prog_node.end(); @@ -2043,8 +2047,9 @@ const TestFetchBuilder = struct { const em = errors.getErrorMessage(errors.getMessages()[0]); try std.testing.expectEqual(1, em.count); - try std.testing.expectEqual(notes_len, em.notes_len); - + if (notes_len > 0) { + try std.testing.expectEqual(notes_len, em.notes_len); + } var al = std.ArrayList(u8).init(std.testing.allocator); defer al.deinit(); try errors.renderToWriter(.{ .ttyconf = .no_color }, al.writer()); @@ -2098,3 +2103,119 @@ fn createTestTarball(dir: fs.Dir, tarball_name: []const u8, with_manifest: bool) try file.writeAll(&[_]u8{0} ** (512 - build_zig_zon.len)); } } + +// Using test cases from: https://github.com/ianprime0509/pathological-packages +// repository. Depends on existence of the FAT32 file system at /tmp/fat32.mnt +// (look at the fat32TmpDir function below how to create it). If that folder is +// not found test will be skipped. Folder is in FAT32 file system because it is +// case insensitive and and does not support symlinks. +test "pathological packages" { + const gpa = std.testing.allocator; + var buf: [128]u8 = undefined; + + const urls: []const []const u8 = &.{ + "https://github.com/ianprime0509/pathological-packages/archive/{s}.tar.gz", + "git+https://github.com/ianprime0509/pathological-packages#{s}", + }; + const branches: []const []const u8 = &.{ + "excluded-case-collisions", + "excluded-symlinks", + "included-case-collisions", + "included-symlinks", + }; + + // Expected fetched package files or error message for each combination of url/branch. + const expected = [_]struct { + files: []const []const u8 = &.{}, + err_msg: []const u8 = "", + }{ + // tar + .{ .files = &.{ "build.zig", "build.zig.zon" } }, + .{ .files = &.{ "build.zig", "build.zig.zon", "main" } }, + .{ .err_msg = + \\error: unable to unpack tarball + \\ note: unable to create file 'main': PathAlreadyExists + \\ note: unable to create file 'subdir/main': PathAlreadyExists + \\ + }, + .{ .err_msg = + \\error: unable to unpack tarball + \\ note: unable to create symlink from 'link' to 'main': AccessDenied + \\ note: unable to create symlink from 'subdir/link' to 'main': AccessDenied + \\ + }, + // git + .{ .files = &.{ "build.zig", "build.zig.zon" } }, + .{ .files = &.{ "build.zig", "build.zig.zon", "main" } }, + .{ .err_msg = + \\error: unable to unpack packfile + \\ note: unable to create file 'main': PathAlreadyExists + \\ note: unable to create file 'subdir/main': PathAlreadyExists + \\ + }, + .{ .err_msg = + \\error: unable to unpack packfile + \\ note: unable to create symlink from 'link' to 'main': AccessDenied + \\ note: unable to create symlink from 'subdir/link' to 'main': AccessDenied + \\ + }, + }; + + var expected_no: usize = 0; + inline for (urls) |url_fmt| { + var tmp = try fat32TmpDir(); + defer tmp.cleanup(); + + for (branches) |branch| { + defer expected_no += 1; + const url = try std.fmt.bufPrint(&buf, url_fmt, .{branch}); + // std.debug.print("fetching url: {s}\n", .{url}); + + var fb: TestFetchBuilder = undefined; + var fetch = try fb.build(gpa, tmp.dir, url); + defer fb.deinit(); + + const ex = expected[expected_no]; + if (ex.err_msg.len > 0) { + try std.testing.expectError(error.FetchFailed, fetch.run()); + try fb.expectFetchErrors(0, ex.err_msg); + } else { + try fetch.run(); + try fb.expectPackageFiles(ex.files); + } + } + } +} + +// Using logic from std.testing.tmpDir() to make temporary directory at specific +// location. +// +// This assumes FAT32 file system in /tmp/fat32.mnt folder, +// created with something like this: +// $ cd /tmp && fallocate -l 1M fat32.fs && mkfs.fat -F32 fat32.fs && mkdir fat32.mnt && sudo mount -o rw,umask=0000 fat32.fs fat32.mnt +// +// To remove that folder: +// $ cd /tmp && sudo umount fat32.mnt && rm -rf fat32.mnt fat32.fs +// +pub fn fat32TmpDir() !std.testing.TmpDir { + const fat32fs_path = "/tmp/fat32.mnt/"; + + const random_bytes_count = 12; + var random_bytes: [random_bytes_count]u8 = undefined; + std.crypto.random.bytes(&random_bytes); + var sub_path: [std.fs.base64_encoder.calcSize(random_bytes_count)]u8 = undefined; + _ = std.fs.base64_encoder.encode(&sub_path, &random_bytes); + + const parent_dir = std.fs.openDirAbsolute(fat32fs_path, .{}) catch |err| switch (err) { + error.FileNotFound => return error.SkipZigTest, + else => return err, + }; + const dir = parent_dir.makeOpenPath(&sub_path, .{}) catch + @panic("unable to make tmp dir for testing: unable to make and open the tmp dir"); + + return .{ + .dir = dir, + .parent_dir = parent_dir, + .sub_path = sub_path, + }; +} From dc61c2e9043f89fcfe540431131563a7269167b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Sun, 7 Apr 2024 01:05:29 +0200 Subject: [PATCH 09/18] add comments --- src/Package/Fetch.zig | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index bf52c9cef219..73263229d80f 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -1744,6 +1744,9 @@ test FileHeader { try std.testing.expect(h.isExecutable()); } +// Result of the `unpackResource` operation. Enables collecting errors from +// tar/git diagnostic, filtering that errors by manifest inclusion rules and +// emitting remaining errors to an `ErrorBundle`. const UnpackResult = struct { allocator: std.mem.Allocator, errors: std.ArrayListUnmanaged(Error) = .{}, @@ -1833,6 +1836,7 @@ const UnpackResult = struct { } }); } + // Filter errors by manifest inclusion rules. fn filterErrors(self: *UnpackResult, filter: Filter) !void { var i = self.errors.items.len; const root_dir: []const u8 = if (self.root_dir) |root_dir| root_dir else ""; @@ -1850,11 +1854,15 @@ const UnpackResult = struct { self.root_error_message = try self.allocator.dupe(u8, msg); } + // Emmit errors to an `ErrorBundle`. fn bundleErrors( self: *UnpackResult, eb: *ErrorBundle.Wip, src_loc: ErrorBundle.SourceLocationIndex, ) !void { + if (self.errors.items.len == 0 and self.root_error_message.len == 0) + return; + const notes_len: u32 = @intCast(self.errors.items.len); try eb.addRootErrorMessage(.{ .msg = try eb.addString(self.root_error_message), @@ -1941,16 +1949,21 @@ test "tarball with error paths excluded" { try fb.expectPackageFiles(expected_files); } +// Builds Fetch with required dependencies, clears dependencies on deinit(). const TestFetchBuilder = struct { thread_pool: ThreadPool, http_client: std.http.Client, global_cache_directory: Cache.Directory, progress: std.Progress, - root_prog_node: *std.Progress.Node, job_queue: Fetch.JobQueue, fetch: Fetch, - fn build(self: *TestFetchBuilder, allocator: std.mem.Allocator, cache_parent_dir: std.fs.Dir, path_or_url: []const u8) !*Fetch { + fn build( + self: *TestFetchBuilder, + allocator: std.mem.Allocator, + cache_parent_dir: std.fs.Dir, + path_or_url: []const u8, + ) !*Fetch { const cache_dir = try cache_parent_dir.makeOpenPath("zig-global-cache", .{}); try self.thread_pool.init(.{ .allocator = allocator }); @@ -1958,7 +1971,6 @@ const TestFetchBuilder = struct { self.global_cache_directory = .{ .handle = cache_dir, .path = null }; self.progress = .{ .dont_print_on_dumb = true }; - self.root_prog_node = self.progress.start("Fetch", 0); self.job_queue = .{ .http_client = &self.http_client, @@ -1979,7 +1991,7 @@ const TestFetchBuilder = struct { .lazy_status = .eager, .parent_package_root = Cache.Path{ .root_dir = undefined }, .parent_manifest_ast = null, - .prog_node = self.root_prog_node, + .prog_node = self.progress.start("Fetch", 0), .job_queue = &self.job_queue, .omit_missing_hash_error = true, .allow_missing_paths_field = false, @@ -1991,7 +2003,6 @@ const TestFetchBuilder = struct { .actual_hash = undefined, .has_build_zig = false, .oom_flag = false, - .module = null, }; return &self.fetch; @@ -2000,7 +2011,7 @@ const TestFetchBuilder = struct { fn deinit(self: *TestFetchBuilder) void { self.fetch.deinit(); self.job_queue.deinit(); - self.root_prog_node.end(); + self.fetch.prog_node.end(); self.global_cache_directory.handle.close(); self.http_client.deinit(); self.thread_pool.deinit(); @@ -2011,6 +2022,8 @@ const TestFetchBuilder = struct { return try root.root_dir.handle.openDir(root.sub_path, .{ .iterate = true }); } + // Test helper, asserts thet package dir constains expected_files. + // expected_files must be sorted. fn expectPackageFiles(self: *TestFetchBuilder, expected_files: []const []const u8) !void { var package_dir = try self.packageDir(); defer package_dir.close(); @@ -2041,6 +2054,7 @@ const TestFetchBuilder = struct { try std.testing.expectEqualDeep(expected_files, actual_files.items); } + // Test helper, asserts that fetch has failed with `msg` error message. fn expectFetchErrors(self: *TestFetchBuilder, notes_len: usize, msg: []const u8) !void { var errors = try self.fetch.error_bundle.toOwnedBundle(""); defer errors.deinit(std.testing.allocator); @@ -2057,6 +2071,10 @@ const TestFetchBuilder = struct { } }; +// Creates tarball with duplicate files names. Simulating case collisions on +// case insensitive file system without use of that kind of the file system. +// Manifest will exclude those files, so adding manifest should remove duplicate +// files problem. fn createTestTarball(dir: fs.Dir, tarball_name: []const u8, with_manifest: bool) !void { const file = try dir.createFile(tarball_name, .{}); defer file.close(); From 84fac2242cb6dfaec4916279ad14ee1256a7aa59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Sun, 31 Mar 2024 21:35:18 +0200 Subject: [PATCH 10/18] fix zig fmt --- src/Package/Fetch.zig | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 73263229d80f..dd0482987809 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -2150,13 +2150,13 @@ test "pathological packages" { // tar .{ .files = &.{ "build.zig", "build.zig.zon" } }, .{ .files = &.{ "build.zig", "build.zig.zon", "main" } }, - .{ .err_msg = + .{ .err_msg = \\error: unable to unpack tarball \\ note: unable to create file 'main': PathAlreadyExists \\ note: unable to create file 'subdir/main': PathAlreadyExists \\ }, - .{ .err_msg = + .{ .err_msg = \\error: unable to unpack tarball \\ note: unable to create symlink from 'link' to 'main': AccessDenied \\ note: unable to create symlink from 'subdir/link' to 'main': AccessDenied @@ -2165,13 +2165,13 @@ test "pathological packages" { // git .{ .files = &.{ "build.zig", "build.zig.zon" } }, .{ .files = &.{ "build.zig", "build.zig.zon", "main" } }, - .{ .err_msg = + .{ .err_msg = \\error: unable to unpack packfile \\ note: unable to create file 'main': PathAlreadyExists \\ note: unable to create file 'subdir/main': PathAlreadyExists \\ }, - .{ .err_msg = + .{ .err_msg = \\error: unable to unpack packfile \\ note: unable to create symlink from 'link' to 'main': AccessDenied \\ note: unable to create symlink from 'subdir/link' to 'main': AccessDenied From fc745fb05c2ab34c5589d3f04700d0e4d537ed68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Wed, 3 Apr 2024 13:39:24 +0200 Subject: [PATCH 11/18] fetch: remove test with mount and net dependencies --- src/Package/Fetch.zig | 116 ------------------------------------------ 1 file changed, 116 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index dd0482987809..fa2bea859fd9 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -2121,119 +2121,3 @@ fn createTestTarball(dir: fs.Dir, tarball_name: []const u8, with_manifest: bool) try file.writeAll(&[_]u8{0} ** (512 - build_zig_zon.len)); } } - -// Using test cases from: https://github.com/ianprime0509/pathological-packages -// repository. Depends on existence of the FAT32 file system at /tmp/fat32.mnt -// (look at the fat32TmpDir function below how to create it). If that folder is -// not found test will be skipped. Folder is in FAT32 file system because it is -// case insensitive and and does not support symlinks. -test "pathological packages" { - const gpa = std.testing.allocator; - var buf: [128]u8 = undefined; - - const urls: []const []const u8 = &.{ - "https://github.com/ianprime0509/pathological-packages/archive/{s}.tar.gz", - "git+https://github.com/ianprime0509/pathological-packages#{s}", - }; - const branches: []const []const u8 = &.{ - "excluded-case-collisions", - "excluded-symlinks", - "included-case-collisions", - "included-symlinks", - }; - - // Expected fetched package files or error message for each combination of url/branch. - const expected = [_]struct { - files: []const []const u8 = &.{}, - err_msg: []const u8 = "", - }{ - // tar - .{ .files = &.{ "build.zig", "build.zig.zon" } }, - .{ .files = &.{ "build.zig", "build.zig.zon", "main" } }, - .{ .err_msg = - \\error: unable to unpack tarball - \\ note: unable to create file 'main': PathAlreadyExists - \\ note: unable to create file 'subdir/main': PathAlreadyExists - \\ - }, - .{ .err_msg = - \\error: unable to unpack tarball - \\ note: unable to create symlink from 'link' to 'main': AccessDenied - \\ note: unable to create symlink from 'subdir/link' to 'main': AccessDenied - \\ - }, - // git - .{ .files = &.{ "build.zig", "build.zig.zon" } }, - .{ .files = &.{ "build.zig", "build.zig.zon", "main" } }, - .{ .err_msg = - \\error: unable to unpack packfile - \\ note: unable to create file 'main': PathAlreadyExists - \\ note: unable to create file 'subdir/main': PathAlreadyExists - \\ - }, - .{ .err_msg = - \\error: unable to unpack packfile - \\ note: unable to create symlink from 'link' to 'main': AccessDenied - \\ note: unable to create symlink from 'subdir/link' to 'main': AccessDenied - \\ - }, - }; - - var expected_no: usize = 0; - inline for (urls) |url_fmt| { - var tmp = try fat32TmpDir(); - defer tmp.cleanup(); - - for (branches) |branch| { - defer expected_no += 1; - const url = try std.fmt.bufPrint(&buf, url_fmt, .{branch}); - // std.debug.print("fetching url: {s}\n", .{url}); - - var fb: TestFetchBuilder = undefined; - var fetch = try fb.build(gpa, tmp.dir, url); - defer fb.deinit(); - - const ex = expected[expected_no]; - if (ex.err_msg.len > 0) { - try std.testing.expectError(error.FetchFailed, fetch.run()); - try fb.expectFetchErrors(0, ex.err_msg); - } else { - try fetch.run(); - try fb.expectPackageFiles(ex.files); - } - } - } -} - -// Using logic from std.testing.tmpDir() to make temporary directory at specific -// location. -// -// This assumes FAT32 file system in /tmp/fat32.mnt folder, -// created with something like this: -// $ cd /tmp && fallocate -l 1M fat32.fs && mkfs.fat -F32 fat32.fs && mkdir fat32.mnt && sudo mount -o rw,umask=0000 fat32.fs fat32.mnt -// -// To remove that folder: -// $ cd /tmp && sudo umount fat32.mnt && rm -rf fat32.mnt fat32.fs -// -pub fn fat32TmpDir() !std.testing.TmpDir { - const fat32fs_path = "/tmp/fat32.mnt/"; - - const random_bytes_count = 12; - var random_bytes: [random_bytes_count]u8 = undefined; - std.crypto.random.bytes(&random_bytes); - var sub_path: [std.fs.base64_encoder.calcSize(random_bytes_count)]u8 = undefined; - _ = std.fs.base64_encoder.encode(&sub_path, &random_bytes); - - const parent_dir = std.fs.openDirAbsolute(fat32fs_path, .{}) catch |err| switch (err) { - error.FileNotFound => return error.SkipZigTest, - else => return err, - }; - const dir = parent_dir.makeOpenPath(&sub_path, .{}) catch - @panic("unable to make tmp dir for testing: unable to make and open the tmp dir"); - - return .{ - .dir = dir, - .parent_dir = parent_dir, - .sub_path = sub_path, - }; -} From 22e9c50376b8d7f2c67e3727bc40c697942576c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Thu, 4 Apr 2024 17:56:14 +0200 Subject: [PATCH 12/18] fetch: fix test tarball Should include folder structure, at least root folder so it can be found in pipeToFileSystem. --- src/Package/Fetch.zig | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index fa2bea859fd9..5a20a85fc9d6 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -1047,10 +1047,6 @@ fn initResource(f: *Fetch, uri: std.Uri, server_header_buffer: []u8) RunError!Re )); } -/// A `null` return value indicates the `tmp_directory` is populated directly -/// with the package contents. -/// A non-null return value means that the package contents are inside a -/// sub-directory indicated by the named path. fn unpackResource( f: *Fetch, resource: *Resource, @@ -1751,6 +1747,9 @@ const UnpackResult = struct { allocator: std.mem.Allocator, errors: std.ArrayListUnmanaged(Error) = .{}, root_error_message: []const u8 = "", + + // A `null` value indicates the `tmp_directory` is populated directly with the package contents. + // A non-null value means that the package contents are inside a sub-directory indicated by the named path. root_dir: ?[]const u8 = null, const Error = union(enum) { @@ -1774,7 +1773,6 @@ const UnpackResult = struct { .unable_to_create_sym_link => |info| info.file_name, .unsupported_file_type => |info| info.file_name, }; - return !filter.includePath(stripRoot(file_name, root_dir)); } @@ -1916,8 +1914,8 @@ test "tarball with duplicate file names" { try fb.expectFetchErrors(2, \\error: unable to unpack tarball - \\ note: unable to create file 'dir/file': PathAlreadyExists - \\ note: unable to create file 'dir1/file1': PathAlreadyExists + \\ note: unable to create file 'package.tar/dir/file': PathAlreadyExists + \\ note: unable to create file 'package.tar/dir1/file1': PathAlreadyExists \\ ); } @@ -2035,7 +2033,6 @@ const TestFetchBuilder = struct { defer walker.deinit(); while (try walker.next()) |entry| { if (entry.kind != .file) continue; - // std.debug.print("{s}\n", .{entry.path}); const path = try std.testing.allocator.dupe(u8, entry.path); errdefer std.testing.allocator.free(path); std.mem.replaceScalar(u8, path, std.fs.path.sep, '/'); @@ -2082,6 +2079,16 @@ fn createTestTarball(dir: fs.Dir, tarball_name: []const u8, with_manifest: bool) const TarHeader = std.tar.output.Header; const prefix = tarball_name; + // add root directory + { + var hdr = TarHeader.init(); + hdr.typeflag = .directory; + try hdr.setPath(prefix, ""); + try hdr.updateChecksum(); + try file.writeAll(std.mem.asBytes(&hdr)); + } + + // add files const files: []const []const u8 = &.{ "build.zig", "src/main.zig", @@ -2091,6 +2098,7 @@ fn createTestTarball(dir: fs.Dir, tarball_name: []const u8, with_manifest: bool) "dir/file", "dir1/file1", }; + for (files) |path| { var hdr = TarHeader.init(); hdr.typeflag = .regular; @@ -2099,6 +2107,7 @@ fn createTestTarball(dir: fs.Dir, tarball_name: []const u8, with_manifest: bool) try file.writeAll(std.mem.asBytes(&hdr)); } + // add manifest if (with_manifest) { const build_zig_zon = \\ .{ From 5f0b434f9031179609c97676bb43425f8710ac95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Thu, 4 Apr 2024 19:12:57 +0200 Subject: [PATCH 13/18] fetch: remove root_dir from error messages To be consistent with paths in manifest. --- src/Package/Fetch.zig | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 5a20a85fc9d6..90ff2314f8b0 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -1861,6 +1861,8 @@ const UnpackResult = struct { if (self.errors.items.len == 0 and self.root_error_message.len == 0) return; + const root_dir = if (self.root_dir) |root_dir| root_dir else ""; + const notes_len: u32 = @intCast(self.errors.items.len); try eb.addRootErrorMessage(.{ .msg = try eb.addString(self.root_error_message), @@ -1873,21 +1875,21 @@ const UnpackResult = struct { .unable_to_create_sym_link => |info| { eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ .msg = try eb.printString("unable to create symlink from '{s}' to '{s}': {s}", .{ - info.file_name, info.link_name, @errorName(info.code), + stripRoot(info.file_name, root_dir), info.link_name, @errorName(info.code), }), })); }, .unable_to_create_file => |info| { eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ .msg = try eb.printString("unable to create file '{s}': {s}", .{ - info.file_name, @errorName(info.code), + stripRoot(info.file_name, root_dir), @errorName(info.code), }), })); }, .unsupported_file_type => |info| { eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ .msg = try eb.printString("file '{s}' has unsupported type '{c}'", .{ - info.file_name, info.file_type, + stripRoot(info.file_name, root_dir), info.file_type, }), })); }, @@ -1914,8 +1916,8 @@ test "tarball with duplicate file names" { try fb.expectFetchErrors(2, \\error: unable to unpack tarball - \\ note: unable to create file 'package.tar/dir/file': PathAlreadyExists - \\ note: unable to create file 'package.tar/dir1/file1': PathAlreadyExists + \\ note: unable to create file 'dir/file': PathAlreadyExists + \\ note: unable to create file 'dir1/file1': PathAlreadyExists \\ ); } From 3d5a9237f7984a37f6aa086bd2b0783305f644e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Thu, 4 Apr 2024 19:45:35 +0200 Subject: [PATCH 14/18] fetch: use empty string instead of null for root_dir Make it consistent with Cache.Path sub_path. Remove null check in many locations. --- lib/std/tar.zig | 17 ++++++----------- src/Package/Fetch.zig | 34 +++++++++++++--------------------- 2 files changed, 19 insertions(+), 32 deletions(-) diff --git a/lib/std/tar.zig b/lib/std/tar.zig index 2977bc16ccc9..9dc5bb4a537b 100644 --- a/lib/std/tar.zig +++ b/lib/std/tar.zig @@ -30,7 +30,7 @@ pub const Diagnostics = struct { errors: std.ArrayListUnmanaged(Error) = .{}, root_entries: usize = 0, - root_dir: ?[]const u8 = null, + root_dir: []const u8 = "", pub const Error = union(enum) { unable_to_create_sym_link: struct { @@ -55,10 +55,8 @@ pub const Diagnostics = struct { d.root_dir = try d.allocator.dupe(u8, root_dir); return; } - if (d.root_dir) |r| { - d.allocator.free(r); - d.root_dir = null; - } + d.allocator.free(d.root_dir); + d.root_dir = ""; } } @@ -103,10 +101,7 @@ pub const Diagnostics = struct { } } d.errors.deinit(d.allocator); - if (d.root_dir) |r| { - d.allocator.free(r); - d.root_dir = null; - } + d.allocator.free(d.root_dir); d.* = undefined; } }; @@ -1060,7 +1055,7 @@ test "pipeToFileSystem root_dir" { }; // there is no root_dir - try testing.expect(diagnostics.root_dir == null); + try testing.expectEqual(0, diagnostics.root_dir.len); try testing.expectEqual(3, diagnostics.root_entries); } @@ -1082,7 +1077,7 @@ test "pipeToFileSystem root_dir" { }; // root_dir found - try testing.expectEqualStrings("example", diagnostics.root_dir.?); + try testing.expectEqualStrings("example", diagnostics.root_dir); try testing.expectEqual(1, diagnostics.root_entries); } } diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 90ff2314f8b0..a841a754f337 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -465,10 +465,7 @@ fn runResource( var unpack_result = try unpackResource(f, resource, uri_path, tmp_directory); defer unpack_result.deinit(); - var pkg_path: Cache.Path = .{ - .root_dir = tmp_directory, - .sub_path = if (unpack_result.root_dir) |root_dir| root_dir else "", - }; + var pkg_path: Cache.Path = .{ .root_dir = tmp_directory, .sub_path = unpack_result.root_dir }; // Apply btrfs workaround if needed. Reopen tmp_directory. if (native_os == .linux and f.job_queue.work_around_btrfs_bug) { @@ -503,8 +500,8 @@ fn runResource( // directory. f.actual_hash = try computeHash(f, pkg_path, filter); - break :blk if (unpack_result.root_dir) |root_dir| - try fs.path.join(arena, &.{ tmp_dir_sub_path, root_dir }) + break :blk if (unpack_result.root_dir.len > 0) + try fs.path.join(arena, &.{ tmp_dir_sub_path, unpack_result.root_dir }) else tmp_dir_sub_path; }; @@ -1185,8 +1182,8 @@ fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!UnpackRes )); var res = UnpackResult.init(gpa); - if (diagnostics.root_dir) |root_dir| { - res.root_dir = try gpa.dupe(u8, root_dir); + if (diagnostics.root_dir.len > 0) { + res.root_dir = try gpa.dupe(u8, diagnostics.root_dir); } if (diagnostics.errors.items.len > 0) { try res.rootErrorMessage("unable to unpack tarball"); @@ -1748,9 +1745,9 @@ const UnpackResult = struct { errors: std.ArrayListUnmanaged(Error) = .{}, root_error_message: []const u8 = "", - // A `null` value indicates the `tmp_directory` is populated directly with the package contents. - // A non-null value means that the package contents are inside a sub-directory indicated by the named path. - root_dir: ?[]const u8 = null, + // A non empty value means that the package contents are inside a + // sub-directory indicated by the named path. + root_dir: []const u8 = "", const Error = union(enum) { unable_to_create_sym_link: struct { @@ -1802,9 +1799,7 @@ const UnpackResult = struct { } self.errors.deinit(self.allocator); self.allocator.free(self.root_error_message); - if (self.root_dir) |root_dir| { - self.allocator.free(root_dir); - } + self.allocator.free(self.root_dir); self.* = undefined; } @@ -1837,11 +1832,10 @@ const UnpackResult = struct { // Filter errors by manifest inclusion rules. fn filterErrors(self: *UnpackResult, filter: Filter) !void { var i = self.errors.items.len; - const root_dir: []const u8 = if (self.root_dir) |root_dir| root_dir else ""; while (i > 0) { i -= 1; const item = self.errors.items[i]; - if (item.excluded(filter, root_dir)) { + if (item.excluded(filter, self.root_dir)) { _ = self.errors.swapRemove(i); item.free(self.allocator); } @@ -1861,8 +1855,6 @@ const UnpackResult = struct { if (self.errors.items.len == 0 and self.root_error_message.len == 0) return; - const root_dir = if (self.root_dir) |root_dir| root_dir else ""; - const notes_len: u32 = @intCast(self.errors.items.len); try eb.addRootErrorMessage(.{ .msg = try eb.addString(self.root_error_message), @@ -1875,21 +1867,21 @@ const UnpackResult = struct { .unable_to_create_sym_link => |info| { eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ .msg = try eb.printString("unable to create symlink from '{s}' to '{s}': {s}", .{ - stripRoot(info.file_name, root_dir), info.link_name, @errorName(info.code), + stripRoot(info.file_name, self.root_dir), info.link_name, @errorName(info.code), }), })); }, .unable_to_create_file => |info| { eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ .msg = try eb.printString("unable to create file '{s}': {s}", .{ - stripRoot(info.file_name, root_dir), @errorName(info.code), + stripRoot(info.file_name, self.root_dir), @errorName(info.code), }), })); }, .unsupported_file_type => |info| { eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ .msg = try eb.printString("file '{s}' has unsupported type '{c}'", .{ - stripRoot(info.file_name, root_dir), info.file_type, + stripRoot(info.file_name, self.root_dir), info.file_type, }), })); }, From b422e4a202f3b4d6e33c1433c3d27152dc5bc925 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Thu, 4 Apr 2024 21:28:28 +0200 Subject: [PATCH 15/18] fetch: save test cases Prepare test cases, store them in Fetch/testdata. They cover changes in this PR as well from previous one #19111. --- src/Package/Fetch.zig | 162 ++++++++++-------- .../Fetch/testdata/duplicate_paths.tar.gz | Bin 0 -> 3230 bytes .../testdata/duplicate_paths_excluded.tar.gz | Bin 0 -> 3237 bytes src/Package/Fetch/testdata/no_root.tar.gz | Bin 0 -> 3172 bytes 4 files changed, 88 insertions(+), 74 deletions(-) create mode 100644 src/Package/Fetch/testdata/duplicate_paths.tar.gz create mode 100644 src/Package/Fetch/testdata/duplicate_paths_excluded.tar.gz create mode 100644 src/Package/Fetch/testdata/no_root.tar.gz diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index a841a754f337..a393c7835e4c 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -1890,13 +1890,27 @@ const UnpackResult = struct { } }; -test "tarball with duplicate file names" { +test "tarball with duplicate paths" { + // This tarball has duplicate path 'dir1/file1' to simulate case sensitve + // file system on any file sytstem. + // + // duplicate_paths/ + // duplicate_paths/dir1/ + // duplicate_paths/dir1/file1 + // duplicate_paths/dir1/file1 + // duplicate_paths/build.zig.zon + // duplicate_paths/src/ + // duplicate_paths/src/main.zig + // duplicate_paths/src/root.zig + // duplicate_paths/build.zig + // + const gpa = std.testing.allocator; var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); - const tarball_name = "package.tar"; - try createTestTarball(tmp.dir, tarball_name, false); + const tarball_name = "duplicate_paths.tar.gz"; + try saveEmbedFile(tarball_name, tmp.dir); const tarball_path = try std.fmt.allocPrint(gpa, "zig-cache/tmp/{s}/{s}", .{ tmp.sub_path, tarball_name }); defer gpa.free(tarball_path); @@ -1906,41 +1920,104 @@ test "tarball with duplicate file names" { defer fb.deinit(); try std.testing.expectError(error.FetchFailed, fetch.run()); - try fb.expectFetchErrors(2, + try fb.expectFetchErrors(1, \\error: unable to unpack tarball - \\ note: unable to create file 'dir/file': PathAlreadyExists \\ note: unable to create file 'dir1/file1': PathAlreadyExists \\ ); } -test "tarball with error paths excluded" { +test "tarball with excluded duplicate paths" { + // Same as previous tarball but has build.zig.zon wich excludes 'dir1'. + // + // .paths = .{ + // "build.zig", + // "build.zig.zon", + // "src", + // } + // + const gpa = std.testing.allocator; var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); - const tarball_name = "package.tar"; - try createTestTarball(tmp.dir, tarball_name, true); + const tarball_name = "duplicate_paths_excluded.tar.gz"; + try saveEmbedFile(tarball_name, tmp.dir); const tarball_path = try std.fmt.allocPrint(gpa, "zig-cache/tmp/{s}/{s}", .{ tmp.sub_path, tarball_name }); defer gpa.free(tarball_path); // Run tarball fetch, should succeed var fb: TestFetchBuilder = undefined; - var fetch = try fb.build(std.testing.allocator, tmp.dir, tarball_path); + var fetch = try fb.build(gpa, tmp.dir, tarball_path); defer fb.deinit(); try fetch.run(); const hex_digest = Package.Manifest.hexDigest(fetch.actual_hash); - try std.testing.expectEqualStrings("122022afac878639d5ea6fcca14a123e21fd0395c1f2ef2c89017fa71390f73024af", &hex_digest); + try std.testing.expectEqualStrings( + "12200bafe035cbb453dd717741b66e9f9d1e6c674069d06121dafa1b2e62eb6b22da", + &hex_digest, + ); const expected_files: []const []const u8 = &.{ "build.zig", "build.zig.zon", "src/main.zig", + "src/root.zig", }; try fb.expectPackageFiles(expected_files); } +test "tarball without root folder" { + // Tarball with root folder. Manifest excludes dir1 and dir2. + // + // build.zig + // build.zig.zon + // dir1/ + // dir1/file2 + // dir1/file1 + // dir2/ + // dir2/file2 + // src/ + // src/main.zig + // + + const gpa = std.testing.allocator; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + + const tarball_name = "no_root.tar.gz"; + try saveEmbedFile(tarball_name, tmp.dir); + const tarball_path = try std.fmt.allocPrint(gpa, "zig-cache/tmp/{s}/{s}", .{ tmp.sub_path, tarball_name }); + defer gpa.free(tarball_path); + + // Run tarball fetch, should succeed + var fb: TestFetchBuilder = undefined; + var fetch = try fb.build(gpa, tmp.dir, tarball_path); + defer fb.deinit(); + try fetch.run(); + + const hex_digest = Package.Manifest.hexDigest(fetch.actual_hash); + try std.testing.expectEqualStrings( + "12209f939bfdcb8b501a61bb4a43124dfa1b2848adc60eec1e4624c560357562b793", + &hex_digest, + ); + + const expected_files: []const []const u8 = &.{ + "build.zig", + "build.zig.zon", + "src/main.zig", + }; + try fb.expectPackageFiles(expected_files); +} + +fn saveEmbedFile(comptime tarball_name: []const u8, dir: fs.Dir) !void { + //const tarball_name = "duplicate_paths_excluded.tar.gz"; + const tarball_content = @embedFile("Fetch/testdata/" ++ tarball_name); + var tmp_file = try dir.createFile(tarball_name, .{}); + defer tmp_file.close(); + try tmp_file.writeAll(tarball_content); +} + // Builds Fetch with required dependencies, clears dependencies on deinit(). const TestFetchBuilder = struct { thread_pool: ThreadPool, @@ -1981,7 +2058,7 @@ const TestFetchBuilder = struct { .hash_tok = 0, .name_tok = 0, .lazy_status = .eager, - .parent_package_root = Cache.Path{ .root_dir = undefined }, + .parent_package_root = Cache.Path{ .root_dir = Cache.Directory{ .handle = cache_dir, .path = null } }, .parent_manifest_ast = null, .prog_node = self.progress.start("Fetch", 0), .job_queue = &self.job_queue, @@ -2061,66 +2138,3 @@ const TestFetchBuilder = struct { try std.testing.expectEqualStrings(msg, al.items); } }; - -// Creates tarball with duplicate files names. Simulating case collisions on -// case insensitive file system without use of that kind of the file system. -// Manifest will exclude those files, so adding manifest should remove duplicate -// files problem. -fn createTestTarball(dir: fs.Dir, tarball_name: []const u8, with_manifest: bool) !void { - const file = try dir.createFile(tarball_name, .{}); - defer file.close(); - - const TarHeader = std.tar.output.Header; - const prefix = tarball_name; - - // add root directory - { - var hdr = TarHeader.init(); - hdr.typeflag = .directory; - try hdr.setPath(prefix, ""); - try hdr.updateChecksum(); - try file.writeAll(std.mem.asBytes(&hdr)); - } - - // add files - const files: []const []const u8 = &.{ - "build.zig", - "src/main.zig", - // duplicate file paths - "dir/file", - "dir1/file1", - "dir/file", - "dir1/file1", - }; - - for (files) |path| { - var hdr = TarHeader.init(); - hdr.typeflag = .regular; - try hdr.setPath(prefix, path); - try hdr.updateChecksum(); - try file.writeAll(std.mem.asBytes(&hdr)); - } - - // add manifest - if (with_manifest) { - const build_zig_zon = - \\ .{ - \\ .name = "fetch", - \\ .version = "0.0.0", - \\ .paths = .{ - \\ "src", - \\ "build.zig", - \\ "build.zig.zon" - \\ }, - \\ } - ; - var hdr = TarHeader.init(); - hdr.typeflag = .regular; - try hdr.setPath(prefix, "build.zig.zon"); - try hdr.setSize(build_zig_zon.len); - try hdr.updateChecksum(); - try file.writeAll(std.mem.asBytes(&hdr)); - try file.writeAll(build_zig_zon); - try file.writeAll(&[_]u8{0} ** (512 - build_zig_zon.len)); - } -} diff --git a/src/Package/Fetch/testdata/duplicate_paths.tar.gz b/src/Package/Fetch/testdata/duplicate_paths.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..118a934c1b03d764e4854f9aeb60ed684467caad GIT binary patch literal 3230 zcmV;P3}N#hiwFoH>keiD17vk@Y-wX*bY)*~VRUG7E_7jX0PR|9a~ro6^=JPIgdZBp zrB+W%GjZLClPFWqxOK-$nwh2@5!?ky#QO?>rA#~e?>+YdSniTi;!GWRl3CaziMznX zeVqHi1+%8kbt;{@s-;`ng9pFCIK((Ve@wrR&L1Ckf5-9Q==ALHF9jdjup@j%N`r$00Am)`$QN491EOq-*Fk7)ms!xPy5^zis}u>VK1|6J$l z=)>Co?CkVC?SDx2KR-EsAPxun5B4AIf3NnRH9F6dw|bGht;+kc|KroMv)2BHXUG#r zkIyLn5BC4%Rx)o?B%Tzy)J0QV!ELU3EFK;G8XhlXx`f!Z zDl=84Yf%83LSat@t(a=1GX+gf8@{s^O~H#`XBPYWbLG;bqw0w{oS#U!a-4P8mcJ zBnzN4+s}2W{6=#Lw)th2!Kgj%lMeMny^%$otDRl_&+nG16iuB0mmuOcCWT6+DAl$2 z_SK6CDcTa^@ibGpa`#EEmta`Lti;~T+wN+wWHLO_@rzzPRcvtE8m3mV{ zZ){BsM9>x$SRpb(y4?+ELSQl3dYeFdgxcuKK?@0XKA2Zkf-spZ-iCnSm+jJT-K;9= z#*s&xs^DFzrXV{zcprm|9-`U+`|N~so?*Rc+U}CBW=_JEnUHfB(_5Z&vGE64*ZJD3 zhfJR44SS+#i0%Ry2K?8zD4iqSG}(D`hj^_6(>Ath3}8j_n8q!iM9;AlKPZ$y6Uu8L zxb72LcMBW6iHw6J$cC+cpioeiyb)l%7IkH~$`nD+EKt z8R8`hMW(45;DANlK+C8cUbA9X#Qa^_K1`abatS3X-X)b*b*X=9y;BhbaN%67kOCQX zy+by4UK+_eg zP_m5LLe}$opjtqey#zVwDInxA4lX;Xgt1V_*C6x=G#`1QTh#6}i+D~UQY=)d3<0N0 zd8V9WnJG7*e9~0wkTh)+-nF4FA*>{Ks_1V{Tg!g>Xv3`!=+exvF&O5sZr4~}2o`n1 z)MFJy0pC

kWwy5ur;oK*Un**`@}<%Lf0W9^A#(fJ;nFN!Lq8?ZKtDVAMO$^j}$% z_8+`>{^i9#E-(5IUtK)=>hBjxk&RkcyLtG>J^w)|AG`<5{lCZW*Z(^@=l<{c+40c- z`&jlbP%Mzk+*AMO=p>0lNf|v~gl4>7t(=j2m6(&42p7%6Qh~zkz)AIspi89~ft**J^EV*S6 z&T~bLyhd`))()u@!Z#={==6tWozlZ|H{mk@74R(eg6I}lPbUVS&RTr=q96 zmyH|mCD%qH&5t2IEm{G>v+K>wSu;oBqq6V(tPZuXpt}o}&8|BHd4eJfZKz)w7b_wF7iL5F=b)yzWXM3>r;j0r7)MiW#U6%1UNAZ^BFAQ1rAc(&(+n4^PrFfi9x zq!6lLtYScrz!^^Osq+POp$aY!m3;k4D?MTv}p% zA~1X6m)BK2-ow&+E-r41|6E$%U;IBjK0n`y|M)e;|BoF1{kwqQ;CkT`yc>ANWXcaR z%f|9E!GNCe?>+hveAg(~7&?*vS;yvao-?LxPA1}f zPdt4pjt=kNSN@3O&ikJaT>t-!|DT_p?D#(>4F3Po>p$v=y zDZ{-Pu4`0i-Xc~0P|dhQS`m#*rMdL;^0hBLOp_wbhEif-WWDrj<86?7qf+h<&DO}q z_(&H8GAC-NH%euaSe!D6F~ZIx3Qu3qr#;cHR4*M8x-=QJEvV-37cZ8Y64kX&>yX28 ztGFz=r`)-|K!uVntIDeO7Kj)Pr0~+qfN~}gf1_^EwTi9ZYK5|xE^WBgt>&`Hot^%-M)uvt;!MGA8wjg*9!IZ zS5zEN#4Ef+m47Mcs{Qb_w9acLzAdoVw(i@in*(z<$A{Rk;R}nr_&dDW~5n;`n~*dLXr!JZWH-GgIGthqZdm}cO^vPzs(zn%qLHEF$bDO# zce>OGm5f)Og?MO5-fh~}-KugVZZ_g1E$I9ooi`mV$3H^{t^-2uqx$&5eN1X0w#UER zYnYu>ag!4U=XPr7BuCptYJ6%S6kq$>h5!m(uBz9ep*{*^Mp&$BG48r2KcS_O-Yu99 z*W$Az(0OK@p5vT42D5#@AOuFhTyQibfBltcc^|vPZqNPMMYZDGX`0QcIRu&i&omoy z!9u^vG~jnzv`48?P`G^rr=Wu|6Ata{PEwl$0o!!Od&`pVcY92MrlKzuuyb)L^w-mt z`X}1a?GfGFzM();`mV^e&z6YyP^GqQO0f-H>vz6L(2ZS?`zkH6h%E4ahnQci>3$*x zM-yfAREa5n$;z7ISCj6yIcR>o`rX|2^JH(v&h?z+R0f&jQ@U%Swf>65_gQ4xy)NGQ zS_h@uQl->MG3#!)^Y_f1mQPo&Y=#Z`+BE9#VmSCIz7Ao4ty}4+<)gy&UtrLmQPc*g zp2eWf5)+m@`0zlWRPUND<{ z)kVFA*NP?ktlP{XbM5aF^&|RmER;*9Oacl7yLe{z#833=C$Wh@Q91zW8D5Qfn(uzx z^yLZe(w2Oz54y&RDqU%RogHrtyI)^fs=S^eqm@njZ6p#Ye4WKt5TyCLM4R?l@lF$b z&2L6?JiNPL?8J{yv=P$UD6{QVyxvy?u;>VnMiaqZvtXxDP)(e?!NZ@rFzGh?$;u9UeM$->&U5!Z>aZEe-K%@>@=;Myhq`s9j;UvVEV#)oh4p_g+$N}}H= z`F1Y5QLH~>C#8NM89#i|+{k!4ms^qg78$7~2W6yOKC=*ZcEY!kCf5`geQ7{)M$F^pjh Q<99Uv2QI@^xByT905N}g`2YX_ literal 0 HcmV?d00001 diff --git a/src/Package/Fetch/testdata/duplicate_paths_excluded.tar.gz b/src/Package/Fetch/testdata/duplicate_paths_excluded.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..760b37cd40fe43e0907f14943a33ae8341f2f649 GIT binary patch literal 3237 zcmV;W3|jLaiwFqP>keiD17vk@Y-wX*bY)*~VRUG7UuAe>Y;|O1WG-}JascgGYjYd7 z71d|`3YZ@n(xq3To-=XXiIb>P&$xBRN}8Fb9TD6GNyPgKfF(^k`tLpW0=wKLrNo)K z@?^5GM-q2|i~BhDfeR*8nd(?NbyG^Wv_}X3hjEN?d3jF1Czt2P?cen{n4Dc4pPpZw zUz|-2jwh#+%hLmKe((UJvQ8Qy4z$d5{4?`@>HTkXBs=y`w3!@zI{TlU9-qSgXU8XJ zz5PF#{pUJWlaFivi;J@lwEr>L|MK+YKpgk>@9p2)|3U3Pt8|(~@AM*iSL6?4|MRoS zNt6H2&o5yA^NW*7Z~uoF(fdPz5#=&d;+Yui;9)T4*GEUYn#FLErJSFSQjto#a5%*DU-LTD$}I!(n)iOQ-( z%nKt*8NZPWB@$Khf(4#fR4)gk#}WR4y$*kKt(`c9dxH3lNF5L*3th6o6 zMihCvdCKf0g|Z?qoY*Lb4?^Y}x77KfCbHIUDI8H+$+S{6@hH=|&Z_JNZgbONaWeTe zJYLCo39(C+Cn}FOA_F#s!k!9RF~v$J3Ywf&CY^${L`;{`E~jGwrQtEzYueD7f)~Ng zES@}>D;F=PB9?ihX;%n-DkRk3?5QC?Oe1R}jD=CqnAIzlZW`LJ@>uP!lQ1unRLQut z{S*onX^}4=gi$|M+9+ES(z#P6hpyuy&sFU7O1rR4)S>-M1e}J2*Gl;OtPmL?-TnqNA+Q+jyp5neLT&BKK?@OfKAIOrgfJN`-i3hR zm+jMU+^opT%8^H#BI8}DrXV{zcprm|9-`U+`|N~so?*Rc+U}DsW=_JEiI8&`(_5Z& zvGoU8mg&Z;hfE%)6?>woi0%Ry2K+a7D4iqSG}?P}k9ds((>AuK3}8j_n941m)}CW2 zeo!cZCY0AgaNVc0?hZCO6B!3lkPTb?K%t<>c_YAjBg(>BJ%dU!ZA@V-1AAGl0fqZi zw{>ff2+CvlhTT3u{%@`FVt=1isnfJ>8~phZ*Z^@G>EfuFax@bD@gC$Dr@&kkMZW+n z_Bt2P6@nq+4DnJ6MW!h#;DANlLd&QWUbA9X#Qc5QK1`aTa1kXd-X)e+wW)t*y;Bhb zaN$&~kOCQX14G6bA9<(YDhWv1MK@<~&zL(()+c;ALPhp>{|si40(Z7sX$qY1Y@pi48u#$cGk zy4_%XCRo%lQ?IKa3izf-S#L;uhzM<>0V0-a&n7hxUbgrjGzXFv?gyPyO8bR1amPOf zFJF9h_4n(m?!(tt&%gfLRg@)z#(i!N4}OeSa2~yM-Ix7p~h68*YjGi7I zmeowmb3u3*j>Mx?p%d}GNzll(2-ip%n68WwL+w7J{(xPANL)EFlXTKV7MNlND#B=? z+_g)tj2VumBc?+y5;dz9QAy;6gXc&TIM^8R-rjOLohdpz(l(Wi{*7RdOge}QECThx zEwLFG0;+SY%9vmB3RJ8&n4|WjZE=w!)aF*5^IV6KZmt%jtQBv_zS2@lKaq zapv4I2tEtfv!$PkN0(F#CqH z>od_&->b?EN7347r1>Glr$sA3c(&fooKE-&}uKYsP`{}ac5|1RJ+xL)`K?*^VR zneu~7lA-)uFra7rdqh8i?;7PQLnrb-irLexmm_@Zn3jrpmB+pls@t-!|6iV+?)g6^^#1?J>p$v=qz)C$B3ygat}4=^ zcw?zNQigjoT-T`3yhW<~p_p-pv>+OZic{(5r5j&*m?}n^4W-1w$a3k|*0(|Gt%|un zG}|B><0D-Z$dss|-pG}S>f)413=wu7Q+WE4K8-}TQoVLa=+Y$AwxF8BU%XgqN)&6K z)**-GR&kzlPq}q{feIyF7KK&KEf6spNa3ZI0p(02{z~1VwW?dc#R_FHUD|M~Tg_#a zIy=UC9WQHQzPCgLVW6l3U)0?U?ZtX6y~|mRk$At~tXTRD+QX8vBMHz4E&ZTV-8atHea6-*G|R)$IqU)~Xb-{czj7 zS}WAoUsG{B7O(LVRsOY{tLDRBq;+01_1g@4?dra%x=CPBavu=&F!d5?jmU6-7<(q` z3xmq~wi(b$HuJsoTDXp)>SrBQy$TP#vf8@xi&8z~`N(3>oYV13Ju}j5hONfnFWkaz z?^C-iYC50Q7ZH47pxz?XL6@A|lpB*Y3TULBB4jt6CODm8{BB7AvyF`|395dd`a~l` z$C3NCI&F2SBPtnhJPY-qC3?SYTeqvqfw7uIFgUkULnpbmT}_Qo4TR#gzikMh(D|x(6B_D+KxTl&suaVvd-4-n zTGP7&^Wj>276m%bjni|SQ-@%-3mAmJ8ZcKJ4ar}BC0ahjE_Ju(;q0OsaqczE_S77L z%>QPZExBN!Uu7EbyDQq`*eEF6K891!!I%k$c6KYNje~$qI^(@%&iA_=ra)8CmkQXq zI2HQqX-oYR?dbN1Zf@UFAjy4KWbLyh;yqNUZIe=LMc4YRFA}t47v#Q)v!q5Ac)v%? zFE?~QQ3pp;%jl^RQ~r{bHN~&S?Qe6?{QBy5d)v>Gy%}5AbCOdTWUimmT@$VKS1i8I zBIEXT@!r=uDBb2NrcR34w8NdhXKuB8+InR(Y|+=GQGXZ1!B6pZ2m@@>NJlLn6|VmR zgZ_-7wm|hP26acIpzxK3SNU*8t(+Hr+iSexgMUY)R%ue_rBYhD4_j233VpX-$=dik z+y?iI+2pG(>NUJkEZOJnW)7L_$rGYJp&utgxp>MXpg^#TXLclhqF+CWZ3L>N1CXBM z)sUz8?#FFcp5QJ`$;bMjYphzOEA6kd>s!P2*H@M*EvLw6dDVOyi9`xtXYmyTY5qRZ zrX5zi(*$4ho6#H(?=~1)@go#%gtR8g?0OZi_Z0ywI>4jBSa8=Y*l7?{Q%~OF;SX(? zw4m@+qctW@@w3Wxb0iN}%G``(VQtfhYsJ{6wyNCr3r=Kk?V@gdazn&#xQ|!Ihi~wq zlXE^vqTed{ZZ6wVtUF^brG6wCKYr5O%6K=IJCXVh8L1`*Wu#m_vk-Rn!ncwp)f5L(T{%gqaXd~M?d<}kAC!{AN}Y@Kl;&+ Xe)OXs{pd$OenaEGmP{QK08jt`nu}hw literal 0 HcmV?d00001 diff --git a/src/Package/Fetch/testdata/no_root.tar.gz b/src/Package/Fetch/testdata/no_root.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..a3a4baf40fd1d6b2bd101ee59f27f88c6a61f97f GIT binary patch literal 3172 zcmV-q44d;GiwFq9_zq?O18#3$a&K>RE_7jX0PPz6Z`(FDKj*LD{KL&I93@U-2Uu^1 zWm|?T(7O)Jb^~sAh-rzoxk{o&Qg+=g|NFi7NXnF*^aES69i}&|ttImDeSe9wTIN~u zMlKE>4o(0jlXLugbTK}O|3>olj2A#*_1t$@t)8G@6`G4%o>9($qk0EZ2-3 zNM6eH9qZor{{IwGRT|5T&DiJc3t3c3+kOvz_6AQ6508)8Wp0bY8huUv_7`byjr@w zYnkV4CYX3F(%OoQ%{Gje@Y4!i@*J`0x-5lG4y$^`=A}c?pMA!j0M*Ht_-nvcN@ncs zAp?LN*Or$V*BJv%FN9^PLLLm`d9Kz#fVZTh50a+n9Qc`XLM(ODi*$^IQ z@m_pak{A>A0-o!@Nk9ffFfBm360`(tn7TrRiiUFo|8t;`uei+lEEfrFbw=RVe9mTx z@e=!v9=&j<^pm%@XcfCsMIs9MhLd5mWcvZ4H6j0v(h!sje`%Mv^s`7t^@#wD0&dhB+~ zPSO~i`$zO9l5+T`dm9_!cSxX%F9PCGX+>$t4Nxw*lQ20c2nDnN3J2O@nW6FasqF)?ZG(Xj z7!2l$q9OY0M_S8==mo9karGh^cJ5V8U0TYx&5L{H*xF&zHibZ3zh}6-^1pY8;C2fLU7c9M~4-O!pB}x z^a^6jHh<&#xmHE2OTXBuF_RkNz5?%rc!H@6cn(h?ZqeSgg!tnQN_mRf44aacGB7Pd zx-CU2i~)y_!`+;=^U8S$oxGJRsbOnPYfC~W+6|YcwGT$ zVr!_5L@(|H1(djxUzGh>0xtE;9eWLL`h#8jib|7^mnv@Hxn9%-9F3*b9%r%r1#A%9 z3nJ4(rkxMLdR__Q?6b)9NnJ-rh<${AoHAk4DWL!X0=e+a4A?LD*Dq`vfdY46NSE-c zPs^UVV9@5ryGv8@kv!P5t--9Mkr)s>4-sugM@x}cQ^;s#Jq$>Zr4$SXcDf1zC4V1l z(-tYtX}qtw!*Go=@7Nfl_~DB-w6rG5?0OaLcNGC>v$}7;q)KoYV%8Z9zKvpRQ(M&#Q~WgNxYEA~?AZ^YwopES2T-c99Bq`rektjPg0q+IG*(02C1 zx8ORL7#Lk}dtE_agZ4E=Ox_2eY4hmb<%tjk)kK_soCB z=cCgw{~3?RlY^7d={daa=0A@BKJLAzmxwAY&VIZWP*Ok#%l-weJro}Q?JupZ%xA~P zMifwXB}y-j2ThFz{VHek+Cm-0R&EDVbM|kDD^bM=VS}b9Q0Jtt_ySXJprxs%lCT{J zJxSm{Ag@K=TndO&m=mcB=lNctnATcD{e-P?1~ms3pqM|O5;;wJ+z8B-IF`W*bkR(H zpdfXT6Uzq?kSB#KWlfOqfwEmc+g&PxaJhHeJJ~25S zj*bjp3HDcz%-^PDL_8F}N7OFSL6ACydzn$%icd%)?1^~Ii^`|{_*D1j57@%hRTi>q zqf!tlhbCmrzJK|ANR=sCJWZ5x;Y-a_f%%zAm02*W7Vbu)sPnZnf^Aeyl0|kPL8akT zXb|z0Ydc7r(hmz2kb-C!xBCZJ>LQ86Wan)H(nG5azU-xtz{$sRrLee37H?XksD1p6 zo2jCrhBq{UZZHLUw$!~S*OzU(KzKlHmmeXUebj1Zq38mXd=AQVnx_uF@dxaMHlfCl zWt!LIiMmF-3$S4@|IHmjr${&5mB+p8H4aQ8BsX?Lespb@PlM-}Fa!ok2qy*ax=*Q> z+{fKoNOq8T-r%U8Fcheg4k~b3427F(m{BuryKBQve}i`6IAFuz!6)5^4-VnHpg5;K#+6k^@Q{9#GWD5ny(d84s`6SGoTG;MY3l2`;r<=xu;l!9{!dlmWL2iKZNn)*o+%!>m zUxzr-5Im!4r7T_ATDH?i6K-8V=XwSQ1Hly5%?9=t^r^wPZ!@6^!ho+;PI8097e=wc z5S)n^tUa65fcCPr|6bFeM0Ve6buIA==*?UH(R=>vtE+!rU$q~;yt@4QA6H3{^&0p2 z6SR7F0+_+*_<_%$kNg+o`}#kG=KKHI$r<#2CzJ8n0UJLsT?Bvf{*UyJPJH@sXS}EX zzeE3{@nqcf|BnHU{zngY#>eUZ#OZ(3>HjeR^nd!tiT@Ypr)L)w|0iAie-t47|6$^P z(En(B(Z&Bq0sNTt&}WeUk3Z1=>Hg=*WHjph|1lu`nE6m=yvP4Xlhd=E`@g5B<1YU{ z3cU0FufJl8_R$}(PujO?ztz&;LxJT&Yu1`LqYU^*Z?_#(s0o=Ju1`n}6M$HBH{Z_O=@Q}Ct}4k{I^P4XYSQ<1#wr3y9y_FjDF z3~;I1x-p>p6wOl{tTA|6)iehq%+ZyZAuSE_en|_OD<(M0!RL721}7hjH?pFMW9hD3 zxOskomw^o3RK}|sz`VaLNjHSHqbEll-IJ#4zQpwc@fLV%X(3wQ!rkg{`=BMh7q#sV zlC_prfE?Xt?G>2tY`tAMtLJl}MfQVR6=Cxm;#2T*`#M7CK9uW?e;qV~pWvgfO76k{ z&e;477x(4dYv?w{1K8(zZI*x>C6TG}Q@RZ3FHqCFPY!Pn59#Jg&yds!U(cHx@<%f) z$Kj>ceDfTB_vP8?Ad#iC$YT=j#o+BJP_WXT+lvl3<2KR*pu4V(lUqTHvNtDnIl`(JQ_qz@Ua z9=h$7a@WBAtb?cyI_RK-4m#+dgAO|Apo0!N=%9lRI_RK-4m#+dgAO|Apo0#675ooA KksQ4MPyhh%Hx@Dg literal 0 HcmV?d00001 From 8c58b8fe0107fe3e06ec8bac2b2c25f706406c82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Thu, 4 Apr 2024 21:39:13 +0200 Subject: [PATCH 16/18] fetch: refactor package root in errors Use stripRoot in less places. Strip it while copying error from diagnostic to unpack result so other palaces can be free of this logic. --- src/Package/Fetch.zig | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index a393c7835e4c..9e91c0e02924 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -1189,9 +1189,9 @@ fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!UnpackRes try res.rootErrorMessage("unable to unpack tarball"); for (diagnostics.errors.items) |item| { switch (item) { - .unable_to_create_file => |i| try res.unableToCreateFile(i.file_name, i.code), - .unable_to_create_sym_link => |i| try res.unableToCreateSymLink(i.file_name, i.link_name, i.code), - .unsupported_file_type => |i| try res.unsupportedFileType(i.file_name, @intFromEnum(i.file_type)), + .unable_to_create_file => |i| try res.unableToCreateFile(stripRoot(i.file_name, res.root_dir), i.code), + .unable_to_create_sym_link => |i| try res.unableToCreateSymLink(stripRoot(i.file_name, res.root_dir), i.link_name, i.code), + .unsupported_file_type => |i| try res.unsupportedFileType(stripRoot(i.file_name, res.root_dir), @intFromEnum(i.file_type)), } } } @@ -1764,13 +1764,13 @@ const UnpackResult = struct { file_type: u8, }, - fn excluded(self: Error, filter: Filter, root_dir: []const u8) bool { + fn excluded(self: Error, filter: Filter) bool { const file_name = switch (self) { .unable_to_create_file => |info| info.file_name, .unable_to_create_sym_link => |info| info.file_name, .unsupported_file_type => |info| info.file_name, }; - return !filter.includePath(stripRoot(file_name, root_dir)); + return !filter.includePath(file_name); } fn free(self: Error, allocator: std.mem.Allocator) void { @@ -1835,7 +1835,7 @@ const UnpackResult = struct { while (i > 0) { i -= 1; const item = self.errors.items[i]; - if (item.excluded(filter, self.root_dir)) { + if (item.excluded(filter)) { _ = self.errors.swapRemove(i); item.free(self.allocator); } @@ -1867,21 +1867,21 @@ const UnpackResult = struct { .unable_to_create_sym_link => |info| { eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ .msg = try eb.printString("unable to create symlink from '{s}' to '{s}': {s}", .{ - stripRoot(info.file_name, self.root_dir), info.link_name, @errorName(info.code), + info.file_name, info.link_name, @errorName(info.code), }), })); }, .unable_to_create_file => |info| { eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ .msg = try eb.printString("unable to create file '{s}': {s}", .{ - stripRoot(info.file_name, self.root_dir), @errorName(info.code), + info.file_name, @errorName(info.code), }), })); }, .unsupported_file_type => |info| { eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ .msg = try eb.printString("file '{s}' has unsupported type '{c}'", .{ - stripRoot(info.file_name, self.root_dir), info.file_type, + info.file_name, info.file_type, }), })); }, From f8dd2a1064154e5e8223c4d52e123b582497a40d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Anic=CC=81?= Date: Sun, 7 Apr 2024 01:21:10 +0200 Subject: [PATCH 17/18] fetch: add executable bit test --- src/Package/Fetch.zig | 52 ++++++++++++++++++ src/Package/Fetch/testdata/executables.tar.gz | Bin 0 -> 430 bytes 2 files changed, 52 insertions(+) create mode 100644 src/Package/Fetch/testdata/executables.tar.gz diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index 9e91c0e02924..c72ebc4a1ee4 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -2010,6 +2010,58 @@ test "tarball without root folder" { try fb.expectPackageFiles(expected_files); } +test "set executable bit based on file content" { + if (!std.fs.has_executable_bit) return error.SkipZigTest; + const gpa = std.testing.allocator; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + + const tarball_name = "executables.tar.gz"; + try saveEmbedFile(tarball_name, tmp.dir); + const tarball_path = try std.fmt.allocPrint(gpa, "zig-cache/tmp/{s}/{s}", .{ tmp.sub_path, tarball_name }); + defer gpa.free(tarball_path); + + // $ tar -tvf executables.tar.gz + // drwxrwxr-x 0 executables/ + // -rwxrwxr-x 170 executables/hello + // lrwxrwxrwx 0 executables/hello_ln -> hello + // -rw-rw-r-- 0 executables/file1 + // -rw-rw-r-- 17 executables/script_with_shebang_without_exec_bit + // -rwxrwxr-x 7 executables/script_without_shebang + // -rwxrwxr-x 17 executables/script + + var fb: TestFetchBuilder = undefined; + var fetch = try fb.build(gpa, tmp.dir, tarball_path); + defer fb.deinit(); + + try fetch.run(); + try std.testing.expectEqualStrings( + "1220fecb4c06a9da8673c87fe8810e15785f1699212f01728eadce094d21effeeef3", + &Manifest.hexDigest(fetch.actual_hash), + ); + + var out = try fb.packageDir(); + defer out.close(); + const S = std.posix.S; + // expect executable bit not set + try std.testing.expect((try out.statFile("file1")).mode & S.IXUSR == 0); + try std.testing.expect((try out.statFile("script_without_shebang")).mode & S.IXUSR == 0); + // expect executable bit set + try std.testing.expect((try out.statFile("hello")).mode & S.IXUSR != 0); + try std.testing.expect((try out.statFile("script")).mode & S.IXUSR != 0); + try std.testing.expect((try out.statFile("script_with_shebang_without_exec_bit")).mode & S.IXUSR != 0); + try std.testing.expect((try out.statFile("hello_ln")).mode & S.IXUSR != 0); + + // + // $ ls -al zig-cache/tmp/OCz9ovUcstDjTC_U/zig-global-cache/p/1220fecb4c06a9da8673c87fe8810e15785f1699212f01728eadce094d21effeeef3 + // -rw-rw-r-- 1 0 Apr file1 + // -rwxrwxr-x 1 170 Apr hello + // lrwxrwxrwx 1 5 Apr hello_ln -> hello + // -rwxrwxr-x 1 17 Apr script + // -rw-rw-r-- 1 7 Apr script_without_shebang + // -rwxrwxr-x 1 17 Apr script_with_shebang_without_exec_bit +} + fn saveEmbedFile(comptime tarball_name: []const u8, dir: fs.Dir) !void { //const tarball_name = "duplicate_paths_excluded.tar.gz"; const tarball_content = @embedFile("Fetch/testdata/" ++ tarball_name); diff --git a/src/Package/Fetch/testdata/executables.tar.gz b/src/Package/Fetch/testdata/executables.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..abc650801ea6d416ad3cef2b87a6df205c71663b GIT binary patch literal 430 zcmV;f0a5-RiwFP!000001MQd5PJ=KMhPx%ZSz_iR)Of2|TPXFynB8c+@Er)uDkKdv zgJrw(NqhmHVDUm9P1_Af<5Z>&66JgsoE&I#AV2?UD;IJ+%YsnJbckw#XoTP&;KaW(m#?)O=_r7r9a~=*PnC3 zsn*|dTo++d!@F$Ia{cemuP(ZsPMLSn88X=djaK>SgE=E~f{Ga?_TD|U<71u`5$&(T z(oB3Ym&@meeC*Snz6^THQ*)QT4tlE}@(9l-Q+3_{{w#4I{AUxm)Q^7%}~y>E8hQe*@V1r{igSm-+KJi~KZ_ zVUXOelF2Od%{V`dv!B$c)L)BT?*Cl9PheCK0s1$=;lVJBlVKY5Rf>-T1 Date: Tue, 9 Apr 2024 14:47:34 +0200 Subject: [PATCH 18/18] fetch: use arena allocator for diagnostic/UnpackResult Reference: https://github.com/ziglang/zig/pull/19500#discussion_r1556476973 Arena is now used for Diagnostic (tar and git). `deinit` is not called on Diagnostic allowing us to reference strings from Diagnostic in UnpackResult without dupe. That seamed reasonable to me. Instead of using gpa for Diagnostic, and then dupe to arena. Or using arena for both and making dupe so we can deinit Diagnostic. --- src/Package/Fetch.zig | 168 ++++++++++++++++++++++-------------------- 1 file changed, 87 insertions(+), 81 deletions(-) diff --git a/src/Package/Fetch.zig b/src/Package/Fetch.zig index c72ebc4a1ee4..80a180f0d453 100644 --- a/src/Package/Fetch.zig +++ b/src/Package/Fetch.zig @@ -463,7 +463,6 @@ fn runResource( // Fetch and unpack a resource into a temporary directory. var unpack_result = try unpackResource(f, resource, uri_path, tmp_directory); - defer unpack_result.deinit(); var pkg_path: Cache.Path = .{ .root_dir = tmp_directory, .sub_path = unpack_result.root_dir }; @@ -487,11 +486,7 @@ fn runResource( // Ignore errors that were excluded by manifest, such as failure to // create symlinks that weren't supposed to be included anyway. - try unpack_result.filterErrors(filter); - if (unpack_result.hasErrors()) { - try unpack_result.bundleErrors(eb, try f.srcLoc(f.location_tok)); - return error.FetchFailed; - } + try unpack_result.validate(f, filter); // Apply the manifest's inclusion rules to the temporary directory by // deleting excluded files. @@ -1117,8 +1112,7 @@ fn unpackResource( .{ uri_path, @errorName(err) }, )); }; - const gpa = f.arena.child_allocator; - return UnpackResult.init(gpa); + return .{}; }, }; @@ -1166,10 +1160,9 @@ fn unpackResource( fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!UnpackResult { const eb = &f.error_bundle; - const gpa = f.arena.child_allocator; + const arena = f.arena.allocator(); - var diagnostics: std.tar.Diagnostics = .{ .allocator = gpa }; - defer diagnostics.deinit(); + var diagnostics: std.tar.Diagnostics = .{ .allocator = arena }; std.tar.pipeToFileSystem(out_dir, reader, .{ .diagnostics = &diagnostics, @@ -1181,17 +1174,14 @@ fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!UnpackRes .{@errorName(err)}, )); - var res = UnpackResult.init(gpa); - if (diagnostics.root_dir.len > 0) { - res.root_dir = try gpa.dupe(u8, diagnostics.root_dir); - } + var res: UnpackResult = .{ .root_dir = diagnostics.root_dir }; if (diagnostics.errors.items.len > 0) { - try res.rootErrorMessage("unable to unpack tarball"); + try res.allocErrors(arena, diagnostics.errors.items.len, "unable to unpack tarball"); for (diagnostics.errors.items) |item| { switch (item) { - .unable_to_create_file => |i| try res.unableToCreateFile(stripRoot(i.file_name, res.root_dir), i.code), - .unable_to_create_sym_link => |i| try res.unableToCreateSymLink(stripRoot(i.file_name, res.root_dir), i.link_name, i.code), - .unsupported_file_type => |i| try res.unsupportedFileType(stripRoot(i.file_name, res.root_dir), @intFromEnum(i.file_type)), + .unable_to_create_file => |i| res.unableToCreateFile(stripRoot(i.file_name, res.root_dir), i.code), + .unable_to_create_sym_link => |i| res.unableToCreateSymLink(stripRoot(i.file_name, res.root_dir), i.link_name, i.code), + .unsupported_file_type => |i| res.unsupportedFileType(stripRoot(i.file_name, res.root_dir), @intFromEnum(i.file_type)), } } } @@ -1199,11 +1189,12 @@ fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!UnpackRes } fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!UnpackResult { + const arena = f.arena.allocator(); const gpa = f.arena.child_allocator; const want_oid = resource.git.want_oid; const reader = resource.git.fetch_stream.reader(); - var res = UnpackResult.init(gpa); + var res: UnpackResult = .{}; // The .git directory is used to store the packfile and associated index, but // we do not attempt to replicate the exact structure of a real .git // directory, since that isn't relevant for fetching a package. @@ -1234,16 +1225,15 @@ fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!Unpac checkout_prog_node.activate(); var repository = try git.Repository.init(gpa, pack_file, index_file); defer repository.deinit(); - var diagnostics: git.Diagnostics = .{ .allocator = gpa }; - defer diagnostics.deinit(); + var diagnostics: git.Diagnostics = .{ .allocator = arena }; try repository.checkout(out_dir, want_oid, &diagnostics); if (diagnostics.errors.items.len > 0) { - try res.rootErrorMessage("unable to unpack packfile"); + try res.allocErrors(arena, diagnostics.errors.items.len, "unable to unpack packfile"); for (diagnostics.errors.items) |item| { switch (item) { - .unable_to_create_file => |i| try res.unableToCreateFile(i.file_name, i.code), - .unable_to_create_sym_link => |i| try res.unableToCreateSymLink(i.file_name, i.link_name, i.code), + .unable_to_create_file => |i| res.unableToCreateFile(i.file_name, i.code), + .unable_to_create_sym_link => |i| res.unableToCreateSymLink(i.file_name, i.link_name, i.code), } } } @@ -1701,6 +1691,7 @@ const native_os = builtin.os.tag; test { _ = Filter; _ = FileType; + _ = UnpackResult; } // Detects executable header: ELF magic header or shebang line. @@ -1741,8 +1732,8 @@ test FileHeader { // tar/git diagnostic, filtering that errors by manifest inclusion rules and // emitting remaining errors to an `ErrorBundle`. const UnpackResult = struct { - allocator: std.mem.Allocator, - errors: std.ArrayListUnmanaged(Error) = .{}, + errors: []Error = undefined, + errors_count: usize = 0, root_error_message: []const u8 = "", // A non empty value means that the package contents are inside a @@ -1772,97 +1763,82 @@ const UnpackResult = struct { }; return !filter.includePath(file_name); } - - fn free(self: Error, allocator: std.mem.Allocator) void { - switch (self) { - .unable_to_create_sym_link => |info| { - allocator.free(info.file_name); - allocator.free(info.link_name); - }, - .unable_to_create_file => |info| { - allocator.free(info.file_name); - }, - .unsupported_file_type => |info| { - allocator.free(info.file_name); - }, - } - } }; - fn init(allocator: std.mem.Allocator) UnpackResult { - return .{ .allocator = allocator }; - } - - fn deinit(self: *UnpackResult) void { - for (self.errors.items) |item| { - item.free(self.allocator); - } - self.errors.deinit(self.allocator); - self.allocator.free(self.root_error_message); - self.allocator.free(self.root_dir); - self.* = undefined; + fn allocErrors(self: *UnpackResult, arena: std.mem.Allocator, n: usize, root_error_message: []const u8) !void { + self.root_error_message = try arena.dupe(u8, root_error_message); + self.errors = try arena.alloc(UnpackResult.Error, n); } fn hasErrors(self: *UnpackResult) bool { - return self.errors.items.len > 0; + return self.errors_count > 0; } - fn unableToCreateFile(self: *UnpackResult, file_name: []const u8, err: anyerror) !void { - try self.errors.append(self.allocator, .{ .unable_to_create_file = .{ + fn unableToCreateFile(self: *UnpackResult, file_name: []const u8, err: anyerror) void { + self.errors[self.errors_count] = .{ .unable_to_create_file = .{ .code = err, - .file_name = try self.allocator.dupe(u8, file_name), - } }); + .file_name = file_name, + } }; + self.errors_count += 1; } - fn unableToCreateSymLink(self: *UnpackResult, file_name: []const u8, link_name: []const u8, err: anyerror) !void { - try self.errors.append(self.allocator, .{ .unable_to_create_sym_link = .{ + fn unableToCreateSymLink(self: *UnpackResult, file_name: []const u8, link_name: []const u8, err: anyerror) void { + self.errors[self.errors_count] = .{ .unable_to_create_sym_link = .{ .code = err, - .file_name = try self.allocator.dupe(u8, file_name), - .link_name = try self.allocator.dupe(u8, link_name), - } }); + .file_name = file_name, + .link_name = link_name, + } }; + self.errors_count += 1; } - fn unsupportedFileType(self: *UnpackResult, file_name: []const u8, file_type: u8) !void { - try self.errors.append(self.allocator, .{ .unsupported_file_type = .{ - .file_name = try self.allocator.dupe(u8, file_name), + fn unsupportedFileType(self: *UnpackResult, file_name: []const u8, file_type: u8) void { + self.errors[self.errors_count] = .{ .unsupported_file_type = .{ + .file_name = file_name, .file_type = file_type, - } }); + } }; + self.errors_count += 1; + } + + fn validate(self: *UnpackResult, f: *Fetch, filter: Filter) !void { + self.filterErrors(filter); + if (self.hasErrors()) { + const eb = &f.error_bundle; + try self.bundleErrors(eb, try f.srcLoc(f.location_tok)); + return error.FetchFailed; + } } // Filter errors by manifest inclusion rules. - fn filterErrors(self: *UnpackResult, filter: Filter) !void { - var i = self.errors.items.len; + fn filterErrors(self: *UnpackResult, filter: Filter) void { + var i = self.errors_count; while (i > 0) { i -= 1; - const item = self.errors.items[i]; - if (item.excluded(filter)) { - _ = self.errors.swapRemove(i); - item.free(self.allocator); + if (self.errors[i].excluded(filter)) { + self.errors_count -= 1; + const tmp = self.errors[i]; + self.errors[i] = self.errors[self.errors_count]; + self.errors[self.errors_count] = tmp; } } } - fn rootErrorMessage(self: *UnpackResult, msg: []const u8) !void { - self.root_error_message = try self.allocator.dupe(u8, msg); - } - // Emmit errors to an `ErrorBundle`. fn bundleErrors( self: *UnpackResult, eb: *ErrorBundle.Wip, src_loc: ErrorBundle.SourceLocationIndex, ) !void { - if (self.errors.items.len == 0 and self.root_error_message.len == 0) + if (self.errors_count == 0 and self.root_error_message.len == 0) return; - const notes_len: u32 = @intCast(self.errors.items.len); + const notes_len: u32 = @intCast(self.errors_count); try eb.addRootErrorMessage(.{ .msg = try eb.addString(self.root_error_message), .src_loc = src_loc, .notes_len = notes_len, }); const notes_start = try eb.reserveNotes(notes_len); - for (self.errors.items, notes_start..) |item, note_i| { + for (self.errors, notes_start..) |item, note_i| { switch (item) { .unable_to_create_sym_link => |info| { eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{ @@ -1888,6 +1864,36 @@ const UnpackResult = struct { } } } + + test filterErrors { + var arena_instance = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_instance.deinit(); + const arena = arena_instance.allocator(); + + // init + var res: UnpackResult = .{}; + try res.allocErrors(arena, 4, "error"); + try std.testing.expectEqual(0, res.errors_count); + + // create errors + res.unableToCreateFile("dir1/file1", error.File1); + res.unableToCreateSymLink("dir2/file2", "", error.File2); + res.unableToCreateFile("dir1/file3", error.File3); + res.unsupportedFileType("dir2/file4", 'x'); + try std.testing.expectEqual(4, res.errors_count); + + // filter errors + var filter: Filter = .{}; + try filter.include_paths.put(arena, "dir2", {}); + res.filterErrors(filter); + + try std.testing.expectEqual(2, res.errors_count); + try std.testing.expect(res.errors[0] == Error.unsupported_file_type); + try std.testing.expect(res.errors[1] == Error.unable_to_create_sym_link); + // filtered: moved to the list end + try std.testing.expect(res.errors[2] == Error.unable_to_create_file); + try std.testing.expect(res.errors[3] == Error.unable_to_create_file); + } }; test "tarball with duplicate paths" {