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

fix some issues with color configuration #36689

Merged
merged 1 commit into from
Jul 23, 2020
Merged

Conversation

JeffBezanson
Copy link
Member

The issues here were truly surprising. Try running julia with --color=no (i guess I never do). Most color goes away, but all prompts are still bold, and while other prompts are un-colored, the Pkg prompt is still blue. I almost abandoned hope when I saw that. Then there is #30703, where redirecting to a pipe (julia | tee) removes most color, except all prompts are colored and bold, and --color=yes doesn't re-enable color. So this does the following:

  • If --color is specified and stdout or stderr is not a TTY, wrap it in an IOContext with the color setting.
  • Consistently use get on an io stream everywhere to determine whether to use color.
  • Thread the color setting through to write_prompt to disable bold.
  • Disable beep color if hascolor=false is passed to the REPL.

Needs #36688 to work. Should fix or help #36671. Fixes #30703.

@JeffBezanson JeffBezanson added the REPL Julia's REPL (Read Eval Print Loop) label Jul 16, 2020
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Epic!

@@ -849,7 +855,7 @@ end

function reset(repl::LineEditREPL)
raw!(repl.t, false)
print(repl.t, Base.text_colors[:normal])
hascolor(repl) && print(repl.t, Base.text_colors[:normal])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible odd to return a Boolean?

opts.hascolor = hascolor
if !hascolor
opts.beep_colors = [""]
end
Copy link
Member

@rfourquet rfourquet Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this logic belongs in the Options constructor taking kwargs? e.g. in the body of that method, replace beep_colors by hascolor ? beep_colors : [""] (or change the default value by hascolor ? ["\e[90m"] : [""]).
Ok, hascolor is not passed to Options... This is indeed quite annoying to have hascolor both in LineEditREPL and Options. I'm not sure which should take precedence, and how they could be merged, but this belongs to another issue/PR.

@@ -1259,21 +1259,21 @@ refresh_line(s, termbuf) = refresh_multi_line(termbuf, s)
default_completion_cb(::IOBuffer) = []
default_enter_cb(_) = true

write_prompt(terminal, s::PromptState) = write_prompt(terminal, s.p)
write_prompt(terminal, s::PromptState, hascolor::Bool) = write_prompt(terminal, s.p, hascolor)
Copy link
Member

@StefanKarpinski StefanKarpinski Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this just color? It's typically only set if the terminal has color, but it could be called with anything. Also, maybe better as a keyword argument since otherwise it's hard to know what boolean args mean.

@Keno
Copy link
Member

Keno commented Jul 16, 2020

Modulo passing CI of course ;)

@giordano
Copy link
Contributor

With this PR now I get

$ ./julia --color=yes -e 'printstyled(stderr, "Hello world\n"; color=:cyan)' 2>&1 | cat -A
^[[36mHello world^[[39m$
$ ./julia --color=yes -e '@info "Hello world"' 2>&1 | cat -A
[ Info: Hello world$

🤔

@giordano
Copy link
Contributor

giordano commented Jul 17, 2020

As far as I can see, the problem is that the stream of the global logger doesn't have the color flag:

$ ./julia --color=yes -e 'using Logging; @show get(global_logger().stream, :color, nothing)' 2>&1 | cat -A
get((global_logger()).stream, :color, nothing) = nothing$
$ ./julia --color=yes -e '@show get(stderr, :color, nothing)' 2>&1 | cat -A
get(stderr, :color, nothing) = true$
$ ./julia --color=yes -e 'using Logging; @show global_logger().stream == stderr' 2>&1 | cat -A
(global_logger()).stream == stderr = false$

even though this stream should be stderr:

function __init__()
global_logger(ConsoleLogger(stderr))
end

@codecov-commenter
Copy link

Codecov Report

Merging #36689 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #36689   +/-   ##
=======================================
  Coverage   86.12%   86.12%           
=======================================
  Files         350      350           
  Lines       65917    65917           
=======================================
  Hits        56773    56773           
  Misses       9144     9144           

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 ad4b1ec...109638e. Read the comment docs.

base/client.jl Outdated Show resolved Hide resolved
@giordano
Copy link
Contributor

giordano commented Jul 18, 2020

Seems to finally work

$ ./julia -e '@info "Hello world"' 2>&1 | cat -A
[ Info: Hello world$
$ ./julia --color=yes -e '@info "Hello world"' 2>&1 | cat -A
^[[36m^[[1m[ ^[[22m^[[39m^[[36m^[[1mInfo: ^[[22m^[[39mHello world$
$ ./julia --color=no -e '@info "Hello world"' 2>&1 | cat -A
[ Info: Hello world$
$ ./julia --color=yes -e 'using Logging; @show global_logger().stream === stderr' 2>&1 | cat -A
(global_logger()).stream === stderr = true$

🎉

@IanButterworth
Copy link
Member

🌈

@IanButterworth
Copy link
Member

IanButterworth commented Jul 18, 2020

Confirmed to work in github actions (by locally building this PR, then mounting as a github self-hosted runner)

image

@giordano
Copy link
Contributor

The failure in macOS seems unrelated? Good to merge otherwise?

- `--color=no` did not remove all color in the REPL
- color was not fully disabled by default when stdout was a pipe
- `--color=yes` did not enable color when stdout was a pipe (fixes #30703)
@JeffBezanson JeffBezanson merged commit a72cfb7 into master Jul 23, 2020
@JeffBezanson JeffBezanson deleted the jb/colorsetting branch July 23, 2020 00:45
aviatesk added a commit to aviatesk/OhMyREPL.jl that referenced this pull request Jul 24, 2020
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
- `--color=no` did not remove all color in the REPL
- color was not fully disabled by default when stdout was a pipe
- `--color=yes` did not enable color when stdout was a pipe (fixes JuliaLang#30703)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No color in printstyled when pipe is used
9 participants