Add .no_unpack = PATH 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.
src/Package/Manifest.zig
Outdated
| 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.
There was a problem hiding this comment.
I agree with @castholm. It should be able to be set to true.
| 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.
There was a problem hiding this comment.
I think it is a good goal of having unpack=false use case be able to have a matching hash with unpack=true use case where the file is for example in a .tar.gz.
In order for this to work, however, we need to also support mirrors and the concept of URL being independent from hash. In the unpack use case, the file names are determined in-bound, not by the URL.
With no unpack, the file name still needs to be independent from the URL. So, I think the bool unpack should be changed to optional string, which specifies the sub-path, which may include path components. This is the string one passes to dep.path to access the file.
This makes it possible to have a unpacked single file have the same hash, and build.zig logic, as a pack that contains the same file, as long as the provided sub-path components match.
zig fetch however can keep the --no-unpack flag and default-populate the string filename based on the URL or the HTTP file name header.
src/Package/Manifest.zig
Outdated
| 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.
I agree with @castholm. It should be able to be set to true.
| 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 think it is a good goal of having unpack=false use case be able to have a matching hash with unpack=true use case where the file is for example in a .tar.gz.
In order for this to work, however, we need to also support mirrors and the concept of URL being independent from hash. In the unpack use case, the file names are determined in-bound, not by the URL.
With no unpack, the file name still needs to be independent from the URL. So, I think the bool unpack should be changed to optional string, which specifies the sub-path, which may include path components. This is the string one passes to dep.path to access the file.
This makes it possible to have a unpacked single file have the same hash, and build.zig logic, as a pack that contains the same file, as long as the provided sub-path components match.
zig fetch however can keep the --no-unpack flag and default-populate the string filename based on the URL or the HTTP file name header.
marler8997
force-pushed
the
noUnpack
branch
2 times, most recently
from
c7816a9 to
8ee494f
Compare
marler8997
changed the title
.unpack = false and --no-unpack to build.zig.zon and zig fetch
|
I've updated the API according to the suggestions. Since we've moved from .no_unpack = "foo/bar.zip",This makes it consistent with the |
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.