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

very minimalistic support for colors (warnings and errors) #207

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
@c-cube
Contributor

c-cube commented Jun 25, 2015

The point of this PR is to poll maintainers and users, do NOT merge (yet) :-)

The idea is simple: print warnings in yellow and errors in red, as other compilers (gcc, clang for instance) do. I didn't spend a lot of time on this (no tests, a problem with Format which indents weirdly because of escape codes). I could use ANSITerminal but I'm not sure about adding dependencies to the compiler.

@Drup

This comment has been minimized.

Show comment
Hide comment
@Drup

Drup Jun 25, 2015

Contributor

You should really use Format.tag for this. It would simplify the enabling/disabling and solve the weird indentation.

Respecting GCC_COLORS (iirc) might be a good idea.

Contributor

Drup commented Jun 25, 2015

You should really use Format.tag for this. It would simplify the enabling/disabling and solve the weird indentation.

Respecting GCC_COLORS (iirc) might be a good idea.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jun 25, 2015

Member

My personal opinion is that while I don't personally care about colors (Emacs and Merlin do that for me), I have heard enough demand from enough people to believe that integrating it in the compiler is worthwhile. I find it rather silly to say that a compiler is "massively more usable" when it adds colors (not saying that's what @c-cube claims, but I have heard this argument around the Clang debates), but if enough people hold that irrational belief it becomes an important aspect of usability to please them -- and also they might be right, who knows?

Cc: @ppedrot which asked me privately about doing such things in the toplevel (but I think there utop may be the ultimate answer).

Member

gasche commented Jun 25, 2015

My personal opinion is that while I don't personally care about colors (Emacs and Merlin do that for me), I have heard enough demand from enough people to believe that integrating it in the compiler is worthwhile. I find it rather silly to say that a compiler is "massively more usable" when it adds colors (not saying that's what @c-cube claims, but I have heard this argument around the Clang debates), but if enough people hold that irrational belief it becomes an important aspect of usability to please them -- and also they might be right, who knows?

Cc: @ppedrot which asked me privately about doing such things in the toplevel (but I think there utop may be the ultimate answer).

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Jun 25, 2015

Contributor

@gasche I think many people use OCaml without emacs, those days, including me. Colors do not have a "massive" effects, but they do help spot where important messages lie.
I'll follow @Drup's advice on Format tags, although it requires a bit more refactoring to ensure all messages are built using Format.

Contributor

c-cube commented Jun 25, 2015

@gasche I think many people use OCaml without emacs, those days, including me. Colors do not have a "massive" effects, but they do help spot where important messages lie.
I'll follow @Drup's advice on Format tags, although it requires a bit more refactoring to ensure all messages are built using Format.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Jun 25, 2015

Contributor

One of the problem with tags is that they are not compositional (though it may not be a problem in that case, it is if you try to provide styling as a library). You can also simply use "@<0>" to solve the weird identation, this is the way this API is implemented.

Contributor

dbuenzli commented Jun 25, 2015

One of the problem with tags is that they are not compositional (though it may not be a problem in that case, it is if you try to provide styling as a library). You can also simply use "@<0>" to solve the weird identation, this is the way this API is implemented.

@objmagic

This comment has been minimized.

Show comment
Hide comment
@objmagic

objmagic Jun 25, 2015

Contributor

I like this idea, personally. Colored warnings and errors are definitely helpful, especially for beginners.

Contributor

objmagic commented Jun 25, 2015

I like this idea, personally. Colored warnings and errors are definitely helpful, especially for beginners.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Jun 25, 2015

Contributor

The code is better now (using Format tags). It prints warnings in magenta, locations in bold, errors in red.

Contributor

c-cube commented Jun 25, 2015

The code is better now (using Format tags). It prints warnings in magenta, locations in bold, errors in red.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Jun 26, 2015

Contributor

I should probably add some support into ocamlbuild (with a tag?)

Contributor

c-cube commented Jun 26, 2015

I should probably add some support into ocamlbuild (with a tag?)

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jun 26, 2015

Member

Yes, I was thinking of that yesterday. If you don't mind, an ocamlbuild tag would be nice -- it's rather easy to add by imitating what is already present, eg -opaque for example. You should also adapt the OCAMLPARAM parsing, I suppose.

Member

gasche commented Jun 26, 2015

Yes, I was thinking of that yesterday. If you don't mind, an ocamlbuild tag would be nice -- it's rather easy to add by imitating what is already present, eg -opaque for example. You should also adapt the OCAMLPARAM parsing, I suppose.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Jun 26, 2015

Member

Three questions:

  1. Any thoughts (from anyone) about whether colors should be enabled by default?
  2. Does Emacs deal with them, or do they have to be disabled when compiling under Emacs?
  3. Should the new code be in a new file?

Also, you definitely need to add this option to the OCAMLPARAM code.

Member

damiendoligez commented Jun 26, 2015

Three questions:

  1. Any thoughts (from anyone) about whether colors should be enabled by default?
  2. Does Emacs deal with them, or do they have to be disabled when compiling under Emacs?
  3. Should the new code be in a new file?

Also, you definitely need to add this option to the OCAMLPARAM code.

@Drup

This comment has been minimized.

Show comment
Hide comment
@Drup

Drup Jun 26, 2015

Contributor

for 2. there is always the possibility to check terminfo ... which should be done anyway. Emacs users can enable ansicolor.

Contributor

Drup commented Jun 26, 2015

for 2. there is always the possibility to check terminfo ... which should be done anyway. Emacs users can enable ansicolor.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Jun 29, 2015

Contributor

There seems to be some Terminfo handling in the compiler, but I have no idea how that works. Also, a lot of printing is made into buffers rather than directly into stdout/stderr.

Contributor

c-cube commented Jun 29, 2015

There seems to be some Terminfo handling in the compiler, but I have no idea how that works. Also, a lot of printing is made into buffers rather than directly into stdout/stderr.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Jul 12, 2015

Contributor

"Massively more usable" is a silly claim, but "massively more readable" isn't one. For that matter, even OCaml agrees and uses some formatting for the toplevel.

Contributor

whitequark commented Jul 12, 2015

"Massively more usable" is a silly claim, but "massively more readable" isn't one. For that matter, even OCaml agrees and uses some formatting for the toplevel.

Show outdated Hide outdated driver/compenv.ml
(* color output *)
| "color" ->
begin try
let b = bool_of_string v in

This comment has been minimized.

@gasche

gasche Aug 2, 2015

Member

There already is a standardized interface for boolean parameters, which is to use 0/1 (see Compenv.setter) rather than bool_of_string. You should feel free to generalize Compenv.setter to support calling an arbitrary function (as an auxiliary function).

@gasche

gasche Aug 2, 2015

Member

There already is a standardized interface for boolean parameters, which is to use 0/1 (see Compenv.setter) rather than bool_of_string. You should feel free to generalize Compenv.setter to support calling an arbitrary function (as an auxiliary function).

