From 470f77600d79a5c17b3734f97eaf2462c452718e Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Wed, 12 Aug 2026 21:59:34 -0700 Subject: [PATCH] Maker: detect modifications to configurer and recompile it including when using `--watch`. This is done by adding an extra auto-generated placeholder Step at the end of `Maker.steps` that contains the file system inputs for the configurer. It is done this way so that the hot path of file system watching does not need to make any special cases, and to avoid more OS-specific logic in file system watching implementation. closes #20602 closes #35460 --- lib/compiler/Maker.zig | 134 ++++++++++++++++++++++++++--------- lib/compiler/Maker/Step.zig | 14 +++- lib/compiler/Maker/Watch.zig | 61 ++++++---------- 3 files changed, 133 insertions(+), 76 deletions(-) diff --git a/lib/compiler/Maker.zig b/lib/compiler/Maker.zig index 51195aaf23bea8bb551d175b61cc57804b3a50e1..0b10b8fdacfa220a87b9e078d2067c9fb468a99d 100644 --- a/lib/compiler/Maker.zig +++ b/lib/compiler/Maker.zig @@ -44,6 +44,10 @@ gpa: Allocator, graph: *Graph, install_paths: InstallPaths, scanned_config: *const ScannedConfig, +/// Includes an extra auto-generated placeholder Step at the end that indicates +/// configure must be rerun. It is done this way so that the hot path of file +/// system watching does not need to make any special cases, and to avoid more +/// OS-specific logic in file system watching implementation. steps: []Step, generated_files: []Path, run_args: ?[]const []const u8, @@ -708,7 +712,12 @@ pub fn main(init: process.Init.Minimal) !void { break :s &protocol_server_allocation; } else null; - while (true) { + configure: while (true) { + // Set of files that, if modified, imply that recompiling and rerunning + // configurer is needed. + var configure_source_files: Cache.Manifest.Files = .empty; + defer Cache.Manifest.freeFiles(gpa, &configure_source_files); + // If this fails, we can still start the server and wait for user // to request a rebuild. If it returns error.FailedButCacheIntact // we can even still do file system watching and automatically @@ -730,6 +739,7 @@ pub fn main(init: process.Init.Minimal) !void { .fetch_only = fetch_only, .print_configuration = print_configuration, .forks = forks.items, + .src_files = &configure_source_files, })) |scanned_config| { if (help_menu) { scanned_config.printUsage(&graph, initStdoutWriter(io)) catch |err| switch (err) { @@ -766,7 +776,9 @@ pub fn main(init: process.Init.Minimal) !void { .include = install_include_path, }, - .steps = try arena.alloc(Step, scanned_config.configuration.steps.len), + // Extra step at the end which is the autogenerated placeholder + // step which indicates that we need to reconfigure. + .steps = try arena.alloc(Step, scanned_config.configuration.steps.len + 1), .generated_files = try arena.alloc(Path, scanned_config.configuration.generated_files_len), .run_args = run_args, @@ -843,7 +855,7 @@ pub fn main(init: process.Init.Minimal) !void { try select.concurrent(.message, Server.receiveMessage, .{s}); maker.watch = body.flags.watch; - maker.prepare(steps) catch |err| switch (err) { + maker.prepare(steps, &configure_source_files) catch |err| switch (err) { error.DependencyLoopDetected, error.InsufficientMemory => { // TODO handle DependencyLoopDetected as error.FailedButCacheIntact // and handle InsufficientMemory as error.AlreadyReported @@ -859,8 +871,11 @@ pub fn main(init: process.Init.Minimal) !void { if (!Watch.have_impl) unreachable; if (w == null) w = try .init(&maker); - try w.?.update(maker.step_stack.keys()); - try select.concurrent(.fs_event, Watch.wait, .{ &w.?, if (in_debounce) .{ .ms = debounce_interval_ms } else .none }); + try updateWatch(&maker, &w.?); + try select.concurrent(.fs_event, Watch.wait, .{ + &w.?, + if (in_debounce) .{ .ms = debounce_interval_ms } else .none, + }); } continue :loop try select.await(); @@ -870,7 +885,13 @@ pub fn main(init: process.Init.Minimal) !void { }, .fs_event => |payload| { if (!Watch.have_impl) unreachable; - switch (try payload) { + switch (payload catch |err| switch (err) { + error.MustReconfigure => { + try io.sleep(.fromMilliseconds(debounce_interval_ms), .awake); + continue :configure; + }, + else => |e| fatal("file watching failed: {t}", .{e}), + }) { .timeout => { assert(in_debounce); markFailedStepsDirty(&maker); @@ -880,7 +901,10 @@ pub fn main(init: process.Init.Minimal) !void { .dirty => in_debounce = true, .clean => {}, } - try select.concurrent(.fs_event, Watch.wait, .{ &w.?, if (in_debounce) .{ .ms = debounce_interval_ms } else .none }); + try select.concurrent(.fs_event, Watch.wait, .{ + &w.?, + if (in_debounce) .{ .ms = debounce_interval_ms } else .none, + }); continue :loop try select.await(); }, } @@ -889,7 +913,7 @@ pub fn main(init: process.Init.Minimal) !void { const initial_steps = try maker.resolveTopLevelSteps(step_names.items); defer gpa.free(initial_steps); - maker.prepare(initial_steps) catch |err| switch (err) { + maker.prepare(initial_steps, &configure_source_files) catch |err| switch (err) { error.DependencyLoopDetected, error.InsufficientMemory => { // TODO handle DependencyLoopDetected as error.FailedButCacheIntact // and handle InsufficientMemory as error.AlreadyReported @@ -938,7 +962,7 @@ pub fn main(init: process.Init.Minimal) !void { // Comptime-known guard to prevent including the logic below when `!Watch.have_impl`. if (!Watch.have_impl) unreachable; - try w.update(maker.step_stack.keys()); + try updateWatch(&maker, &w); // Wait until a file system notification arrives. Read all such events // until the buffer is empty. Then wait for a debounce interval, resetting @@ -950,21 +974,34 @@ pub fn main(init: process.Init.Minimal) !void { w.dir_count, countSubProcesses(&maker), }) catch &caption_buf; var debouncing_node = main_progress_node.start(caption, 0); + defer debouncing_node.end(); var in_debounce = false; - while (true) switch (try w.wait(if (in_debounce) .{ .ms = debounce_interval_ms } else .none)) { - .timeout => { - assert(in_debounce); - debouncing_node.end(); - markFailedStepsDirty(&maker); - continue :rebuild; - }, - .dirty => if (!in_debounce) { - in_debounce = true; - debouncing_node.end(); - debouncing_node = main_progress_node.start("Debouncing (Change Detected)", 0); - }, - .clean => {}, - }; + while (true) { + const timeout: Watch.Timeout = if (in_debounce) .{ .ms = debounce_interval_ms } else .none; + switch (w.wait(timeout) catch |err| switch (err) { + error.MustReconfigure => { + debouncing_node.end(); + debouncing_node = main_progress_node.start("Debouncing (Change Detected)", 0); + try io.sleep(.fromMilliseconds(debounce_interval_ms), .awake); + continue :configure; + }, + else => |e| fatal("file watching failed: {t}", .{e}), + }) { + .timeout => { + assert(in_debounce); + debouncing_node.end(); + debouncing_node = .none; + markFailedStepsDirty(&maker); + continue :rebuild; + }, + .dirty => if (!in_debounce) { + in_debounce = true; + debouncing_node.end(); + debouncing_node = main_progress_node.start("Debouncing (Change Detected)", 0); + }, + .clean => {}, + } + } } } else |err| { const can_fs_watch = switch (err) { @@ -991,6 +1028,15 @@ pub fn main(init: process.Init.Minimal) !void { } } +/// Temporarily adds the reconfigure pseudostep to step_stack, calls +/// `Watch.update`, and then pops it again. +fn updateWatch(maker: *Maker, watch: *Watch) !void { + const step_stack = &maker.step_stack; + try step_stack.putNoClobber(maker.gpa, @fromBackingInt(@intCast(maker.steps.len - 1)), {}); + defer _ = step_stack.pop().?; + try watch.update(step_stack.keys()); +} + const ConfigureOptions = struct { configure_argv: [][]const u8, conf_argv_index_build_root: usize, @@ -1008,6 +1054,7 @@ const ConfigureOptions = struct { fetch_only: bool, print_configuration: PrintConfiguration, forks: []Fork, + src_files: *Cache.Manifest.Files, }; fn configure(graph: *Graph, options: ConfigureOptions) !ScannedConfig { @@ -1362,14 +1409,14 @@ fn configure(graph: *Graph, options: ConfigureOptions) !ScannedConfig { if (config_man) |man| { if (try man.hit(compile_prog_node)) { + log.debug("configuration cache hit", .{}); const digest = man.final(); - break :cp .{ - .{ - .root_dir = graph.local_cache_root, - .sub_path = try arena.print("c/{s}", .{&digest}), - }, - man.toOwnedLock(), + const path: Path = .{ + .root_dir = graph.local_cache_root, + .sub_path = try arena.print("c/{s}", .{&digest}), }; + options.src_files.* = man.takeFiles(); + break :cp .{ path, man.toOwnedLock() }; } } const configure_exe_path: Path = if (std.zig.buildExeSubprocess(gpa, io, .{ @@ -1505,6 +1552,7 @@ fn configure(graph: *Graph, options: ConfigureOptions) !ScannedConfig { }); }; man.writeManifest() catch |err| log.warn("failed to write cache manifest: {t}", .{err}); + options.src_files.* = man.takeFiles(); break :cp .{ final_path, man.toOwnedLock() }; } }; @@ -2119,7 +2167,13 @@ fn markFailedStepsDirty(maker: *Maker) void { for (all_steps) |step_index| { const step = maker.stepByIndex(step_index); switch (step.state) { - .dependency_failure, .dependency_skipped, .failure, .skipped => _ = maker.invalidateResult(step), + .dependency_failure, + .dependency_skipped, + .failure, + .skipped, + => _ = maker.invalidateResult(step) catch |err| switch (err) { + error.MustReconfigure => unreachable, + }, else => continue, } } @@ -2173,7 +2227,11 @@ fn resolveTopLevelSteps(maker: *Maker, step_names: []const []const u8) ![]const return try gpa.dupe(Configuration.Step.Index, result.keys()); } -fn prepare(maker: *Maker, step_indices: []const Configuration.Step.Index) !void { +fn prepare( + maker: *Maker, + step_indices: []const Configuration.Step.Index, + configure_source_files: *const Cache.Manifest.Files, +) !void { const gpa = maker.gpa; const graph = maker.graph; const arena = graph.arena; @@ -2182,10 +2240,17 @@ fn prepare(maker: *Maker, step_indices: []const Configuration.Step.Index) !void const step_stack = &maker.step_stack; const c = &maker.scanned_config.configuration; - for (maker.steps, 0..) |*step, step_index_usize| { + // The last element is a reserved special pseudostep which contains the + // watch inputs for the configurer executable. + for (maker.steps[0 .. maker.steps.len - 1], 0..) |*step, step_index_usize| { const step_index: Configuration.Step.Index = @fromBackingInt(@intCast(step_index_usize)); step.* = .{ .extended = .init(step_index.ptr(c).flags(c).tag) }; } + { + const last_step = &maker.steps[maker.steps.len - 1]; + last_step.* = .{ .extended = .init(.top_level) }; + try last_step.setWatchInputsFromManifestFiles(maker, configure_source_files, graph.cache.prefixes()); + } try initial_steps.ensureUnusedCapacity(gpa, step_indices.len); try step_stack.ensureUnusedCapacity(gpa, step_indices.len); @@ -3045,14 +3110,15 @@ fn constructGraphAndCheckForDependencyLoop( /// When file watching, prepares the step for being re-evaluated. Returns /// `true` if the step was newly invalidated, `false` if it was already /// invalidated. -pub fn invalidateResult(maker: *Maker, step: *Step) bool { +pub fn invalidateResult(maker: *Maker, step: *Step) error{MustReconfigure}!bool { + if (step == &maker.steps[maker.steps.len - 1]) return error.MustReconfigure; if (step.state == .precheck_done) return false; assert(step.pending_deps == 0); step.state = .precheck_done; step.reset(maker); for (step.dependants.items) |dependant_index| { const dependant = maker.stepByIndex(dependant_index); - _ = invalidateResult(maker, dependant); + _ = try invalidateResult(maker, dependant); dependant.pending_deps += 1; } return true; diff --git a/lib/compiler/Maker/Step.zig b/lib/compiler/Maker/Step.zig index 181cdf2bd75fc678fc45d72cbab16a9e9bbeecd1..2ce90b5a9b7725d452258af167de82b60b973a6b 100644 --- a/lib/compiler/Maker/Step.zig +++ b/lib/compiler/Maker/Step.zig @@ -781,12 +781,20 @@ pub fn writeManifestAndWatch(s: *Step, maker: *Maker, man: *Cache.Manifest) !voi try setWatchInputsFromManifest(s, maker, man); } -fn setWatchInputsFromManifest(s: *Step, maker: *Maker, man: *Cache.Manifest) !void { +pub fn setWatchInputsFromManifest(s: *Step, maker: *Maker, man: *Cache.Manifest) !void { + return setWatchInputsFromManifestFiles(s, maker, &man.files, man.cache.prefixes()); +} + +pub fn setWatchInputsFromManifestFiles( + s: *Step, + maker: *Maker, + files: *const Cache.Manifest.Files, + prefixes: []const Cache.Directory, +) !void { const graph = maker.graph; const arena = graph.arena; // TODO don't leak into process arena - const prefixes = man.cache.prefixes(); clearWatchInputs(s, maker); - for (man.files.keys()) |file| { + for (files.keys()) |file| { // The file path data is freed when the cache manifest is cleaned up at the end of `make`. const sub_path = try arena.dupe(u8, file.prefixed_path.sub_path); try addWatchInputFromPath(s, maker, .{ diff --git a/lib/compiler/Maker/Watch.zig b/lib/compiler/Maker/Watch.zig index 4d40facdfc71076eb18f5958856fa3fce4f56741..6d3fd41ad9baa30b6e26195f1de015e359d3248e 100644 --- a/lib/compiler/Maker/Watch.zig +++ b/lib/compiler/Maker/Watch.zig @@ -49,7 +49,10 @@ const Os = switch (builtin.os.tag) { poll_fds: std.array_hash_map.Auto(MountId, posix.pollfd), const MountId = i32; - const HandleTable = std.array_hash_map.Custom(FileHandle, struct { mount_id: MountId, reaction_set: ReactionSet }, FileHandle.Adapter, false); + const HandleTable = std.array_hash_map.Custom(FileHandle, struct { + mount_id: MountId, + reaction_set: ReactionSet, + }, FileHandle.Adapter, false); const fan_mask: std.os.linux.fanotify.MarkMask = .{ .CLOSE_WRITE = true, @@ -152,10 +155,8 @@ const Os = switch (builtin.os.tag) { }) { assert(meta[0].vers == M.VERSION); if (meta[0].mask.Q_OVERFLOW) { - any_dirty = true; - std.log.warn("file system watch queue overflowed; falling back to fstat", .{}); - markAllFilesDirty(w); - return true; + std.log.warn("file system watch queue overflowed; reconfiguring", .{}); + return error.MustReconfigure; } const fid: *align(1) fanotify.event_info_fid = @ptrCast(meta + 1); switch (fid.hdr.info_type) { @@ -166,9 +167,9 @@ const Os = switch (builtin.os.tag) { const lfh: FileHandle = .{ .handle = file_handle }; if (w.os.handle_table.getPtr(lfh)) |value| { if (value.reaction_set.getPtr(".")) |glob_set| - any_dirty = markStepSetDirty(maker, glob_set, any_dirty); + any_dirty = try markStepSetDirty(maker, glob_set, any_dirty); if (value.reaction_set.getPtr(file_name)) |step_set| - any_dirty = markStepSetDirty(maker, step_set, any_dirty); + any_dirty = try markStepSetDirty(maker, step_set, any_dirty); } }, else => |t| std.log.warn("unexpected fanotify event '{t}'", .{t}), @@ -304,8 +305,11 @@ const Os = switch (builtin.os.tag) { if (events_len == 0) return .timeout; for (w.os.poll_fds.values()) |poll_fd| { - if (poll_fd.revents & std.posix.POLL.IN == std.posix.POLL.IN and try markDirtySteps(w, poll_fd.fd)) + if (poll_fd.revents & std.posix.POLL.IN == std.posix.POLL.IN and + try markDirtySteps(w, poll_fd.fd)) + { return .dirty; + } } return .clean; } @@ -521,10 +525,8 @@ const Os = switch (builtin.os.tag) { var any_dirty = false; const bytes_returned = dir.iosb.Information; if (bytes_returned == 0) { - std.log.warn("file system watch queue overflowed; falling back to fstat", .{}); - markAllFilesDirty(w); - try dir.startListening(w); - return true; + std.log.warn("file system watch queue overflowed; reconfiguring", .{}); + return error.MustReconfigure; } var file_name_buf: [std.fs.max_path_bytes]u8 = undefined; var offset: usize = 0; @@ -532,9 +534,9 @@ const Os = switch (builtin.os.tag) { const notify: *windows.FILE.NOTIFY.INFORMATION = @ptrCast(@alignCast(&dir.buffer[offset])); const file_name = file_name_buf[0..std.unicode.wtf16LeToWtf8(&file_name_buf, notify.fileName())]; if (dir.reaction_set.getPtr(".")) |glob_set| - any_dirty = markStepSetDirty(maker, glob_set, any_dirty); + any_dirty = try markStepSetDirty(maker, glob_set, any_dirty); if (dir.reaction_set.getPtr(file_name)) |step_set| - any_dirty = markStepSetDirty(maker, step_set, any_dirty); + any_dirty = try markStepSetDirty(maker, step_set, any_dirty); if (notify.NextEntryOffset == 0) break; @@ -851,13 +853,13 @@ const Os = switch (builtin.os.tag) { // If we knew the basename of the changed file, here we would // mark only the step set dirty, and possibly the glob set: //if (reaction_set.getPtr(".")) |glob_set| - // any_dirty = markStepSetDirty(maker, glob_set, any_dirty); + // any_dirty = try markStepSetDirty(maker, glob_set, any_dirty); //if (reaction_set.getPtr(file_name)) |step_set| - // any_dirty = markStepSetDirty(maker, step_set, any_dirty); + // any_dirty = try markStepSetDirty(maker, step_set, any_dirty); // However we don't know the file name so just mark all the // sets dirty for this directory. for (reaction_set.values()) |*step_set| { - any_dirty = markStepSetDirty(maker, step_set, any_dirty); + any_dirty = try markStepSetDirty(maker, step_set, any_dirty); } } return any_dirty; @@ -915,31 +917,11 @@ pub const Match = struct { }; }; -fn markAllFilesDirty(w: *Watch) void { - const maker = w.maker; - - for (switch (builtin.os.tag) { - .windows => w.os.handle_table.keys(), - else => w.os.handle_table.values(), - }) |item| { - const reaction_set = switch (builtin.os.tag) { - .linux, .windows => item.reaction_set, - else => item, - }; - for (reaction_set.values()) |step_set| { - for (step_set.keys()) |step_index| { - const step = maker.stepByIndex(step_index); - _ = maker.invalidateResult(step); - } - } - } -} - -fn markStepSetDirty(maker: *Maker, step_set: *StepSet, any_dirty: bool) bool { +fn markStepSetDirty(maker: *Maker, step_set: *StepSet, any_dirty: bool) error{MustReconfigure}!bool { var this_any_dirty = false; for (step_set.keys()) |step_index| { const step = maker.stepByIndex(step_index); - if (maker.invalidateResult(step)) this_any_dirty = true; + if (try maker.invalidateResult(step)) this_any_dirty = true; } return any_dirty or this_any_dirty; } @@ -984,6 +966,7 @@ pub const WaitResult = enum { clean, }; +/// May return `error.MustReconfigure`. pub fn wait(w: *Watch, timeout: Timeout) !WaitResult { return Os.wait(w, timeout); } -- 2.54.0