Disable spec colorization when redirecting stdout and add command line flag to re-enable

[paulhopkins]

Fixes #1369.
Fixes #4761.

This PR disables spec colorization when piping or redirecting stdout and adds a global option to either enable it again, or always disable it. Here is an example using find

$ spack find -lvf libxml2 | cat -v
-- linux-ubuntu16-x86_64 / gcc@5.4.0 ----------------------------
ivzisvs libxml2@2.9.4%gcc~python
vmkp7vv libxml2@2.9.4%gcc~python
4sypony libxml2@2.9.4%gcc+python

$ spack --color find -lvf libxml2 | cat -v
-- ^[[0;35mlinux-ubuntu16-x86_64^[[0m / ^[[0;32mgcc@5.4.0^[[0m ----------------------------
^[[0;90mivzisvs^[[0m libxml2^[[0;36m@2.9.4^[[0m^[[0;32m%gcc^[[0m^[[0;32m^[[0m^[[0;94m~python^[[0m
^[[0;90mvmkp7vv^[[0m libxml2^[[0;36m@2.9.4^[[0m^[[0;32m%gcc^[[0m^[[0;32m^[[0m^[[0;94m~python^[[0m
^[[0;90m4sypony^[[0m libxml2^[[0;36m@2.9.4^[[0m^[[0;32m%gcc^[[0m^[[0;32m^[[0m^[[0;94m+python^[[0m

For users who always prefer colored output then having command defaults, #2705, could be useful.

[adamjstewart]

What about not args.color and not args.no_color? What is the default? You can actually make these flags mutually exclusive. See spack edit -c edit for an example.

[paulhopkins]

The default is to colorize if stdout.isatty() is True, therefore the behaviour depends on whether you are piping. have redirected stdout, or not. The command line arguments override the default behaviour to either always or never colorize.

Good call on the mutually exclusive arguments, I have changed the code and pushed. I did try to copy the ls or grep style which allows --color or e.g. --color always but it always consumed the subcommand e.g. invalid choice: 'find' choose from: alway, auto, never. Let me know if this not appropriate.

[adamjstewart]

Can you fix the flake8 warnings?

[paulhopkins]

@adamjstewart Fixed the flake8 warning, didn't show up offline. Thanks

[eschnett]

Can we pull this request? It seems ready to go.

[adamjstewart]

One last request. Since you're adding new flags, can you update our tab-completion script? Just add -c --color --no-color to https://github.com/LLNL/spack/blob/develop/share/spack/spack-completion.bash#L115.

[paulhopkins]

@adamjstewart I have changed the short form of the argument to '-C' because it clashes with #3142, and add -C --color --no-color to spack-completion.bash.

[adamjstewart]

LGTM. I'm a little wary of merging core changes myself, so I'll let @tgamblin or someone else merge this one.

[tgamblin]

This LGTM but I have one request -- can the argument be reworked slightly to match the semantics of other command line tools like ls and grep?

Those support three options:

1. --color=auto

2. --color=never

3. --color=always

[adamjstewart]

I agree with @tgamblin's UI. In that case we can just drop the -C short option.

[paulhopkins]

I originally wanted to use the --color[=WHEN] approach by allowing an optional argument using nargs=?. However, if you use the plain --color version then it still consumes the next positional argument e.g. install, or find, etc. That's why I went with the --color, --no-color approach. I see now that you can also explicitly specify extra arguments such as --color=always, which does work though you have to enter the whole expression in otherwise it is not recognised.

I am afraid that this is not perfect but I have been going round and round with this, and I think it is sufficient without adding extra dummy arguments to get the correct usage statement.

[eschnett]

ping -- is this ready to be applied? It's very inconvenient to run Spack on an HPC system and not being able to use less.

[tgamblin]

@paulhopkins @adamjstewart: I rebased this on develop but noticed that the tests failed because Spec.format() now defaults to automatic colorization (color=None) instead of color=False, and this breaks places where Spec.format is used for programmatic things (like paths, module names, and variable values) and not for the user interface.

I went through the various usages of Spec.format, and here is what I found:

Wrong:

directory_layout.py:200         used in a path for directory_layout
modules.py:269                  used in module file short description
modules.py:293                  used in module name
modules.py:465                  used in an environment variable name
modules.py:466                  used in an environment variable value
modules.py:789                  used in a module name
spec.py:1363                    return string from Spec.short_spec
spec.py:2859                    return string from Spec.dep_string
spec.py:2862                    return from Spec.__str__()
cmd/mirror.py:178               used as a string sort key
cmd/module.py:152               used as output from `spack module loads`
test/directory_layout.py:59     used to validate directory_layout.py:200
test/graph.py:87                used in dot graph label
test/graph.py:88                used in dot graph label
test/graph.py:90                used in dot graph label
test/graph.py:92                used in dot graph label
test/graph.py:94                used in dot graph label
test/graph.py:96                used in dot graph label

Questionable:

spec.py:3421                    used in AmbiguousHashError msg string
package.py:922                  used in a FetchError msg string

Probably OK:

database.py:291                 passed to tty.warn
mirror.py:227                   used in tty.msg
mirror.py:234                   used in tty.msg
mirror.py:262                   used in tty.warn
package.py:908                  used in tty.warn
package.py:1653                 used in tty.msg
package.py:1712                 used in tty.msg
spec.py:1369                    return string from Spec.cshort_spec
spec.py:2939                    return from tree()
cmd/__init__.py:158             used in tty.die in disambiguate_specs()
cmd/__init__.py:202             returned as string from display_specs()
cmd/__init__.py:228             returned as string from display_specs()
cmd/__init__.py:238             returned as string from display_specs()
cmd/dependents.py:50            used in tty.msg
cmd/mirror.py:216               passed to colify() to be output
cmd/module.py:273               used as part of tty.error
cmd/uninstall.py:184            used in tty.error

The "wrong" ones are places where the return value of Spec.format is either always or sometimes are used programmatically. The "questionable" ones are error messages in exceptions, where color doesn't hurt, but seems weird to me, even though the error messages do eventually just get printed out. The "probably ok" ones are places where the specs are being output to the screen.

I think that to do this right, we should do a couple things:

1. Retain the default of no color in Spec.format, and have the "probably ok" uses say they want automatic coloring explicitly.
2. The color=None argument feels weird to me; I think the set_color and get_color calls in tty.color should just accept 'always', 'never', and 'auto', and we should pass those around.
3. It might also make some sense to have a convenience method, Spec.cformat, which has a default color='auto' instead of default color='never', and use that in the "probably ok" places.

I'll go ahead and do the stuff above since it's relatively easy and doesn't hurt the current situation. If anyone has any objections let me know.

There is one more thing to consider. To really do this right, we should be deciding on whether to colorize something based on whether the stream we'll print to is a TTY. The logic in this PR decides universally based on whether sys.stdout is a TTY, so it won't catch weird cases like only redirecting sys.stderr. I think there should be some reworking of the various color output routines so that it's easier to know where they're printing, and easier to test whether it will eventually go to a TTY. That is a fair amount of work, and probably requires rethinking the color interface a bit, so I think it can wait.

Thoughts?