-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fetch: Fix various issues related to zig fetch --save
#21931
base: master
Are you sure you want to change the base?
Conversation
/// If `location` was `path_or_url`, this will be set to indicate | ||
/// whether the CLI input was identified as a file path or a URL. | ||
actual_path_or_url_kind: enum { path, url }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to note that this does mean that if the user invokes a command like zig fetch --save ./foo.tar.gz
, the package will first be copied to the global cache before printing the error. This is obviously not perfect; ideally, the command should detect that the user passed a file path in combination with --save
and exit before doing any work. However, if we want a maximally consistent and correct behavior, each solution comes with its own downsides
- Currently, we test if the
path_or_url
input is a directory we can open, then a file we can open, then finally a URL. Moving this logic to thecmdFetch()
function before we actually fetch the package would mean we now have open file handles that we need to pass to therun()
function and correctly close them if any errors occur on the way there (but not ifrun()
returns an error sincerun()
claims ownership of the handle). This is not trivial and will result in ugly code. - We could change the order of operations and instead try parsing the input as an URL first. However, an input like
c:foo
(on Windows) orhttp:foo
(on POSIX) would be interpreted as a URL despite possibly being a valid file path, so this isn't ideal. Even if we try to rule out values we know are not valid URLs that can be fetched, an input likehttp://example.com/example.tar.gz
could be a valid file path to the subpathexample.com/example.tar.gz
inside the directoryhttp:
. - We could also change the fetch command so that the user explicitly has to specify whether the input is a path or url, as in
zig fetch --path http:foo --save
.
I've thought about this a lot and I'm sure what the best way to handle this is, or if it's even worth putting energy into. I just think edge cases matter, however obscure they might be.
@@ -379,32 +379,6 @@ pub fn build(b: *std.Build) !void { | |||
const test_target_filters = b.option([]const []const u8, "test-target-filter", "Skip tests whose target triple do not match any filter") orelse &[0][]const u8{}; | |||
const test_slow_targets = b.option(bool, "test-slow-targets", "Enable running module tests for targets that have a slow compiler backend") orelse false; | |||
|
|||
const test_cases_options = b.addOptions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were unreferenced.
The purpose of `zig fetch` is to copy a package to the global cache to avoid needing to access the network. `path` entries in a build.zig.zon are for relative paths inside a package. It does not make sense to `--save` a package fetched by a (CWD-relative) file path. Closes ziglang#18639
This did not work at all previously because the `parent_package_root` field was `undefined`. This fix additionally validates that the file URL is a conformant local URL and decodes absolute Windows paths correctly. See RFC 8089 and https://url.spec.whatwg.org/ for relevant specs.
There are several test decls inside `/src` that are not currently being tested and have bitrotted as a result. This commit revives those tests and adds the `test-compiler-internals` set of tests which tests everything reachable from `/src/main.zig`.
Closes #18639
Closes #21690
zig fetch --save file/path
is now an error.--save
ing over an existingpath
entry will now correctly replace it with anurl
entry.zig fetch file:///C:/foo/bar.zip
) did not previously work, but does now.The purpose of
zig fetch
is to copy a package to the global cache to avoid needing to access the network. It is not to manage yourbuild.zig.zon
.--save
is provided as a convenience since calculating the hash by hand would be unreasonable, but it does not make sense to--save
a package fetched by a (CWD-relative) file path sincepath
entries in a build.zig.zon are for relative paths inside a package.path
entries can only be added by manually editing yourbuild.zig.zon
.Both
zig fetch
andbuild.zig.zon
support fetching by file URL scheme, for example if a package is on an intranet network drive, but this did not previously always work due to a field being uninitialized and due to file URL-encoded absolute paths on Windows containing drive letters requiring extra attention. This PR fixes those bugs and improves validation to ensure file URLs are spec-compliant (based on RFC 8089 and https://url.spec.whatwg.org/).I've verified the correctness of these changes by running the following commands locally (on Windows):
zig fetch --save ..\..\foo\
-> error (can't save when fetching by file path)zig fetch --save ..\..\foo.zip
-> error (can't save when fetching by file path)zig fetch --save file:///foo.zip
-> okzig fetch --save file:/foo.zip
-> okzig fetch --save file:///C:/foo.zip
-> okzig fetch --save file:/C:/foo.zip
-> okzig fetch --save file://localhost/C:/foo.zip
-> okzig fetch --save file:foo.zip
-> error (invalid file URL)Additionally, it turns out that there were a lot of test declarations in
/src
that were unreferenced and untested, some of which were for the package manager internals. I de-bitrotted those tests and added them under a newtest-cli-internals
test set, which tests everything referenced by/src/main.zig
.