Replace all ansi_term crates #2923

jasl · 2024-01-13T15:34:52Z

Now Polkadot-SDK is ansi_term free

jasl · 2024-01-13T15:59:30Z

substrate/client/tracing/src/logging/event_format.rs

@@ -244,10 +243,9 @@ mod time {
 		T: FormatTime,
 	{
 		if with_ansi {
-			let style = Style::new().dimmed();
-			write!(writer, "{}", style.prefix())?;
+			write!(writer, "\x1B[2m")?;


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!

Looks like console supports this now? https://docs.rs/console/latest/console/struct.Style.html#method.dim

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

Here is how console render ANSI string https://github.com/console-rs/console/blob/master/src/utils.rs#L653-L656

Yeah I see what you mean now!

jasl · 2024-01-13T18:23:30Z

I see the newer tracing-subscriber uses nu-ansi-term, which is a fork of the retired ansi_term crate.
What do you think about using nu-ansi-term instead of console?

bkchr · 2024-07-17T12:38:57Z

@jasl could you fix the merge conflicts?

jasl · 2024-07-17T14:51:15Z

@jasl could you fix the merge conflicts?

I can, but I'm also thinking, what if we replace console with nu_ansi_term ?
because nu_ansi_term is 100% compatible with the obsoleted ansi_term, and a few dependencies use it, too.

So, if we switch to nu_ansi_term, we can simplify dependencies a bit, and there's no feature or performance difference.

What do you think?

jasl · 2024-07-17T14:56:21Z

I just searched Cargo.toml, only tracing-subscriber uses nu-ansi-term, rebase this one seems OK

# 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

jasl · 2024-07-17T18:14:59Z

substrate/client/informant/src/lib.rs

-		color: impl Into<ColorOrStyle>,
-		data: impl ToString,
-	) -> impl Display {
+	fn print(&self, color: Color, attr: Option<Attribute>, data: impl Display) -> impl Display {


@bkchr console API design is not good here. Do you have any suggestions?

What is the problem here? I don't see it.

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

I think this is fine. This is some internal api any way.

jasl · 2024-07-17T18:19:02Z

Note: bridges/relay uses ansi_term, I will make another PR later

bkchr · 2024-07-17T19:14:51Z

I can, but I'm also thinking, what if we replace console with nu_ansi_term ?

So, you don't want to do this? I'm fine with either way.

jasl · 2024-07-17T19:18:12Z

I can, but I'm also thinking, what if we replace console with nu_ansi_term ?

So, you don't want to do this? I'm fine with either way.

I decide to move forward with this one.

jasl · 2024-07-17T19:23:01Z

PR doc attached

substrate/client/informant/src/lib.rs

bkchr · 2024-07-17T19:29:59Z

substrate/client/tracing/src/logging/event_format.rs

@@ -244,10 +243,9 @@ mod time {
 		T: FormatTime,
 	{
 		if with_ansi {
-			let style = Style::new().dimmed();
-			write!(writer, "{}", style.prefix())?;
+			write!(writer, "\x1B[2m")?;


Looks like console supports this now? https://docs.rs/console/latest/console/struct.Style.html#method.dim

Co-authored-by: Bastian Köcher <git@kchr.de>

bkchr

I just realized that we don't need all these to_string. Sorry 🙈

substrate/client/informant/src/lib.rs

bkchr · 2024-07-17T20:30:45Z

substrate/client/tracing/src/logging/event_format.rs

@@ -244,10 +243,9 @@ mod time {
 		T: FormatTime,
 	{
 		if with_ansi {
-			let style = Style::new().dimmed();
-			write!(writer, "{}", style.prefix())?;
+			write!(writer, "\x1B[2m")?;


Yeah I see what you mean now!

Co-authored-by: Bastian Köcher <git@kchr.de>

jasl · 2024-07-17T20:49:30Z

@bkchr Do I need to send PR for the bridge directly to the repo? I remember they have a standalone repo (?).

bkchr · 2024-07-17T20:50:19Z

No the standalone repo doesn't exist anymore.

lexnv

LGTM! Thanks for contributing!

paritytech-cicd-pr · 2024-07-19T09:58:46Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6749196

jasl · 2024-07-19T13:15:54Z

The CI fails seems unrelated

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 13 filtered out; finished in 72.00s
--- TRY 1 STDERR:        staging-node-cli::basic wasm_big_block_import_fails ---
2024-07-19T09:54:49.791584Z ERROR runtime::timestamp: `pallet_timestamp::UnixTime::now` is called at genesis, invalid value returned: 0    
thread 'wasm_big_block_import_fails' panicked at substrate/primitives/io/src/lib.rs:1567:36:
Failed to allocate memory: "Allocator ran out of space"

UPDATE:

--- TRY 6 STDERR:        sc-tracing logging::tests::control_characters_are_always_stripped_out_from_the_log_messages ---
thread 'logging::tests::control_characters_are_always_stripped_out_from_the_log_messages' panicked at substrate/client/tracing/src/logging/mod.rs:680:13:
assertion failed: stderr.contains("\x1B[31mERROR\x1B[0m")

It seems my PR changed the output style, let me dig it

jasl · 2024-07-19T15:55:40Z

@bkchr Sorry to bother you again... when I checked the test fails, I saw console has a global ANSI style control

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 console style control, we don't need to pass enable_color and custom output format, we can remove all relates codes, do you suggest I append the change to the PR, or make a following PR?

I already prepared the code

BTW, the CI fails shows I need to get approve from
@bkontur @serban300 @acatangiu
Could you help review this PR?

jasl · 2024-07-20T05:39:48Z

@bkchr
Since I need to get approval from bridge devs,
I pushed a refactor here. We no longer need manual ANSI-style stripping, which can simplify the implementations

Sorry for you have to review one more time

jasl · 2024-07-20T05:42:01Z

substrate/client/tracing/src/logging/mod.rs

@@ -652,36 +654,4 @@ mod tests {
 			assert!(stderr.contains(&line));
 		}
 	}
-
-	#[test]
-	fn control_characters_are_always_stripped_out_from_the_log_messages() {


The case is impossible now, so the test is no longer needed.

@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

The only restriction is we must style strings using console only, it doesn't take care of other sources of ANSI control codes

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.

prdoc/pr_2923.prdoc

substrate/client/informant/src/display.rs

substrate/client/informant/src/lib.rs

substrate/client/tracing/src/logging/event_format.rs

substrate/client/tracing/src/logging/mod.rs

bkchr · 2024-07-20T15:45:44Z

substrate/client/tracing/src/logging/event_format.rs

-}
-
-// 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> {


We should not remove this code.

Or is console taking care of this?

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.

About the global settings, check https://github.com/console-rs/console/blob/master/src/utils.rs#L30-L75

is this change OK?

I'm not sure what you mean.
Do you mean the case that runtime panicked?

https://github.com/paritytech/polkadot-sdk/pull/2923/files#r1685265036 I mean this one.

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?

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 <git@kchr.de>

jasl · 2024-07-23T11:50:16Z

Kindly ping @bkontur @serban300 @acatangiu
Could you help to review my PR?

jasl · 2024-07-23T14:04:57Z

@bkchr required reviewers have been approved and CI green.

For the remaining questions, do you have a conclusion?

bkchr · 2024-07-23T16:43:32Z

Ty @jasl for the PR and all the work.

Now Polkadot-SDK is ansi_term free --------- Co-authored-by: Bastian Köcher <git@kchr.de>

jasl requested a review from koute as a code owner January 13, 2024 15:34

jasl commented Jan 13, 2024

View reviewed changes

bkchr approved these changes Jan 14, 2024

View reviewed changes

jasl added 4 commits July 18, 2024 02:06

Remove all ansi_term::Colour usage

d1ed7b0

# 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

Remove usage of ansi_term::Style

52ed6f8

# Conflicts: # substrate/client/tracing/Cargo.toml

ansi_term's Purple should be console's Magenta

577b9ec

Remove ansi_term usage in test

7b5846e

jasl force-pushed the replace-ansi_term branch from 15cbac1 to 7b5846e Compare July 17, 2024 18:13

jasl commented Jul 17, 2024

View reviewed changes

jasl added 2 commits July 18, 2024 03:19

cargo +nightly fmt --all

02ebaa2

Add PRDoc

cb7ad77

bkchr approved these changes Jul 17, 2024

View reviewed changes

bkchr requested a review from a team July 17, 2024 19:31

bkchr added the T0-node This PR/Issue is related to the topic “node”. label Jul 17, 2024

Apply suggestions from code review

96ccbe3

Co-authored-by: Bastian Köcher <git@kchr.de>

bkchr reviewed Jul 17, 2024

View reviewed changes

Apply suggestions from code review

2f610df

Co-authored-by: Bastian Köcher <git@kchr.de>

jasl added 3 commits July 18, 2024 05:14

ansi_term free for relay-utils

2de565f

removed unused ansi_term dep

33f7d36

Merge branch 'master' into replace-ansi_term

6c94770

Merge branch 'master' into replace-ansi_term

658eabf

bkchr enabled auto-merge July 19, 2024 09:38

lexnv approved these changes Jul 19, 2024

View reviewed changes

enforce console colors policy, keep the same behavior with ansi_term

1c13aaf

auto-merge was automatically disabled July 19, 2024 14:41
Head branch was pushed to by a user without write access

bkchr enabled auto-merge July 19, 2024 14:50

auto-merge was automatically disabled July 20, 2024 05:35
Head branch was pushed to by a user without write access

jasl commented Jul 20, 2024

View reviewed changes

Refactor to use console global ANSI style control

2891d3f

jasl force-pushed the replace-ansi_term branch from 8631296 to 2891d3f Compare July 20, 2024 05:46

bkchr reviewed Jul 20, 2024

View reviewed changes

Apply suggestions from code review

9c6fe60

Co-authored-by: Bastian Köcher <git@kchr.de>

davxy approved these changes Jul 22, 2024

View reviewed changes

serban300 approved these changes Jul 23, 2024

View reviewed changes

Merge branch 'master' into replace-ansi_term

4339ec5

bkontur approved these changes Jul 23, 2024

View reviewed changes

bkchr approved these changes Jul 23, 2024

View reviewed changes

bkchr added this pull request to the merge queue Jul 23, 2024

Merged via the queue into paritytech:master with commit bbb8668 Jul 23, 2024
160 of 163 checks passed

jasl deleted the replace-ansi_term branch July 23, 2024 16:58

TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024

Replace all ansi_term crates (paritytech#2923)

ac945db

Now Polkadot-SDK is ansi_term free --------- Co-authored-by: Bastian Köcher <git@kchr.de>

ggwpez mentioned this pull request Sep 9, 2024

Parent issue for stable2409 LTS release #5583

Open

Replace all ansi_term crates #2923

Replace all ansi_term crates #2923

Conversation

jasl commented Jan 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasl commented Jan 13, 2024

bkchr commented Jul 17, 2024

jasl commented Jul 17, 2024

jasl commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasl commented Jul 17, 2024

bkchr commented Jul 17, 2024

jasl commented Jul 17, 2024

jasl commented Jul 17, 2024

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasl commented Jul 17, 2024

bkchr commented Jul 17, 2024

lexnv left a comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Jul 19, 2024

jasl commented Jul 19, 2024 • edited Loading

jasl commented Jul 19, 2024 • edited Loading

jasl commented Jul 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasl commented Jul 23, 2024

jasl commented Jul 23, 2024

bkchr commented Jul 23, 2024

jasl commented Jan 13, 2024 •

edited

Loading

jasl commented Jul 19, 2024 •

edited

Loading

jasl commented Jul 19, 2024 •

edited

Loading