Skip to content

fix(cache): handle missing cache hits when chaining two run steps #19974

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

Closed
bfredl wants to merge 1 commit into from
Closed

fix(cache): handle missing cache hits when chaining two run steps #19974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions lib/std/Build.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2148,6 +2148,9 @@ pub const LazyPath = union(enum) {

/// Applied after `up`.
sub_path: []const u8 = "",

/// If true, the file is hashed only on the suffix, not the full absolute path
content_hashed: bool = false,
},

/// An absolute path or a path relative to the current working directory of
Expand Down Expand Up @@ -2339,6 +2342,7 @@ pub const LazyPath = union(enum) {
.file = gen.file,
.up = gen.up,
.sub_path = b.dupePath(gen.sub_path),
.content_hashed = gen.content_hashed,
} },
.dependency => |dep| .{ .dependency = dep },
};
Expand Down
49 changes: 30 additions & 19 deletions lib/std/Build/Cache.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ pub const Manifest = struct {
/// ```
/// var file_contents = cache_hash.files.keys()[file_index].contents.?;
/// ```
pub fn addFile(self: *Manifest, file_path: []const u8, max_file_size: ?usize) !usize {
pub fn addFile(self: *Manifest, file_path: []const u8, max_file_size: ?usize, content_hashed: bool) !usize {
assert(self.manifest_file == null);

const gpa = self.cache.gpa;
Expand All @@ -378,22 +378,33 @@ pub const Manifest = struct {
.bin_digest = undefined,
};

self.hash.add(prefixed_path.prefix);
self.hash.addBytes(prefixed_path.sub_path);
const content_prefix = "zig-cache" ++ std.fs.path.sep_str ++ "o";
// if a file is hashed based on content, we want to skip the zig-build/o/{HASH}/ part
// as the HASH will represent the content of a previous step
// but keep the suffix as it might be relevant to a custom command
if (content_hashed and prefixed_path.sub_path.len > content_prefix.len and
mem.eql(u8, prefixed_path.sub_path[0..content_prefix.len], content_prefix))
{
self.hash.add(@as(u8, '?'));
self.hash.addBytes(prefixed_path.sub_path[content_prefix.len + hex_digest_len + 1 ..]);
} else {
self.hash.add(prefixed_path.prefix);
self.hash.addBytes(prefixed_path.sub_path);
}

return gop.index;
}

pub fn addOptionalFile(self: *Manifest, optional_file_path: ?[]const u8) !void {
self.hash.add(optional_file_path != null);
const file_path = optional_file_path orelse return;
_ = try self.addFile(file_path, null);
_ = try self.addFile(file_path, null, false);
}

pub fn addListOfFiles(self: *Manifest, list_of_files: []const []const u8) !void {
self.hash.add(list_of_files.len);
for (list_of_files) |file_path| {
_ = try self.addFile(file_path, null);
_ = try self.addFile(file_path, null, false);
}
}

Expand Down Expand Up @@ -506,16 +517,14 @@ pub const Manifest = struct {
if (prefix >= self.cache.prefixes_len) return error.InvalidFormat;

if (file_path.len == 0) return error.InvalidFormat;
const prefixed_path: PrefixedPath = .{
.prefix = prefix,
.sub_path = file_path, // expires with file_contents
};

const cache_hash_file = f: {
const prefixed_path: PrefixedPath = .{
.prefix = prefix,
.sub_path = file_path, // expires with file_contents
};
if (idx < input_file_count) {
const file = &self.files.keys()[idx];
if (!file.prefixed_path.eql(prefixed_path))
return error.InvalidFormat;

file.stat = .{
.size = stat_size,
Expand Down Expand Up @@ -561,11 +570,13 @@ pub const Manifest = struct {
self.failed_file_index = idx;
return err;
};

const name_match = pp.eql(prefixed_path);
const size_match = actual_stat.size == cache_hash_file.stat.size;
const mtime_match = actual_stat.mtime == cache_hash_file.stat.mtime;
const inode_match = actual_stat.inode == cache_hash_file.stat.inode;

if (!size_match or !mtime_match or !inode_match) {
if (!name_match or !size_match or !mtime_match or !inode_match) {
self.manifest_dirty = true;

cache_hash_file.stat = .{
Expand Down Expand Up @@ -1078,7 +1089,7 @@ test "cache file and then recall it" {
ch.hash.add(true);
ch.hash.add(@as(u16, 1234));
ch.hash.addBytes("1234");
_ = try ch.addFile(temp_file, null);
_ = try ch.addFile(temp_file, null, false);

// There should be nothing in the cache
try testing.expectEqual(false, try ch.hit());
Expand All @@ -1093,7 +1104,7 @@ test "cache file and then recall it" {
ch.hash.add(true);
ch.hash.add(@as(u16, 1234));
ch.hash.addBytes("1234");
_ = try ch.addFile(temp_file, null);
_ = try ch.addFile(temp_file, null, false);

// Cache hit! We just "built" the same file
try testing.expect(try ch.hit());
Expand Down Expand Up @@ -1144,7 +1155,7 @@ test "check that changing a file makes cache fail" {
defer ch.deinit();

ch.hash.addBytes("1234");
const temp_file_idx = try ch.addFile(temp_file, 100);
const temp_file_idx = try ch.addFile(temp_file, 100, false);

// There should be nothing in the cache
try testing.expectEqual(false, try ch.hit());
Expand All @@ -1163,7 +1174,7 @@ test "check that changing a file makes cache fail" {
defer ch.deinit();

ch.hash.addBytes("1234");
const temp_file_idx = try ch.addFile(temp_file, 100);
const temp_file_idx = try ch.addFile(temp_file, 100, false);

// A file that we depend on has been updated, so the cache should not contain an entry for it
try testing.expectEqual(false, try ch.hit());
Expand Down Expand Up @@ -1267,7 +1278,7 @@ test "Manifest with files added after initial hash work" {
defer ch.deinit();

ch.hash.addBytes("1234");
_ = try ch.addFile(temp_file1, null);
_ = try ch.addFile(temp_file1, null, false);

// There should be nothing in the cache
try testing.expectEqual(false, try ch.hit());
Expand All @@ -1282,7 +1293,7 @@ test "Manifest with files added after initial hash work" {
defer ch.deinit();

ch.hash.addBytes("1234");
_ = try ch.addFile(temp_file1, null);
_ = try ch.addFile(temp_file1, null, false);

try testing.expect(try ch.hit());
digest2 = ch.final();
Expand All @@ -1305,7 +1316,7 @@ test "Manifest with files added after initial hash work" {
defer ch.deinit();

ch.hash.addBytes("1234");
_ = try ch.addFile(temp_file1, null);
_ = try ch.addFile(temp_file1, null, false);

// A file that we depend on has been updated, so the cache should not contain an entry for it
try testing.expectEqual(false, try ch.hit());
Expand Down
2 changes: 1 addition & 1 deletion lib/std/Build/Step/ObjCopy.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
man.hash.add(@as(u32, 0xe18b7baf));

const full_src_path = objcopy.input_file.getPath2(b, step);
_ = try man.addFile(full_src_path, null);
_ = try man.addFile(full_src_path, null, false);
man.hash.addOptionalListOfBytes(objcopy.only_sections);
man.hash.addOptional(objcopy.pad_to);
man.hash.addOptional(objcopy.format);
Expand Down
16 changes: 10 additions & 6 deletions lib/std/Build/Step/Run.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ pub fn addPrefixedOutputFileArg(
run.setName(b.fmt("{s} ({s})", .{ run.step.name, basename }));
}

return .{ .generated = .{ .file = &output.generated_file } };
return .{ .generated = .{ .file = &output.generated_file, .content_hashed = true } };
}

/// Appends an input file to the command line arguments.
Expand Down Expand Up @@ -596,7 +596,11 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
const file_path = file.lazy_path.getPath2(b, step);
try argv_list.append(b.fmt("{s}{s}", .{ file.prefix, file_path }));
man.hash.addBytes(file.prefix);
_ = try man.addFile(file_path, null);
const content_hashed = switch (file.lazy_path) {
.generated => |gf| gf.content_hashed,
else => false,
};
_ = try man.addFile(file_path, null, content_hashed);
},
.directory_source => |file| {
const file_path = file.lazy_path.getPath2(b, step);
Expand All @@ -613,7 +617,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {

try argv_list.append(file_path);

_ = try man.addFile(file_path, null);
_ = try man.addFile(file_path, null, false);
},
.output_file, .output_directory => |output| {
man.hash.addBytes(output.prefix);
Expand All @@ -637,7 +641,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
},
.lazy_path => |lazy_path| {
const file_path = lazy_path.getPath2(b, step);
_ = try man.addFile(file_path, null);
_ = try man.addFile(file_path, null, true);
},
.none => {},
}
Expand All @@ -653,10 +657,10 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
hashStdIo(&man.hash, run.stdio);

for (run.extra_file_dependencies) |file_path| {
_ = try man.addFile(b.pathFromRoot(file_path), null);
_ = try man.addFile(b.pathFromRoot(file_path), null, false);
}
for (run.file_inputs.items) |lazy_path| {
_ = try man.addFile(lazy_path.getPath2(b, step), null);
_ = try man.addFile(lazy_path.getPath2(b, step), null, false);
}

if (try step.cacheHit(&man) and !has_side_effects) {
Expand Down
2 changes: 1 addition & 1 deletion lib/std/Build/Step/WriteFile.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
man.hash.addBytes(bytes);
},
.copy => |file_source| {
_ = try man.addFile(file_source.getPath2(b, step), null);
_ = try man.addFile(file_source.getPath2(b, step), null, true);
},
}
}
Expand Down
18 changes: 9 additions & 9 deletions src/Compilation.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ pub const cache_helpers = struct {
}

pub fn hashCSource(self: *Cache.Manifest, c_source: CSourceFile) !void {
_ = try self.addFile(c_source.src_path, null);
_ = try self.addFile(c_source.src_path, null, false);
// Hash the extra flags, with special care to call addFile for file parameters.
// TODO this logic can likely be improved by utilizing clang_options_data.zig.
const file_args = [_][]const u8{"-include"};
Expand All @@ -872,7 +872,7 @@ pub const cache_helpers = struct {
for (file_args) |file_arg| {
if (mem.eql(u8, file_arg, arg) and arg_i + 1 < c_source.extra_flags.len) {
arg_i += 1;
_ = try self.addFile(c_source.extra_flags[arg_i], null);
_ = try self.addFile(c_source.extra_flags[arg_i], null, false);
}
}
}
Expand Down Expand Up @@ -1179,7 +1179,7 @@ fn addModuleTableToCacheHash(
},
.files => |man| if (mod.root_src_path.len != 0) {
const pkg_zig_file = try mod.root.joinString(arena, mod.root_src_path);
_ = try man.addFile(pkg_zig_file, null);
_ = try man.addFile(pkg_zig_file, null, false);
},
}

Expand Down Expand Up @@ -2403,13 +2403,13 @@ fn addNonIncrementalStuffToCacheManifest(
}

for (comp.objects) |obj| {
_ = try man.addFile(obj.path, null);
_ = try man.addFile(obj.path, null, false);
man.hash.add(obj.must_link);
man.hash.add(obj.loption);
}

for (comp.c_object_table.keys()) |key| {
_ = try man.addFile(key.src.src_path, null);
_ = try man.addFile(key.src.src_path, null, false);
man.hash.addOptional(key.src.ext);
man.hash.addListOfBytes(key.src.extra_flags);
}
Expand All @@ -2418,11 +2418,11 @@ fn addNonIncrementalStuffToCacheManifest(
for (comp.win32_resource_table.keys()) |key| {
switch (key.src) {
.rc => |rc_src| {
_ = try man.addFile(rc_src.src_path, null);
_ = try man.addFile(rc_src.src_path, null, false);
man.hash.addListOfBytes(rc_src.extra_flags);
},
.manifest => |manifest_path| {
_ = try man.addFile(manifest_path, null);
_ = try man.addFile(manifest_path, null, false);
},
}
}
Expand Down Expand Up @@ -4770,7 +4770,7 @@ fn updateWin32Resource(comp: *Compilation, win32_resource: *Win32Resource, win32
// the XML data as a RT_MANIFEST resource. This means we can skip preprocessing,
// include paths, CLI options, etc.
if (win32_resource.src == .manifest) {
_ = try man.addFile(src_path, null);
_ = try man.addFile(src_path, null, false);

const rc_basename = try std.fmt.allocPrint(arena, "{s}.rc", .{src_basename});
const res_basename = try std.fmt.allocPrint(arena, "{s}.res", .{src_basename});
Expand Down Expand Up @@ -4853,7 +4853,7 @@ fn updateWin32Resource(comp: *Compilation, win32_resource: *Win32Resource, win32
// We now know that we're compiling an .rc file
const rc_src = win32_resource.src.rc;

_ = try man.addFile(rc_src.src_path, null);
_ = try man.addFile(rc_src.src_path, null, false);
man.hash.addListOfBytes(rc_src.extra_flags);

const rc_basename_noext = src_basename[0 .. src_basename.len - std.fs.path.extension(src_basename).len];
Expand Down
2 changes: 1 addition & 1 deletion src/glibc.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ pub fn buildSharedObjects(comp: *Compilation, prog_node: *std.Progress.Node) !vo
man.hash.add(target_version);

const full_abilists_path = try comp.zig_lib_directory.join(arena, &.{abilists_path});
const abilists_index = try man.addFile(full_abilists_path, abilists_max_size);
const abilists_index = try man.addFile(full_abilists_path, abilists_max_size, false);

if (try man.hit()) {
const digest = man.final();
Expand Down
8 changes: 4 additions & 4 deletions src/link.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub fn hashAddSystemLibs(
for (hm.values()) |value| {
man.hash.add(value.needed);
man.hash.add(value.weak);
if (value.path) |p| _ = try man.addFile(p, null);
if (value.path) |p| _ = try man.addFile(p, null, false);
}
}

Expand Down Expand Up @@ -736,16 +736,16 @@ pub const File = struct {
base.releaseLock();

for (objects) |obj| {
_ = try man.addFile(obj.path, null);
_ = try man.addFile(obj.path, null, false);
man.hash.add(obj.must_link);
man.hash.add(obj.loption);
}
for (comp.c_object_table.keys()) |key| {
_ = try man.addFile(key.status.success.object_path, null);
_ = try man.addFile(key.status.success.object_path, null, false);
}
if (!build_options.only_core_functionality) {
for (comp.win32_resource_table.keys()) |key| {
_ = try man.addFile(key.status.success.res_path, null);
_ = try man.addFile(key.status.success.res_path, null, false);
}
}
try man.addOptionalFile(zcu_obj_path);
Expand Down
6 changes: 3 additions & 3 deletions src/link/Coff/lld.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,15 @@ pub fn linkWithLLD(self: *Coff, arena: Allocator, prog_node: *std.Progress.Node)
comptime assert(Compilation.link_hash_implementation_version == 13);

for (comp.objects) |obj| {
_ = try man.addFile(obj.path, null);
_ = try man.addFile(obj.path, null, false);
man.hash.add(obj.must_link);
}
for (comp.c_object_table.keys()) |key| {
_ = try man.addFile(key.status.success.object_path, null);
_ = try man.addFile(key.status.success.object_path, null, false);
}
if (!build_options.only_core_functionality) {
for (comp.win32_resource_table.keys()) |key| {
_ = try man.addFile(key.status.success.res_path, null);
_ = try man.addFile(key.status.success.res_path, null, false);
}
}
try man.addOptionalFile(module_obj_path);
Expand Down
4 changes: 2 additions & 2 deletions src/link/Elf.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2217,12 +2217,12 @@ fn linkWithLLD(self: *Elf, arena: Allocator, prog_node: *std.Progress.Node) !voi
man.hash.add(self.allow_undefined_version);
man.hash.addOptional(self.enable_new_dtags);
for (comp.objects) |obj| {
_ = try man.addFile(obj.path, null);
_ = try man.addFile(obj.path, null, false);
man.hash.add(obj.must_link);
man.hash.add(obj.loption);
}
for (comp.c_object_table.keys()) |key| {
_ = try man.addFile(key.status.success.object_path, null);
_ = try man.addFile(key.status.success.object_path, null, false);
}
try man.addOptionalFile(module_obj_path);
try man.addOptionalFile(compiler_rt_path);
Expand Down
2 changes: 1 addition & 1 deletion src/link/MachO.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ pub fn hashAddFrameworks(man: *Cache.Manifest, hm: []const Framework) !void {
for (hm) |value| {
man.hash.add(value.needed);
man.hash.add(value.weak);
_ = try man.addFile(value.path, null);
_ = try man.addFile(value.path, null, false);
}
}

Expand Down
Loading