Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package hudson.plugins.ansicolor;

import java.io.Serializable;

/**
* Represents an HTML elements which maps to an ANSI attribute.
*
Expand All @@ -12,7 +14,9 @@
* other software or testing the HTML may be emitted otherwise.
*/

class AnsiAttributeElement {
class AnsiAttributeElement implements Serializable {
private static final long serialVersionUID = 1L;

public static enum AnsiAttrType {
DEFAULT, BOLD, ITALIC, UNDERLINE, STRIKEOUT, FRAMED, OVERLINE, FG, BG, FGBG
}
Expand Down Expand Up @@ -65,6 +69,11 @@ public int hashCode() {
return result;
}

@Override
public String toString() {
return "AnsiAttributeElement{ansiAttrType=" + ansiAttrType + ",name=" + name + ",attributes=" + attributes + "}";
}

public static AnsiAttributeElement bold() {
return new AnsiAttributeElement(AnsiAttributeElement.AnsiAttrType.BOLD, "b", "");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@
import java.io.IOException;
import java.io.OutputStream;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Stack;
import javax.annotation.Nonnull;

/**
* Filters an outputstream of ANSI escape sequences and emits appropriate HTML elements instead.
Expand Down Expand Up @@ -59,16 +62,27 @@ private static enum State {
private boolean swapColors = false; // true if negative / inverse mode is active (esc[7m)

// A Deque might be a better choice, but we are constrained by the Java 5 API.
private ArrayList<AnsiAttributeElement> openTags = new ArrayList<AnsiAttributeElement>();
private ArrayList<AnsiAttributeElement> openTags;

private OutputStream logOutput;

public AnsiHtmlOutputStream(final OutputStream os, final AnsiColorMap colorMap,
final AnsiAttributeElement.Emitter emitter) {
/**
* @param tagsToOpen A list of tags to open in the given order immediately after opening the tag for the default
* foreground/background colors (if such colors are specified by the color map) before any data is written to the
* underlying stream.
*/
/*package*/ AnsiHtmlOutputStream(final OutputStream os, final AnsiColorMap colorMap,
final AnsiAttributeElement.Emitter emitter, @Nonnull List<AnsiAttributeElement> tagsToOpen) {
super(os);
this.logOutput = os;
this.colorMap = colorMap;
this.emitter = emitter;
this.openTags = new ArrayList<>(tagsToOpen);
}

public AnsiHtmlOutputStream(final OutputStream os, final AnsiColorMap colorMap,
final AnsiAttributeElement.Emitter emitter) {
this(os, colorMap, emitter, Collections.emptyList());
}

// Debug output for plugin developers. Puts the debug message into the html page
Expand All @@ -93,6 +107,13 @@ private void stopConcealing() {
this.out = logOutput;
}

/**
* @return A copy of the {@link AnsiAttributeElement}s which are currently opened, in order from outermost to innermost tag.
*/
/*package*/ List<AnsiAttributeElement> getOpenTags() {
return new ArrayList<>(openTags);
}

private void openTag(AnsiAttributeElement tag) throws IOException {
openTags.add(tag);
tag.emitOpen(emitter);
Expand Down Expand Up @@ -198,6 +219,9 @@ public void write(int data) throws IOException {
// the preamble is an ANSI escape sequence itself.

if (state == State.INIT) {
List<AnsiAttributeElement> tagsToOpen = new ArrayList<>(openTags);
openTags.clear();

Integer defaultFg = colorMap.getDefaultForeground();
Integer defaultBg = colorMap.getDefaultBackground();

Expand All @@ -207,6 +231,10 @@ public void write(int data) throws IOException {
(defaultFg != null ? "color: " + colorMap.getNormal(defaultFg) + ";" : "") + "\""));
}

for (AnsiAttributeElement tag : tagsToOpen) {
openTag(tag);
}

state = State.DATA;
}

Expand Down
82 changes: 64 additions & 18 deletions src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,11 @@
import hudson.model.Queue;
import hudson.model.Run;
import java.io.IOException;
import java.util.Collections;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nonnull;
import jenkins.model.Jenkins;
import org.apache.commons.io.output.CountingOutputStream;
import org.apache.commons.io.output.NullOutputStream;
Expand All @@ -51,59 +54,102 @@ final class ColorConsoleAnnotator extends ConsoleAnnotator<Object> {

private final String colorMapName;
private final String charset;
private final @Nonnull List<AnsiAttributeElement> openTags;

ColorConsoleAnnotator(String colorMapName, String charset) {
ColorConsoleAnnotator(String colorMapName, String charset, List<AnsiAttributeElement> openTags) {
this.colorMapName = colorMapName;
this.charset = charset;
LOGGER.fine("creating annotator with colorMapName=" + colorMapName + " charset=" + charset);
this.openTags = openTags;
LOGGER.log(Level.FINE, "creating annotator with colorMapName={0} charset={1} openTags={2}", new Object[] { colorMapName, charset, openTags });
}

ColorConsoleAnnotator(String colorMapName, String charset) {
this(colorMapName, charset, Collections.emptyList());
}

@Override
public ConsoleAnnotator<Object> annotate(Object context, MarkupText text) {
String s = text.getText();
if (s.indexOf('\u001B') != -1) {
AnsiColorMap colorMap = Jenkins.get().getDescriptorByType(AnsiColorBuildWrapper.DescriptorImpl.class).getColorMap(colorMapName);
List<AnsiAttributeElement> nextOpenTags = openTags;
// TODO: As a performance improvement, we could create a branch where `s.indexOf('\u001B') == -1` but other
// conditions are true that surrounds the text in the appropriate tags without going through AnsiHtmlOutputStream.
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary—this case is likely quite rare.

AnsiColorMap colorMap = Jenkins.get().getDescriptorByType(AnsiColorBuildWrapper.DescriptorImpl.class).getColorMap(colorMapName);
if (s.indexOf('\u001B') != -1 || !openTags.isEmpty() || colorMap.getDefaultBackground() != null || colorMap.getDefaultForeground() != null) {
CountingOutputStream outgoing = new CountingOutputStream(new NullOutputStream());
class EmitterImpl implements AnsiAttributeElement.Emitter {
CountingOutputStream incoming;
int adjustment;
int lastPoint = -1; // multiple HTML tags may be emitted for one control sequence
@Override
public void emitHtml(String html) {
LOGGER.finest("emitting " + html + " @" + incoming.getCount());
text.addMarkup(incoming.getCount(), html);
if (incoming.getCount() != lastPoint) {
lastPoint = incoming.getCount();
int hide = incoming.getCount() - outgoing.getCount() - adjustment;
LOGGER.finest("hiding " + hide + " @" + (outgoing.getCount() + adjustment));
text.addMarkup(outgoing.getCount() + adjustment, outgoing.getCount() + adjustment + hide, "<span style=\"display: none\">", "</span>");
adjustment += hide;
int inCount = incoming.getCount();
// All ANSI escapes sequences contain at least 2 bytes on modern platforms, so any HTML emitted
// directly after the first byte is received is due to the initialization process of the stream and
// belongs at position 0 (i.e. default background/foreground colors).
Copy link
Member

Choose a reason for hiding this comment

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

I think I need more caffeine to understand this. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this code was and still is quite confusing to me. Thankfully, the if statement is covered by the newly added tests, and if it is removed the tests fail, so we should be able to avoid future regressions.

if (inCount == 1) {
inCount = 0;
}
LOGGER.log(Level.FINEST, "emitting {0} @{1}", new Object[] { html, inCount });
text.addMarkup(inCount, html);
if (inCount != lastPoint) {
lastPoint = inCount;
int hide = inCount - outgoing.getCount() - adjustment;
// If openTags is not empty, but there are no escape sequences directly on this line, or if we
// are emitting closing tags when closing the stream, there is nothing to hide.
if (hide != 0) {
LOGGER.log(Level.FINEST, "hiding {0} @{1}", new Object[] { hide, outgoing.getCount() + adjustment });
text.addMarkup(outgoing.getCount() + adjustment, outgoing.getCount() + adjustment + hide, "<span style=\"display: none\">", "</span>");
adjustment += hide;
}
}
}
public void emitHtmlDirect(String html) {
text.addMarkup(incoming.getCount(), html);
}
}
EmitterImpl emitter = new EmitterImpl();
CountingOutputStream incoming = new CountingOutputStream(new AnsiHtmlOutputStream(outgoing, colorMap, emitter));
emitter.incoming = incoming;
try {
// We need to reopen tags that were still open at the end of the previous line so the stream's state is
// correct in case those tags are closed in the middle of this line.
try (AnsiHtmlOutputStream ansiOs = new AnsiHtmlOutputStream(outgoing, colorMap, emitter, openTags);
CountingOutputStream incoming = new CountingOutputStream(ansiOs)) {
emitter.incoming = incoming;
byte[] data = s.getBytes(charset);
for (int i = 0; i < data.length; i++) {
// Do not use write(byte[]) as offsets in incoming would not be accurate.
incoming.write(data[i]);
}
nextOpenTags = ansiOs.getOpenTags();
if (colorMap.getDefaultBackground() != null || colorMap.getDefaultForeground() != null) {
// The default color scheme will be opened automatically at the beginning of the stream on the next
// line, so we don't want to duplicate it.
nextOpenTags.remove(0);
Copy link
Member

Choose a reason for hiding this comment

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

.subList(1) might be less scary to readers, though I see that the getter does make a fresh copy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I tried switching to sublist, but the implementation is not serializable so it would require another copy, which seems excessive, so I added a comment to help clarify for future readers but left the original code.

}
// Tags open at the end of the line are closed when the stream is closed by the try-with-resources block.
} catch (IOException x) {
LOGGER.log(Level.WARNING, null, x);
}
LOGGER.finer(() -> "\"" + StringEscapeUtils.escapeJava(s) + "\" → \"" + StringEscapeUtils.escapeJava(text.toString(true)) + "\"");
}
return this;
return openTags == nextOpenTags
? this
: new ColorConsoleAnnotator(colorMapName, charset, nextOpenTags);
Copy link
Member Author

Choose a reason for hiding this comment

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

Based on my reading of the ConsoleAnnotator Javadocs, if we want to track state from line-to-line then we need to handle it by returning a new instance like this. Maybe wrong to return this instead of creating a new state when the open tags match.

Copy link
Member

Choose a reason for hiding this comment

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

I thought the suggested fix for JENKINS-34019 was just to close all outstanding tags at the end of a line, as if this were a user error, not to try to make them apply across lines.

Copy link
Member Author

@dwnusbaum dwnusbaum Nov 19, 2018

Choose a reason for hiding this comment

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

When a style continues to the next line, the ConsoleAnnotatorFactory can insert a closing tag at the end of the line and an opening tag at the beginning of the next line. This prevents the timestamps from being affected.

Full comment. What you propose would certainly be simpler, but I'm not sure what the expected behavior is. What do you think should be the behavior for the test case I added?

Copy link
Member

@jglick jglick Jan 3, 2019

Choose a reason for hiding this comment

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

I feel like it does not matter much, since it is unlikely people would have multiline formatting intentionally, so it is OK to keep the implementation simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

#145 shows a problem with output of terraform at the command line, and based on a screenshot of the text it looks like it might be using multiline formatting, but I'm not sure. I'll leave this as-is for now until I have time to investigate in more depth.

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed via

docker run --rm hashicorp/terraform plan 2>&1 | od -a

(Though it also looks like the Terraform output is buggy—ANSI sequences are not nestable, and it is emitting several esc [ 0 m sequences in a row.)

}

private Object readResolve() {
// Compatibility for instances serialized before the openTags field was added.
if (openTags == null) {
return new ColorConsoleAnnotator(colorMapName, charset);
} else {
return this;
}
}

@Extension
public static final class Factory extends ConsoleAnnotatorFactory<Object> {

@Override
public ConsoleAnnotator<Object> newInstance(Object context) {
LOGGER.fine("context=" + context);
LOGGER.log(Level.FINE, "context={0}", context);
if (context instanceof Run) {
ColorizedAction action = ((Run) context).getAction(ColorizedAction.class);
if (action != null) {
Expand All @@ -122,7 +168,7 @@ public ConsoleAnnotator<Object> newInstance(Object context) {
if (exec instanceof Run) {
ColorizedAction action = ((Run) exec).getAction(ColorizedAction.class);
if (action != null) {
return new ColorConsoleAnnotator(action.colorMapName, /* JEp-206 */ "UTF-8");
return new ColorConsoleAnnotator(action.colorMapName, /* JEP-206 */ "UTF-8");
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,61 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
});
}

@Test
Copy link
Member Author

Choose a reason for hiding this comment

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

I should create a JIRA issue for these and add @Issue.

public void testMultilineEscapeSequence() throws Exception {
story.then(r -> {
FreeStyleProject p = r.createFreeStyleProject();
p.getBuildWrappersList().add(new AnsiColorBuildWrapper(null));
p.getBuildersList().add(new TestBuilder() {
@Override
public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
listener.getLogger().println("\u001B[1;34mThis text should be bold and blue");
listener.getLogger().println("Still bold and blue");
listener.getLogger().println("\u001B[mThis text should be normal");
return true;
}
});
FreeStyleBuild b = r.buildAndAssertSuccess(p);
StringWriter writer = new StringWriter();
b.getLogText().writeHtmlTo(0L, writer);
String html = writer.toString();
System.out.print(html);
assertThat(html.replaceAll("<span style=\"display: none\">.+?</span>", ""),
allOf(
containsString("<b><span style=\"color: #1E90FF;\">This text should be bold and blue\n</span></b>"),
Copy link
Member Author

Choose a reason for hiding this comment

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

The literal \n characters in these tests would probably cause them to fail on Windows.

containsString("<b><span style=\"color: #1E90FF;\">Still bold and blue\n</span></b>"),
not(containsString("\u001B[m"))));
});
}

@Test
public void testDefaultForegroundBackground() throws Exception {
story.then(r -> {
FreeStyleProject p = r.createFreeStyleProject();
// The VGA ColorMap sets default foreground and background colors.
p.getBuildWrappersList().add(new AnsiColorBuildWrapper("vga"));
p.getBuildersList().add(new TestBuilder() {
@Override
public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
listener.getLogger().println("White on black");
listener.getLogger().println("\u001B[1;34mBold and blue on black");
listener.getLogger().println("Still bold and blue on black\u001B[mBack to white on black");
return true;
}
});
FreeStyleBuild b = r.buildAndAssertSuccess(p);
StringWriter writer = new StringWriter();
b.getLogText().writeHtmlTo(0L, writer);
String html = writer.toString();
System.out.print(html);
assertThat(html.replaceAll("<span style=\"display: none\">.+?</span>", ""),
allOf(
containsString("<div style=\"background-color: #000000;color: #AAAAAA;\">White on black\n</div>"),
containsString("<div style=\"background-color: #000000;color: #AAAAAA;\"><b><span style=\"color: #0000AA;\">Bold and blue on black\n</span></b></div>"),
containsString("<div style=\"background-color: #000000;color: #AAAAAA;\"><b><span style=\"color: #0000AA;\">Still bold and blue on black</span></b>Back to white on black\n</div>")));
});
}

@Issue("JENKINS-54133")
@Test
public void testWorkflowWrap() throws Exception {
Expand Down