Skip to content

Commit

Permalink
Merge pull request jenkinsci#112 from jglick/missingNewline-redux
Browse files Browse the repository at this point in the history 
Final line of shell output without newline lost when using LineTransformationOutputStream-based decorators
  • Loading branch information
@dwnusbaum
dwnusbaum authored Jul 25, 2019
2 parents ba378bb + 6677baa commit 00db760
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 26 deletions.
3 changes: 2 additions & 1 deletion pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
<revision>2.33</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.176.1</jenkins.version>
<jenkins-test-harness.version>2.54</jenkins-test-harness.version> <!-- TODO pending plugin-pom update -->
<java.level>8</java.level>
<useBeta>true</useBeta>
<workflow-step-api-plugin.version>2.20</workflow-step-api-plugin.version>
Expand Down Expand Up @@ -132,7 +133,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>credentials-binding</artifactId>
<version>1.16</version>
<version>1.17</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
66 changes: 63 additions & 3 deletions src/main/java/org/jenkinsci/plugins/workflow/steps/durable_task/DurableTaskStep.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.io.PrintStream;
import java.io.UnsupportedEncodingException;
import java.lang.reflect.Field;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -254,6 +255,7 @@ static final class Execution extends AbstractStepExecutionImpl implements Runnab
private static final long MAX_RECURRENCE_PERIOD = 15000; // 15s
private static final float RECURRENCE_PERIOD_BACKOFF = 1.2f;

private transient TaskListener newlineSafeTaskListener;
/** Used only during {@link #start}. */
private transient final DurableTaskStep step;
/** Current “live” connection to the workspace, or null if we might be offline at the moment. */
Expand Down Expand Up @@ -303,7 +305,7 @@ static final class Execution extends AbstractStepExecutionImpl implements Runnab
if (returnStdout) {
durableTask.captureOutput();
}
TaskListener listener = context.get(TaskListener.class);
TaskListener listener = listener();
if (step.encoding != null) {
durableTask.charset(Charset.forName(step.encoding));
} else {
Expand Down Expand Up @@ -398,7 +400,14 @@ private void getWorkspaceProblem(Exception x) {
}
}

