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

Use OCAML_COLOR environment variable for deciding whether to use colors in output #1098

Merged
merged 1 commit into from Mar 15, 2017

Conversation

Projects
None yet
4 participants
@hannesm
Member

hannesm commented Mar 11, 2017

Since 4.03, OCaml supports coloring its messages to standard output and standard
error, depending on the -color argument ({always,never,auto}). The semantics
of always and never are unmodified, but auto (default) now takes the value
of the environment variable OCAML_COLOR into account.

The user should have control over coloring (or not) OCamls output messages. A
widely used build tool executes OCaml not under a tty (and OCaml does not
colorize errors and warnings), which lead various packages use color(always)
in their _tags files, which breaks with other (non-interactive) programs (i.e.
editor helpers).

Motivation for this PR stems from bug reports mirage/mirage#797 and mirage/mirage#761 (comment).

An extensive discussion where the solution implemented here was proposed is
ocaml/ocamlbuild#87 (thanks to @Gbury, @gasche, and @dbuenzli for the discussion, sorry @Gbury if you intended to submit a similar PR).

I'd consider this as a minor bugfix which enables proper coloring, and would be glad if included in the 4.05 release.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 11, 2017

Member

I could agree in principle with having a feature in 4.05. Some remarks:

  • you need a Changes entry
  • the documentation of color-setting should be updated (in manual/manual/cmds/unified-options.etex and man/ocamlc.m)
  • I wonder whether this should be a yes/no setting or again an always/never/auto setting. I like the idea of having the same choice structure as the command-line, although it is not completely clear what OCAML_COLOR=auto would be for (besides a way to override and disable a previous choice for people like me who forgot how to unset environment variables).
Member

gasche commented Mar 11, 2017

I could agree in principle with having a feature in 4.05. Some remarks:

  • you need a Changes entry
  • the documentation of color-setting should be updated (in manual/manual/cmds/unified-options.etex and man/ocamlc.m)
  • I wonder whether this should be a yes/no setting or again an always/never/auto setting. I like the idea of having the same choice structure as the command-line, although it is not completely clear what OCAML_COLOR=auto would be for (besides a way to override and disable a previous choice for people like me who forgot how to unset environment variables).
@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm Mar 11, 2017

Member

@gasche thanks for your quick feedback.

I fixed your first and second remark in 7db01ef.

I think OCAML_COLOR being a boolean variable is all we need. Other ways to influence the coloring output (such as command-line argument, or via the _tags file) take precedence over the environment variable anyways. Only if color auto, the environment variable OCAML_COLOR is considered.

Member

hannesm commented Mar 11, 2017

@gasche thanks for your quick feedback.

I fixed your first and second remark in 7db01ef.

I think OCAML_COLOR being a boolean variable is all we need. Other ways to influence the coloring output (such as command-line argument, or via the _tags file) take precedence over the environment variable anyways. Only if color auto, the environment variable OCAML_COLOR is considered.

@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm Mar 11, 2017

Member

documentation of -color documentation is missing entirely from ocamlopt.m, should I copy + paste it from ocamlc.m?

Member

hannesm commented Mar 11, 2017

documentation of -color documentation is missing entirely from ocamlopt.m, should I copy + paste it from ocamlc.m?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 11, 2017

Member

