Skip to content

std.io.tty: cleanup detectConfig#16110

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

Merged
andrewrk merged 2 commits into from
Jun 22, 2023
Merged

std.io.tty: cleanup detectConfig #16110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e054126
std.io.tty: add detectTtyConfigForce for forcing color
Jun 20, 2023
e45d24c
rename ZIG_DEBUG_COLOR env variable to YES_COLOR
Jun 22, 2023
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
2 changes: 1 addition & 1 deletion doc/docgen.zig
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1305,7 +1305,7 @@ fn genHtml(
defer root_node.end();

var env_map = try process.getEnvMap(allocator);
try env_map.put("ZIG_DEBUG_COLOR", "1");
try env_map.put("YES_COLOR", "1");

const host = try std.zig.system.NativeTargetInfo.detect(.{});
const builtin_code = try getBuiltinCode(allocator, &env_map, zig_exe, opt_zig_lib_dir);
Expand Down
2 changes: 1 addition & 1 deletion lib/build_runner.zig
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -282,7 +282,7 @@ pub fn main() !void {
const ttyconf = get_tty_conf(color, stderr);
switch (ttyconf) {
.no_color => try builder.env_map.put("NO_COLOR", "1"),
.escape_codes => try builder.env_map.put("ZIG_DEBUG_COLOR", "1"),
.escape_codes => try builder.env_map.put("YES_COLOR", "1"),
.windows_api => {},
}

Expand Down
33 changes: 19 additions & 14 deletions lib/std/io/tty.zig
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,29 +7,34 @@ const native_os = builtin.os.tag;

/// Detect suitable TTY configuration options for the given file (commonly stdout/stderr).
/// This includes feature checks for ANSI escape codes and the Windows console API, as well as
/// respecting the `NO_COLOR` environment variable.
/// respecting the `NO_COLOR` and `YES_COLOR` environment variables to override the default.
pub fn detectConfig(file: File) Config {
if (builtin.os.tag == .wasi) {
// Per https://github.com/WebAssembly/WASI/issues/162 ANSI codes
// aren't currently supported.
return .no_color;
Comment on lines -12 to -15

@dweillerdweillerJun 21, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally had this behaviour (forcing colour on WASI returning .escape_codes but I think the least surprising behaviour is for detectConfigForce to not output escapes codes on platforms that don't support them. If someone actually wants them in an environment that doesn't support them they should just not call detectConfig*.

@andrewrkandrewrkJun 22, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When color is forced on, it is irrelevant whether a platform supports it or not. The intention of forcing it on is that the output stream will be interpreted to have escape codes in them, and handled appropriately. This can be done on WASI, or Windows, or any platform.

} else if (process.hasEnvVarConstant("ZIG_DEBUG_COLOR")) {
return .escape_codes;
} else if (process.hasEnvVarConstant("NO_COLOR")) {
return .no_color;
} else if (file.supportsAnsiEscapeCodes()) {
return .escape_codes;
} else if (native_os == .windows and file.isTty()) {
const force_color: ?bool = if (builtin.os.tag == .wasi)
null // wasi does not support environment variables
else if (process.hasEnvVarConstant("NO_COLOR"))
false
else if (process.hasEnvVarConstant("YES_COLOR"))
true
else
null;

if (force_color == false) return .no_color;

if (native_os == .windows and file.isTty()) {
var info: windows.CONSOLE_SCREEN_BUFFER_INFO = undefined;
if (windows.kernel32.GetConsoleScreenBufferInfo(file.handle, &info) != windows.TRUE) {
// TODO: Should this return an error instead?
return .no_color;
return if (force_color == true) .escape_codes else .no_color;
Comment on lines 24 to +26

@dweillerdweillerJun 21, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't actually sure how windows works with colour so I left the previous behaviour. If windows would support escape codes here this seems fine, if not similar to WASI I'd suggest returning .no_color unconditionally.

}
return .{ .windows_api = .{
.handle = file.handle,
.reset_attributes = info.wAttributes,
} };
}

if (force_color == true or file.supportsAnsiEscapeCodes()) {
return .escape_codes;
}

return .no_color;
}

Expand Down
2 changes: 1 addition & 1 deletion test/src/StackTrace.zig
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -81,7 +81,7 @@ fn addExpect(
});

const run = b.addRunArtifact(exe);
run.removeEnvironmentVariable("ZIG_DEBUG_COLOR");
run.removeEnvironmentVariable("YES_COLOR");
run.setEnvironmentVariable("NO_COLOR", "1");
run.expectExitCode(1);
run.expectStdOutEqual("");
Expand Down