Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add .unpack = false and --no-unpack to build.zig.zon and zig fetch #22042

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions doc/build.zig.zon.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@ Boolean.
When this is set to `true`, a package is declared to be lazily fetched. This
makes the dependency only get fetched if it is actually used.

#### `unpack`

Boolean.

When this is set to `false`, the package is not unpacked but kept as a single
file.

### `paths`

List. Required.
Expand Down
4 changes: 4 additions & 0 deletions lib/init/build.zig.zon
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@
// // fetched. This makes the dependency only get fetched if it is
// // actually used.
// .lazy = false,
//
// // When this is set to `false`, the package is not unpacked but kept as a single
// // file.
// .unpack = false,
//},
},

Expand Down
40 changes: 35 additions & 5 deletions src/Package/Fetch.zig
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ location_tok: std.zig.Ast.TokenIndex,
hash_tok: std.zig.Ast.TokenIndex,
name_tok: std.zig.Ast.TokenIndex,
lazy_status: LazyStatus,
unpack: bool,
parent_package_root: Cache.Path,
parent_manifest_ast: ?*const std.zig.Ast,
prog_node: std.Progress.Node,
Expand Down Expand Up @@ -345,12 +346,12 @@ pub fn run(f: *Fetch) RunError!void {
.path_or_url => |path_or_url| {
if (fs.cwd().openDir(path_or_url, .{ .iterate = true })) |dir| {
var resource: Resource = .{ .dir = dir };
return f.runResource(path_or_url, &resource, null);
return f.runResource(path_or_url, &resource, null, f.unpack);
} else |dir_err| {
const file_err = if (dir_err == error.NotDir) e: {
if (fs.cwd().openFile(path_or_url, .{})) |file| {
var resource: Resource = .{ .file = file };
return f.runResource(path_or_url, &resource, null);
return f.runResource(path_or_url, &resource, null, f.unpack);
} else |err| break :e err;
} else dir_err;

Expand All @@ -362,7 +363,7 @@ pub fn run(f: *Fetch) RunError!void {
};
var server_header_buffer: [header_buffer_size]u8 = undefined;
var resource = try f.initResource(uri, &server_header_buffer);
return f.runResource(try uri.path.toRawMaybeAlloc(arena), &resource, null);
return f.runResource(try uri.path.toRawMaybeAlloc(arena), &resource, null, f.unpack);
}
},
};
Expand Down Expand Up @@ -424,7 +425,7 @@ pub fn run(f: *Fetch) RunError!void {
);
var server_header_buffer: [header_buffer_size]u8 = undefined;
var resource = try f.initResource(uri, &server_header_buffer);
return f.runResource(try uri.path.toRawMaybeAlloc(arena), &resource, remote.hash);
return f.runResource(try uri.path.toRawMaybeAlloc(arena), &resource, remote.hash, f.unpack);
}

pub fn deinit(f: *Fetch) void {
Expand All @@ -438,6 +439,7 @@ fn runResource(
uri_path: []const u8,
resource: *Resource,
remote_hash: ?Manifest.MultiHashHexDigest,
unpack: bool,
) RunError!void {
defer resource.deinit();
const arena = f.arena.allocator();
Expand Down Expand Up @@ -468,7 +470,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);
var unpack_result = try unpackResource(f, resource, uri_path, tmp_directory, unpack);

var pkg_path: Cache.Path = .{ .root_dir = tmp_directory, .sub_path = unpack_result.root_dir };

Expand Down Expand Up @@ -712,6 +714,7 @@ fn queueJobsForDeps(f: *Fetch) RunError!void {
.hash_tok = dep.hash_tok,
.name_tok = dep.name_tok,
.lazy_status = if (dep.lazy) .available else .eager,
.unpack = dep.unpack,
.parent_package_root = f.package_root,
.parent_manifest_ast = &f.manifest_ast,
.prog_node = f.prog_node,
Expand Down Expand Up @@ -1055,8 +1058,35 @@ fn unpackResource(
resource: *Resource,
uri_path: []const u8,
tmp_directory: Cache.Directory,
unpack: bool,
) RunError!UnpackResult {
const eb = &f.error_bundle;

if (!unpack) {
const basename = std.fs.path.basename(uri_path);
var out_file = tmp_directory.handle.createFile(
basename,
.{},
) catch |err| return f.fail(f.location_tok, try eb.printString(
"failed to create temporary file: {s}",
.{@errorName(err)},
));
defer out_file.close();
var buf: [std.mem.page_size]u8 = undefined;
while (true) {
const len = resource.reader().readAll(&buf) catch |err| return f.fail(f.location_tok, try eb.printString(
"read stream failed: {s}",
.{@errorName(err)},
));
if (len == 0) break;
out_file.writer().writeAll(buf[0..len]) catch |err| return f.fail(f.location_tok, try eb.printString(
"write temporary file failed: {s}",
.{@errorName(err)},
));
}
return .{};
}

const file_type = switch (resource.*) {
.file => FileType.fromPath(uri_path) orelse
return f.fail(f.location_tok, try eb.printString("unknown file type: '{s}'", .{uri_path})),
Expand Down
8 changes: 8 additions & 0 deletions src/Package/Manifest.zig
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub const Dependency = struct {
node: Ast.Node.Index,
name_tok: Ast.TokenIndex,
lazy: bool,
unpack: bool,

pub const Location = union(enum) {
url: []const u8,
Expand Down Expand Up @@ -302,6 +303,7 @@ const Parse = struct {
.node = node,
.name_tok = 0,
.lazy = false,
.unpack = true,
};
var has_location = false;

Expand Down Expand Up @@ -350,6 +352,12 @@ const Parse = struct {
error.ParseFailure => continue,
else => |e| return e,
};
} else if (mem.eql(u8, field_name, "unpack")) {
dep.unpack = parseBool(p, field_init) catch |err| switch (err) {
error.ParseFailure => continue,
else => |e| return e,
};
if (dep.unpack) return fail(p, main_tokens[field_init], "unpack cannot be set to true, omit it instead", .{});
Copy link
Contributor

Choose a reason for hiding this comment

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

.lazy = false is allowed despite it being the default, so it seems inconsistent that .unpack = true is an error. Either both should be forbidden or both should be allowed. I don't think there's any harm in allowing .unpack = true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think anytime you can decrease the "surface area" of an interface it's a win, failing to do so does more harm than people realize.

} else {
// Ignore unknown fields so that we can add fields in future zig
// versions without breaking older zig versions.
Expand Down
6 changes: 6 additions & 0 deletions src/main.zig
Original file line number Diff line number Diff line change
Expand Up @@ -5073,6 +5073,7 @@ fn cmdBuild(gpa: Allocator, arena: Allocator, args: []const []const u8) !void {
.hash_tok = 0,
.name_tok = 0,
.lazy_status = .eager,
.unpack = true,
.parent_package_root = build_mod.root,
.parent_manifest_ast = null,
.prog_node = fetch_prog_node,
Expand Down Expand Up @@ -6814,6 +6815,7 @@ const usage_fetch =
\\ --save=[name] Add the fetched package to build.zig.zon as name
\\ --save-exact Add the fetched package to build.zig.zon, storing the URL verbatim
\\ --save-exact=[name] Add the fetched package to build.zig.zon as name, storing the URL verbatim
\\ --no-unpack Don't unpack the package
\\
;

Expand All @@ -6835,6 +6837,7 @@ fn cmdFetch(
yes: ?[]const u8,
exact: ?[]const u8,
} = .no;
var unpack = true;

{
var i: usize = 0;
Expand All @@ -6859,6 +6862,8 @@ fn cmdFetch(
save = .{ .exact = null };
} else if (mem.startsWith(u8, arg, "--save-exact=")) {
save = .{ .exact = arg["--save-exact=".len..] };
} else if (mem.eql(u8, arg, "--no-unpack")) {
unpack = false;
} else {
fatal("unrecognized parameter: '{s}'", .{arg});
}
Expand Down Expand Up @@ -6913,6 +6918,7 @@ fn cmdFetch(
.hash_tok = 0,
.name_tok = 0,
.lazy_status = .eager,
.unpack = unpack,
.parent_package_root = undefined,
.parent_manifest_ast = null,
.prog_node = root_prog_node,
Expand Down
3 changes: 3 additions & 0 deletions test/standalone/build.zig.zon
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@
.@"extern" = .{
.path = "extern",
},
.fetch = .{
.path = "fetch",
},
.dep_diamond = .{
.path = "dep_diamond",
},
Expand Down
52 changes: 52 additions & 0 deletions test/standalone/fetch/build.zig
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
const std = @import("std");

pub fn build(b: *std.Build) !void {
const test_step = b.step("test", "Test it");
b.default_step = test_step;

{
const codegen_exe = b.addExecutable(.{
.name = "codegen",
.target = b.host,
.root_source_file = b.path("codegen.zig"),
});
const run_codegen = b.addRunArtifact(codegen_exe);
const example_dir = run_codegen.addOutputDirectoryArg("example");

const run = b.addSystemCommand(&.{
b.graph.zig_exe,
"build",
"install",
"--build-file",
});
run.addFileArg(example_dir.path(b, "build.zig"));
run.addArg("--prefix");
const install_dir = run.addOutputDirectoryArg("install");
const check_file = b.addCheckFile(install_dir.path(b, "example_dep_file.txt"), .{
.expected_exact = "This is an example file.\n",
});
test_step.dependOn(&check_file.step);
}

{
const run = b.addSystemCommand(&.{
b.graph.zig_exe,
"build",
"--build-file",
});
run.addFileArg(b.path("unpacktrue/build.zig"));
run.addCheck(.{ .expect_stderr_match = "error: unpack cannot be set to true, omit it instead" });
test_step.dependOn(&run.step);
}

{
const run = b.addSystemCommand(&.{
b.graph.zig_exe,
"fetch",
"--no-unpack",
});
run.addFileArg(b.path("example/example_dep_file.txt"));
run.expectStdOutEqual("12200f68aca70ebc76057200af436aab5720ec53a780713c5dc614825db42a39dbfb\n");
test_step.dependOn(&run.step);
}
}
36 changes: 36 additions & 0 deletions test/standalone/fetch/codegen.zig
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
const std = @import("std");

pub fn main() !void {
var arena_instance = std.heap.ArenaAllocator.init(std.heap.page_allocator);
const arena = arena_instance.allocator();
const args = try std.process.argsAlloc(arena);
std.debug.assert(args.len == 2);
const out_dir = args[1];

if (!std.fs.path.isAbsolute(out_dir)) {
std.log.err("directory '{s}' must be absolute", .{out_dir});
std.process.exit(0xff);
}

var dir = try std.fs.openDirAbsolute(out_dir, .{});
defer dir.close();

try writeFile(dir, "build.zig", @embedFile("example/build.zig"));
try writeFile(dir, "example_dep_file.txt", @embedFile("example/example_dep_file.txt"));

{
const template = @embedFile("example/build.zig.zon.template");
const package_path_absolute = try arena.dupe(u8, out_dir);
for (package_path_absolute) |*c| {
c.* = if (c.* == '\\') '/' else c.*;
}
const content = try std.mem.replaceOwned(u8, arena, template, "<PACKAGE_PATH_ABSOLUTE>", package_path_absolute);
try writeFile(dir, "build.zig.zon", content);
}
}

fn writeFile(dir: std.fs.Dir, name: []const u8, content: []const u8) !void {
const file = try dir.createFile(name, .{});
defer file.close();
try file.writer().writeAll(content);
}
9 changes: 9 additions & 0 deletions test/standalone/fetch/example/build.zig
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const std = @import("std");

pub fn build(b: *std.Build) void {
const dep = b.dependency("somedependency", .{});
b.getInstallStep().dependOn(&b.addInstallFile(
dep.path("example_dep_file.txt"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if one is necessarily better than the other, but personally, if I have a dependency like .url = "https://example.com/foo/bar/file.txt", my natural assumption would be that I use dep.path("") to obtain a path to the file, not dep.path("file.txt").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with the solution that I didn't have to add any code for as the existing code paths already supported dep.path("file.txt"), willing to add extra code to support a special mapping if we feel it's advantageous.

"example_dep_file.txt",
).step);
}
16 changes: 16 additions & 0 deletions test/standalone/fetch/example/build.zig.zon.template
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
.{
.name = "fetch",
.version = "0.0.0",
.dependencies = .{
.somedependency = .{
.url = "file://<PACKAGE_PATH_ABSOLUTE>/example_dep_file.txt",
.hash = "12200f68aca70ebc76057200af436aab5720ec53a780713c5dc614825db42a39dbfb",
.unpack = false,
},
},
.paths = .{
"build.zig",
"build.zig.zon",
"example_dep_file.txt",
},
}
1 change: 1 addition & 0 deletions test/standalone/fetch/example/example_dep_file.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This is an example file.
3 changes: 3 additions & 0 deletions test/standalone/fetch/unpacktrue/build.zig
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const std = @import("std");

pub fn build(_: *std.Build) void {}
15 changes: 15 additions & 0 deletions test/standalone/fetch/unpacktrue/build.zig.zon
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
.{
.name = "unpacktrue",
.version = "0.0.0",
.dependencies = .{
.somedependency = .{
.url = "file://localhost/bar",
.hash = "1220f3b02ca452c26a96b48d2912b7fc907bef8d0b85c2e8f7e4a5c8bd95cdbfbae6",
.unpack = true,
},
},
.paths = .{
"build.zig",
"build.zig.zon",
},
}
Loading