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.

Sign up for GitHub

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

Reset color settings after writing body #844

Closed
wants to merge 1 commit into from
Closed

Reset color settings after writing body #844

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions libherokubuildpack/src/log.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ pub fn log_error(header: impl AsRef<str>, body: impl AsRef<str>) {
.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();
Copy link
Member

@edmorley edmorley Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is broken on main at the moment (see also #555), and that we need to adjust the resets here, however, I think we need a few more changes.

Something that came up during the buildpack_output work, is that because other tools can end up adding prefixes to each log line (for example, endosome adds the remote: prefix, and Pack CLI can add the [builder] prefixes for an untrusted builder, or the timestamp prefixes when using --timestamps), the log helpers need to wrap each line separately, rather than being able to rely on the ANSI codes from an earlier line still applying for the current line. See:

libcnb.rs/libherokubuildpack/src/buildpack_output/ansi_escape.rs

Lines 1 to 27 in 3fa2c6c

/// Wraps each line in an ANSI escape sequence while preserving prior ANSI escape sequences.
///
/// ## Why does this exist?
///
/// When buildpack output is streamed to the user, each line is prefixed with `remote: ` by Git.
/// Any colorization of text will apply to those prefixes which is not the desired behavior. This
/// function colors lines of text while ensuring that styles are disabled at the end of each line.
///
/// ## Supports recursive colorization
///
/// Strings that are previously colorized will not be overridden by this function. For example,
/// if a word is already colored yellow, that word will continue to be yellow.
pub(crate) fn wrap_ansi_escape_each_line(ansi: &ANSI, body: impl AsRef<str>) -> String {
let ansi_escape = ansi.to_str();
body.as_ref()
.split('\n')
// If sub contents are colorized it will contain SUBCOLOR ... RESET. After the reset,
// ensure we change back to the current color
.map(|line| line.replace(RESET, &format!("{RESET}{ansi_escape}"))) // Handles nested color
// Set the main color for each line and reset after so we don't colorize `remote:` by accident
.map(|line| format!("{ansi_escape}{line}{RESET}"))
// The above logic causes redundant colors and resets, clean them up
.map(|line| line.replace(&format!("{ansi_escape}{ansi_escape}"), ansi_escape)) // Reduce useless color
.map(|line| line.replace(&format!("{ansi_escape}{RESET}"), "")) // Empty lines or where the nested color is at the end of the line
.collect::<Vec<String>>()
.join("\n")
}

The log_error() behaviour on main is currently equivalent to:

<colour>
[Error: Foo]
<reset><colour>Message

And with prefixes this ends up being:

[builder] <colour>
[builder] [Error: Foo]
[builder] <reset><colour>Message body

When really what we need is:

[builder] 
[builder] <colour>[Error: Foo]<reset>
[builder] <colour>Message body<reset>

ie: I think we need to change a few things:

  1. Add the extra .reset() after writing the message body (as this PR does)
  2. Leave the .reset() after writing the header (ie: skip the removal currently in this PR)
  3. For the header, also change it to use two writeln! calls, rather than including the leading \n in a single call. The heading colour attributes would only then be specified on the second call.

Making those changes should fix #555.

(Those changes still won't fix the case where the message body is a multi-line string, but fixing the latter would be more involved and perhaps not worth doing given the log_* helpers are deprecated in favour of buildpack_output.)


stream
.set_color(ColorSpec::new().set_fg(Some(Color::Red)))
.unwrap();
writeln!(&mut stream, "{}", body.as_ref()).unwrap();
stream.reset().unwrap();
stream.flush().unwrap();
}

Expand All @@ -36,12 +36,12 @@ pub fn log_warning(header: impl AsRef<str>, body: impl AsRef<str>) {
.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();

stream
.set_color(ColorSpec::new().set_fg(Some(Color::Yellow)))
.unwrap();
writeln!(&mut stream, "{}", body.as_ref()).unwrap();
stream.reset().unwrap();
stream.flush().unwrap();
}

Expand Down
Loading