-
Notifications
You must be signed in to change notification settings - Fork 741
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
Replace all ansi_term crates #2923
Conversation
@@ -244,10 +243,9 @@ mod time { | |||
T: FormatTime, | |||
{ | |||
if with_ansi { | |||
let style = Style::new().dimmed(); | |||
write!(writer, "{}", style.prefix())?; | |||
write!(writer, "\x1B[2m")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newer tracing-subscriber
has removed with_ansi
format support.
to keep it simple, I copy the dimmed
style and replace its usage in write!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like console
supports this now? https://docs.rs/console/latest/console/struct.Style.html#method.dim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't allow to get prefix
and suffix
.
timer.format_time(writer)?;
will do write!
so we can't wrap it with style
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is how console
render ANSI string https://github.com/console-rs/console/blob/master/src/utils.rs#L653-L656
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I see what you mean now!
I see the newer |
@jasl could you fix the merge conflicts? |
I can, but I'm also thinking, what if we replace So, if we switch to What do you think? |
I just searched |
# Conflicts: # Cargo.lock # substrate/client/informant/Cargo.toml # substrate/client/informant/src/display.rs # substrate/client/informant/src/lib.rs # substrate/client/tracing/Cargo.toml Fix compile Fix compile Fix compile
# Conflicts: # substrate/client/tracing/Cargo.toml
color: impl Into<ColorOrStyle>, | ||
data: impl ToString, | ||
) -> impl Display { | ||
fn print(&self, color: Color, attr: Option<Attribute>, data: impl Display) -> impl Display { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkchr console
API design is not good here. Do you have any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the problem here? I don't see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ansi_term
, the function can just pass Colour::Red.bold()
to get red and bold style, but console
doesn't provide such fluent API, I separate color
and attribute
, and because there are usages do not have attribute
, so I have to make it optional which feels not good than previous one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine. This is some internal api any way.
Note: |
So, you don't want to do this? I'm fine with either way. |
I decide to move forward with this one. |
PR doc attached |
@@ -244,10 +243,9 @@ mod time { | |||
T: FormatTime, | |||
{ | |||
if with_ansi { | |||
let style = Style::new().dimmed(); | |||
write!(writer, "{}", style.prefix())?; | |||
write!(writer, "\x1B[2m")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like console
supports this now? https://docs.rs/console/latest/console/struct.Style.html#method.dim
Co-authored-by: Bastian Köcher <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that we don't need all these to_string
. Sorry 🙈
@@ -244,10 +243,9 @@ mod time { | |||
T: FormatTime, | |||
{ | |||
if with_ansi { | |||
let style = Style::new().dimmed(); | |||
write!(writer, "{}", style.prefix())?; | |||
write!(writer, "\x1B[2m")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I see what you mean now!
Co-authored-by: Bastian Köcher <[email protected]>
@bkchr Do I need to send PR for the bridge directly to the repo? I remember they have a standalone repo (?). |
No the standalone repo doesn't exist anymore. |
The CI pipeline was cancelled due to failure one of the required jobs. |
The CI fails seems unrelated
UPDATE:
It seems my PR changed the output style, let me dig it |
Head branch was pushed to by a user without write access
@bkchr Sorry to bother you again... when I checked the test fails, I saw See https://github.com/console-rs/console/blob/master/src/utils.rs#L42-L49 and https://github.com/console-rs/console/blob/master/src/utils.rs#L625-L632 If we use the I already prepared the code BTW, the CI fails shows I need to get approve from |
Head branch was pushed to by a user without write access
@bkchr Sorry for you have to review one more time |
@@ -652,36 +654,4 @@ mod tests { | |||
assert!(stderr.contains(&line)); | |||
} | |||
} | |||
|
|||
#[test] | |||
fn control_characters_are_always_stripped_out_from_the_log_messages() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case is impossible now, so the test is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkchr If we only use console
to style strings, when it renders ANSI strings, it will check the global colors_enabled()
and colors_enabled_stderr()
, if they're false
, it will skip control codes.
So we don't need to strip ANSI control codes because they will not be rendered if colors diasbled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only restriction is we must style strings using console
only, it doesn't take care of other sources of ANSI control codes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this case, console
won't sanitize the RAW_LINE
.
But if we only use console
, it is impossible to get a string with ANSI control codes.
} | ||
|
||
// NOTE: When making any changes here make sure to also change this function in `sp-panic-handler`. | ||
fn strip_control_codes(input: &str) -> std::borrow::Cow<str> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not remove this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is console
taking care of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://github.com/console-rs/console/blob/master/src/utils.rs#L625-L632
When console
renders a styled string, it will check the global colors_enabled()
and colors_enabled_stderr()
.
If false, it will skip rendering ANSI control codes.
So I think console
taking care of this.
There is only one restriction we need to ensure we only use console
for the ANSI styling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the global settings, check https://github.com/console-rs/console/blob/master/src/utils.rs#L30-L75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean.
Do you mean the case that runtime panicked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question:
Why need to call strip_control_codes(msg)
in panic_hook()
of sp-panic-handler
?
The PanicInfo#payload()
should be the argument of panic!
. When will it contain ANSI control codes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIR it is just a protection against someone being able to trigger a panic and putting ANSI control codes into it. "It is possible", but very unlikely.
Co-authored-by: Bastian Köcher <[email protected]>
Kindly ping @bkontur @serban300 @acatangiu |
@bkchr required reviewers have been approved and CI green. For the remaining questions, do you have a conclusion? |
bbb8668
Ty @jasl for the PR and all the work. |
Now Polkadot-SDK is ansi_term free --------- Co-authored-by: Bastian Köcher <[email protected]>
Now Polkadot-SDK is ansi_term free