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.

Sign up for GitHub

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 5 commits into
base: master
Choose a base branch
from
+89 −33
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7867b74
fetch: Disallow `--save` when fetching by file path
castholm Nov 6, 2024
e353694
fetch: `--save` over existing `path` entry correctly
castholm Nov 6, 2024
2ee8bda
fetch: Fix fetching by file URL
castholm Nov 6, 2024
33d2a58
fetch: Remove some redundancy from usage text
castholm Nov 6, 2024
9da3e10
fetch: Update tests
castholm Dec 15, 2024
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
67 changes: 58 additions & 9 deletions src/Package/Fetch.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ oom_flag: bool,
/// If the resource pointed to by the location is not a Git-repository, this
/// will be left unchanged.
latest_commit: ?git.Oid,
/// 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 },
Comment on lines +68 to +70
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.


// This field is used by the CLI only, untouched by this file.

Expand Down Expand Up @@ -344,25 +347,28 @@ pub fn run(f: *Fetch) RunError!void {
.remote => |remote| remote,
.path_or_url => |path_or_url| {
if (fs.cwd().openDir(path_or_url, .{ .iterate = true })) |dir| {
f.actual_path_or_url_kind = .path;
var resource: Resource = .{ .dir = dir };
return f.runResource(path_or_url, &resource, null);
} else |dir_err| {
const file_err = if (dir_err == error.NotDir) e: {
if (fs.cwd().openFile(path_or_url, .{})) |file| {
f.actual_path_or_url_kind = .path;
var resource: Resource = .{ .file = file };
return f.runResource(path_or_url, &resource, null);
} else |err| break :e err;
} else dir_err;

const uri = std.Uri.parse(path_or_url) catch |uri_err| {
return f.fail(0, try eb.printString(
"'{s}' could not be recognized as a file path ({s}) or an URL ({s})",
"'{s}' could not be recognized as a file path ({s}) or a URL ({s})",
.{ path_or_url, @errorName(file_err), @errorName(uri_err) },
));
};
f.actual_path_or_url_kind = .url;
var server_header_buffer: [header_buffer_size]u8 = undefined;
const uri_path = try uri.path.toRawMaybeAlloc(arena);
var resource = try f.initResource(uri, &server_header_buffer);
return f.runResource(try uri.path.toRawMaybeAlloc(arena), &resource, null);
return f.runResource(uri_path, &resource, null);
}
},
};
Expand Down Expand Up @@ -420,11 +426,12 @@ pub fn run(f: *Fetch) RunError!void {

const uri = std.Uri.parse(remote.url) catch |err| return f.fail(
f.location_tok,
try eb.printString("invalid URI: {s}", .{@errorName(err)}),
try eb.printString("invalid URL: {s}", .{@errorName(err)}),
);
var server_header_buffer: [header_buffer_size]u8 = undefined;
const uri_path = try uri.path.toRawMaybeAlloc(arena);
var resource = try f.initResource(uri, &server_header_buffer);
return f.runResource(try uri.path.toRawMaybeAlloc(arena), &resource, remote.hash);
return f.runResource(uri_path, &resource, remote.hash);
}

