Skip to content
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

Make with_output_color respect have_color #36671

Closed
wants to merge 1 commit into from

Conversation

giordano
Copy link
Contributor

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
Copy link
Member

Keno commented Jul 15, 2020

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

@giordano
Copy link
Contributor Author

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
Copy link
Member

Keno commented Jul 15, 2020

--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
Copy link
Contributor Author

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
Copy link
Member

Keno commented Jul 15, 2020

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
Copy link
Contributor Author

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

@tkf
Copy link
Member

tkf commented Jul 15, 2020

Maybe do this

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

for PipeEndpoint as well?

@tkf
Copy link
Member

tkf commented Jul 15, 2020

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
Copy link
Member

Keno commented Jul 15, 2020

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

@tkf
Copy link
Member

tkf commented Jul 15, 2020

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

@tkf
Copy link
Member

tkf commented Jul 15, 2020

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
Copy link
Member

Keno commented Jul 15, 2020

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

@kimikage
Copy link
Contributor

cf #30703

@giordano
Copy link
Contributor Author

Yes, also with a pipe stdout is a PipeEndpoint:

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

@giordano
Copy link
Contributor Author

giordano commented Jul 15, 2020

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

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
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
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
Copy link

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

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
Copy link
Contributor Author

giordano commented Jul 15, 2020

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
Copy link
Member

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
Copy link
Contributor Author

Closing in favour of #36689 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants