Add color option for disabling ANSI escape codes

[tenginkai]

Motivation

Evaluation errors from nix-expr-c are not in text form, they are interspersed with ANSI terminal control sequences.

This commit adds a build-time option to completely remove coloring, so the source doesn't need to be patched downstream to fix this issue.

Context

Fix #14017

---

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[xokdvium]

I wonder if that should just be configurable via a new function that gets the error message? We already do conditional ansi escape filtering. Making this a compile-time parameter seems like an unnecessary sledgehammer.

[tenginkai]

I wonder if that should just be configurable via a new function that gets the error message?

That is obviously possible. However it would be too complicated to pass a review.

We already do conditional ansi escape filtering. Making this a compile-time parameter seems like an unnecessary sledgehammer.

You haven't looked at the relevant code.

[xokdvium]

That is obviously possible. However it would be too complicated to pass a review.

I don't see how. It would be akin to adding a new function like nix_err_info_msg that has it configurable and we already have filterANSIEscapes function. Something like:

diff --git a/src/libutil-c/nix_api_util.cc b/src/libutil-c/nix_api_util.cc
index a2f7710bc..6246a2c12 100644
--- a/src/libutil-c/nix_api_util.cc
+++ b/src/libutil-c/nix_api_util.cc
@@ -1,6 +1,7 @@
 #include "nix_api_util.h"
 #include "nix/util/config-global.hh"
 #include "nix/util/error.hh"
+#include "nix/util/terminal.hh"
 #include "nix_api_util_internal.h"
 #include "nix/util/util.hh"
 
@@ -144,7 +145,8 @@ nix_err nix_err_info_msg(
     if (read_context->last_err_code != NIX_ERR_NIX_ERROR) {
         return nix_set_err_msg(context, NIX_ERR_UNKNOWN, "Last error was not a nix error");
     }
-    return call_nix_get_string_callback(read_context->info->msg.str(), callback, user_data);
+    return call_nix_get_string_callback(
+        nix::filterANSIEscapes(read_context->info->msg.str(), /*filterAll=*/true), callback, user_data);
 }
 
 nix_err nix_err_code(const nix_c_context * read_context)

(But a new function with a flag to make this configurable)

[tenginkai]

@xokdvium You don't understand. I want to disable color, I'm not interested in importing an ANSI escape sequence parser and run all my errors through it. A half dozen "conditional" solutions to the color problem have already been proposed.

[xokdvium]

I'm not interested in importing an ANSI escape sequence parser and run all my errors through it

I'm not suggesting that you "import" the parser. It would be implemented in the nix_api_util and it would be exposed the same way error message getter is implemented now.

[xokdvium]

This meson.build is only used for the dev shell.

[xokdvium]

This won't do anything, since the top-level meson.options is only used for the dev-shell (and other packaging approaches by downstream distros).

[tenginkai]

I'm aware, this is for other packaging approaches.

[tenginkai]

I'm not suggesting that you "import" the parser. It would be implemented in the nix_api_util and it would be exposed the same way error message getter is implemented now.

My contention is that putting any ANSI escaping parsing in nix_api_util is sloppy engineering.

[xokdvium]

Doing this roundtrip with putting and then stripping ansi escapes is rather suboptimal, but that's the reality of the codebase. Fixing this isn't exactly worth the trouble. But a compile-time parameter that doesn't get built in any CI is even worse from the maintenance perspective and is bound to bitrot.

in nix_api_util

It already links with libutil so it's already available anyway. It's also used all over place in libexpr, libmain and libutil itself.

[tenginkai]

Doing this roundtrip with putting and then stripping ansi escapes is rather suboptimal, but that's the reality of the codebase. Fixing this isn't exactly worth the trouble. But a compile-time parameter that doesn't get built in any CI is even worse from the maintenance perspective and is bound to bitrot.

I get it. Upstream doesn't care about this issue. All tests assume the error outputs are color-barfed and upstream CI isn't going to actually test solutions in any case. Hence this PR is the minimally invasive solution. It's really not that much for you to read and understand. There are zero nested conditional cases that would not be hit by CI.

[roberth]

The current situation, where ANSI color codes are the internal format, is a pragmatic choice that gets the job done.
I wouldn't call it sloppy engineering, but exactly the result of minimally invasive solutions, of which a project can only bear a finite number.

Relatedly, there's some support for more structured error messages, certainly from a Nix language perspective.
Might be worth picking or defining an internal markup representation, but that's more something for the Nix team to decide.

If there's an internal representation for this stuff, that would resolve this argument by adding a second plain-text rendering function to the C API.
Going from a structured format to text when needed is most certainly not sloppy engineering :)