pub fn deinit(f: *Fetch) void {
Expand Down Expand Up @@ -728,6 +735,7 @@ fn queueJobsForDeps(f: *Fetch) RunError!void {
.has_build_zig = false,
.oom_flag = false,
.latest_commit = null,
.actual_path_or_url_kind = undefined,

.module = null,
};
Expand Down Expand Up @@ -914,10 +922,45 @@ fn initResource(f: *Fetch, uri: std.Uri, server_header_buffer: []u8) RunError!Re
const eb = &f.error_bundle;

if (ascii.eqlIgnoreCase(uri.scheme, "file")) {
const path = try uri.path.toRawMaybeAlloc(arena);
return .{ .file = f.parent_package_root.openFile(path, .{}) catch |err| {
return f.fail(f.location_tok, try eb.printString("unable to open '{}{s}': {s}", .{
f.parent_package_root, path, @errorName(err),
var path = try uri.path.toRawMaybeAlloc(arena);

if (uri.user != null or
uri.password != null or
uri.port != null or
path.len == 0 or path[0] != '/' or
uri.query != null)
{
return f.fail(f.location_tok, try eb.printString(
"invalid file URL: {}",
.{uri},
));
}
if (uri.host) |host| {
const raw_host = try host.toRawMaybeAlloc(arena);
if (raw_host.len != 0 and !ascii.eqlIgnoreCase(raw_host, "localhost")) {
return f.fail(f.location_tok, try eb.printString(
"unable to process non-local file URL: {}",
.{uri},
));
}
}

// On Windows, an absolute path starting with a drive letter gains a leading
// slash when encoded as a file URL that must be stripped when decoding.
if (native_os == .windows and
path.len >= "/C:/".len and
std.ascii.isAlphabetic(path[1]) and
path[2] == ':' and
path[3] == '/')
{
path = path[1..];
}

assert(std.fs.path.isAbsolute(path));

return .{ .file = fs.cwd().openFile(path, .{}) catch |err| {
return f.fail(f.location_tok, try eb.printString("unable to open '{s}': {s}", .{
path, @errorName(err),
}));
} };
}
Expand Down Expand Up @@ -2035,6 +2078,7 @@ test "zip" {
defer fb.deinit();

try fetch.run();
try std.testing.expectEqual(.path, fetch.actual_path_or_url_kind);

var out = try fb.packageDir();
defer out.close();
Expand Down Expand Up @@ -2068,6 +2112,7 @@ test "zip with one root folder" {
defer fb.deinit();

try fetch.run();
try std.testing.expectEqual(.path, fetch.actual_path_or_url_kind);

var out = try fb.packageDir();
defer out.close();
Expand Down Expand Up @@ -2142,6 +2187,7 @@ test "tarball with excluded duplicate paths" {
"12200bafe035cbb453dd717741b66e9f9d1e6c674069d06121dafa1b2e62eb6b22da",
&hex_digest,
);
try std.testing.expectEqual(.path, fetch.actual_path_or_url_kind);

const expected_files: []const []const u8 = &.{
"build.zig",
Expand Down Expand Up @@ -2186,6 +2232,7 @@ test "tarball without root folder" {
"12209f939bfdcb8b501a61bb4a43124dfa1b2848adc60eec1e4624c560357562b793",
&hex_digest,
);
try std.testing.expectEqual(.path, fetch.actual_path_or_url_kind);

const expected_files: []const []const u8 = &.{
"build.zig",
Expand Down Expand Up @@ -2224,6 +2271,7 @@ test "set executable bit based on file content" {
"1220fecb4c06a9da8673c87fe8810e15785f1699212f01728eadce094d21effeeef3",
&Manifest.hexDigest(fetch.actual_hash),
);
try std.testing.expectEqual(.path, fetch.actual_path_or_url_kind);

var out = try fb.packageDir();
defer out.close();
Expand Down Expand Up @@ -2308,6 +2356,7 @@ const TestFetchBuilder = struct {
.has_build_zig = false,
.oom_flag = false,
.latest_commit = null,
.actual_path_or_url_kind = undefined,

.module = null,
};
Expand Down
55 changes: 31 additions & 24 deletions src/main.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5207,6 +5207,7 @@ fn cmdBuild(gpa: Allocator, arena: Allocator, args: []const []const u8) !void {
.has_build_zig = true,
.oom_flag = false,
.latest_commit = null,
.actual_path_or_url_kind = undefined,

.module = build_mod,
};
Expand Down Expand Up @@ -6988,10 +6989,8 @@ const usage_fetch =
\\ -h, --help Print this help and exit
\\ --global-cache-dir [path] Override path to global Zig cache directory
\\ --debug-hash Print verbose hash information to stdout
\\ --save Add the fetched package to build.zig.zon
\\ --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
\\ --save[=name] Add the fetched package to build.zig.zon
\\ --save-exact[=name] Add the fetched package to build.zig.zon, storing the URL verbatim
\\
;

Expand Down Expand Up @@ -7107,6 +7106,7 @@ fn cmdFetch(
.has_build_zig = false,
.oom_flag = false,
.latest_commit = null,
.actual_path_or_url_kind = undefined,

.module = null,
};
Expand Down Expand Up @@ -7134,10 +7134,13 @@ fn cmdFetch(
return cleanExit();
},
.yes, .exact => |name| name: {
if (fetch.actual_path_or_url_kind == .path) {
fatal("options '--save' and `--save-exact` cannot be used when fetching by file path", .{});
}

if (name) |n| break :name n;
const fetched_manifest = fetch.manifest orelse
fatal("unable to determine name; fetched package has no build.zig.zon file", .{});
break :name fetched_manifest.name;
if (fetch.manifest) |fm| break :name fm.name;
fatal("unable to determine name; fetched package has no build.zig.zon file", .{});
},
};

Expand All @@ -7163,7 +7166,8 @@ fn cmdFetch(
var fixups: Ast.Fixups = .{};
defer fixups.deinit(gpa);

var saved_path_or_url = path_or_url;
var saved_url = path_or_url;
assert(fetch.actual_path_or_url_kind == .url);

if (fetch.latest_commit) |latest_commit| resolved: {
const latest_commit_hex = try std.fmt.allocPrint(arena, "{}", .{latest_commit});
Expand All @@ -7188,7 +7192,7 @@ fn cmdFetch(
uri.fragment = .{ .raw = latest_commit_hex };

switch (save) {
.yes => saved_path_or_url = try std.fmt.allocPrint(arena, "{}", .{uri}),
.yes => saved_url = try std.fmt.allocPrint(arena, "{}", .{uri}),
.no, .exact => {}, // keep the original URL
}
}
Expand All @@ -7199,7 +7203,7 @@ fn cmdFetch(
\\ .hash = "{}",
\\ }}
, .{
std.zig.fmtEscapes(saved_path_or_url),
std.zig.fmtEscapes(saved_url),
std.zig.fmtEscapes(&hex_digest),
});

Expand All @@ -7219,7 +7223,7 @@ fn cmdFetch(
if (dep.hash) |h| {
switch (dep.location) {
.url => |u| {
if (mem.eql(u8, h, &hex_digest) and mem.eql(u8, u, saved_path_or_url)) {
if (mem.eql(u8, h, &hex_digest) and mem.eql(u8, u, saved_url)) {
std.log.info("existing dependency named '{s}' is up-to-date", .{name});
process.exit(0);
}
Expand All @@ -7228,20 +7232,23 @@ fn cmdFetch(
}
}

const location_replace = try std.fmt.allocPrint(
arena,
"\"{}\"",
.{std.zig.fmtEscapes(saved_path_or_url)},
);
const hash_replace = try std.fmt.allocPrint(
arena,
"\"{}\"",
.{std.zig.fmtEscapes(&hex_digest)},
);

warn("overwriting existing dependency named '{s}'", .{name});
try fixups.replace_nodes_with_string.put(gpa, dep.location_node, location_replace);
try fixups.replace_nodes_with_string.put(gpa, dep.hash_node, hash_replace);
if (dep.location == .url) {
const location_replace = try std.fmt.allocPrint(
arena,
"\"{}\"",
.{std.zig.fmtEscapes(saved_url)},
);
const hash_replace = try std.fmt.allocPrint(
arena,
"\"{}\"",
.{std.zig.fmtEscapes(&hex_digest)},
);
try fixups.replace_nodes_with_string.put(gpa, dep.location_node, location_replace);
try fixups.replace_nodes_with_string.put(gpa, dep.hash_node, hash_replace);
} else {
try fixups.replace_nodes_with_string.put(gpa, dep.node, new_node_init);
}
} else if (manifest.dependencies.count() > 0) {
// Add fixup for adding another dependency.
const deps = manifest.dependencies.values();
Expand Down
Loading