Show outdated Hide outdated driver/main.ml
@@ -125,6 +125,7 @@ module Options = Main_args.Make_bytecomp_options (struct
let _w = (Warnings.parse_options false)
let _warn_error = (Warnings.parse_options true)
let _warn_help = Warnings.help_warnings
let _color = Location.enable_color

This comment has been minimized.

@gasche

gasche Aug 2, 2015

Member

I am unconvinced that this is the right interface for this code. It would be more standard to have a Cflags.color : bool ref. Doing this would force you to have some logic to initialize color-printing (a Location.initialize_color function doing much of what this one does today) on the first location to print; but that would not increase the code complexity much as you already have this only-on-the-first-call logic anyway.

Also something global outside Location scales better to a future where other aspects of the compiler output could be colored (-help, -warn-help for example).

@gasche

gasche Aug 2, 2015

Member

I am unconvinced that this is the right interface for this code. It would be more standard to have a Cflags.color : bool ref. Doing this would force you to have some logic to initialize color-printing (a Location.initialize_color function doing much of what this one does today) on the first location to print; but that would not increase the code complexity much as you already have this only-on-the-first-call logic anyway.

Also something global outside Location scales better to a future where other aspects of the compiler output could be colored (-help, -warn-help for example).

Show outdated Hide outdated parsing/location.ml
@@ -250,6 +251,110 @@ let get_pos_info pos =
(pos.pos_fname, pos.pos_lnum, pos.pos_cnum - pos.pos_bol)
;;
(* basic color handling *)
module Color = struct

This comment has been minimized.

@gasche

gasche Aug 2, 2015

Member

This should go to misc (I think misc/utils.ml or misc/terminfo.ml would be reasonable; creating a new misc/ansi_colors.ml appears to be a nice choice, but there is a small risk of module conflicts among people linking compiler-libs), except for the styles stuff which makes sense in Location. mark_{open,close}_tag could either stay there or be parametrized over a tag:string -> style list option mapping.

@gasche

gasche Aug 2, 2015

Member

This should go to misc (I think misc/utils.ml or misc/terminfo.ml would be reasonable; creating a new misc/ansi_colors.ml appears to be a nice choice, but there is a small risk of module conflicts among people linking compiler-libs), except for the styles stuff which makes sense in Location. mark_{open,close}_tag could either stay there or be parametrized over a tag:string -> style list option mapping.

This comment has been minimized.

@gasche

gasche Aug 2, 2015

Member

Thinking twice: the styles stuff could remain inside Color if we consider that it should centralize all color styles in the compiler distribution (and happens to only contain location/error/warning stuff for now). Having the information that is currently in default_styles centralized in the same place may make it easier for people to customize default colors in the future. So regarding the location of the styles stuff, I would accept whatever @c-cube feels is best.

@gasche

gasche Aug 2, 2015

Member

Thinking twice: the styles stuff could remain inside Color if we consider that it should centralize all color styles in the compiler distribution (and happens to only contain location/error/warning stuff for now). Having the information that is currently in default_styles centralized in the same place may make it easier for people to customize default colors in the future. So regarding the location of the styles stuff, I would accept whatever @c-cube feels is best.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 2, 2015

Member

I think the current codebase is reasonable and could be mergeable after having been made a bit less Location-centric. (Once we open the colored box, I am sure that plenty of people will have ideas for even better color output, so it makes sense to put it in a central place from the start.)

Re. default setting: I would enable it by default on TERM values that are not empty or dumb. (I just checked: it seems that the toplevel started by Emacs modes is set with dumb, so that would disable it there by default.). There seem to be terminfo settings to retrive color support information (eg. maximal number of colors), but I would wait for a PR of someone willing to implement this.

On the other hand, checking how other projects handle this question (eg. clang?) may suffice to have a good/consensual answer.

Member

gasche commented Aug 2, 2015

I think the current codebase is reasonable and could be mergeable after having been made a bit less Location-centric. (Once we open the colored box, I am sure that plenty of people will have ideas for even better color output, so it makes sense to put it in a central place from the start.)

Re. default setting: I would enable it by default on TERM values that are not empty or dumb. (I just checked: it seems that the toplevel started by Emacs modes is set with dumb, so that would disable it there by default.). There seem to be terminfo settings to retrive color support information (eg. maximal number of colors), but I would wait for a PR of someone willing to implement this.

On the other hand, checking how other projects handle this question (eg. clang?) may suffice to have a good/consensual answer.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Aug 2, 2015

Contributor

@gasche Usually, you check the TERM and also look at the result of isatty(fileno(stdout)). This is what clang does.

Contributor

whitequark commented Aug 2, 2015

@gasche Usually, you check the TERM and also look at the result of isatty(fileno(stdout)). This is what clang does.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 3, 2015

Contributor

I'm working on @gasche 's recommendations. I moved Color to Misc.Color (for fear of module conflicts) with some one-shot initialization code. I put a flag Clflags.color (but it's not a boolean, it's a ternary value On | Off | Auto). The value Auto enables colors depending on Sys.getenv "TERM" (isatty is only available in Unix, which is not currently linked, right?).

Contributor

c-cube commented Aug 3, 2015

I'm working on @gasche 's recommendations. I moved Color to Misc.Color (for fear of module conflicts) with some one-shot initialization code. I put a flag Clflags.color (but it's not a boolean, it's a ternary value On | Off | Auto). The value Auto enables colors depending on Sys.getenv "TERM" (isatty is only available in Unix, which is not currently linked, right?).

Show outdated Hide outdated driver/main_args.ml
let mk_no_color f =
"-no-color", Arg.Unit f,
" <bool> Disable ANSI color codes for warning/error messages"

This comment has been minimized.

@gasche

gasche Aug 3, 2015

Member

It would be nice if there was a way to explicitly ask on the command line the "auto" mode -- you had a good idea in separating these three options. You could have -color-auto as a third flag, or maybe accept -color {on,off,auto} (or {always,never,auto}, these are the option names in git and grep). This would mean that the OCAMLPARAM handling code would have to be changed again...

@gasche

gasche Aug 3, 2015

Member

It would be nice if there was a way to explicitly ask on the command line the "auto" mode -- you had a good idea in separating these three options. You could have -color-auto as a third flag, or maybe accept -color {on,off,auto} (or {always,never,auto}, these are the option names in git and grep). This would mean that the OCAMLPARAM handling code would have to be changed again...

Show outdated Hide outdated ocamlbuild/ocaml_specific.ml
@@ -638,6 +638,8 @@ let () =
(fun param -> S [A "-for-pack"; A param]);
pflag ["ocaml"; "native"; "compile"] "inline"
(fun param -> S [A "-inline"; A param]);
pflag ["ocaml"; "compile"] "color"
(fun param -> S [A "-color"; A param]);

This comment has been minimized.

@gasche

gasche Aug 3, 2015

Member

This may need to be updated when you are settled on a command-line usage.

@gasche

gasche Aug 3, 2015

Member

This may need to be updated when you are settled on a command-line usage.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 3, 2015

Contributor

Indeed, forgot about the ocamlbuild tag (which should become several tags: color, no_color and color_auto, I guess). The past has shown it was good to provide an explicit option for each possible value of a parameter (including the default one), so I agree we need -color-auto and the corresponding OCAMLPARAM flag (maybe a 'a').

Contributor

c-cube commented Aug 3, 2015

Indeed, forgot about the ocamlbuild tag (which should become several tags: color, no_color and color_auto, I guess). The past has shown it was good to provide an explicit option for each possible value of a parameter (including the default one), so I agree we need -color-auto and the corresponding OCAMLPARAM flag (maybe a 'a').

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 3, 2015

Member

I'm not sure why the testsuite fails, but it may be the case that some tests in the testsuite do not run with TERM=dumb while they should (I have had issues with this in the past because the location-printing code in the toplevel depends on TERM, and some tests run a toplevel: e2b6629 ).

Member

gasche commented Aug 3, 2015

I'm not sure why the testsuite fails, but it may be the case that some tests in the testsuite do not run with TERM=dumb while they should (I have had issues with this in the past because the location-printing code in the toplevel depends on TERM, and some tests run a toplevel: e2b6629 ).

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 3, 2015

Contributor

The errors were because the output was slightly changed by this branch; the last commit should fix this.

Contributor

c-cube commented Aug 3, 2015

The errors were because the output was slightly changed by this branch; the last commit should fix this.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 3, 2015

Contributor

yay!

Contributor

c-cube commented Aug 3, 2015

yay!

Show outdated Hide outdated parsing/location.ml
print ppf loc;
fprintf ppf "Warning %a@." Warnings.print w
pp_open_tag ppf "warning";

This comment has been minimized.

@gasche

gasche Aug 3, 2015

Member

is there a particular reason for not using a format string here?

@gasche

gasche Aug 3, 2015

Member

is there a particular reason for not using a format string here?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Aug 3, 2015

Contributor

For what it's worth, keep in mind that the Windows terminal does not support ANSI escapes, despite MSDOS's famous ANSI.SYS extension that made them ubiquitous:
http://stackoverflow.com/questions/16755142/how-to-make-win32-console-recognize-ansi-vt100-escape-sequences
So, one way or another, make sure that color support is turned off in a Windows terminal.

Contributor

xavierleroy commented Aug 3, 2015

For what it's worth, keep in mind that the Windows terminal does not support ANSI escapes, despite MSDOS's famous ANSI.SYS extension that made them ubiquitous:
http://stackoverflow.com/questions/16755142/how-to-make-win32-console-recognize-ansi-vt100-escape-sequences
So, one way or another, make sure that color support is turned off in a Windows terminal.

Show outdated Hide outdated parsing/location.ml
let print_error_prefix ppf () =
setup_colors ();
pp_open_tag ppf "error";

This comment has been minimized.

@gasche

gasche Aug 3, 2015

Member

Is there a particular reason for not using a format here? (I don't mind either the format or plain-pp styles, but mixing both is a bit surprising)

@gasche

gasche Aug 3, 2015

Member

Is there a particular reason for not using a format here? (I don't mind either the format or plain-pp styles, but mixing both is a bit surprising)

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 3, 2015

Contributor

@gasche I'm moving to format strings, right.
@xavierleroy I don't have access to a windows machine, sadly. Do you know whether the TERM environment variable is set on windows? If it's not set (or equal to "dumb"), then colors will be disabled by default.

Contributor

c-cube commented Aug 3, 2015

@gasche I'm moving to format strings, right.
@xavierleroy I don't have access to a windows machine, sadly. Do you know whether the TERM environment variable is set on windows? If it's not set (or equal to "dumb"), then colors will be disabled by default.

Show outdated Hide outdated parsing/syntaxerr.ml
Location.error ~loc:opening_loc
(Printf.sprintf "Error: This '%s' might be unmatched" opening)
Location.errorf ~loc:opening_loc
"%a This '%s' might be unmatched" Location.print_error_prefix () opening

This comment has been minimized.

@gasche

gasche Aug 3, 2015

Member

Could we export a Location.errorf_prefixed to factor these Location.print_error_prefix? There are a couple of calls to Location.errorf in the compiler without the Error: prefix (I see one in ctype.ml and three in typetexp.ml), and you should feel free to convert all of them to errorf_prefixed (it is inconsistent that they don't use Error:). Users of raise_errorf is a slightly different matter, it's probably best to leave them unchanged.

@gasche

gasche Aug 3, 2015

Member

Could we export a Location.errorf_prefixed to factor these Location.print_error_prefix? There are a couple of calls to Location.errorf in the compiler without the Error: prefix (I see one in ctype.ml and three in typetexp.ml), and you should feel free to convert all of them to errorf_prefixed (it is inconsistent that they don't use Error:). Users of raise_errorf is a slightly different matter, it's probably best to leave them unchanged.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 3, 2015

Member

@alainfrisch , any opinion on the error-printing code?

Member

gasche commented Aug 3, 2015

@alainfrisch , any opinion on the error-printing code?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 3, 2015

Member

(It may look like I have new remarks to make each time I look at the patch, but I'm actually happy with the convergence and thinking of adding it to my merge list.)

Member

gasche commented Aug 3, 2015

(It may look like I have new remarks to make each time I look at the patch, but I'm actually happy with the convergence and thinking of adding it to my merge list.)

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 3, 2015

Contributor

I think the errorf_prefixed is indeed a good idea. Glad you consider merging the patch! :)

Contributor

c-cube commented Aug 3, 2015

I think the errorf_prefixed is indeed a good idea. Glad you consider merging the patch! :)

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Aug 3, 2015

Contributor

Do you know whether the TERM environment variable is set on windows? If it's not set (or equal to "dumb"), then colors will be disabled by default.

On a windows 10 tech preview IE virtual machine (available here). It seems that in cmd.exe TERM is not set at all, i.e. Sys.getenv raises Not_found.

Contributor

dbuenzli commented Aug 3, 2015

Do you know whether the TERM environment variable is set on windows? If it's not set (or equal to "dumb"), then colors will be disabled by default.

On a windows 10 tech preview IE virtual machine (available here). It seems that in cmd.exe TERM is not set at all, i.e. Sys.getenv raises Not_found.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 3, 2015

Member

Onto the merge list!

We'll leave it to the future brave soul to provide support for whatever colors-in-terminal API Windows support - I know clang prints colored diagnostics on Windows as well.

Member

gasche commented Aug 3, 2015

Onto the merge list!

We'll leave it to the future brave soul to provide support for whatever colors-in-terminal API Windows support - I know clang prints colored diagnostics on Windows as well.

Show outdated Hide outdated driver/main_args.ml
let mk_color_auto f =
"-color-auto", Arg.Unit f,
" Automatic mode for ANSI color codes for warning/error messages"

This comment has been minimized.

@gasche

gasche Aug 3, 2015

Member

You should say that this is the default.

@gasche

gasche Aug 3, 2015

Member

You should say that this is the default.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Aug 3, 2015

Contributor

Re: Windows, a quick test shows that Cygwin shell windows have TERM=xterm and support ANSI escape sequences. That's good!

Contributor

xavierleroy commented Aug 3, 2015

Re: Windows, a quick test shows that Cygwin shell windows have TERM=xterm and support ANSI escape sequences. That's good!

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 3, 2015

Contributor

@alainfrisch , any opinion on the error-printing code?

Nope.

Contributor

alainfrisch commented Aug 3, 2015

@alainfrisch , any opinion on the error-printing code?

Nope.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 5, 2015

Contributor

vim doesn't automatically change $TERM when it invokes the compiler (with :make), so it requires a workaround :set makeprg=TERM=dumb\ make. This is annoying.

The real solution would be to disable colors when stderr is not a tty, as mentioned above, but that requires Unix.isatty. Should I rather add a primitive in the compiler?

Contributor

c-cube commented Aug 5, 2015

vim doesn't automatically change $TERM when it invokes the compiler (with :make), so it requires a workaround :set makeprg=TERM=dumb\ make. This is annoying.

The real solution would be to disable colors when stderr is not a tty, as mentioned above, but that requires Unix.isatty. Should I rather add a primitive in the compiler?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 5, 2015

Member

I think having a primitive in the compiler would be nice -- it could go in byterun/sys.c. (For example, this would scale to adding Windows-specific logic in the future, whereas we wouldn't extend Unix with non-portable code.)

Member

gasche commented Aug 5, 2015

I think having a primitive in the compiler would be nice -- it could go in byterun/sys.c. (For example, this would scale to adding Windows-specific logic in the future, whereas we wouldn't extend Unix with non-portable code.)

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 5, 2015

Contributor

So you'd be ok with adding a primitive dedicated to, basically, "detecting whether some FD is color-ready"? Or whether a FD is an interactive terminal? That would currently reduce to calling "isatty" on unix, I think.

Contributor

c-cube commented Aug 5, 2015

So you'd be ok with adding a primitive dedicated to, basically, "detecting whether some FD is color-ready"? Or whether a FD is an interactive terminal? That would currently reduce to calling "isatty" on unix, I think.

Show outdated Hide outdated parsing/location.ml
| Warnings.Errors n ->
Some
(errorf ~loc:(in_file !input_name)
"Error: Some fatal warnings were triggered (%d occurrences)" n)
"%a Some fatal warnings were triggered (%d occurrences)"
print_error_prefix () n)

This comment has been minimized.

@gasche

gasche Aug 5, 2015

Member

errorf_prefixed?

@gasche

gasche Aug 5, 2015

Member

errorf_prefixed?

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Aug 5, 2015

Contributor

I think exposing the new primitive in the stdlib is a bad idea.

It can be useful in more exotic OS environments where the full Unix library is not available but reimplementing the primitive can make sense.

Contributor

dbuenzli commented Aug 5, 2015

I think exposing the new primitive in the stdlib is a bad idea.

It can be useful in more exotic OS environments where the full Unix library is not available but reimplementing the primitive can make sense.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 5, 2015

Contributor

It would be nice to have the opinion of more maintainers over this primitive.

Contributor

c-cube commented Aug 5, 2015

It would be nice to have the opinion of more maintainers over this primitive.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 5, 2015

Member

In preparation of an eventual merge, it would be nice if you could eventually rebase the patch series in a few independent patches with a good commit message. We could have, for example, one patch for Colors, one patch for the actual message printing (location and its users), one patch for the command-line stuff (and ocamlbuild and OCAMLPARAM), and one patch for the isatty stuff.

Member

gasche commented Aug 5, 2015

In preparation of an eventual merge, it would be nice if you could eventually rebase the patch series in a few independent patches with a good commit message. We could have, for example, one patch for Colors, one patch for the actual message printing (location and its users), one patch for the command-line stuff (and ocamlbuild and OCAMLPARAM), and one patch for the isatty stuff.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Aug 5, 2015

Contributor

Some random comments.

  • Don't use terminfo. Up to the late 80's, there were a lot of (physical) terminals (of the kind that's attached via a RS232 serial link), each with its own quirky command language. The termcap and later terminfo databases were essential for Unix programs to be compatible with as many terminals as possible. Nowadays, our terminals are just windows in a GUI, and they all talk ANSI escape codes (or are dumb and talk no codes at all, like Microsoft's command window). So, go ahead, send ANSI escapes if TERM=ansi or TERM=xterm. While you're at it, you could rewrite Location.highlight_locations in the same style. This way I could remove this terminfo historical junk from OCaml altogether.
  • I agree with gasche that Unix.isatty is fine for other programs, and that whatever primitive you introduce for isatty() should not be exported in the stdlib, at least initially. Keep it simple, just wrap "isatty(0)" as a unit -> bool OCaml function for compiler internal use only.
  • By the was, "isatty" is a time-honored name that, for Unix people, is clearer than "chan_is_interactive" or most other names you could come up with.
  • +1 for the remark about white backgrounds. Any color choice that is not readable on a white background will be rejected.
Contributor

xavierleroy commented Aug 5, 2015

Some random comments.

  • Don't use terminfo. Up to the late 80's, there were a lot of (physical) terminals (of the kind that's attached via a RS232 serial link), each with its own quirky command language. The termcap and later terminfo databases were essential for Unix programs to be compatible with as many terminals as possible. Nowadays, our terminals are just windows in a GUI, and they all talk ANSI escape codes (or are dumb and talk no codes at all, like Microsoft's command window). So, go ahead, send ANSI escapes if TERM=ansi or TERM=xterm. While you're at it, you could rewrite Location.highlight_locations in the same style. This way I could remove this terminfo historical junk from OCaml altogether.
  • I agree with gasche that Unix.isatty is fine for other programs, and that whatever primitive you introduce for isatty() should not be exported in the stdlib, at least initially. Keep it simple, just wrap "isatty(0)" as a unit -> bool OCaml function for compiler internal use only.
  • By the was, "isatty" is a time-honored name that, for Unix people, is clearer than "chan_is_interactive" or most other names you could come up with.
  • +1 for the remark about white backgrounds. Any color choice that is not readable on a white background will be rejected.
@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Aug 5, 2015

Contributor

Keep it simple, just wrap "isatty(0)" as a unit -> bool OCaml function for compiler internal use only.

You want to do that on stderr instead, so that if it is redirected to a file you don't produce ANSI escapes.

Contributor

dbuenzli commented Aug 5, 2015

Keep it simple, just wrap "isatty(0)" as a unit -> bool OCaml function for compiler internal use only.

You want to do that on stderr instead, so that if it is redirected to a file you don't produce ANSI escapes.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 5, 2015

Contributor

Here we are, I did a big interactive rebase. There is a new primitive in sys.c but it is not exported in Sys. The color for warnings is bold magenta, which I hope will be visible on all terminals (it is similar to what gcc and clang do).

About Terminfo, I removed the color handling part; however, I don't know how totally removing Terminfo from the compiler would work. @xavierleroy do you have in mind using ansi codes for moving the cursor in the terminal?

Contributor

c-cube commented Aug 5, 2015

Here we are, I did a big interactive rebase. There is a new primitive in sys.c but it is not exported in Sys. The color for warnings is bold magenta, which I hope will be visible on all terminals (it is similar to what gcc and clang do).

About Terminfo, I removed the color handling part; however, I don't know how totally removing Terminfo from the compiler would work. @xavierleroy do you have in mind using ansi codes for moving the cursor in the terminal?

/* Return true if the value is a filedescriptor (int) that is
* (presumably) open on an interactive terminal */
CAMLprim value caml_sys_is_interactive(value chan)

This comment has been minimized.

@gasche

gasche Aug 5, 2015

Member

I think caml_sys_isatty would be a better name, but that is nitpicking.

@gasche

gasche Aug 5, 2015

Member

I think caml_sys_isatty would be a better name, but that is nitpicking.

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Aug 6, 2015

Member

About Terminfo, I removed the color handling part; however, I don't know how totally removing Terminfo from the compiler would work. @xavierleroy do you have in mind using ansi codes for moving the cursor in the terminal?

That's already what terminfo.c does, only it does it using tputs. If we use ansi codes directly for colors we might as well use ansi codes directly for underline and cursor movements.

Member

diml commented Aug 6, 2015

About Terminfo, I removed the color handling part; however, I don't know how totally removing Terminfo from the compiler would work. @xavierleroy do you have in mind using ansi codes for moving the cursor in the terminal?

That's already what terminfo.c does, only it does it using tputs. If we use ansi codes directly for colors we might as well use ansi codes directly for underline and cursor movements.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Aug 6, 2015

Contributor

Yes, as @diml wrote, I was thinking of hard-coding ANSI escape sequences for moving the cursor and going into/out of standout mode. One difficulty I didn't anticipate is getting the number of lines of the terminal. Terminfo has some special magic to query the current size of xterm-like terminal windows. Perhaps we can hard-wire a conservative 24 lines... but it's less satisfactory.

Contributor

xavierleroy commented Aug 6, 2015

Yes, as @diml wrote, I was thinking of hard-coding ANSI escape sequences for moving the cursor and going into/out of standout mode. One difficulty I didn't anticipate is getting the number of lines of the terminal. Terminfo has some special magic to query the current size of xterm-like terminal windows. Perhaps we can hard-wire a conservative 24 lines... but it's less satisfactory.

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Aug 6, 2015

Member

For the terminal size we can use the TIOCGWINSZ ioctl (or GetConsoleScreenBufferInfo on windows), as done here: https://github.com/diml/lambda-term/blob/master/src/lTerm_term_stubs.c

Member

diml commented Aug 6, 2015

For the terminal size we can use the TIOCGWINSZ ioctl (or GetConsoleScreenBufferInfo on windows), as done here: https://github.com/diml/lambda-term/blob/master/src/lTerm_term_stubs.c

@edwintorok

This comment has been minimized.

Show comment
Hide comment
@edwintorok

edwintorok Aug 6, 2015

Contributor

@gasche "On the other hand, checking how other projects handle this question (eg. clang?) may suffice to have a good/consensual answer." Guess I am to blame for the color diagnostics in clang. It was a long time ago, but IIRC the motivation was that clang started to display source code fragments in error messages, and it became hard for me to follow what the error message is and which file it come from, and using colors/bold for the file and error message made it easier to read.

When I have initially added color diagnostics to LLVM/Clang I used the simple approach with checking isatty and that TERM is not dumb. Several years later someone changed it to use ncurses for the detection and now it uses tigetnum("colors"). Another way would be to call tput colors. Unfortunately the commit message doesn't say whether the TERM based detection was a problem in practice or not.
FWIW git and grep still use the isatty and TERM based test, I think it should be fine for OCaml too.

Also see how colors were handled in Windows without ANSI escapes. I no longer have/use Windows though so I don't know if that still works on latest versions and I doubt it'd be worth the trouble to add windows-specific color support to OCaml.

Contributor

edwintorok commented Aug 6, 2015

@gasche "On the other hand, checking how other projects handle this question (eg. clang?) may suffice to have a good/consensual answer." Guess I am to blame for the color diagnostics in clang. It was a long time ago, but IIRC the motivation was that clang started to display source code fragments in error messages, and it became hard for me to follow what the error message is and which file it come from, and using colors/bold for the file and error message made it easier to read.

When I have initially added color diagnostics to LLVM/Clang I used the simple approach with checking isatty and that TERM is not dumb. Several years later someone changed it to use ncurses for the detection and now it uses tigetnum("colors"). Another way would be to call tput colors. Unfortunately the commit message doesn't say whether the TERM based detection was a problem in practice or not.
FWIW git and grep still use the isatty and TERM based test, I think it should be fine for OCaml too.

Also see how colors were handled in Windows without ANSI escapes. I no longer have/use Windows though so I don't know if that still works on latest versions and I doubt it'd be worth the trouble to add windows-specific color support to OCaml.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 6, 2015

Contributor

I would like to keep this PR relatively simple, and I believe it is ready — as its name indicates it only adds the bare minimum for color handling. I think adding a second primitive to check the number of lines and get rid of terminfo is a good idea, but it needs not be done right now; same goes for allowing the user to choose colors or other things.

Contributor

c-cube commented Aug 6, 2015

I would like to keep this PR relatively simple, and I believe it is ready — as its name indicates it only adds the bare minimum for color handling. I think adding a second primitive to check the number of lines and get rid of terminfo is a good idea, but it needs not be done right now; same goes for allowing the user to choose colors or other things.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 6, 2015

Member

Yeah, I also think the current state is fine.

@edwintorok thanks for the write-up!

Member

gasche commented Aug 6, 2015

Yeah, I also think the current state is fine.

@edwintorok thanks for the write-up!

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 8, 2015

Member

Fun fact: because your rebased the bootstrap that was done on the old patch state, applying this patch series makes the compiler print warning in yellow, while the code specifies purple -- with no mention of yellow in the code or its history. On trusting trust, tastes and colors.

Member

gasche commented Aug 8, 2015

Fun fact: because your rebased the bootstrap that was done on the old patch state, applying this patch series makes the compiler print warning in yellow, while the code specifies purple -- with no mention of yellow in the code or its history. On trusting trust, tastes and colors.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 8, 2015

Member

Sorry to nitpick, but the current patch series isn't really usable as is -- for example, this commit is not at all what it says. Would it be possible to have another rebase? I will do it myself otherwise, but probably not today.

Other points I noticed when trying to rebase myself this morning:

  • the code says caml_sys_is_interactive, the commit message says caml_sys_isatty, you should pick one
  • I think there is a useless let color = ref false definition in one commit
Member

gasche commented Aug 8, 2015

Sorry to nitpick, but the current patch series isn't really usable as is -- for example, this commit is not at all what it says. Would it be possible to have another rebase? I will do it myself otherwise, but probably not today.

Other points I noticed when trying to rebase myself this morning:

  • the code says caml_sys_is_interactive, the commit message says caml_sys_isatty, you should pick one
  • I think there is a useless let color = ref false definition in one commit
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 9, 2015

Member

Also, I noticed during my testing that, when printing a location, the characters are not highlighted with the rest of the location (this corresponds to a small bug in print_loc).

Member

gasche commented Aug 9, 2015

Also, I noticed during my testing that, when printing a location, the characters are not highlighted with the rest of the location (this corresponds to a small bug in print_loc).

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 10, 2015

Contributor

I will do the rebase and change the bad commit/commit messages. What do you mean by non-highlighted 'characters'?

Contributor

c-cube commented Aug 10, 2015

I will do the rebase and change the bad commit/commit messages. What do you mean by non-highlighted 'characters'?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 10, 2015

Member

Only the line numbers are highlighted, not the character range.

Member

gasche commented Aug 10, 2015

Only the line numbers are highlighted, not the character range.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 10, 2015

Contributor

Right, fixed. About the bootstrap, I admit it's funny to have yellow colors (it illustrates the idea of the compiler hidden backdoor perfectly imho); I can change the bootstrap again if you want, either using the latest version or the one just after caml_sys_is_interactive was added.

Contributor

c-cube commented Aug 10, 2015

Right, fixed. About the bootstrap, I admit it's funny to have yellow colors (it illustrates the idea of the compiler hidden backdoor perfectly imho); I can change the bootstrap again if you want, either using the latest version or the one just after caml_sys_is_interactive was added.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 10, 2015

Member

I think it's nice to bootstrap with the color-enabled compiler, as it ensures that other maintainers will see the effect of the change -- early adopters! But then you should use the right colors. (Bootstrapping before is fine as well.)

You should also merge "add option handling" and "add color handling in OCAMLPARAM and ocamlbuild". Thanks for the changelog message. Would it be possible to also have a test in the testsuite that checks that the behavior is as expected for -color and -no-color?

(I'm thinking of adding two changes on top of this PR in the future: change the option handling to use -color {always,never,auto} instead, and add a color for hints. I might also experiment with having locations underlined instead of bold, or maybe not emphasized at all, because right now they are a bit invasive visually.)

Member

gasche commented Aug 10, 2015

I think it's nice to bootstrap with the color-enabled compiler, as it ensures that other maintainers will see the effect of the change -- early adopters! But then you should use the right colors. (Bootstrapping before is fine as well.)

You should also merge "add option handling" and "add color handling in OCAMLPARAM and ocamlbuild". Thanks for the changelog message. Would it be possible to also have a test in the testsuite that checks that the behavior is as expected for -color and -no-color?

(I'm thinking of adding two changes on top of this PR in the future: change the option handling to use -color {always,never,auto} instead, and add a color for hints. I might also experiment with having locations underlined instead of bold, or maybe not emphasized at all, because right now they are a bit invasive visually.)

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 10, 2015

Contributor

Ok, will do. About the bold locations, I followed what clang and gcc do, and I don't find it too invasive. It will be easy to change later anyway.

Is there something that would help me write tests that set specific options to the compiler before compiling the sample?

Contributor

c-cube commented Aug 10, 2015

Ok, will do. About the bold locations, I followed what clang and gcc do, and I don't find it too invasive. It will be easy to change later anyway.

Is there something that would help me write tests that set specific options to the compiler before compiling the sample?

c-cube added some commits Aug 5, 2015

add Color module Misc
use bold magenta for warnings (works on white background)
add option handling for colors in compiler, OCAMLPARAM and ocamlbuild
use -color, -no-color, -color-auto in the compiler
use "0", "1" or "a" for the "color" option in OCAMLPARAM
add tags 'color', 'color_auto', 'no_color' to ocamlbuild
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 10, 2015

Member

Now that I think of it, testsuite would be a pain because you'd have to disable them under Windows and other unsupported environment. I think it's not worth it, so feel free to skip these. (If color get broken, people will complain anyway, hopefully...).

An example of test controlling the command-line parameters of the compiler is lib-dynlink-bytecode.

Member

gasche commented Aug 10, 2015

Now that I think of it, testsuite would be a pain because you'd have to disable them under Windows and other unsupported environment. I think it's not worth it, so feel free to skip these. (If color get broken, people will complain anyway, hopefully...).

An example of test controlling the command-line parameters of the compiler is lib-dynlink-bytecode.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 10, 2015

Contributor

Ok. Then I think the current set of commits is ready (I re-bootstraped and fixed the location bug, please check!).

Contributor

c-cube commented Aug 10, 2015

Ok. Then I think the current set of commits is ready (I re-bootstraped and fixed the location bug, please check!).

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 15, 2015

I'll fix it at merge time, but the Changes entry should be in the 4.03.0 changelog, not 4.02.3 which is already released.

gasche commented on 052b3be Aug 15, 2015

I'll fix it at merge time, but the Changes entry should be in the 4.03.0 changelog, not 4.02.3 which is already released.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 15, 2015

The .mli file was forgotten here, it is included in the next commit. I'll rebase this, as this means the build of this commit breaks with warnings.

gasche commented on e1d347a Aug 15, 2015

The .mli file was forgotten here, it is included in the next commit. I'll rebase this, as this means the build of this commit breaks with warnings.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 15, 2015

Clflags.color is defined in the next commit.

Clflags.color is defined in the next commit.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 15, 2015

Member

Merged in trunk. Thanks for the patch!

Member

gasche commented Aug 15, 2015

Merged in trunk. Thanks for the patch!

@gasche gasche closed this Aug 15, 2015

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Aug 22, 2015

Contributor

apologies for the bad rebasing (I did rebase a lot tbh, it must have gone wrong at some iteration).

Contributor

c-cube commented Aug 22, 2015

apologies for the bad rebasing (I did rebase a lot tbh, it must have gone wrong at some iteration).

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 23, 2015

Member

Based on this caml-list discussion, it seems that the current patch breaks the ability to define a custom margin for toplevel messages. We should fix that, and add a regression test to the testsuite.

Member

gasche commented Sep 23, 2015

Based on this caml-list discussion, it seems that the current patch breaks the ability to define a custom margin for toplevel messages. We should fix that, and add a regression test to the testsuite.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Sep 23, 2015

Contributor

I don't think it breaks this feature, I think it was already broken. Formatting was done using ksprintf and buffers. I can try to fix it nonetheless.

Contributor

c-cube commented Sep 23, 2015

I don't think it breaks this feature, I think it was already broken. Formatting was done using ksprintf and buffers. I can try to fix it nonetheless.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 23, 2015

Member

Indeed, it seems you were right and the change happened between 4.01 and 4.02. Sorry for jumping to the wrong conclusions.

Member

gasche commented Sep 23, 2015

Indeed, it seems you were right and the change happened between 4.01 and 4.02. Sorry for jumping to the wrong conclusions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment