Make with_output_color respect have_color

[giordano]

Since v1.6 has to be the most colourful release yet, I thought it would be nice to have logging use colours in CI, which is currently never the case in GitHub Actions for example.

I'm not sure this is the right way to check whether the function should use colours since it ignores the stream, but I believe iscolor should be true when --color=yes is passed. I figured I'd open a questionable PR to get the right answer 🙂

[Keno]

Why doesn't the io object have the color flag set? I don't think checking the global flag is correct here.

[giordano]

Why doesn't the io object have the color flag set?

On GitHub Actions it's not clear.

I don't think checking the global flag is correct here.

Fair enough, but at least checking whether --color=yes is set should be?

[Keno]

--color=yes should just force the color flag to be set on stdout. It's supposed to be a force override for the tty detection.

[giordano]

On GitHub Actions, stdout isn't a TTY:

stdout = Base.PipeEndpoint(RawFD(0x0000000d) open, 0 bytes waiting)

But ANSI escape sequences are rendered correctly as colours when forced.

[Keno]

Right, so --color=yes should override wherever we add the color property to try objects and also do that for other kinds of io.

[giordano]

Sorry, I'm not sure what you suggest to do.

[tkf]

Maybe do this

julia/base/ttyhascolor.jl

Line 25 in 8320fcc

get(::TTY, key::Symbol, default) = key === :color ? get_have_color() : default

for PipeEndpoint as well?

[tkf]

But I think it's bad how tput on stdout is reflected to TTY and PipeEndpoint of potentially irrelevant file descriptors. It'd be nice if TTY and PipeEndpoint structs have have_color field (maybe that's what Keno was saying).

[Keno]

Just wrap stdout in an iocontext when the io object is initialized and the color flag is set

[tkf]

Doesn't it break redirect_stdout etc.? Though maybe they should work with IOContext.

[tkf]

julia> methods(redirect_stdout)
# 4 methods for generic function "redirect_stdout":
[1] redirect_stdout(handle::Union{IOStream, Base.LibuvStream}) in Base at stream.jl:1148
[2] redirect_stdout() in Base at stream.jl:1154
[3] redirect_stdout(::Base.DevNull) in Base at stream.jl:1160
[4] redirect_stdout(f::Function, stream) in Base at stream.jl:1213

To edit a specific method, type the corresponding number into the REPL and press Ctrl+Q

julia> redirect_stdout(IOContext(stdout, :color => false))
ERROR: MethodError: no method matching redirect_stdout(::IOContext{Base.TTY})

[Keno]

Yeah, just fix those to make them work with an IOContext

[kimikage]

cf #30703

[giordano]

Yes, also with a pipe stdout is a PipeEndpoint:

% julia -e '@show stdout' | cat
stdout = Base.PipeEndpoint(RawFD(0x0000000e) open, 0 bytes waiting)

[giordano]

I had a look at wrapping stdout in an IOContext: stdout is defined in

julia/base/coreio.jl

Line 31 in 7748761

stdout = Core.stdout

IOContext is defined in

julia/base/show.jl

Lines 222 to 230 in 7748761

	struct IOContext{IO_t <: IO} <: AbstractPipe
	io::IO_t
	dict::ImmutableDict{Symbol, Any}
	function IOContext{IO_t}(io::IO_t, dict::ImmutableDict{Symbol, Any}) where IO_t<:IO
	@assert !(IO_t <: IOContext) "Cannot create `IOContext` from another `IOContext`."
	return new(io, dict)
	end
	end

get_have_color is defined in

julia/base/ttyhascolor.jl

Lines 17 to 21 in 7748761

	function get_have_color()
	global have_color
	have_color === nothing && (have_color = ttyhascolor())
	return have_color::Bool
	end

base/coreio.jl is the very first file included in base/Base.jl

julia/base/Base.jl

Line 36 in 7748761

include("coreio.jl")

All other files are loaded further below, so I don't see how it's possible to wrap stdout as suggested without reshuffling some definitions (which I assume isn't that straightforward to do).

As an alternative, what about this change in this PR:

@@ -69,7 +69,7 @@ text_colors
 
 function with_output_color(@nospecialize(f::Function), color::Union{Int, Symbol}, io::IO, args...; bold::Bool = false)
     buf = IOBuffer()
-    iscolor = get(io, :color, false)::Bool
+    iscolor = io === stdout ? get_have_color() : get(io, :color, false)::Bool
     try f(IOContext(buf, io), args...)
     finally
         str = String(take!(buf))

with_output_color would use get_have_color only for stdout, as intended, and fallback to get(io, :color) in all other cases. I ran make testall locally and all tests pass (apart from a couple in Sockets that seem related to DNS issues, not printing). Also a couple of doctests that I saw failing here would be successful with this change.

Related to #30703, with this change:

$ julia -e 'printstyled("Hello world\n"; color=:cyan)' | cat -A
^[[36mHello world^[[39m$
$ julia --color=yes -e 'printstyled("Hello world\n"; color=:cyan)' | cat -A
^[[36mHello world^[[39m$
$ julia --color=no -e 'printstyled("Hello world\n"; color=:cyan)' | cat -A
Hello world$

[codecov-commenter]

Codecov Report

Merging #36671 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #36671      +/-   ##
==========================================
+ Coverage   86.14%   86.17%   +0.02%     
==========================================
  Files         349      349              
  Lines       65855    65855              
==========================================
+ Hits        56734    56753      +19     
+ Misses       9121     9102      -19     

Impacted Files	Coverage Δ
base/bitset.jl	91.74% <0.00%> (-1.46%)	⬇️
stdlib/SparseArrays/src/sparsematrix.jl	89.68% <0.00%> (-0.11%)	⬇️
stdlib/LinearAlgebra/src/triangular.jl	96.57% <0.00%> (+0.25%)	⬆️
base/loading.jl	82.59% <0.00%> (+0.26%)	⬆️
stdlib/SparseArrays/src/linalg.jl	95.24% <0.00%> (+0.29%)	⬆️
base/missing.jl	91.25% <0.00%> (+0.54%)	⬆️
stdlib/Distributed/src/remotecall.jl	97.64% <0.00%> (+1.41%)	⬆️
base/weakkeydict.jl	95.31% <0.00%> (+1.56%)	⬆️
stdlib/LinearAlgebra/src/givens.jl	62.61% <0.00%> (+4.67%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29826c2...7ea6d58. Read the comment docs.

[JeffBezanson]

I think the main problem is that multiple places check get(io, :color, false), and this would introduce a discrepancy. Also get_have_color calls ttyhascolor, which will colorize output even if it is redirected to a file. I also worry about command line options changing the output that a program sends to some random I/O stream. I think the right way to do this is to manually pass :color=>true when you know you have a non-TTY that wants color ouput --- in other words the testsuite code should call redirect_stdout to something wrapped in an IOContext.

[giordano]

I think the main problem is that multiple places check get(io, :color, false), and this would introduce a discrepancy.

We maybe can find a good interface?

Also get_have_color calls ttyhascolor, which will colorize output even if it is redirected to a file.

Not if --color=no like in the example above. Sometimes one really wants to have a colourful output also when redirecting to file/pipe. Should we introduce a --color=force?

in other words the testsuite code should call redirect_stdout to something wrapped in an IOContext.

It doesn't look like redirect_stdout works with IOContext?

julia> methods(redirect_stdout)
# 3 methods for generic function "redirect_stdout":
[1] redirect_stdout() in Base at stream.jl:1098
[2] redirect_stdout(handle::Union{IOStream, Base.LibuvStream}) in Base at stream.jl:1092
[3] redirect_stdout(f::Function, stream) in Base at stream.jl:1148

[JeffBezanson]

stdout = Core.stdout

Good news; it does not need to be set that early. We can re-set stdout while processing command line arguments. It should also be possible to implement the redirect functions for IOContext; I will experiment.

[giordano]

Closing in favour of #36689 🙂