Enable colors in the native Windows console #13147

nojb · 2024-05-06T06:58:04Z

This PR enables colors in the native Windows console (currently on Windows we support colors only when running under Cygwin, see #1406). The PR follows related discussion in the context of adding Windows support to @dbuenzli's toplevel upgrade, Down (see https://discuss.ocaml.org/t/ann-down-0-2-0-and-omod-0-4-0/14380/11 and dbuenzli/down#35). See also https://learn.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences.

In the future, we may want to expose this functionality in the standard library, or even enable it by default for programs produced by the compiler, but all that is for later.

cc @dra27 @MisterDA as our Windows experts, but any and all are welcome to take a look. Thanks!

runtime/win32.c

Co-authored-by: Miod Vallat <118974489+dustanddreams@users.noreply.github.com>

dbuenzli

I left a few comments. I didn't make individual comment but @nojb, note that Val_true and Val_false are a thing in case you want to use them.

dbuenzli · 2024-05-06T07:15:44Z

runtime/sys.c

+#ifdef _WIN32
+  return caml_win32_is_ansi_capable(chan);
+#else
+  return Val_bool(0);


Shouldn't that be Val_true on other platforms ? This makes it possible to have logics that doesn't even has to condition on Sys.win32.

dbuenzli · 2024-05-06T07:16:19Z

runtime/sys.c

+#ifdef _WIN32
+  return caml_win32_set_ansi_capable(chan, set);
+#else
+  return Val_bool(0);


dbuenzli · 2024-05-06T07:23:14Z

utils/misc.ml

@@ -549,13 +549,21 @@ let ordinal_suffix n =
 (* Color support handling *)
 module Color = struct
  external isatty : out_channel -> bool = "caml_sys_isatty"
+  external is_ansi_capable : out_channel -> bool = "caml_is_ansi_capable"
+  external set_ansi_capable : out_channel -> bool -> bool = "caml_set_ansi_capable"


This should perhaps clarify that it returns true on sucesss (and not the old setting value).

Fixed, thanks.

dbuenzli · 2024-05-06T07:26:40Z

utils/misc.ml

-    && term <> ""
-    && isatty stderr
+    (term <> "dumb" && term <> "" && isatty oc) ||
+    (Sys.win32 && enable_win32_vt oc)


Perhaps Windows users will have a better say on this but why not have:

(term <> "dumb" && Sys.win32 && enable_win32_vt oc)

The reason is that the TERM variable is not typically set or used in the native Windows console. The code would still work (because the Not_found case above gets translated into ""), but it would be misleading in my opinion.

I know, that's the reason why I didn't suggest term <> "". The reasoning is as follows.

But then once we are in Windows vt mode, I expect the program to behave like it would do on unix when it interacts with a tty (a nice touch from Microsoft would have been to have set the process' TERM environment variable to a suitable value when the mode is enabled).

And these programs interacting with ttys disable ANSI output when the environment variable TERM is dumb.

This gives a way for windows users to disable coloring the standard way you would do on Unix.

I made this change in the last commit even if I am not 100% convinced it is a good idea (but don't have strong arguments against either).

I'm not a windows user, I think they should chime in to say what they think.

I'm not terribly convinced, to be honest. TERM is part of Posix; VT100 is not - there is the part where we work out if we want coloured output (cf. NO_COLOR etc. in #10560) and then the part where we work out how to make those colours happen. On Unix, having decided we want to display in colour, we follow its standards and inspect TERM (which may cause us to discover that the terminal does not support colour) but on Windows, having decided we want to display, we follow its standards and select between either using the Windows API (which we have never done, but opam has) or using by the same API to indicate that we'd like VT100 enabled.

I'm all for cross-platform, but this seems unnecessarily Unix-ish on Windows for me!

I don't mind, you be the judge.

But note for use in a more general setting: people are confusing ANSI escapes with colors, but they provide much more than that (not to mention that the NO_COLOR "standard" was a bad idea to start with, aka solving the problem at the wrong level).

It seems to me that you'd still want a user mecanism on Windows to be able to tell the program "please don't treat that as an ANSI enabled terminal", on Unix setting TERM=dumb works for that, what is is the equivalent on Windows ?

Agreed on the ANSI escapes. It's not necessarily fully coherent, but having given a Windows program the handle for the console, it is (from a native Windows perspective) strange to me to expect to be able to say "please don't do anything with it". I'd expect that the Windows equivalent of TERM=dumb would rather be not connecting stdout to a console in the first place. The bit that feels strange is that that TERM is meant to control what is written to the console, but there's still no reason not to control the Windows console using its API (it's neither deprecated nor particularly recommended that all programs start using VT100), and it certainly feels strange to have "TERM=dumb" starting to mean "don't use these syscalls please".

Slightly more coherently, it definitely does feel incorrect that we unilaterally adopt the interpretation of TERM this way (i.e. doing this with a lack of a standard) 👨‍⚖️

(FWIW when I first wrote that emulation in opam in 2014 dra27/opam@02e86b4 the support was indeed unconditional; apparently when I rebased it in 2015 past #1882, which introduced the TERM=dumb check, I was not concentrating in dra27/opam@91c4a90, although there were a lot of commits to rebase back in those days...)

nojb · 2024-05-06T07:44:19Z

note that Val_true and Val_false are a thing in case you want to use them.

Indeed, amended as suggested. Thanks.

gasche

I'm not fond of the current API where you expose an API in the runtime that looks OS-agnostic but in fact has weird/nonsensical return values on non-Windows systems.

If we want to expose this in the runtime/stdlib for other programs than the compiler, I would prefer to have a function in Sys that does the right thing (or at least what we believe to be the right thing), and hide the runtime primitives as a non-portable implementation detail -- it could keep win32 in the name and fail on non-Windows systems.

nojb · 2024-05-06T10:19:52Z

I'm not fond of the current API where you expose an API in the runtime that looks OS-agnostic but in fact has weird/nonsensical return values on non-Windows systems.

Good point. In the last commit I tweaked the runtime functions to make it clear they are win32-only. If we wanted to expose something in Sys, it should be essentially the function Misc.Color.should_enable_color I think (but I am not proposing to do so here).

MisterDA

LGTM, thanks!

runtime/win32.c

Co-authored-by: Antonin Décimo <antonin.decimo@gmail.com>

gasche

I am fine with the new API (with the understanding that it is not designed to make stdlib users life easier, the easy-to-use logic is only in compiler-libs). With the addition of @MisterDA looking at the Windows code I am happy to approve.

dra27 · 2024-05-09T15:05:03Z

I haven't checked that it still happens, but especially given that ocamlopt calls Cygwin executables (the assembler and the C compiler), I'd be concerned that we may hit the same thing as ocaml/opam#3897. It's a while since I wrote that console code, so both my memory be hazy and the console may have changed again, but the point is that VT100 mode isn't enabled at the fd (or even HANDLE) level - it's actually a setting of the physical console window and is therefore shared (and can be overwritten by other processes).

The code in opam is complicated by having legacy support for the pre-VT100 console, but IIRC it boils down to having to set VT100 prior to every VT100 sequence being sent - or the very least needing to reset it after calling Sys.command.

nojb · 2024-05-09T19:23:32Z

IIRC it boils down to having to set VT100 prior to every VT100 sequence being sent - or the very least needing to reset it after calling Sys.command.

You are right, I forgot about this; we got bitten by this in Down as well: https://github.com/dbuenzli/down/blob/e3a4f38e02bf75fd0206d97dc74c6a5c26342023/src/down_std.ml#L261-L263.

I'll think about it to see what is the most painless way to go about it.

Win32: enable colors in the native Windows console

693194b

nojb added the windows label May 6, 2024

dustanddreams reviewed May 6, 2024

View reviewed changes

runtime/win32.c Outdated Show resolved Hide resolved

Update runtime/win32.c

fb08ee0

Co-authored-by: Miod Vallat <118974489+dustanddreams@users.noreply.github.com>

dbuenzli reviewed May 6, 2024

View reviewed changes

Use Val_false, Val_true

d9bd20b

Doc

15dc66f

gasche reviewed May 6, 2024

View reviewed changes

Make it clear API is Win32-only

3a0ab18

nojb added 3 commits May 6, 2024 11:21

Fix

b253aa6

Fix

2ba2351

check-typo

238b650

MisterDA approved these changes May 7, 2024

View reviewed changes

runtime/win32.c Outdated Show resolved Hide resolved

Update runtime/win32.c

9ab28d9

Co-authored-by: Antonin Décimo <antonin.decimo@gmail.com>

gasche approved these changes May 7, 2024

View reviewed changes

gasche assigned dra27 May 15, 2024

gasche added enhancement submitter-action-needed This PR is waiting for an action of its submitter. labels May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable colors in the native Windows console #13147

Enable colors in the native Windows console #13147

nojb commented May 6, 2024 •

edited

dbuenzli left a comment

dbuenzli May 6, 2024

dbuenzli May 6, 2024

dbuenzli May 6, 2024

nojb May 6, 2024

dbuenzli May 6, 2024 •

edited

nojb May 6, 2024

dbuenzli May 6, 2024

nojb May 6, 2024

dbuenzli May 6, 2024

dra27 May 9, 2024

dbuenzli May 9, 2024

dra27 May 14, 2024

nojb commented May 6, 2024

gasche left a comment

nojb commented May 6, 2024

MisterDA left a comment

gasche left a comment

dra27 commented May 9, 2024

nojb commented May 9, 2024

Enable colors in the native Windows console #13147

Are you sure you want to change the base?

Enable colors in the native Windows console #13147

Conversation

nojb commented May 6, 2024 • edited

dbuenzli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbuenzli May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojb commented May 6, 2024

gasche left a comment

Choose a reason for hiding this comment

nojb commented May 6, 2024

MisterDA left a comment

Choose a reason for hiding this comment

gasche left a comment

Choose a reason for hiding this comment

dra27 commented May 9, 2024

nojb commented May 9, 2024

nojb commented May 6, 2024 •

edited

dbuenzli May 6, 2024 •

edited