Colored messages from C/C++ compiler in C stubs #4083

MisterDA · 2021-01-05T14:55:13Z

A pet peeve of mine is to have the C/C++ compiler emit coloured output when there are warnings or errors in C/C++ sources. GCC has the peculiarity of emitting an ESC K control sequence after each ANSI colour sequence. Dune doesn't support this sequence, and this messes up the output when forcing colour emission. The first patch fixes that.
The second patch forces C/C++ compiler to emit coloured output. They usually use isatty(3) to decide wether they should emit ANSI codes or not. Dune doesn't attach a tty or a pseudo tty, so they never emit colours. We can use the -fdiagnostics-color=always parameter, supported by clang and gcc.

rgrinberg · 2021-01-06T03:54:18Z

Looks good me. @voodoos are you ok with hardcoding this flag for gcc/clang?

voodoos

At first I was worried that this could break dune output in Emacs shells (for users who do not use some incantations to support ANSI colors).

And I just noticed that the default opam-user-setup adds the necessary config to the .emacs file:

;; ANSI color in compilation buffer
(require 'ansi-color)
(defun colorize-compilation-buffer ()
  (toggle-read-only)
  (ansi-color-apply-on-region (point-min) (point-max))
  (toggle-read-only))
(add-hook 'compilation-filter-hook 'colorize-compilation-buffer)

So I guess this should be ok.

However I am not fond of hardcoding the flags: we have recently been jumping hoops to reduce these (see #3565, #3875).
In the current proposal the flag is added at the end of the command line and thus is not user-overridable.

This could be fixed by moving the color flag to the :standard set of flag so that it can be overridden by users in a Dune-idiomatic way. Maybe only when Ansi_color.stdout_supports_color is true ?

However, I wonder if it would be possible to forward "tty" information to the C compiler, so that its "auto" mode works as expected ?

src/dune_rules/foreign_rules.ml

voodoos · 2021-01-06T14:36:10Z

It is much better like that, thanks.
Can you add a test for this behaviour ?

You can use dune rules some_stub.o to print the command line arguments and then grep for what you are looking for as in these tests (but you don't need to use an intermediate file in this simple case).

emillon · 2021-01-12T15:41:06Z

src/dune_rules/super_context.ml

+  let c, cxx =
+    if
+      Lazy.force Ansi_color.stderr_supports_color
+      && Ocaml_config.ccomp_type ctx.ocaml_config <> Msvc


I think this should use pattern matching to be future proof. Is there a way to make sure we're using gcc and not say clang?

Note that the flag is compatible with both gcc and clang.
The type is below, maybe we can use the string to differentiate but I doubt it.

type t = | Msvc | Other of string

I added some infrastructure to detect the C compiler in #3802 which is currently reviewed by @dra27

I think we need a way to make sure that the compiler supports this (compared to making sure that the compiler is not msvc). Old versions of gcc and clang might not support it either. Do you know when this was added?

Support is documented in GCC 4.9.0, released in April 2014.
Clang support was added in llvm/llvm-project@7e2da79, in 2013 around llvm 3.3.

Apart from digging the history, another reliable way to tell if the flag is supported could be to test the C compiler supports it. Maybe using configurator?
As the flag is added in :standard it can also easily be deactivated, right?

MisterDA · 2021-01-14T16:55:20Z

To rebase this, it would be nice to have the compiler detection functionality from the inside of Cxx_flags exposed. Maybe that could replace detection from the config variable

dune/src/ocaml-config/ocaml_config.ml

Line 459 in be9f587

let ccomp_type = Ccomp_type.of_string (get vars "ccomp_type") in

and expand Ccomp_type.t

dune/src/ocaml-config/ocaml_config.mli

Lines 52 to 55 in be9f587

    
           module Ccomp_type : sig 
        
             type t = 
        
               | Msvc 
        
               | Other of string

rgrinberg · 2022-10-09T22:32:59Z

@MisterDA @emillon what's the status of this?

MisterDA · 2022-10-10T08:19:12Z

It needs rebasing for detection of the C/C++ compiler, and fixes to the tests (at some point I changed how I preload isatty and they stopped working).
It would have been way better if GCC or Clang provided and environment variable to turn on the color, but that's not the case. Even if we don't end up turning the color on, the first commit is actually a bug fix.

emillon · 2022-10-11T07:54:35Z

I have a fix for #5528 that look like it could take care of the particular issue with K. I'll open that

MisterDA · 2022-11-03T16:10:33Z

I've rebased the code and I'm now using CLICOLOR and CLICOLOR_FORCE for the tests, it's much easier. When Dune uses colors (such as when running in a tty or when CLICOLOR_FORCE=1), it will now add the -fdiagnostics-color=always parameter of GCC and Clang when compiling a C stub to the :standard list of flags given to the C/C++ compiler.
I haven't found how to factor the bit of code because of the Action_builder monad, however I suspect that duplication is not too bad because of memoïzation.

cc @emillon @voodoos @rgrinberg

MisterDA · 2022-11-09T16:10:30Z

The new CI failure is due to #6415.

rgrinberg · 2022-12-15T16:20:49Z

@MisterDA #6415 has been fixed. Is the PR ready now?

MisterDA · 2022-12-15T17:56:54Z

Yes! The CI passes with the fix and the tests of this feature.

rgrinberg · 2022-12-15T22:33:23Z

💯

Can you update CHANGES?

Signed-off-by: Antonin Décimo <antonin@tarides.com>

The flag -fdiagnostics-color=always is added to the :standard set of flags for C stubs, which can be easily removed by the user if needed. Signed-off-by: Antonin Décimo <antonin@tarides.com>

MisterDA · 2022-12-16T15:12:30Z

Can you update CHANGES?

Done!

@rgrinberg

…, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.7.0~alpha1) CHANGES: - Fix parsing of OCaml errors that contain code excerpts with `...` in them. (ocaml/dune#7008, @rgrinberg) - Pre-emptively clear screen in watch mode (ocaml/dune#6987, fixes ocaml/dune#6884, @rgrinberg) - Fix cross compilation configuration when a context with targets is itself a host of another context (ocaml/dune#6958, fixes ocaml/dune#6843, @rgrinberg) - Fix parsing of the `<=` operator in *blang* expressions of `dune` files. Previously, the operator would be interpreted as `,`. (ocaml/dune#6928, @tatchi) - Fix `--trace-file` output. Dune now emits a single *complete* event for every executed process. Unterminated *async* events are no longer written. (ocaml/dune#6892, @rgrinberg) - Fix preprocessing with `staged_pps` (ocaml/dune#6748, fixes ocaml/dune#6644, @rgrinberg) - Use colored output with MDX when Dune colors are enabled. (ocaml/dune#6462, @MisterDA) - Make `dune describe workspace` return consistent dependencies for executables and for libraries. By default, compile-time dependencies towards PPX-rewriters are from now not taken into account (but runtime dependencies always are). Compile-time dependencies towards PPX-rewriters can be taken into account by providing the `--with-pps` flag. (ocaml/dune#6727, fixes ocaml/dune#6486, @esope) - Print missing newline after `$ dune exec`. (ocaml/dune#6821, fixes ocaml/dune#6700, @rgrinberg, @Alizter) - Fix binary corruption when installing or promoting in parallel (ocaml/dune#6669, fixes ocaml/dune#6668, @edwintorok) - Use colored output with GCC and Clang when compiling C stubs. The flag `-fdiagnostics-color=always` is added to the `:standard` set of flags. (ocaml/dune#4083, @MisterDA) - Fix the parsing of decimal and hexadecimal escape literals in `dune`, `dune-package`, and other dune s-expression based files (ocaml/dune#6710, @shym) - Report an error if `dune init ...` would create a "dune" file in a location which already contains a "dune" directory (ocaml/dune#6705, @gridbugs) - Fix the parsing of alerts. They will now show up in diagnostics correctly. (ocaml/dune#6678, @rginberg) - Fix the compilation of modules generated at link time when `implicit_transitive_deps` is enabled (ocaml/dune#6642, @rgrinberg) - Allow `$ dune utop` to load libraries defined in data only directories defined using `(subdir ..)` (ocaml/dune#6631, @rgrinberg) - Format dune files when they are named `dune-file`. This occurs when we enable the alternative file names project option. (ocaml/dune#6566, @rgrinberg) - Move `$ dune ocaml-merlin -dump-config=$dir` to `$ dune ocaml merlin dump-config $dir`. (ocaml/dune#6547, @rgrinberg) - Allow compilation rules to be impacted by `(env ..)` stanzas that modify the environment or set binaries. (ocaml/dune#6527, @rgrinberg) - Coq native mode is now automatically detected by Dune starting with Coq lang 0.7. `(mode native)` has been deprecated in favour of detection from the configuration of Coq. (ocaml/dune#6409, @Alizter) - Print "Leaving Directory" whenever "Entering Directory" is printed. (ocaml/dune#6149, fixes ocaml/dune#138, @cpitclaudel, @rgrinberg) - Allow `$ dune ocaml dump-dot-merlin` to run in watch mode. Also this command shouldn't print "Entering Directory" mesages. (ocaml/dune#6497, @rgrinberg) - `dune clean` should no longer fail under Windows due to the inability to remove the `.lock` file. Also, bring the implementation of the global lock under Windows closer to that of Unix. (ocaml/dune#6523, @nojb) - Remove "Entering Directory" messages for `$ dune install`. (ocaml/dune#6513, @rgrinberg) - Stop passing `-q` flag in `dune coq top`, which allows for `.coqrc` to be loaded. (ocaml/dune#6848, fixes ocaml/dune#6847, @Alizter) - Fix missing dependencies when detecting the kind of C compiler we're using (ocaml/dune#6610, fixes ocaml/dune#6415, @emillon) - Allow `(include_subdirs qualified)` for OCaml projects. (ocaml/dune#6594, fixes ocaml/dune#1084, @rgrinberg) - Accurately determine merlin configuration for all sources selected with `copy#` and `copy_files#`. The old heuristic of looking for a module in parent directories is removed (ocaml/dune#6594, @rgrinberg) - Fix inline tests with *js_of_ocaml* and whole program compilation mode enabled (ocaml/dune#6645, @hhugo) - Fix *js_of_ocaml* separate compilation rules when `--enable=effects` ,`--enable=use-js-string` or `--toplevel` is used. (ocaml/dune#6714, ocaml/dune#6828, ocaml/dune#6920, @hhugo) - Fix *js_of_ocaml* separate compilation in presence of linkall (ocaml/dune#6832, ocaml/dune#6916, @hhugo) - Remove spurious build dir created when running `dune init proj ...` (ocaml/dune#6707, fixes ocaml/dune#5429, @gridbugs) - Allow `--sandbox` to affect `ocamldep` invocations. Previously, they were wrongly marked as incompatible (ocaml/dune#6749, @rgrinberg) - Validate the command line arguments for `$ dune ocaml top-module`. This command requires one positional argument (ocaml/dune#6796, fixes ocaml/dune#6793, @rgrinberg) - Add a `dune cache size` command for displaying the size of the cache (ocaml/dune#6638, @Alizter) - Add 4.14.0 MSVC to CI (ocaml/dune#6917, @jonahbeckford) - Fix dependency cycle when installing files to the bin section with `glob_files` (ocaml/dune#6764, fixes ocaml/dune#6708, @gridbugs) - Handle "Too many links" errors when using Dune cache on Windows (ocaml/dune#6993, @nojb)

@gridbugs

…, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.7.0) CHANGES: - Allow running `$ dune exec` in watch mode (with the `-w` flag). In watch mode, `$ dune exec` the executed binary whenever it is recompiled. (ocaml/dune#6966, @gridbugs) - `coqdep` is now called once per theory, instead of one time per Coq file. This should significantly speed up some builds, as `coqdep` startup time is often heavy (ocaml/dune#7048, @Alizter, @ejgallego) - Add `map_workspace_root` dune-project stanza to allow disabling of mapping of workspace root to `/workspace_root`. (ocaml/dune#6988, fixes ocaml/dune#6929, @richardlford) - Fix handling of support files generated by odoc. (ocaml/dune#6913, @jonludlam) - Fix parsing of OCaml errors that contain code excerpts with `...` in them. (ocaml/dune#7008, @rgrinberg) - Pre-emptively clear screen in watch mode (ocaml/dune#6987, fixes ocaml/dune#6884, @rgrinberg) - Fix cross compilation configuration when a context with targets is itself a host of another context (ocaml/dune#6958, fixes ocaml/dune#6843, @rgrinberg) - Fix parsing of the `<=` operator in *blang* expressions of `dune` files. Previously, the operator would be interpreted as `<`. (ocaml/dune#6928, @tatchi) - Fix `--trace-file` output. Dune now emits a single *complete* event for every executed process. Unterminated *async* events are no longer written. (ocaml/dune#6892, @rgrinberg) - Fix preprocessing with `staged_pps` (ocaml/dune#6748, fixes ocaml/dune#6644, @rgrinberg) - Use colored output with MDX when Dune colors are enabled. (ocaml/dune#6462, @MisterDA) - Make `dune describe workspace` return consistent dependencies for executables and for libraries. By default, compile-time dependencies towards PPX-rewriters are from now not taken into account (but runtime dependencies always are). Compile-time dependencies towards PPX-rewriters can be taken into account by providing the `--with-pps` flag. (ocaml/dune#6727, fixes ocaml/dune#6486, @esope) - Print missing newline after `$ dune exec`. (ocaml/dune#6821, fixes ocaml/dune#6700, @rgrinberg, @Alizter) - Fix binary corruption when installing or promoting in parallel (ocaml/dune#6669, fixes ocaml/dune#6668, @edwintorok) - Use colored output with GCC and Clang when compiling C stubs. The flag `-fdiagnostics-color=always` is added to the `:standard` set of flags. (ocaml/dune#4083, @MisterDA) - Fix the parsing of decimal and hexadecimal escape literals in `dune`, `dune-package`, and other dune s-expression based files (ocaml/dune#6710, @shym) - Report an error if `dune init ...` would create a "dune" file in a location which already contains a "dune" directory (ocaml/dune#6705, @gridbugs) - Fix the parsing of alerts. They will now show up in diagnostics correctly. (ocaml/dune#6678, @rginberg) - Fix the compilation of modules generated at link time when `implicit_transitive_deps` is enabled (ocaml/dune#6642, @rgrinberg) - Allow `$ dune utop` to load libraries defined in data only directories defined using `(subdir ..)` (ocaml/dune#6631, @rgrinberg) - Format dune files when they are named `dune-file`. This occurs when we enable the alternative file names project option. (ocaml/dune#6566, @rgrinberg) - Move `$ dune ocaml-merlin -dump-config=$dir` to `$ dune ocaml merlin dump-config $dir`. (ocaml/dune#6547, @rgrinberg) - Allow compilation rules to be impacted by `(env ..)` stanzas that modify the environment or set binaries. (ocaml/dune#6527, @rgrinberg) - Coq native mode is now automatically detected by Dune starting with Coq lang 0.7. `(mode native)` has been deprecated in favour of detection from the configuration of Coq. (ocaml/dune#6409, @Alizter) - Print "Leaving Directory" whenever "Entering Directory" is printed. (ocaml/dune#6419, fixes ocaml/dune#138, @cpitclaudel, @rgrinberg) - Allow `$ dune ocaml dump-dot-merlin` to run in watch mode. Also this command shouldn't print "Entering Directory" mesages. (ocaml/dune#6497, @rgrinberg) - `dune clean` should no longer fail under Windows due to the inability to remove the `.lock` file. Also, bring the implementation of the global lock under Windows closer to that of Unix. (ocaml/dune#6523, @nojb) - Remove "Entering Directory" messages for `$ dune install`. (ocaml/dune#6513, @rgrinberg) - Stop passing `-q` flag in `dune coq top`, which allows for `.coqrc` to be loaded. (ocaml/dune#6848, fixes ocaml/dune#6847, @Alizter) - Fix missing dependencies when detecting the kind of C compiler we're using (ocaml/dune#6610, fixes ocaml/dune#6415, @emillon) - Allow `(include_subdirs qualified)` for OCaml projects. (ocaml/dune#6594, fixes ocaml/dune#1084, @rgrinberg) - Accurately determine merlin configuration for all sources selected with `copy#` and `copy_files#`. The old heuristic of looking for a module in parent directories is removed (ocaml/dune#6594, @rgrinberg) - Fix inline tests with *js_of_ocaml* and whole program compilation mode enabled (ocaml/dune#6645, @hhugo) - Fix *js_of_ocaml* separate compilation rules when `--enable=effects` ,`--enable=use-js-string` or `--toplevel` is used. (ocaml/dune#6714, ocaml/dune#6828, ocaml/dune#6920, @hhugo) - Fix *js_of_ocaml* separate compilation in presence of linkall (ocaml/dune#6832, ocaml/dune#6916, @hhugo) - Remove spurious build dir created when running `dune init proj ...` (ocaml/dune#6707, fixes ocaml/dune#5429, @gridbugs) - Allow `--sandbox` to affect `ocamldep` invocations. Previously, they were wrongly marked as incompatible (ocaml/dune#6749, @rgrinberg) - Validate the command line arguments for `$ dune ocaml top-module`. This command requires one positional argument (ocaml/dune#6796, fixes ocaml/dune#6793, @rgrinberg) - Add a `dune cache size` command for displaying the size of the cache (ocaml/dune#6638, @Alizter) - Fix dependency cycle when installing files to the bin section with `glob_files` (ocaml/dune#6764, fixes ocaml/dune#6708, @gridbugs) - Handle "Too many links" errors when using Dune cache on Windows (ocaml/dune#6993, @nojb) - Allow the `cinaps` stanza to set a custom alias. By default, if the alias is not set then the cinaps actions will be attached to both `@cinaps` and `@runtest` (ocaml/dune#6991, @rgrinberg) - Add `(using ctypes 0.3)`. When used, paths in `(ctypes)` are interpreted relative to where the stanza is defined. (ocaml/dune#6883, fixes ocaml/dune#5325, @emillon) - Auto-detect `dune-workspace` files as `dune` files in Emacs (ocaml/dune#7061, @ilankri) - Add native support for polling mode on Windows (ocaml/dune#7010, @yams-yams, @nojb)

MisterDA requested a review from rgrinberg as a code owner January 5, 2021 14:55

voodoos requested changes Jan 6, 2021

View reviewed changes

src/dune_rules/foreign_rules.ml Outdated Show resolved Hide resolved

MisterDA force-pushed the coloured-c-stubs branch 3 times, most recently from 7aad7d5 to cf0328a Compare January 6, 2021 13:20

emillon reviewed Jan 12, 2021

View reviewed changes

Base automatically changed from master to main January 14, 2021 17:09

MisterDA force-pushed the coloured-c-stubs branch 3 times, most recently from 1aa25a0 to e5f0646 Compare January 19, 2021 13:42

emillon mentioned this pull request Oct 11, 2022

Ansi_color.parse: handle CSI n K #6214

Merged

rgrinberg marked this pull request as draft October 19, 2022 03:27

MisterDA mentioned this pull request Oct 28, 2022

Color handling: let's pass -colors instead of setting an env var #2339

Open

MisterDA changed the title ~~Coloured c stubs~~ Colored messages from C/C++ compiler in C stubs Nov 3, 2022

MisterDA force-pushed the coloured-c-stubs branch from ab39b80 to 50a431e Compare November 3, 2022 16:04

MisterDA marked this pull request as ready for review November 3, 2022 16:04

MisterDA requested review from voodoos and emillon and removed request for rgrinberg, voodoos and emillon November 3, 2022 16:11

MisterDA force-pushed the coloured-c-stubs branch from 50a431e to ef82197 Compare November 9, 2022 16:08

MisterDA force-pushed the coloured-c-stubs branch 2 times, most recently from 6e74020 to 6ed5b8a Compare December 6, 2022 10:18

MisterDA added 2 commits December 16, 2022 16:08

Export C/C++ compiler found in Cxx_flags

670eb20

Signed-off-by: Antonin Décimo <antonin@tarides.com>

Use colored output with GCC and Clang

e062d65

The flag -fdiagnostics-color=always is added to the :standard set of flags for C stubs, which can be easily removed by the user if needed. Signed-off-by: Antonin Décimo <antonin@tarides.com>

MisterDA force-pushed the coloured-c-stubs branch from 6ed5b8a to e062d65 Compare December 16, 2022 15:11

rgrinberg merged commit c0415c5 into ocaml:main Dec 16, 2022

rgrinberg added this to the 3.7.0 milestone Dec 16, 2022

MisterDA deleted the coloured-c-stubs branch December 16, 2022 17:32

MisterDA mentioned this pull request Jan 17, 2023

Enable colors when compiling foreign stubs. #3263

Closed

emillon mentioned this pull request Feb 6, 2023

[new release] xdg, stdune, ordering, ocamlc-loc, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.7.0~alpha1) ocaml/opam-repository#23248

Closed

emillon mentioned this pull request Feb 17, 2023

[new release] xdg, stdune, ordering, ocamlc-loc, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.7.0) ocaml/opam-repository#23349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colored messages from C/C++ compiler in C stubs #4083

Colored messages from C/C++ compiler in C stubs #4083

MisterDA commented Jan 5, 2021 •

edited

Loading

rgrinberg commented Jan 6, 2021

voodoos left a comment •

edited

Loading

voodoos commented Jan 6, 2021

emillon Jan 12, 2021

MisterDA Jan 12, 2021

voodoos Jan 12, 2021

emillon Jan 12, 2021

MisterDA Jan 12, 2021

MisterDA commented Jan 14, 2021

rgrinberg commented Oct 9, 2022

MisterDA commented Oct 10, 2022

emillon commented Oct 11, 2022

MisterDA commented Nov 3, 2022

MisterDA commented Nov 9, 2022

rgrinberg commented Dec 15, 2022

MisterDA commented Dec 15, 2022

rgrinberg commented Dec 15, 2022

MisterDA commented Dec 16, 2022

Colored messages from C/C++ compiler in C stubs #4083

Colored messages from C/C++ compiler in C stubs #4083

Conversation

MisterDA commented Jan 5, 2021 • edited Loading

rgrinberg commented Jan 6, 2021

voodoos left a comment • edited Loading

Choose a reason for hiding this comment

voodoos commented Jan 6, 2021

emillon Jan 12, 2021

Choose a reason for hiding this comment

MisterDA Jan 12, 2021

Choose a reason for hiding this comment

voodoos Jan 12, 2021

Choose a reason for hiding this comment

emillon Jan 12, 2021

Choose a reason for hiding this comment

MisterDA Jan 12, 2021

Choose a reason for hiding this comment

MisterDA commented Jan 14, 2021

rgrinberg commented Oct 9, 2022

MisterDA commented Oct 10, 2022

emillon commented Oct 11, 2022

MisterDA commented Nov 3, 2022

MisterDA commented Nov 9, 2022

rgrinberg commented Dec 15, 2022

MisterDA commented Dec 15, 2022

rgrinberg commented Dec 15, 2022

MisterDA commented Dec 16, 2022

MisterDA commented Jan 5, 2021 •

edited

Loading

voodoos left a comment •

edited

Loading