Add .unpack = false and --no-unpack to build.zig.zon and zig fetch
#22042
Conversation
marler8997
force-pushed
the
noUnpack
branch
3 times, most recently
from
e81c7c5 to
7c07035
Compare
src/Package/Fetch.zig
Outdated
| const path_relative = std.mem.trimLeft(u8, path, "/"); | ||
| return .{ .file = f.parent_package_root.openFile(path_relative, .{}) catch |err| { | ||
| return f.fail(f.location_tok, try eb.printString("unable to open '{}{s}': {s}", .{ | ||
| f.parent_package_root, path, @errorName(err), | ||
| f.parent_package_root, path_relative, @errorName(err), |
There was a problem hiding this comment.
This is incorrect. A file URL like file:///foo represents the absolute path /foo. Relative paths can not be represented as file URLs (something like file:foo is not spec-compliant, there must always be a slash after the colon), see RFC 8089 and https://url.spec.whatwg.org/ for specifics.
I have an open PR #21931 which improves the validation and processing of file URLs but even if that PR were to be rejected, interpreting a path starting with a slash as relative is wrong and these changes should be reverted.
This also means that we can't use .url = "file://localhost/example_dep_file.txt" in test packages since absolute paths are not portable. For the tests to work, the dependency would need to be added as a regular HTTP URL and the tests themselves would need to start a web server that serves the file over HTTP.
There was a problem hiding this comment.
I've pushed a new iteration that uses an absolute file path instead.
| 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", .{}); |
There was a problem hiding this comment.
.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.
There was a problem hiding this comment.
The unpack field should also be documented in https://github.com/ziglang/zig/blob/master/doc/build.zig.zon.md and https://github.com/ziglang/zig/blob/master/lib/init/build.zig.zon.
There was a problem hiding this comment.
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.
| pub fn build(b: *std.Build) void { | ||
| const dep = b.dependency("somedependency", .{}); | ||
| b.getInstallStep().dependOn(&b.addInstallFile( | ||
| dep.path("example_dep_file.txt"), |
There was a problem hiding this comment.
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").
There was a problem hiding this comment.
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.
closes ziglang#17895 Enhances zig with the ability to fetch a dependency of any file type.
closes #17895
Enhances zig with the ability to fetch a dependency of any file type.