-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
clang doesn't print colored diagnostics when invoked from ninja #174
Comments
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. |
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. |
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.) |
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.) |
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. |
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). |
I'd also be ok with extending Ninja to obey environment variables. |
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.) |
Wrong button, sorry. |
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. |
That landed in 2ef3a54 |
When invoked from Ninja, clang does not detect that it can use colors : see ninja-build/ninja#174 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@182878 91177308-0d34-0410-b5e6-96231b3b80d8
When invoked from Ninja, clang does not detect that it can use colors : see ninja-build/ninja#174 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@182878 91177308-0d34-0410-b5e6-96231b3b80d8
Related: ninja-build/ninja#174 Signed-off-by: Igor Gnatenko <[email protected]>
Related: ninja-build/ninja#174 Signed-off-by: Igor Gnatenko <[email protected]>
Ninja can strip colors if needed, but otherwise it needs to be forced on ninja-build/ninja#174
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: 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. |
Try |
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. |
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 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 That seems like a bad solution. Why can't Ninja just do something like this:
Clang uses So you can just create the subprocess in a pseudo-terminal instead ( The only downside I can see is that you can't get |
I did find this mentioned in the wiki:
|
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. |
For CMake users, CMake 3.24 (which looks like it releases on June 15th?) will have 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. |
Hi there, Even with the latest CMake and ninja release I don't get colorised output when building with -DCMAKE_COLOR_DIAGNOSTICS=On.
If I set the FYI: This doesn't cause CMake itself to colorize the output like it does in Make. So the |
In what kind of terminal simulator are you running ninja? |
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: 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 🤔. |
I think that wiki page should also mention you can prevent ninja escaping color output by setting that env variable.
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. |
The compiler would behave in the same way when invoked directly, that's the reasoning ;) |
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. 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. 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 😞. |
I mean without You're right that |
Environment variables are significantly more nonstandard than -fdiagnostics-color, but perhaps you meant that GCC should add support for |
Note that CLICOLOR_FORCE is a nonstarter since setting it will break the Which is exactly what a CLI flag is, and exactly the opposite of what an environment variable is. |
Huh why? There's a standard set of environment variables and there's a standard set of command line flags.
Can you elaborate? |
Add compile option '-fdiagnostics-color=always' as a workaround to a bug in GCC for ninja build where the compiler output doesn't add color. For reference: ninja-build/ninja#174 Signed-off-by: Arnold Gabriel Benedict <[email protected]> Change-Id: I36da3d659216e92c6e40ff4923f4548306fe712d
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)
The text was updated successfully, but these errors were encountered: