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

Log color is broken sometimes #11020

Closed
gshuflin opened this issue Oct 22, 2020 · 4 comments · Fixed by #12585
Closed

Log color is broken sometimes #11020

gshuflin opened this issue Oct 22, 2020 · 4 comments · Fixed by #12585

Comments

@gshuflin
Copy link
Contributor

gshuflin commented Oct 22, 2020

The decision of whether to use color is made very early in the startup process, while the default value for the --[no-]colors flag is being computed:

register(
"--colors",
type=bool,
default=sys.stdout.isatty(),
help=(
"Whether Pants should use colors in output or not. This may also impact whether "
"some tools Pants run use color."
),
)

Instead, the --[no-]colors flag should become a ternary (either explicitly e.g. "yes, no, auto" or implicitly e.g. "true, false, None"), and in case of "auto", we should lazily detect the TTY per destination:

// TODO: Fix application of color for log-files: see https://github.com/pantsbuild/pants/issues/11020
let use_color = self.use_color.load(Ordering::SeqCst);

@gshuflin gshuflin self-assigned this Oct 22, 2020
@Eric-Arellano
Copy link
Contributor

I've noticed this. I think it's when pantsd is used. In fact, I don't think colors for the WARN part ever work for me with pantsd.

@stuhood
Copy link
Member

stuhood commented Mar 1, 2021

The way that we should likely solve this will be shaken up quite a bit by #11536, but it is still an issue.

Post #11536, we should likely convert the --colors flag into a ternary of None/True/False, and move the defaulting of the None case deeper into 1) the Console object at creation time based on stdio (which will have been replaced), and 2) the logging module, which should inspect whether the destination accepts color.

@stuhood stuhood assigned stuhood and unassigned gshuflin Mar 1, 2021
@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Mar 1, 2021

we should likely convert the --colors flag into a ternary of None/True/False

What is None here? Auto-detect what to do?

@stuhood
Copy link
Member

stuhood commented Mar 1, 2021

we should likely convert the --colors flag into a ternary of None/True/False

What is None here? Auto-detect what to do?

Yea. So could convert into an actual ternary... would probably want to do --dynamic-ui at the same time. I didn't see a reason for it before, but the ternary allow us to delay deciding what we're going to do until post-options parsing as we launch the UI, or create a Console.

EDIT: Updated the description with this info.

@stuhood stuhood removed their assignment Jun 8, 2021
stuhood added a commit that referenced this issue Aug 18, 2021
As described in #11020, stderr logging was not colorized with `pantsd`. This was due to three-ish issues:
* Whether to use color was computed via bootstrap options in the daemon, and set in logging initialization. This was too early for per-connection options to take effect.
* The `console` crate was using environment variables to detect whether to enable color rendering, and this detection needed to be turned off.
* The `colored` crate was also using environment variables which needed to be overridden.

Additionally, simplify atomicity of the logger by using an inner mostly-immutable struct inside of an `ArcSwap`.

Fixes #11020.

[ci skip-build-wheels]
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 a pull request may close this issue.

3 participants