-
-
Notifications
You must be signed in to change notification settings - Fork 86
[JENKINS-54133] Pregenerating console notes sent to agents for some common cases #128
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,12 +3,16 @@ | |
| import hudson.console.ConsoleLogFilter; | ||
| import hudson.console.LineTransformationOutputStream; | ||
| import hudson.model.AbstractBuild; | ||
| import java.io.ByteArrayOutputStream; | ||
|
|
||
| import java.io.IOException; | ||
| import java.io.OutputStream; | ||
| import java.io.Serializable; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
| import jenkins.util.JenkinsJVM; | ||
|
|
||
| /** | ||
| * {@link ConsoleLogFilter} that adds a {@link SimpleHtmlNote} to each line. | ||
|
|
@@ -20,10 +24,40 @@ public final class AnsiColorConsoleLogFilter extends ConsoleLogFilter implements | |
| private static final Logger LOG = Logger.getLogger(AnsiColorConsoleLogFilter.class.getName()); | ||
|
|
||
| private AnsiColorMap colorMap; | ||
| private final Map<String, byte[]> notes; | ||
|
|
||
| public AnsiColorConsoleLogFilter(AnsiColorMap colorMap) { | ||
| super(); | ||
| this.colorMap = colorMap; | ||
| this.notes = new HashMap<>(); | ||
| // some cases of AnsiHtmlOutputStream.setForegroundColor: | ||
| for (AnsiColorMap.Color color : AnsiColorMap.Color.values()) { | ||
| pregenerateNote(new AnsiAttributeElement(AnsiAttributeElement.AnsiAttrType.FG, "span", "style=\"color: " + colorMap.getNormal(color.ordinal()) + ";\"")); | ||
| } | ||
| // TODO other cases, and other methods | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tedious part. Handling bright colors, |
||
| LOG.log(Level.FINE, "Notes pregenerated for {0}", notes.keySet()); | ||
| } | ||
|
|
||
| private void pregenerateNote(AnsiAttributeElement element) { | ||
| element.emitOpen(html -> pregenerateNote(html)); | ||
| element.emitClose(html -> pregenerateNote(html)); | ||
| } | ||
|
|
||
| private void pregenerateNote(String html) { | ||
| if (!notes.containsKey(html)) { | ||
| JenkinsJVM.checkJenkinsJVM(); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See jenkinsci/ant-plugin#32 for a simpler patch that illustrates the idiom. |
||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| try { | ||
| new SimpleHtmlNote(html).encodeTo(baos); | ||
| } catch (IOException x) { // should be impossible | ||
| throw new RuntimeException(x); | ||
| } | ||
| notes.put(html, baos.toByteArray()); | ||
| } | ||
| } | ||
|
|
||
| private Object readResolve() { // handle old program.dat | ||
| return notes == null ? new AnsiColorConsoleLogFilter(colorMap) : this; | ||
| } | ||
|
|
||
| @SuppressWarnings("rawtypes") | ||
|
|
@@ -39,7 +73,12 @@ public OutputStream decorateLogger(AbstractBuild build, final OutputStream logge | |
| @Override | ||
| public void emitHtml(String html) { | ||
| try { | ||
| new SimpleHtmlNote(html).encodeTo(logger); | ||
| byte[] pregenerated = notes.get(html); | ||
| if (pregenerated != null) { | ||
| logger.write(pregenerated); | ||
| } else { | ||
| new SimpleHtmlNote(html).encodeTo(logger); | ||
| } | ||
| } catch (IOException e) { | ||
| LOG.log(Level.WARNING, "Failed to add HTML markup '" + html + "'", e); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,21 @@ | ||
| package hudson.plugins.ansicolor; | ||
|
|
||
| import hudson.Functions; | ||
| import hudson.console.ConsoleNote; | ||
| import java.io.StringWriter; | ||
| import java.util.logging.Level; | ||
| import static org.hamcrest.CoreMatchers.instanceOf; | ||
| import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; | ||
| import org.jenkinsci.plugins.workflow.job.WorkflowJob; | ||
| import org.jenkinsci.plugins.workflow.job.WorkflowRun; | ||
| import org.junit.Test; | ||
| import static org.junit.Assert.*; | ||
| import org.junit.Assume; | ||
| import org.junit.ClassRule; | ||
| import org.junit.Rule; | ||
| import org.junit.runners.model.Statement; | ||
| import org.jvnet.hudson.test.BuildWatcher; | ||
| import org.jvnet.hudson.test.Issue; | ||
| import org.jvnet.hudson.test.LoggerRule; | ||
| import org.jvnet.hudson.test.RestartableJenkinsRule; | ||
|
|
||
| public class AnsiColorBuildWrapperTest { | ||
|
|
@@ -42,17 +45,21 @@ public void testDecorateLogger() { | |
| public static BuildWatcher buildWatcher = new BuildWatcher(); | ||
| @Rule | ||
| public RestartableJenkinsRule story = new RestartableJenkinsRule(); | ||
| @Rule | ||
| public LoggerRule logging = new LoggerRule().recordPackage(ConsoleNote.class, Level.FINE); | ||
|
|
||
| @Issue("JENKINS-54133") | ||
| @Test | ||
| public void testWorkflowWrap() throws Exception { | ||
| story.addStep(new Statement() { | ||
|
|
||
| @Override | ||
| public void evaluate() throws Throwable { | ||
| Assume.assumeTrue(!Functions.isWindows()); | ||
| story.j.createSlave(); | ||
| WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p"); | ||
| p.setDefinition(new CpsFlowDefinition( | ||
| "node {\n" | ||
| "node('!master') {\n" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feels like this should be a separate test.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seemed unnecessary as |
||
| + " wrap([$class: 'AnsiColorBuildWrapper', 'colorMapName': 'XTerm', 'defaultFg': 1, 'defaultBg': 2]) {\n" | ||
| + " sh(\"\"\"#!/bin/bash\n" | ||
| + " printf 'The following word is supposed to be \\\\e[31mred\\\\e[0m\\\\n'\"\"\"\n" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to stats this is not a big deal, even if there are active trunk releases of this plugin for unrelated changes.
But, if baseline is an obstacle, the POM changes could be reverted. In that case the only
src/main/change would be to convert some uses of lambdas to inner classes, pretty simple; thesrc/test/code could remain, but would no longer reproduce the problem unless run underplugin-compat-tester.