refactor: Switch from termcolor to anstream

[epage]

What does this PR try to resolve?

anstream asks the question "what if you took fwdansi and removed termcolor underneath it. It wraps output streams, adapting ANSI escape codes to what is needed

• Pass through if its supported
• Strip if its not
• Adapt to wincon API if needed

Benefits

• Lower boilerplate: we can use write! with styled text rather than the back-and-forth between colors and writing that termcolor needs
• Allows richer styling as Shell can accept styled messages and adapt them as needed

Side effects

• We'll now respect NO_COLOR, CLICOLOR_FORCE, and CLICOLOR

Fixes #12627

How should we test and review this PR?

This is broken up by commits for easier browsing.

However, as there aren't really tests for colored output, this needs hand inspection to verify

Additional information

This allowed us to remove the need for stripping ansi escape codes completely. Even if it didn't, it exposes its internal stripping API for reuse, saving on a dependency and being significantly faster.

[rustbot]

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

[arlosi]

Thanks! I like the cleaner code fewer dependencies.

[folkertdev]

obviously I'm just some outsider, but I'm surprised by how this PR adds many small allocations. The previous implementation seems to carefully avoid making those allocations.

It seems to me that some "code pain" in cargo is worth it given the huge number of daily cargo invocations.

In this particular case using a Cursor around an array seems like a simple alternative.

Suggested change

	let mut buffer = Vec::new();
	if justified {
	write!(&mut buffer, "{style}{status:>12}{reset}")?;
	} else {
	write!(&mut buffer, "{style}{status}{reset}{bold}:{reset}")?;
	}
	match message {
	Some(message) => writeln!(buffer, " {message}")?,
	None => write!(buffer, " ")?,
	}
	self.stderr().write_all(&buffer)?;
	let mut buffer = std::io::Cursor::new([0u8; 1024]);
	if justified {
	write!(&mut buffer, "{style}{status:>12}{reset}")?;
	} else {
	write!(&mut buffer, "{style}{status}{reset}{bold}:{reset}")?;
	}
	match message {
	Some(message) => writeln!(buffer, " {message}")?,
	None => write!(buffer, " ")?,
	}
	self.stderr()
	.write_all(&buffer.get_ref()[..buffer.position() as usize])?;

I'm not entirely sure why writing into a separate buffer before actually writing to stderr is required here. If it turns out that separate buffer is not needed, then this could work?

Suggested change

	let mut buffer = Vec::new();
	if justified {
	write!(&mut buffer, "{style}{status:>12}{reset}")?;
	} else {
	write!(&mut buffer, "{style}{status}{reset}{bold}:{reset}")?;
	}
	match message {
	Some(message) => writeln!(buffer, " {message}")?,
	None => write!(buffer, " ")?,
	}
	self.stderr().write_all(&buffer)?;
	let stderr = self.stderr();
	if justified {
	write!(stderr, "{style}{status:>12}{reset} ")?;
	} else {
	write!(stderr, "{style}{status}{reset}{bold}:{reset} ")?;
	}
	if let Some(message) = message {
	write!(stderr, "{message}")?;
	}

obviously this is not the main point of this PR, it just surprised me. This is just one example, the same pattern seems to occur in a bunch of places and the allocations can be removed in similar ways.

[epage]

The original buffering was added in #12727 to workaround the fact that we aren't locking the stream (unsure why lock wasn't used instead).

The difference between #12727 and this is that we aren't reusing the buffer.

I expect allocations aren't our biggest concern here

• In most cases, we are writing to the screen which has its own performance concerns
• A good number of calls to these functions will already be having to do a format!. I've found the biggest difference is between 'no allocation" and "allocation" code paths. The difference between "3 allocations" and "4 allocations" is small.

If allocations were to be an issue, our options are

• Cache the Vec inside of ShellOutput
• Extend anstream to allow locking without transferring ownership
  • AutoStream::lock transfers ownership, which doesn't work here, because (1) AutoStream is stateful and it adds a lot of complexity in both the implementation and the caller (the locked stream needs a &mut back to the original stream) (2) most of the time none of this will matter but when it doesn, I found enough cases want access to an owned stream

[arlosi]

Since the number of logged status messages is relatively small, I don't think there's a going to be any measurable performance impact from this allocation. Using a fixed-length stack buffer would avoid the allocation, but also uses an unnecessary amount of stack space in most cases, and limits message length.

[arlosi]

Thanks!

@bors r+

[bors]

📌 Commit a770d36 has been approved by arlosi

It is now in the queue for this repository.

[bors]

⌛ Testing commit a770d36 with merge 59596f0...

[bors]

☀️ Test successful - checks-actions
Approved by: arlosi
Pushing 59596f0 to master...