private @Nonnull TaskListener listener() {
private synchronized @Nonnull TaskListener listener() {
if (newlineSafeTaskListener == null) {
newlineSafeTaskListener = new NewlineSafeTaskListener(_listener());
}
return newlineSafeTaskListener;
}

private @Nonnull TaskListener _listener() {
TaskListener l;
StepContext context = getContext();
try {
Expand All @@ -417,6 +426,56 @@ private void getWorkspaceProblem(Exception x) {
return l;
}

/**
* Interprets {@link PrintStream#close} as a signal to end a final newline if necessary.
*/
private static final class NewlineSafeTaskListener implements TaskListener {

private static final long serialVersionUID = 1;

private final TaskListener delegate;
private transient PrintStream logger;

NewlineSafeTaskListener(TaskListener delegate) {
this.delegate = delegate;
}

// Similar to DecoratedTaskListener:
@Override public synchronized PrintStream getLogger() {
if (logger == null) {
LOGGER.fine("creating filtered stream");
OutputStream base = delegate.getLogger();
OutputStream filtered = new FilterOutputStream(base) {
boolean nl = true; // empty string does not need a newline
@Override public void write(int b) throws IOException {
super.write(b);
nl = b == '\n';
}
@Override public void write(byte[] b, int off, int len) throws IOException {
super.write(b, off, len);
if (len > 0) {
nl = b[off + len - 1] == '\n';
}
}
@Override public void close() throws IOException {
LOGGER.log(Level.FINE, "calling close with nl={0}", nl);
if (!nl) {
super.write('\n');
}
flush(); // do *not* call base.close() here, unlike super.close()
}
};
try {
logger = new PrintStream(filtered, false, "UTF-8");
} catch (UnsupportedEncodingException x) {
throw new AssertionError(x);
}
}
return logger;
}

}

private @Nonnull Launcher launcher() throws IOException, InterruptedException {
StepContext context = getContext();
Launcher l = context.get(Launcher.class);
Expand Down Expand Up @@ -599,6 +658,7 @@ private void handleExit(int exitCode, OutputSupplier output) throws IOException,
getContext().onFailure(new AbortException("script returned exit code " + exitCode));
}
}
listener().getLogger().close();
}

// ditto
Expand Down Expand Up @@ -678,7 +738,7 @@ private static class HandlerImpl extends Handler {
}

@Override public void exited(int code, byte[] output) throws Exception {
listener.getLogger().flush();
listener.getLogger().close();
execution.exited(code, output);
}

Expand Down
49 changes: 27 additions & 22 deletions src/test/java/org/jenkinsci/plugins/workflow/steps/durable_task/ShellStepTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@
import hudson.console.ConsoleNote;
import hudson.console.LineTransformationOutputStream;
import hudson.model.BallColor;
import hudson.model.BooleanParameterDefinition;
import hudson.model.BooleanParameterValue;
import hudson.model.BuildListener;
import hudson.model.FreeStyleProject;
import hudson.model.Node;
import hudson.model.ParametersAction;
import hudson.model.ParametersDefinitionProperty;
import hudson.model.Result;
import hudson.model.Run;
import hudson.remoting.Channel;
Expand All @@ -44,7 +48,6 @@
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.logging.Level;
import java.util.logging.LogRecord;
import javax.annotation.CheckForNull;
Expand Down Expand Up @@ -85,7 +88,6 @@
import org.junit.Assume;
import static org.junit.Assume.assumeFalse;
import org.junit.ClassRule;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ErrorCollector;
Expand Down Expand Up @@ -601,39 +603,42 @@ private static final class HelloNote extends ConsoleNote<Object> {
j.assertLogContains("¡Čau → there!", j.buildAndAssertSuccess(p));
}

@Ignore("TODO missing final line and in some cases even the `+ set +x` (but passes without withCredentials)")
@Test public void missingNewline() throws Exception {
assumeFalse(Functions.isWindows()); // TODO create Windows equivalent
String credentialsId = "creds";
String username = "bob";
String password = "s3cr3t";
UsernamePasswordCredentialsImpl c = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, "sample", username, password);
CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), c);
j.createSlave("remote", null, null);
DumbSlave s = j.createSlave("remote", null, null);
j.waitOnline(s);
logging.record(DurableTaskStep.class, Level.FINE).record(FileMonitoringTask.class, Level.FINE);
j.showAgentLogs(s, logging);
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
p.addProperty(new ParametersDefinitionProperty(new BooleanParameterDefinition("WATCHING", false, null)));
p.setDefinition(new CpsFlowDefinition(
"node('master') {\n" +
" withCredentials([usernameColonPassword(variable: 'USERPASS', credentialsId: '" + credentialsId + "')]) {\n" +
" sh 'set +x; printf \"some local output\"'\n" +
" }\n" +
"}\n" +
"node('remote') {\n" +
" withCredentials([usernameColonPassword(variable: 'USERPASS', credentialsId: '" + credentialsId + "')]) {\n" +
" sh 'set +x; printf \"some remote output\"'\n" +
"['master', 'remote'].each {label ->\n" +
" node(label) {\n" +
" withCredentials([usernameColonPassword(variable: 'USERPASS', credentialsId: '" + credentialsId + "')]) {\n" +
" sh 'set +x; echo \"with final newline node=$NODE_NAME watching=$WATCHING\"'\n" +
" sh 'set +x; printf \"missing final newline node=$NODE_NAME watching=$WATCHING\"'\n" +
" }\n" +
" }\n" +
"}", true));
Callable<Void> test = () -> {
WorkflowRun b = j.assertBuildStatusSuccess(p.scheduleBuild2(0));
j.assertLogContains("some local output", b);
j.assertLogContains("some remote output", b);
return null;
};
boolean origUseWatching = DurableTaskStep.USE_WATCHING;
try {
DurableTaskStep.USE_WATCHING = false;
errors.checkSucceeds(test);
DurableTaskStep.USE_WATCHING = true;
errors.checkSucceeds(test);
for (boolean watching : new boolean[] {false, true}) {
DurableTaskStep.USE_WATCHING = watching;
String log = JenkinsRule.getLog(j.assertBuildStatusSuccess(p.scheduleBuild2(0, new ParametersAction(new BooleanParameterValue("WATCHING", watching)))));
for (String node : new String[] {"master", "remote"}) {
for (String mode : new String[] {"with", "missing"}) {
errors.checkThat(log, containsString(mode + " final newline node=" + node + " watching=" + watching));
}
}
errors.checkThat("no blank lines with watching=" + watching, log, not(containsString("\n\n")));
errors.checkThat(log, not(containsString("watching=false[Pipeline]")));
errors.checkThat(log, not(containsString("watching=true[Pipeline]")));
}
} finally {
DurableTaskStep.USE_WATCHING = origUseWatching;
}
Expand Down

0 comments on commit 00db760

Please sign in to comment.