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

fetch: Fix various issues related to zig fetch --save #21931

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

castholm
Copy link
Contributor

@castholm castholm commented Nov 6, 2024

Closes #18639
Closes #21690

  • zig fetch --save file/path is now an error.
  • --saveing over an existing path entry will now correctly replace it with an url entry.
  • Fetching by file URL (as in 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 your build.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 since path entries in a build.zig.zon are for relative paths inside a package. path entries can only be added by manually editing your build.zig.zon.

Both zig fetch and build.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 -> ok
  • zig fetch --save file:/foo.zip -> ok
  • zig fetch --save file:///C:/foo.zip -> ok
  • zig fetch --save file:/C:/foo.zip -> ok
  • zig fetch --save file://localhost/C:/foo.zip -> ok
  • zig 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 new test-cli-internals test set, which tests everything referenced by /src/main.zig.

Comment on lines +68 to +70
/// 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 },
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 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 the cmdFetch() function before we actually fetch the package would mean we now have open file handles that we need to pass to the run() function and correctly close them if any errors occur on the way there (but not if run() returns an error since run() 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) or http: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 like http://example.com/example.tar.gz could be a valid file path to the subpath example.com/example.tar.gz inside the directory http:.
  • 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();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were unreferenced.

build.zig Outdated Show resolved Hide resolved
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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants