authorgravatar for mlugg@mlugg.co.ukMatthew Lugg <mlugg@mlugg.co.uk> 2025-11-13 09:46:57+00:00
committergravatar for alex@alexrp.comAlex Rønne Petersen <alex@alexrp.com> 2025-11-14 21:50:24+01:00
logc6b5945356568f6ec70ca00f9a844bd180f8ca62
tree24d4350469cf7fae42cbbabd697eb43b65e90640
parentb38fb4bff31b76cbaa27157784139a71e290f2e9

std.Build: don't force all children to inherit color option

The build runner was previously forcing child processes to have their stderr colorization match the build runner by setting `CLICOLOR_FORCE` or `NO_COLOR`. This is a nice idea in some cases---for instance a simple `Run` step which we just expect to exit with code 0 and whose stderr is not being programmatically inspected---but is a bad idea in others, for instance if there is a check on stderr or if stderr is captured, in which case forcing color on the child could cause checks to fail. Instead, this commit adds a field to `std.Build.Step.Run` which specifies a behavior for the build runner to employ in terms of assigning the `CLICOLOR_FORCE` and `NO_COLOR` environment variables. The default behavior is to set `CLICOLOR_FORCE` if the build runner's output is colorized and the step's stderr is not captured, and to set `NO_COLOR` otherwise. Alternatively, colors can be always enabled, always disabled, always match the build runner, or the environment variables can be left untouched so they can be manually controlled through `env_map`. Notably, this fixes a failure when running `zig build test-cli` in a TTY (or with colors explicitly enabled). GitHub CI hadn't caught this because it does not request color, but Codeberg CI now does, and we were seeing a failure in the `zig init` test because the actual output had color escape codes in it due to 6d280dc.

6 files changed, 76 insertions(+), 19 deletions(-)

