From 3d785897658fd8b61660649b24d8943bda545982 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Mon, 4 May 2026 15:37:01 -0700 Subject: [PATCH] std.Build: port Fmt step to new system and integrate properly with LazyPath --- BRANCH_TODO | 16 +++++- build.zig | 4 +- lib/compiler/Maker/Step.zig | 2 +- lib/compiler/Maker/Step/Fmt.zig | 57 +++++++++++++++++++ lib/compiler/Maker/Step/InstallDir.zig | 2 +- lib/compiler/Maker/Step/Options.zig | 4 +- lib/std/Build.zig | 50 ++++++++++++----- lib/std/Build/Configuration.zig | 7 ++- lib/std/Build/Step/Fmt.zig | 78 ++++++++------------------ 9 files changed, 143 insertions(+), 77 deletions(-) create mode 100644 lib/compiler/Maker/Step/Fmt.zig diff --git a/BRANCH_TODO b/BRANCH_TODO index c51f87fd6916014f8f61687a37e4b21109fb5805..8f95fb7ec6a5b6cd97a3478d534188e8d8216944 100644 --- a/BRANCH_TODO +++ b/BRANCH_TODO @@ -27,10 +27,13 @@ - but artifact install steps also add paths for dyn libs on windows * no more "artifact arg" to run step. if you want to run the post-install binary, get the lazy path from the install step. - +* build system fmt step with check=false does not acquire a write lock on source files #35204 +* fmt step: import zig fmt code directly rather than child proc ## Release Notes +### Run Step: Passthru Args + In the Run step, passthru args are all together now, not observable in configure phase whether run args are provided. @@ -51,3 +54,14 @@ those arguments. In exchange, it means that when changing those arguments, build scripts no longer must be rebuilt from source. closes #31397 + +### Fmt Step: Options + +`paths` and `exclude_paths` are now LazyPath lists. There is a convenience method to create them: `b.pathList`. + +```diff +- const fmt_include_paths = &.{ "lib", "src", "test", "tools", "build.zig", "build.zig.zon" }; +- const fmt_exclude_paths = &.{ "test/cases", "test/behavior/zon" }; ++ const fmt_include_paths = b.pathList(&.{ "lib", "src", "test", "tools", "build.zig", "build.zig.zon" }); ++ const fmt_exclude_paths = b.pathList(&.{ "test/cases", "test/behavior/zon" }); +``` diff --git a/build.zig b/build.zig index 5400169113ebc1ffac82a3d54df215ea1238dc03..f539f8e0d9fa0b75b6e53e1664e992bb448784bc 100644 --- a/build.zig +++ b/build.zig @@ -427,8 +427,8 @@ pub fn build(b: *std.Build) !void { else null; - const fmt_include_paths = &.{ "lib", "src", "test", "tools", "build.zig", "build.zig.zon" }; - const fmt_exclude_paths = &.{ "test/cases", "test/behavior/zon" }; + const fmt_include_paths = b.pathList(&.{ "lib", "src", "test", "tools", "build.zig", "build.zig.zon" }); + const fmt_exclude_paths = b.pathList(&.{ "test/cases", "test/behavior/zon" }); const do_fmt = b.addFmt(.{ .paths = fmt_include_paths, .exclude_paths = fmt_exclude_paths, diff --git a/lib/compiler/Maker/Step.zig b/lib/compiler/Maker/Step.zig index d67ee25ebc839cfb89c7fd30134a004ba301882e..261310f726d9f6f0776c7e2b5b7def4689ae1d47 100644 --- a/lib/compiler/Maker/Step.zig +++ b/lib/compiler/Maker/Step.zig @@ -311,7 +311,7 @@ pub fn captureChildProcess( ) !std.process.RunResult { const gpa = maker.gpa; const graph = maker.graph; - const arena = graph.arena; + const arena = graph.arena; // TODO stop leaking into process arena const io = graph.io; // If an error occurs, it's happened in this command: diff --git a/lib/compiler/Maker/Step/Fmt.zig b/lib/compiler/Maker/Step/Fmt.zig new file mode 100644 index 0000000000000000000000000000000000000000..292a172ac1a4bf4c38da868a3f7467fbb40e6242 --- /dev/null +++ b/lib/compiler/Maker/Step/Fmt.zig @@ -0,0 +1,57 @@ +const Fmt = @This(); + +const std = @import("std"); +const Configuration = std.Build.Configuration; + +const Step = @import("../Step.zig"); +const Maker = @import("../../Maker.zig"); + +/// Persisted to reuse memory on subsequent calls to `make`. +argv: std.ArrayList([]const u8) = .empty, + +pub fn make( + fmt: *Fmt, + step_index: Configuration.Step.Index, + maker: *Maker, + progress_node: std.Progress.Node, +) Step.ExtendedMakeError!void { + const graph = maker.graph; + const step = maker.stepByIndex(step_index); + const gpa = maker.gpa; + const arena = graph.arena; // TODO don't leak into the process arena + const argv = &fmt.argv; + const conf = &maker.scanned_config.configuration; + const conf_step = step_index.ptr(conf); + const conf_fmt = conf_step.extended.get(conf.extra).fmt; + const paths = conf_fmt.paths.slice; + const exclude_paths = conf_fmt.paths.exclude_paths; + + argv.clearRetainingCapacity(); + try argv.ensureUnusedCapacity(gpa, 2 + 1 + paths.len + 2 * exclude_paths.len); + + argv.appendAssumeCapacity(graph.zig_exe); + argv.appendAssumeCapacity("fmt"); + + if (fmt.check) + argv.appendAssumeCapacity("--check"); + + for (fmt.paths) |lp| + argv.appendAssumeCapacity(try maker.resolveLazyPathIndexAbs(arena, lp, step_index)); + + for (fmt.exclude_paths) |lp| { + argv.appendAssumeCapacity("--exclude"); + argv.appendAssumeCapacity(try maker.resolveLazyPathIndexAbs(arena, lp, step_index)); + } + + const run_result = try step.captureChildProcess(maker, progress_node, argv.items); + if (fmt.check) switch (run_result.term) { + .exited => |code| if (code != 0 and run_result.stdout.len != 0) { + var it = std.mem.tokenizeScalar(u8, run_result.stdout, '\n'); + while (it.next()) |bad_file_name| { + try step.addError("{s}: non-conforming formatting", .{bad_file_name}); + } + }, + else => {}, + }; + try step.handleChildProcessTerm(maker, run_result.term); +} diff --git a/lib/compiler/Maker/Step/InstallDir.zig b/lib/compiler/Maker/Step/InstallDir.zig index fb5f288c68404ad3307c2293bf5dc8942efcfa6e..7d079380dd9158a79d4e194490f168a5512e7621 100644 --- a/lib/compiler/Maker/Step/InstallDir.zig +++ b/lib/compiler/Maker/Step/InstallDir.zig @@ -11,7 +11,7 @@ pub fn make( step_index: Configuration.Step.Index, maker: *Maker, progress_node: std.Progress.Node, -) !void { +) Step.ExtendedMakeError!void { const graph = maker.graph; const arena = maker.graph.arena; // TODO don't leak into process arena const io = graph.io; diff --git a/lib/compiler/Maker/Step/Options.zig b/lib/compiler/Maker/Step/Options.zig index dcdc49a4a0cc051cb67a64afd3fa20a88b098557..f6f3d532c2769be522dba1e0a456400e1f3c7d2a 100644 --- a/lib/compiler/Maker/Step/Options.zig +++ b/lib/compiler/Maker/Step/Options.zig @@ -7,12 +7,12 @@ const Step = @import("../Step.zig"); const Maker = @import("../../Maker.zig"); -fn make( +pub fn make( options: *Options, step_index: Configuration.Step.Index, maker: *Maker, progress_node: std.Progress.Node, -) !void { +) Step.ExtendedMakeError!void { // This step completes so quickly that no progress reporting is necessary. _ = progress_node; diff --git a/lib/std/Build.zig b/lib/std/Build.zig index 8bc4f6af407979e3f48feb69cd803520154065bd..be9d9940fa255e795401cd6de22490a5b87ffb7d 100644 --- a/lib/std/Build.zig +++ b/lib/std/Build.zig @@ -115,8 +115,7 @@ pub const Graph = struct { } pub fn dupeStrings(graph: *const Graph, strings: []const []const u8) []const []const u8 { - const arena = graph.arena; - const array = arena.alloc([]const u8, strings.len) catch @panic("OOM"); + const array = graph.alloc([]const u8, strings.len); for (array, strings) |*dest, source| dest.* = dupeString(graph, source); return array; } @@ -134,6 +133,21 @@ pub const Graph = struct { }, }; } + + /// Allocates using the global process arena, failing the build on + /// allocation failure. + pub fn alloc(graph: *const Graph, comptime T: type, n: usize) []T { + return graph.arena.allocAdvancedWithRetAddr(T, null, n, @returnAddress()) catch @panic("OOM"); + } + + /// Allocates using the global process arena, failing the build on + /// allocation failure. + pub fn create(graph: *const Graph, comptime T: type) *T { + return if (@sizeOf(T) == 0) + comptime @ptrFromInt(mem.alignBackward(usize, std.math.maxInt(usize), @alignOf(T))) + else + @ptrCast(graph.arena.allocBytesWithAlignment(.of(T), @sizeOf(T), @returnAddress()) catch @panic("OOM")); + } }; const AvailableDeps = []const struct { []const u8, []const u8 }; @@ -953,9 +967,10 @@ pub fn getUninstallStep(b: *Build) *Step { /// these options when calling the dependency's build.zig script as a function. /// `null` is returned when an option is left to default. pub fn option(b: *Build, comptime T: type, name_raw: []const u8, description_raw: []const u8) ?T { - const arena = b.allocator; - const name = b.dupe(name_raw); - const description = b.dupe(description_raw); + const graph = b.graph; + const arena = graph.arena; + const name = graph.dupeString(name_raw); + const description = graph.dupeString(description_raw); const type_id = comptime typeToEnum(T); const enum_options = if (type_id == .@"enum" or type_id == .enum_list) blk: { const EnumType = if (type_id == .enum_list) @typeInfo(T).pointer.child else T; @@ -1105,7 +1120,7 @@ pub fn option(b: *Build, comptime T: type, name_raw: []const u8, description_raw }, .list => |lst| { const Child = @typeInfo(T).pointer.child; - const new_list = arena.alloc(Child, lst.items.len) catch @panic("OOM"); + const new_list = graph.alloc(Child, lst.items.len); for (new_list, lst.items) |*new_item, str| { new_item.* = std.meta.stringToEnum(Child, str) orelse { log.err("expected -D{s} to be of type {s}", .{ name, @typeName(Child) }); @@ -1130,7 +1145,7 @@ pub fn option(b: *Build, comptime T: type, name_raw: []const u8, description_raw .scalar => |s| return arena.dupe(LazyPath, &[_]LazyPath{.{ .cwd_relative = s }}) catch @panic("OOM"), .lazy_path => |lp| return arena.dupe(LazyPath, &[_]LazyPath{lp}) catch @panic("OOM"), .list => |lst| { - const new_list = arena.alloc(LazyPath, lst.items.len) catch @panic("OOM"); + const new_list = graph.alloc(LazyPath, lst.items.len); for (new_list, lst.items) |*new_item, str| { new_item.* = .{ .cwd_relative = str }; } @@ -1553,7 +1568,7 @@ pub fn truncateFile(b: *Build, dest_path: []const u8) (Io.Dir.CreateDirError || /// References a file or directory relative to the source root. pub fn path(b: *Build, sub_path: []const u8) LazyPath { if (fs.path.isAbsolute(sub_path)) { - panic("sub_path is expected to be relative to the build root, but was this absolute path: '{s}'. It is best avoid absolute paths, but if you must, it is supported by LazyPath.cwd_relative", .{ + panic("sub_path is expected to be relative to the build root, but was this absolute path: '{s}'. Absolute paths can cause problems but can be created via Graph.cwdRelativePath", .{ sub_path, }); } @@ -1563,6 +1578,14 @@ pub fn path(b: *Build, sub_path: []const u8) LazyPath { } }; } +/// Creates a list of files and/or directories relative to the source root. +pub fn pathList(b: *Build, sub_paths: []const []const u8) []const LazyPath { + const graph = b.graph; + const result = graph.alloc(LazyPath, sub_paths.len); + for (result, sub_paths) |*d, s| d.* = path(b, s); + return result; +} + pub fn pathJoin(b: *Build, paths: []const []const u8) []u8 { return fs.path.join(b.allocator, paths) catch @panic("OOM"); } @@ -2022,10 +2045,11 @@ fn dependencyInner( pkg_deps: AvailableDeps, args: anytype, ) *Dependency { - const io = b.graph.io; - const arena = b.graph.arena; + const graph = b.graph; + const io = graph.io; + const arena = graph.arena; const user_input_options = userInputOptionsFromArgs(arena, args); - if (b.graph.dependency_cache.getContext(.{ + if (graph.dependency_cache.getContext(.{ .build_root_string = build_root_string, .user_input_options = user_input_options, }, .{ .allocator = arena })) |dep| return dep; @@ -2048,10 +2072,10 @@ fn dependencyInner( } } - const dep = arena.create(Dependency) catch @panic("OOM"); + const dep = graph.create(Dependency); dep.* = .{ .builder = sub_builder }; - b.graph.dependency_cache.putContext(b.graph.arena, .{ + graph.dependency_cache.putContext(arena, .{ .build_root_string = build_root_string, .user_input_options = user_input_options, }, dep, .{ .allocator = arena }) catch @panic("OOM"); diff --git a/lib/std/Build/Configuration.zig b/lib/std/Build/Configuration.zig index 2e81421313f2300092d7deaae15601a40807849f..fc172609a42be5bb6d65940d6a2007b8d3aeb886 100644 --- a/lib/std/Build/Configuration.zig +++ b/lib/std/Build/Configuration.zig @@ -1038,10 +1038,15 @@ pub const Step = extern struct { pub const Fmt = struct { flags: @This().Flags, + paths: Storage.FlagLengthPrefixedList(.flags, .paths, LazyPath.Index), + exclude_paths: Storage.FlagLengthPrefixedList(.flags, .exclude_paths, LazyPath.Index), pub const Flags = packed struct(u32) { tag: Tag = .fmt, - _: u27 = 0, + paths: bool, + exclude_paths: bool, + check: bool, + _: u24 = 0, }; }; diff --git a/lib/std/Build/Step/Fmt.zig b/lib/std/Build/Step/Fmt.zig index bca5385a541ca992e4429e2e64b945ddcf8dfc4e..68f31e36d7fed6c6d39d7053bdcc22feb56d1636 100644 --- a/lib/std/Build/Step/Fmt.zig +++ b/lib/std/Build/Step/Fmt.zig @@ -1,81 +1,47 @@ //! This step has two modes: //! * Modify mode: directly modify source files, formatting them in place. //! * Check mode: fail the step if a non-conforming file is found. +const Fmt = @This(); + const std = @import("std"); const Step = std.Build.Step; -const Fmt = @This(); +const LazyPath = std.Build.LazyPath; +const Configuration = std.Build.Configuration; step: Step, -paths: []const []const u8, -exclude_paths: []const []const u8, +/// Intended to be read-only after the `Fmt` step is created. +paths: []const LazyPath, +/// Intended to be read-only after the `Fmt` step is created. +exclude_paths: []const LazyPath, check: bool, pub const base_tag: Step.Tag = .fmt; pub const Options = struct { - paths: []const []const u8 = &.{}, - exclude_paths: []const []const u8 = &.{}, + paths: []const LazyPath = &.{}, + exclude_paths: []const LazyPath = &.{}, /// If true, fails the build step when any non-conforming files are encountered. check: bool = false, }; pub fn create(owner: *std.Build, options: Options) *Fmt { - const fmt = owner.allocator.create(Fmt) catch @panic("OOM"); - const name = if (options.check) "zig fmt --check" else "zig fmt"; + const graph = owner.graph; + const arena = graph.arena; + const fmt = arena.create(Fmt) catch @panic("OOM"); + fmt.* = .{ - .step = Step.init(.{ + .step = .init(.{ .tag = base_tag, - .name = name, + .name = if (options.check) "zig fmt --check" else "zig fmt", .owner = owner, - .makeFn = make, }), - .paths = owner.dupeStrings(options.paths), - .exclude_paths = owner.dupeStrings(options.exclude_paths), + .paths = options.paths, + .exclude_paths = options.exclude_paths, .check = options.check, }; + + for (options.paths) |lp| lp.addStepDependencies(&fmt.step); + for (options.exclude_paths) |lp| lp.addStepDependencies(&fmt.step); + return fmt; } - -fn make(step: *Step, options: Step.MakeOptions) !void { - const prog_node = options.progress_node; - - // TODO: if check=false, this means we are modifying source files in place, which - // is an operation that could race against other operations also modifying source files - // in place. In this case, this step should obtain a write lock while making those - // modifications. - - const b = step.owner; - const arena = b.allocator; - const fmt: *Fmt = @fieldParentPtr("step", step); - - var argv: std.ArrayList([]const u8) = .empty; - try argv.ensureUnusedCapacity(arena, 2 + 1 + fmt.paths.len + 2 * fmt.exclude_paths.len); - - argv.appendAssumeCapacity(b.graph.zig_exe); - argv.appendAssumeCapacity("fmt"); - - if (fmt.check) { - argv.appendAssumeCapacity("--check"); - } - - for (fmt.paths) |p| { - argv.appendAssumeCapacity(b.pathFromRoot(p)); - } - - for (fmt.exclude_paths) |p| { - argv.appendAssumeCapacity("--exclude"); - argv.appendAssumeCapacity(b.pathFromRoot(p)); - } - - const run_result = try step.captureChildProcess(options.gpa, prog_node, argv.items); - if (fmt.check) switch (run_result.term) { - .exited => |code| if (code != 0 and run_result.stdout.len != 0) { - var it = std.mem.tokenizeScalar(u8, run_result.stdout, '\n'); - while (it.next()) |bad_file_name| { - try step.addError("{s}: non-conforming formatting", .{bad_file_name}); - } - }, - else => {}, - }; - try step.handleChildProcessTerm(run_result.term); -} -- 2.54.0