ninja-build / ninja

a small build system with a focus on speed
https://ninja-build.org/
Apache License 2.0
11.22k stars 1.6k forks source link

clang doesn't print colored diagnostics when invoked from ninja #174

Closed nico closed 12 years ago

nico commented 12 years ago

The reason for this is that ninja sets subprocess stdout/stderr to a pipe (Subprocess::Start(), subprocess.cc), and clang checks if StandardErrHasColors() (tools/clang/lib/Driver/Tools.cpp), which is false if !isatty(2) (lib/Support/Unix/Process.inc).

I looked around a bit, and the way to do this seems to be to call fork_pty() to run the child in a pseudo terminal. I don't know if this would impact subprocess creation time, and if opening ~4000 pseudo ttys (a chrome build at -j10000) is considered good form.

(It's possible to force clang to always emit color escaped codes using "-Xclang -fcolor-diagnostics", but that's pretty hacky. make doesn't seem to override stderr on unix as far as I can tell, child_execute_job() in job.c)

evmar commented 12 years ago

You have summarized everything (the problem, workaround, and possible problem with fix) exactly as I think about it. (I have done the forkpty thing in another project.

I kinda wonder if we should make gyp know to add this flag when using clang. (That is, by virtue of the fact that it's a specific command it belongs in the generator program to know what to do.)

One other thing that makes me nervous about the magic-pty-fix option is that if a program shells out to an editor (e.g. something runs "svn up" for some reason and then that hits a conflict), we don't want the subprocess to think that they're getting a real terminal.

nico commented 12 years ago

Hm, I'm not sure if gyp is in a good position to do this. It sounds like these flags would need to be passed at build time if isatty(stderr) in the ninja process (else the control characters would appear on build bots, too); gyp time is too early.

For now, I'm running ninja without the dup2(pipe, 2) locally. That's not ideal (some missing newlines on warning output), but it works better than the current behavior for me.

evmar commented 12 years ago

That's a good point about gyp-time vs run-time. Hmm. I wonder if Ninja could expose some $isatty variable at runtime. Feels slippery.

(FWIW, the reason for the pipe is that otherwise, output from parallel commands can be interleaved.)

nico commented 12 years ago

Make doesn't have the pipe, and I can't remember ever being annoyed by interleaved commands. Another reason for the pipe is to print a newline after the "progress line" before writing stderr tho, and I can't think of a good way to make that work without writing a newline after every command. Now that I think about this, I think blaze manages to print colored diagnostics from clang while not printing every command. (time passes) I talked to someone, blaze just passes -fcolor-diagnostics, lame.

So, to sum up:

I think I've heard people asking for ninja to respect $CC, and you said "it's just nobody has implemented this yet". To support this, ninja would have to grow the concept of "compiler" anyway. If it did that, maybe checking for cc == clang and passing in 'force colors' isn't so bad? But now that I wrote this, ninja could just have the general concept of "try to get env var FOO from the env" in its build rules to get CC support, and then it wouldn't have to know about colors.)

With env support, I could set CFLAGS=-fcolor-diagnostics…or clang could check for env("SOME_PARENT_IN_TTY") and I could set that…that'd be workable, but it wouldn't Just Work.

All in all, I probably like your $isatty suggestion best, if we could agree on a name that the clang guys would be comfortable supporting. Then all kinds of build systems could use that if they wanted. (In theory, it could build break steps that grep for warnings, so maybe there would have to be a flag to disable setting this variable…but build steps like that are questionable anyhow.)

okuoku commented 12 years ago

IMHO, without having pipe won't scale well for muti-core parallel compilation. Currently, we have 12 thread processor and we need 16 or more parallel jobs to hide I/O latencies.. In the situation like this, commands output might get intermixed and breaks ANSI sequences.

Having $isatty might introduce another complexity to Ninja and would cause hard-to-track problem like "My job won't complete when it invoked from cron-job!". So i feel Ninja shouldn't have multiple ways to invoke a command and current one; invoke program and buffer its output via pipe is good for multi-core era.

PS. Fortunately, clang has machine-readable diagnostics as well as TTY-based diags. So I think someone could implement "external colorizer for Ninja log" for clang. Integrating to emacs or other notification system like Growl is a plus :)

nico commented 12 years ago

Sounds like there's no good solution here.

For chrome, I'll try adding a CLANG_FORCE_COLOR_DIAGNOSTICS gyp define that defaults to 1 and is set to 0 on the bots (which adds -fcolor-diagnostics).

evmar commented 12 years ago

I'd also be ok with extending Ninja to obey environment variables. I.e. make it so "$ENV_foobar" expands to $foobar from the environment. (All the other machinery is already in place such that Ninja will rebuild the world if you change such a variable between builds.)

nico commented 12 years ago

For chrome, colors need to Just Work. Env vars don't really help with that, so let's WontFix this issue.

(I ran a small test, and my system – OS X 10.7 – runs out of pseudo ttys after allocating 125.)

nico commented 12 years ago

Wrong button, sorry.

nico commented 12 years ago

We noticed that forcing colors on for clang confuses programs that parse its output, such as emacs and vim. The current plan is to let ninja strip ansi escapes from subprocesses if it's not writing to a terminal. I'll prepare a patch for that.

nico commented 12 years ago

That landed in https://github.com/martine/ninja/commit/2ef3a546476987d6565141337c6309e7ae60f9fd

CatalinMustata commented 5 years ago

Many years later, I'm confused about how this is supposed to work... because it's not working on my configuration. My CMake compile options are like so: set_property(DIRECTORY APPEND PROPERTY COMPILE_OPTIONS $<$<COMPILE_LANGUAGE:C>:-Wimplicit-int -Wimplicit-function-declaration -Wno-gnu-zero-variadic-macro-arguments -std=c11> -Xclang -fcolor-diagnostics)

No colored output is available unfortunately (MacOS, iTerm2 with zsh). And I've checked the output, the flags do get added to the compile options at runtime. If I switch Ninja with Make, all is well.

jhasse commented 5 years ago

Try -fdiagnostics-color. Are you running ninja in a terminal?

CatalinMustata commented 5 years ago

Sorry, I didn't specify I'm using clang. That flag is for gcc.

Also yes, I’m running Ninja in a terminal. I need to have the libs built as close to what AS outputs from command line as well.

Timmmm commented 5 years ago

So as I understand it, Clang detects if it is outputting to a terminal and enables colours automatically in that case. Ninja captures Clang's output so Clang things it isn't outputting to a terminal.

So you have to force Clang to output colours with -fdiagnostics-color=always (or -fcolor-diagnostics for GCC - yeay consistency). But then that means that when Ninja is outputting to something that isn't a terminal (e.g. Emacs) that program gets confused by the colour codes.

And your "solution" was to have Ninja strip the colour codes if it is not outputting to a terminal.

So Ninja knows if it is meant to output colours. It can "tell" Clang whether it should output colours (by pretending to be a terminal). But instead it throws that information away and forces the user to set -fdiagnostics-color manually?

That seems like a bad solution. Why can't Ninja just do something like this:

if (output_is_terminal) {
   use_pseudo_terminal = true;
}

Clang uses istty() on Unix and GetConsoleMode() on Windows to determine if the output is a console.

So you can just create the subprocess in a pseudo-terminal instead (openpty() on Linux, not sure about Windows).

The only downside I can see is that you can't get stdout and stderr separately from openpty(). There is a question about doing that on StackOverflow but the only solution uses a dirty LD_PRELOAD hack. However it seems like Ninja doesn't distinguish between them anyway.

Timmmm commented 5 years ago

I did find this mentioned in the wiki:

Ninja could use pseudo ttys to work around this further, but different systems have different limits on the number of ptys available. I also worry that commands like svn up would assume they can interact with the user.

jhasse commented 5 years ago

Yeah, using a pseudo tty is overkill when the only thing you want is color codes. See https://bugs.llvm.org/show_bug.cgi?id=23609 and https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80271 for a possible solution.

david-fong commented 2 years ago

For CMake users, CMake 3.24 (which looks like it releases on June 15th?) will have CMAKE_COLOR_DIAGNOSTICS. Here's their issue thread where the feature was discussed.

Here's a link to a comment in the thread by Craig about how the feature might be best used due to some IDEs not supporting ansi escape coloring- the environment variable allows per-user config.

mohkale commented 2 months ago

Hi there,

Even with the latest CMake and ninja release I don't get colorised output when building with -DCMAKE_COLOR_DIAGNOSTICS=On.

$ cmake --version
cmake version 3.30.3

CMake suite maintained and supported by Kitware (kitware.com/cmake).
$ ninja --version
1.12.1

$ gcc --version
gcc (GCC) 14.2.1 20240805
Copyright (C) 2024 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

If I set the CLICOLOR_FORCE=1 in the environment running the CMake build then I do get output. Just sharing for anyone else that stumbles on this.

FYI: This doesn't cause CMake itself to colorize the output like it does in Make. So the [x%] build target outputs won't be colorised. But the build-tool/compiler output will be.

jhasse commented 2 months ago

In what kind of terminal simulator are you running ninja?

mohkale commented 2 months ago

Tmux within suckless terminal. I'm also using a custom terminfo definition that adapts the combines the sequences supported by tmux and st. To be clear color does work when I run CMake/Ninja into a tty but if I put a pager in front of it like cat then color is suppressed unless I add that CLICOLOR_FORCE option.

Example for with and without:

Screenshot_20240907_143851

Screenshot_20240907_143754

In both cases the -fdiagnostics-color=always option is passed to gcc. I'd wager ninja is probably stripping it out but I haven't investigated that far yet :thinking:.

jhasse commented 1 month ago

Yes, see https://github.com/ninja-build/ninja/wiki/FAQ#why-does-my-program-with-colored-output-not-have-color-under-ninja

mohkale commented 1 month ago

I think that wiki page should also mention you can prevent ninja escaping color output by setting that env variable.

Ninja itself will detect if it's writing to a terminal and escape color codes from commands if it isn't.

Also I don't really follow the reasoning for this. Its one thing to explicitly pass a flag to tools to enable color in environments that ordinarily wouldn't have it but needing to pass another different flag to ninja to prevent it overriding that choice is a bit odd. I guess there's a uniformity aspect to the output but really I think ninja should defer to the tools it runs instead of deciding for them. This might make more sense if output from ninja was colored. Like task descriptions. But as of now I don't think that's supported.

jhasse commented 1 month ago

The compiler would behave in the same way when invoked directly, that's the reasoning ;)

mohkale commented 1 month ago

When invoked directly the compiler would print color... unless I'm misunderstanding. If I just run the command that ninja is doing directly and pipe to cat it still has color.

Screenshot_20240912_191514

On the hander if I stop setting the CMake option that passes the compiler option that enables color but do still supply the CLI_COLOR force env variable then I no longer get color regardless of if I'm writing to a terminal or not.

Screenshot_20240912_191722

I think what I dislike about the current behaviour is it's an override of an override. If gcc respected CLICOLOR_FORCE I'd disagree but adding support is still missing and the interim you need to both set CLI_COLOR and pass compiler options to get color working with ninja :disappointed:.

jhasse commented 1 month ago

I mean without -fdiagnostics-color=always the compiler will not print color codes when it gets piped into cat. So ninja wants to mimic that, but it's kinda hard since the compiler is ALWAYS piped when ninja executes it. And ninja doesn't (and IMHO shouldn't) know/parse the flags passed to the compiler.

You're right that -fdiagnostics-color is a non standard way and it would be better if GCC used the same environment variable instead. You can raise your support for that here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80271

eli-schwartz commented 1 month ago

You're right that -fdiagnostics-color is a non standard way and it would be better if GCC used the same environment variable instead. You can raise your support for that here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80271

Environment variables are significantly more nonstandard than -fdiagnostics-color, but perhaps you meant that GCC should add support for --color={always|auto|never}?

eli-schwartz commented 1 month ago

Note that CLICOLOR_FORCE is a nonstarter since setting it will break the ls program on some operating systems, so the only way to safely use it is to make sure that a specific process but not subprocesses launched as a child of that process see the environment variable.

Which is exactly what a CLI flag is, and exactly the opposite of what an environment variable is.

jhasse commented 1 month ago

Environment variables are significantly more nonstandard [...]

Huh why? There's a standard set of environment variables and there's a standard set of command line flags.

Note that CLICOLOR_FORCE is a nonstarter since setting it will break the ls program on some operating systems, [...]

Can you elaborate?