lib/compiler/build_runner.zig+1-5
......@@ -443,11 +443,6 @@ pub fn main() !void {
443443 }
444444
445445 const ttyconf = color.detectTtyConf();
446 switch (ttyconf) {
447 .no_color => try graph.env_map.put("NO_COLOR", "1"),
448 .escape_codes => try graph.env_map.put("CLICOLOR_FORCE", "1"),
449 .windows_api => {},
450 }
451446
452447 const main_progress_node = std.Progress.start(.{
453448 .disable_printing = (color == .off),
......@@ -1389,6 +1384,7 @@ fn workerMakeOneStep(
13891384 .thread_pool = thread_pool,
13901385 .watch = run.watch,
13911386 .web_server = if (run.web_server) |*ws| ws else null,
1387 .ttyconf = run.ttyconf,
13921388 .unit_test_timeout_ns = run.unit_test_timeout_ns,
13931389 .gpa = run.gpa,
13941390 });
lib/std/Build/Step.zig+1
......@@ -118,6 +118,7 @@ pub const MakeOptions = struct {
118118 // it currently breaks because `std.net.Address` doesn't work there. Work around for now.
119119 .wasm32 => void,
120120 },
121 ttyconf: std.Io.tty.Config,
121122 /// If set, this is a timeout to enforce on all individual unit tests, in nanoseconds.
122123 unit_test_timeout_ns: ?u64,
123124 /// Not to be confused with `Build.allocator`, which is an alias of `Build.graph.arena`.
lib/std/Build/Step/Run.zig+55-10
......@@ -24,6 +24,21 @@ cwd: ?Build.LazyPath,
2424/// Override this field to modify the environment, or use setEnvironmentVariable
2525env_map: ?*EnvMap,
2626
27/// Controls the `NO_COLOR` and `CLICOLOR_FORCE` environment variables.
28color: enum {
29 /// `CLICOLOR_FORCE` is set, and `NO_COLOR` is unset.
30 enable,
31 /// `NO_COLOR` is set, and `CLICOLOR_FORCE` is unset.
32 disable,
33 /// If the build runner is using color, equivalent to `.enable`. Otherwise, equivalent to `.disable`.
34 inherit,
35 /// If stderr is captured or checked, equivalent to `.disable`. Otherwise, equivalent to `.inherit`.
36 auto,
37 /// The build runner does not modify the `CLICOLOR_FORCE` or `NO_COLOR` environment variables.
38 /// They are treated like normal variables, so can be controlled through `setEnvironmentVariable`.
39 manual,
40} = .auto,
41
2742/// When `true` prevents `ZIG_PROGRESS` environment variable from being passed
2843/// to the child process, which otherwise would be used for the child to send
2944/// progress updates to the parent.
......@@ -525,7 +540,7 @@ pub fn setCwd(run: *Run, cwd: Build.LazyPath) void {
525540pub fn clearEnvironment(run: *Run) void {
526541 const b = run.step.owner;
527542 const new_env_map = b.allocator.create(EnvMap) catch @panic("OOM");
528 new_env_map.* = EnvMap.init(b.allocator);
543 new_env_map.* = .init(b.allocator);
529544 run.env_map = new_env_map;
530545}
531546
......@@ -806,6 +821,9 @@ fn make(step: *Step, options: Step.MakeOptions) !void {
806821 }
807822 }
808823
824 man.hash.add(run.color);
825 man.hash.add(run.disable_zig_progress);
826
809827 for (run.argv.items) |arg| {
810828 switch (arg) {
811829 .bytes => |bytes| {
......@@ -1130,6 +1148,7 @@ pub fn rerunInFuzzMode(
11301148 .thread_pool = undefined, // not used by `runCommand`
11311149 .watch = undefined, // not used by `runCommand`
11321150 .web_server = null, // only needed for time reports
1151 .ttyconf = fuzz.ttyconf,
11331152 .unit_test_timeout_ns = null, // don't time out fuzz tests for now
11341153 .gpa = undefined, // not used by `runCommand`
11351154 }, .{
......@@ -1234,9 +1253,40 @@ fn runCommand(
12341253 var interp_argv = std.array_list.Managed([]const u8).init(b.allocator);
12351254 defer interp_argv.deinit();
12361255
1237 var env_map = run.env_map orelse &b.graph.env_map;
1256 var env_map: EnvMap = env: {
1257 const orig = run.env_map orelse &b.graph.env_map;
1258 break :env try orig.clone(gpa);
1259 };
1260 defer env_map.deinit();
1261
1262 color: switch (run.color) {
1263 .manual => {},
1264 .enable => {
1265 try env_map.put("CLICOLOR_FORCE", "1");
1266 env_map.remove("NO_COLOR");
1267 },
1268 .disable => {
1269 try env_map.put("NO_COLOR", "1");
1270 env_map.remove("CLICOLOR_FORCE");
1271 },
1272 .inherit => switch (options.ttyconf) {
1273 .no_color, .windows_api => continue :color .disable,
1274 .escape_codes => continue :color .enable,
1275 },
1276 .auto => {
1277 const capture_stderr = run.captured_stderr != null or switch (run.stdio) {
1278 .check => |checks| checksContainStderr(checks.items),
1279 .infer_from_args, .inherit, .zig_test => false,
1280 };
1281 if (capture_stderr) {
1282 continue :color .disable;
1283 } else {
1284 continue :color .inherit;
1285 }
1286 },
1287 }
12381288
1239 const opt_generic_result = spawnChildAndCollect(run, argv, env_map, has_side_effects, options, fuzz_context) catch |err| term: {
1289 const opt_generic_result = spawnChildAndCollect(run, argv, &env_map, has_side_effects, options, fuzz_context) catch |err| term: {
12401290 // InvalidExe: cpu arch mismatch
12411291 // FileNotFound: can happen with a wrong dynamic linker path
12421292 if (err == error.InvalidExe or err == error.FileNotFound) interpret: {
......@@ -1273,12 +1323,7 @@ fn runCommand(
12731323 // Wine's excessive stderr logging is only situationally helpful. Disable it by default, but
12741324 // allow the user to override it (e.g. with `WINEDEBUG=err+all`) if desired.
12751325 if (env_map.get("WINEDEBUG") == null) {
1276 // We don't own `env_map` at this point, so create a copy in order to modify it.
1277 const new_env_map = arena.create(EnvMap) catch @panic("OOM");
1278 new_env_map.hash_map = try env_map.hash_map.cloneWithAllocator(arena);
1279 try new_env_map.put("WINEDEBUG", "-all");
1280
1281 env_map = new_env_map;
1326 try env_map.put("WINEDEBUG", "-all");
12821327 }
12831328 } else {
12841329 return failForeign(run, "-fwine", argv[0], exe);
......@@ -1377,7 +1422,7 @@ fn runCommand(
13771422 step.result_failed_command = null;
13781423 try Step.handleVerbose2(step.owner, cwd, run.env_map, interp_argv.items);
13791424
1380 break :term spawnChildAndCollect(run, interp_argv.items, env_map, has_side_effects, options, fuzz_context) catch |e| {
1425 break :term spawnChildAndCollect(run, interp_argv.items, &env_map, has_side_effects, options, fuzz_context) catch |e| {
13811426 if (!run.failing_to_execute_foreign_is_an_error) return error.MakeSkipped;
13821427 if (e == error.MakeFailed) return error.MakeFailed; // error already reported
13831428 return step.fail("unable to spawn interpreter {s}: {s}", .{
lib/std/Io/tty.zig+2-4
......@@ -37,7 +37,7 @@ pub const Color = enum {
3737pub const Config = union(enum) {
3838 no_color,
3939 escape_codes,
40 windows_api: if (native_os == .windows) WindowsContext else void,
40 windows_api: if (native_os == .windows) WindowsContext else noreturn,
4141
4242 /// Detect suitable TTY configuration options for the given file (commonly stdout/stderr).
4343 /// This includes feature checks for ANSI escape codes and the Windows console API, as well as
......@@ -105,7 +105,7 @@ pub const Config = union(enum) {
105105 };
106106 try w.writeAll(color_string);
107107 },
108 .windows_api => |ctx| if (native_os == .windows) {
108 .windows_api => |ctx| {
109109 const attributes = switch (color) {
110110 .black => 0,
111111 .red => windows.FOREGROUND_RED,
......@@ -130,8 +130,6 @@ pub const Config = union(enum) {
130130 };
131131 try w.flush();
132132 try windows.SetConsoleTextAttribute(ctx.handle, attributes);
133 } else {
134 unreachable;
135133 },
136134 };
137135 }
lib/std/process.zig+16
......@@ -206,6 +206,22 @@ pub const EnvMap = struct {
206206 return self.hash_map.iterator();
207207 }
208208
209 /// Returns a full copy of `em` allocated with `gpa`, which is not necessarily
210 /// the same allocator used to allocate `em`.
211 pub fn clone(em: *const EnvMap, gpa: Allocator) Allocator.Error!EnvMap {
212 var new: EnvMap = .init(gpa);
213 errdefer new.deinit();
214 // Since we need to dupe the keys and values, the only way for error handling to not be a
215 // nightmare is to add keys to an empty map one-by-one. This could be avoided if this
216 // abstraction were a bit less... OOP-esque.
217 try new.hash_map.ensureUnusedCapacity(em.hash_map.count());
218 var it = em.hash_map.iterator();
219 while (it.next()) |entry| {
220 try new.put(entry.key_ptr.*, entry.value_ptr.*);
221 }
222 return new;
223 }
224
209225 fn free(self: EnvMap, value: []const u8) void {
210226 self.hash_map.allocator.free(value);
211227 }
test/standalone/empty_env/build.zig+1
......@@ -31,6 +31,7 @@ pub fn build(b: *std.Build) void {
3131 const run = b.addRunArtifact(main);
3232 run.clearEnvironment();
3333 run.disable_zig_progress = true;
34 run.color = .manual;
3435
3536 test_step.dependOn(&run.step);
3637}