From 4367854de4b96d7058742dc78f819ea2f22f16d7 Mon Sep 17 00:00:00 2001 From: Ed Morley <501702+edmorley@users.noreply.github.com> Date: Mon, 9 Dec 2024 23:59:21 +0000 Subject: [PATCH] Fix colour resetting for the `log_*` macros This fixes some long-standing ANSI colour bugs with the `log_header`, `log_error` and `log_warning` macros. Whilst we soon want to move away to more advanced logging libraries that use the new logging style, there are still many buildpacks using these macros which will benefit short term from these fixes (Procfile, Go, Node.js, JVM, Python, PHP, buildpacks-release-phase, buildpacks-frontend-web). The logging macros would previously emit output roughly like: ``` [Error: Foo] Message line one. Message line two. ``` This was not only missing the final ``, but also didn't wrap each line individually with colour codes/resets. This causes issues when lines end up prefixed - such as the Git `remote:` prefix, or when using Pack CLI locally with an untrusted build (which adds the colourised `[builder]` prefixes) or `--timestamps` mode. For example in this: ``` remote: remote: [Error: Foo] remote: Message line one. remote: Message line two. ``` ...several of the `remote:`s would inherit the colours. Instead what we need is: ``` remote: remote: [Error: Foo] remote: Message line one. remote: Message line two. ``` Fixes #555. Closes #844. --- CHANGELOG.md | 7 +++- libherokubuildpack/src/log.rs | 69 +++++++++++++++++++++++------------ 2 files changed, 51 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d9638cb..63691c0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,13 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `libherokubuildpack`: + - Fixed `log_header`, `log_error` and `log_warning` to correctly reset the ANSI colour styles at the end of each line. ([#890](https://github.com/heroku/libcnb.rs/pull/890)) + ## [0.26.0] - 2024-11-18 ### Changed - `libherokubuildpack`: - - Removed `buildpack_output` module. This functionality from ([#721](https://github.com/heroku/libcnb.rs/pull/721)) was experimental. The API was not stable and it is being removed. A similar API is available at [bullet_stream](https://crates.io/crates/bullet_stream). ([#852](https://github.com/heroku/libcnb.rs/pull/852) + - Removed `buildpack_output` module. This functionality from ([#721](https://github.com/heroku/libcnb.rs/pull/721)) was experimental. The API was not stable and it is being removed. A similar API is available at [bullet_stream](https://crates.io/crates/bullet_stream). ([#852](https://github.com/heroku/libcnb.rs/pull/852)) ## [0.25.0] - 2024-10-23 diff --git a/libherokubuildpack/src/log.rs b/libherokubuildpack/src/log.rs index 9e270e27..aa87d734 100644 --- a/libherokubuildpack/src/log.rs +++ b/libherokubuildpack/src/log.rs @@ -1,4 +1,4 @@ -use std::io::Write; +use std::io::{self, Write}; use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; /// # Panics @@ -10,16 +10,14 @@ use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; #[allow(clippy::unwrap_used)] pub fn log_error(header: impl AsRef, body: impl AsRef) { let mut stream = StandardStream::stderr(ColorChoice::Always); - stream - .set_color(ColorSpec::new().set_fg(Some(Color::Red)).set_bold(true)) - .unwrap(); - writeln!(&mut stream, "\n[Error: {}]", header.as_ref()).unwrap(); - stream.reset().unwrap(); + write_styled_message( + &mut stream, + format!("\n[Error: {}]", header.as_ref()), + ColorSpec::new().set_fg(Some(Color::Red)).set_bold(true), + ) + .unwrap(); - stream - .set_color(ColorSpec::new().set_fg(Some(Color::Red))) - .unwrap(); - writeln!(&mut stream, "{}", body.as_ref()).unwrap(); + write_styled_message(&mut stream, body, ColorSpec::new().set_fg(Some(Color::Red))).unwrap(); stream.flush().unwrap(); } @@ -32,16 +30,19 @@ pub fn log_error(header: impl AsRef, body: impl AsRef) { #[allow(clippy::unwrap_used)] pub fn log_warning(header: impl AsRef, body: impl AsRef) { let mut stream = StandardStream::stderr(ColorChoice::Always); - stream - .set_color(ColorSpec::new().set_fg(Some(Color::Yellow)).set_bold(true)) - .unwrap(); - writeln!(&mut stream, "\n[Warning: {}]", header.as_ref()).unwrap(); - stream.reset().unwrap(); + write_styled_message( + &mut stream, + format!("\n[Warning: {}]", header.as_ref()), + ColorSpec::new().set_fg(Some(Color::Yellow)).set_bold(true), + ) + .unwrap(); - stream - .set_color(ColorSpec::new().set_fg(Some(Color::Yellow))) - .unwrap(); - writeln!(&mut stream, "{}", body.as_ref()).unwrap(); + write_styled_message( + &mut stream, + body, + ColorSpec::new().set_fg(Some(Color::Yellow)), + ) + .unwrap(); stream.flush().unwrap(); } @@ -54,11 +55,12 @@ pub fn log_warning(header: impl AsRef, body: impl AsRef) { #[allow(clippy::unwrap_used)] pub fn log_header(title: impl AsRef) { let mut stream = StandardStream::stdout(ColorChoice::Always); - stream - .set_color(ColorSpec::new().set_fg(Some(Color::Magenta)).set_bold(true)) - .unwrap(); - writeln!(&mut stream, "\n[{}]", title.as_ref()).unwrap(); - stream.reset().unwrap(); + write_styled_message( + &mut stream, + format!("\n[{}]", title.as_ref()), + ColorSpec::new().set_fg(Some(Color::Magenta)).set_bold(true), + ) + .unwrap(); stream.flush().unwrap(); } @@ -72,3 +74,22 @@ pub fn log_info(message: impl AsRef) { println!("{}", message.as_ref()); std::io::stdout().flush().unwrap(); } + +// Styles each line of text separately, so that when buildpack output is streamed to the +// user (and prefixes like `remote:` added) the line colour doesn't leak into the prefixes. +fn write_styled_message( + stream: &mut StandardStream, + message: impl AsRef, + spec: &ColorSpec, +) -> io::Result<()> { + // Using `.split('\n')` rather than `.lines()` since the latter eats trailing newlines in + // the passed message, which would (a) prevent the caller from being able to add spacing at + // the end of their message and (b) differ from the `println!` / `writeln!` behaviour. + for line in message.as_ref().split('\n') { + stream.set_color(spec)?; + write!(stream, "{line}")?; + stream.reset()?; + writeln!(stream)?; + } + Ok(()) +}