Yes, I think duplicating in ocamlopt.m would probably be good. (It would be nice to factorize the duplicated part, like @Octachron did for the manual with unified-options; I do some related manpage-stitching in ocamlbuild's Makefile -- there the list of options is generated from the --help rather than just shared, but the same build-time generation would work for this. Food for thought for another GPR.)

@dbuenzli would you by chance be willing to give an opinion on the proposed UI?

Member

gasche commented Mar 11, 2017

Yes, I think duplicating in ocamlopt.m would probably be good. (It would be nice to factorize the duplicated part, like @Octachron did for the manual with unified-options; I do some related manpage-stitching in ocamlbuild's Makefile -- there the list of options is generated from the --help rather than just shared, but the same build-time generation would work for this. Food for thought for another GPR.)

@dbuenzli would you by chance be willing to give an opinion on the proposed UI?

@dbuenzli

@gasche says:

or again an always/never/auto setting. I like the idea of having the same choice structure as the command-line,

FWIW that's what happen if you use this cmdliner term (edited wrong link) and specify and environment variable via the env argument.

Show outdated Hide outdated utils/misc.ml
&& isatty stderr
try
let v = String.lowercase_ascii (Sys.getenv "OCAML_COLOR") in
List.mem v ["1";"true";"yes"]

This comment has been minimized.

@dbuenzli

dbuenzli Mar 13, 2017

Contributor

I'd say there are a few problems with this implementation:

  1. You implicitly equate "" with Never, I wouldn't, I would equate it with Auto.
  2. You cannot give error messages on typos. E.g. if I write say tru or y this will be Never.
  3. It seems to me that environment variable resolution logic is wrong. The environment variable should only be consulted if no option is specified on the command line (i.e. the environment variable changes the default value of the parameter). This function seems to be invoked below on Auto, so if I understand correctly in the OCAML_COLOR=yes ocamlc -color auto invocation I will get unconditional colors whereas it should be the auto behaviour (the cli takes precedence).
@dbuenzli

dbuenzli Mar 13, 2017

Contributor

I'd say there are a few problems with this implementation:

  1. You implicitly equate "" with Never, I wouldn't, I would equate it with Auto.
  2. You cannot give error messages on typos. E.g. if I write say tru or y this will be Never.
  3. It seems to me that environment variable resolution logic is wrong. The environment variable should only be consulted if no option is specified on the command line (i.e. the environment variable changes the default value of the parameter). This function seems to be invoked below on Auto, so if I understand correctly in the OCAML_COLOR=yes ocamlc -color auto invocation I will get unconditional colors whereas it should be the auto behaviour (the cli takes precedence).
@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm Mar 14, 2017

Member

@gasche @dbuenzli thanks for the remarks, I updated code and documentation in 312deb4 to fulfill your suggestions (OCAML_COLOR is only considered if no -color provided; it is tri-state now; fails on unknown values (is it ok to throw invalid_arg here?))

Member

hannesm commented Mar 14, 2017

@gasche @dbuenzli thanks for the remarks, I updated code and documentation in 312deb4 to fulfill your suggestions (OCAML_COLOR is only considered if no -color provided; it is tri-state now; fails on unknown values (is it ok to throw invalid_arg here?))

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Mar 14, 2017

Member

is it ok to throw invalid_arg here?

No: Invalid_argument means a bug in your code. You should handle this case by printing a warning as compenv.ml does for OCAMLPARAM.

Member

damiendoligez commented Mar 14, 2017

is it ok to throw invalid_arg here?

No: Invalid_argument means a bug in your code. You should handle this case by printing a warning as compenv.ml does for OCAMLPARAM.

@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm Mar 14, 2017

Member

@damiendoligez thanks. I moved the environment variable parsing in 1397c84 to driver/compmisc.ml (or should it be in compenv.ml? I cannot easily see what the purpose of the two separate files is), and report a warning instead of throwing an invalid_arg

Member

hannesm commented Mar 14, 2017

@damiendoligez thanks. I moved the environment variable parsing in 1397c84 to driver/compmisc.ml (or should it be in compenv.ml? I cannot easily see what the purpose of the two separate files is), and report a warning instead of throwing an invalid_arg

@gasche

gasche approved these changes Mar 14, 2017

I like the new version. I find the explicit call to Compmisc.read_color_env right after Clflags.parse_arguments a bit irritating (it feels like breaking some abstraction convention), but I don't see a better way to do it right now -- I think that if more options start doing this we could see a pattern emerge for a more abstract/general construction.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 15, 2017

Member

@hannesm I'd like to merge, but I wonder if you would like to do a squash/fixup part first; in particular, I think that the explanations in your first commit message may need to be amended with the behavior implemented by the later fixup commits.

Member

gasche commented Mar 15, 2017

@hannesm I'd like to merge, but I wonder if you would like to do a squash/fixup part first; in particular, I think that the explanations in your first commit message may need to be amended with the behavior implemented by the later fixup commits.

Respect OCAML_COLOR environment variable for deciding whether to use …
…colors

Since 4.03, OCaml supports coloring its messages to standard output and standard
error, depending on the "-color" argument ({always,never,auto}).  This commit
adds support for the environment variable "OCAML_COLOR" (which value can as well
be {always,never,auto}).

The command line argument "-color" takes precedence, "OCAML_COLOR" is only
taken into consideration if no "-color" is provided.

The motivation for this is that the user should have control over coloring
OCaml's output messages.  OCamlbuild, a widely used build tool executes OCaml
not under a tty (and OCaml does not colorize errors and warnings), which lead
various packages use `color(always)` in their `_tags` files, which breaks with
other (non-interactive) programs (i.e.  editor helpers).

Further discussion was done at ocaml/ocamlbuild#87 and
#1098.
@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm Mar 15, 2017

Member

@gasche thanks. I squashed, amended the commit message and force-pushed

Member

hannesm commented Mar 15, 2017

@gasche thanks. I squashed, amended the commit message and force-pushed

@gasche gasche merged commit 9b16ad1 into ocaml:trunk Mar 15, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

gasche added a commit that referenced this pull request Mar 15, 2017

Respect OCAML_COLOR environment variable for deciding whether to use …
…colors

Since 4.03, OCaml supports coloring its messages to standard output and standard
error, depending on the "-color" argument ({always,never,auto}).  This commit
adds support for the environment variable "OCAML_COLOR" (which value can as well
be {always,never,auto}).

The command line argument "-color" takes precedence, "OCAML_COLOR" is only
taken into consideration if no "-color" is provided.

The motivation for this is that the user should have control over coloring
OCaml's output messages.  OCamlbuild, a widely used build tool executes OCaml
not under a tty (and OCaml does not colorize errors and warnings), which lead
various packages use `color(always)` in their `_tags` files, which breaks with
other (non-interactive) programs (i.e.  editor helpers).

Further discussion was done at ocaml/ocamlbuild#87 and
#1098.
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 15, 2017

Member

Merged, and cherry-picked in 4.05 as d377215 . (It is borderline to argue that this is a bugfix rather than a feature, but it is also a fairly non-invasive change that we should probably have merged as soon as colors were introduced.)

Member

gasche commented Mar 15, 2017

Merged, and cherry-picked in 4.05 as d377215 . (It is borderline to argue that this is a bugfix rather than a feature, but it is also a fairly non-invasive change that we should probably have merged as soon as colors were introduced.)

@hannesm hannesm deleted the hannesm:color-env branch Mar 15, 2017

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Respect OCAML_COLOR environment variable for deciding whether to use …
…colors

Since 4.03, OCaml supports coloring its messages to standard output and standard
error, depending on the "-color" argument ({always,never,auto}).  This commit
adds support for the environment variable "OCAML_COLOR" (which value can as well
be {always,never,auto}).

The command line argument "-color" takes precedence, "OCAML_COLOR" is only
taken into consideration if no "-color" is provided.

The motivation for this is that the user should have control over coloring
OCaml's output messages.  OCamlbuild, a widely used build tool executes OCaml
not under a tty (and OCaml does not colorize errors and warnings), which lead
various packages use `color(always)` in their `_tags` files, which breaks with
other (non-interactive) programs (i.e.  editor helpers).

Further discussion was done at ocaml/ocamlbuild#87 and
ocaml#1098.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment