Skip to content

[JEP-206] Gather command output in a local encoding #64

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

Merged
svanoort merged 11 commits into from
Aug 7, 2018
Merged

[JEP-206] Gather command output in a local encoding #64

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
dec68db
[JENKINS-31096] Gather command output in a local encoding.
jglick Feb 12, 2018
6f2bf2e
Cannot downgrade Jenkins in tests!
jglick Feb 13, 2018
23a1dde
Merge branch 'master' into UTF-8-JENKINS-31096
jglick Feb 19, 2018
1f4493f
Bump.
jglick Feb 19, 2018
0dcf951
Emphasizing that specifying an explicit encoding is wise.
jglick Feb 19, 2018
35b0234
Merge branch 'incrementals' into UTF-8-JENKINS-31096
jglick Jun 7, 2018
36722de
Review comments.
jglick Jun 7, 2018
75181a5
Merge branch 'master' into UTF-8-JENKINS-31096
jglick Jun 8, 2018
54b2187
Restricting doCheckEncoding.
jglick Jun 9, 2018
8c763a4
Updated parent and incremental deps.
jglick Aug 7, 2018
d3508c3
Consume released workflow-support and durable-task plugins
svanoort Aug 7, 2018
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
24 changes: 15 additions & 9 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.2</version>
<version>3.4</version>
Copy link
Member

@oleg-nenashev oleg-nenashev Apr 2, 2018

Choose a reason for hiding this comment

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

3.6 if 2.107.1 is added to CI

Copy link
Member Author

@jglick jglick Apr 3, 2018

Choose a reason for hiding this comment

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

Now 3.7.

<relativePath />
</parent>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down Expand Up @@ -62,9 +62,10 @@
</pluginRepository>
</pluginRepositories>
<properties>
<jenkins.version>2.60.3</jenkins.version>
<jenkins.version>2.73.3</jenkins.version>
Copy link
Member

@svanoort svanoort Feb 13, 2018

Choose a reason for hiding this comment

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

Is this gratuitous or do we need a new API?

Copy link
Member Author

@jglick jglick Feb 16, 2018

Choose a reason for hiding this comment

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

Test dep on workflow-job requires this.

<java.level>8</java.level>
<workflow-step-api-plugin.version>2.13</workflow-step-api-plugin.version>
<workflow-support-plugin.version>2.19-20180209.204154-1</workflow-support-plugin.version> <!-- TODO https://github.com/jenkinsci/workflow-support-plugin/pull/56 -->
</properties>
<dependencies>
<dependency>
Expand All @@ -75,17 +76,17 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>durable-task</artifactId>
<version>1.18-20180125.194359-1</version> <!-- TODO https://github.com/jenkinsci/durable-task-plugin/pull/57 -->
<version>1.18-20180212.221815-3</version> <!-- TODO https://github.com/jenkinsci/durable-task-plugin/pull/61 -->
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-api</artifactId>
<version>2.22</version>
<version>2.25</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-support</artifactId>
<version>2.16</version>
<version>${workflow-support-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand All @@ -96,7 +97,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-job</artifactId>
<version>2.9</version>
<version>2.18-20180209.215341-1</version> <!-- TODO https://github.com/jenkinsci/workflow-job-plugin/pull/89 -->
<scope>test</scope>
</dependency>
<dependency>
Expand All @@ -121,7 +122,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-support</artifactId>
<version>2.13</version>
<version>${workflow-support-plugin.version}</version>
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
Expand All @@ -134,12 +135,17 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1.27</version>
<version>1.39</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>structs</artifactId>
<version>1.7</version>
<version>1.10</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>scm-api</artifactId>
<version>2.2.6</version>
</dependency>
</dependencies>
</project>
23 changes: 13 additions & 10 deletions src/main/java/org/jenkinsci/plugins/workflow/steps/durable_task/DurableTaskStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,15 @@
import hudson.EnvVars;
import hudson.FilePath;
import hudson.Launcher;
import hudson.Util;
import hudson.model.TaskListener;
import hudson.util.DaemonThreadFactory;
import hudson.util.FormValidation;
import hudson.util.LogTaskListener;
import hudson.util.NamingThreadFactory;
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.Set;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.ScheduledThreadPoolExecutor;
Expand Down Expand Up @@ -67,7 +69,7 @@ public abstract class DurableTaskStep extends Step {
private static final Logger LOGGER = Logger.getLogger(DurableTaskStep.class.getName());

private boolean returnStdout;
private String encoding = DurableTaskStepDescriptor.defaultEncoding;
private String encoding;
Copy link
Member Author

@jglick jglick Feb 13, 2018

Choose a reason for hiding this comment

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

Note that this is a behavioral change—we are changing the default value of encoding from UTF-8 to null (i.e., use the node’s system encoding). Also the encoding now applies to regular streamed output, whereas previously it only mattered for returnStdout: true. (Which is the main point of this whole exercise: JENKINS-31096.)

I tried to think of a way to keep the previous default but it was just going to be ugly. With the change of the overall build log to UTF-8, it would mean that you would get mojibake in case you were running, say, entirely on CP-1252 Windows (with or without agents) and just did something simple like

node {
  bat 'something producing Western European characters…'
}

which seems wrong.

This interpretation of encoding is also consistent with the readFile and writeFile steps, as well as the behavior of freestyle builds (which define the entire build’s log encoding by the node’s system encoding), and I think would be considered most intuitive going forward. Otherwise we would need a special value ('', 'system', …) indicating system default encoding, and then try to explain that in the snippet generator.

Obviously if you have specific knowledge that a given process is going to be producing UTF-8 yet will be running on a node with a different system encoding (typically Windows, since every Linux distribution has defaulted to UTF-8 for a long time), you can make that explicit with encoding: 'UTF-8'.

Copy link
Member

@svanoort svanoort Feb 16, 2018

Choose a reason for hiding this comment

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

Obviously if you have specific knowledge that a given process is going to be producing UTF-8 yet will be running on a node with a different system encoding (typically Windows, since every Linux distribution has defaulted to UTF-8 for a long time), you can make that explicit with encoding: 'UTF-8'.

🐛 This should be called out really explicitly in the documentation / UI here.

Copy link
Member Author

@jglick jglick Feb 19, 2018

Choose a reason for hiding this comment

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

Yeah I can emphasize that.

Copy link
Member

@oleg-nenashev oleg-nenashev Apr 2, 2018

Choose a reason for hiding this comment

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

Seems reasonable. Whatever is done with encodings in Durable Task and this plugin, it is a risky change anyway. Since the idea is to NOT enforce the UTF encoding, the current approach looks preferable

private boolean returnStatus;

protected abstract DurableTask task();
Expand All @@ -85,7 +87,7 @@ public String getEncoding() {
}

@DataBoundSetter public void setEncoding(String encoding) {
this.encoding = encoding;
this.encoding = Util.fixEmpty(encoding);
}

public boolean isReturnStatus() {
Expand All @@ -102,17 +104,15 @@ public boolean isReturnStatus() {

public abstract static class DurableTaskStepDescriptor extends StepDescriptor {

public static final String defaultEncoding = "UTF-8";

public FormValidation doCheckEncoding(@QueryParameter boolean returnStdout, @QueryParameter String encoding) {
if (encoding.isEmpty()) {
Copy link
Member

@oleg-nenashev oleg-nenashev Apr 2, 2018

Choose a reason for hiding this comment

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

StringUtils.isBlank()? otherwise the behavior will differ from Util.fixEmpty
🐜

Copy link
Member Author

@jglick jglick Apr 3, 2018

Choose a reason for hiding this comment

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

This is a web method; the text field may be blank but not missing.

return FormValidation.ok();
}
try {
Charset.forName(encoding);
} catch (Exception x) {
return FormValidation.error(x, "Unrecognized encoding");
}
if (!returnStdout && !encoding.equals(DurableTaskStepDescriptor.defaultEncoding)) {
return FormValidation.warning("encoding is ignored unless returnStdout is checked.");
}
return FormValidation.ok();
}

Expand Down Expand Up @@ -154,7 +154,6 @@ static final class Execution extends AbstractStepExecutionImpl implements Runnab
private String node;
private String remote;
private boolean returnStdout; // serialized default is false
private String encoding; // serialized default is irrelevant
private boolean returnStatus; // serialized default is false

Execution(StepContext context, DurableTaskStep step) {
Expand All @@ -164,7 +163,6 @@ static final class Execution extends AbstractStepExecutionImpl implements Runnab

@Override public boolean start() throws Exception {
returnStdout = step.returnStdout;
encoding = step.encoding;
returnStatus = step.returnStatus;
StepContext context = getContext();
ws = context.get(FilePath.class);
Expand All @@ -173,6 +171,11 @@ static final class Execution extends AbstractStepExecutionImpl implements Runnab
if (returnStdout) {
durableTask.captureOutput();
}
if (step.encoding != null) {
durableTask.charset(Charset.forName(step.encoding));
Copy link
Member

@oleg-nenashev oleg-nenashev Apr 2, 2018

Choose a reason for hiding this comment

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

Catch/rethrow exception with more debug details?

Copy link
Member Author

@jglick jglick Apr 3, 2018

Choose a reason for hiding this comment

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

Which debug details are you looking for here?

} else {
durableTask.defaultCharset();
}
controller = durableTask.launch(context.get(EnvVars.class), ws, context.get(Launcher.class), context.get(TaskListener.class));
this.remote = ws.getRemote();
setupTimer();
Expand Down Expand Up @@ -327,7 +330,7 @@ private void check() {
LOGGER.log(Level.FINE, "last-minute output in {0} on {1}", new Object[] {remote, node});
}
if (returnStatus || exitCode == 0) {
getContext().onSuccess(returnStatus ? exitCode : returnStdout ? new String(controller.getOutput(workspace, launcher()), encoding) : null);
getContext().onSuccess(returnStatus ? exitCode : returnStdout ? new String(controller.getOutput(workspace, launcher()), StandardCharsets.UTF_8) : null);
} else {
if (returnStdout) {
listener.getLogger().write(controller.getOutput(workspace, launcher())); // diagnostic
Expand Down
2 changes: 1 addition & 1 deletion .../resources/org/jenkinsci/plugins/workflow/steps/durable_task/DurableTaskStep/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ THE SOFTWARE.
<f:checkbox/>
</f:entry>
<f:entry field="encoding" title="${%Encoding of standard output}">
<f:textbox default="${descriptor.defaultEncoding}"/>
<f:textbox/>
</f:entry>
<f:entry field="returnStatus" title="${%Return exit status}">
<f:checkbox/>
Expand Down
5 changes: 4 additions & 1 deletion ...rces/org/jenkinsci/plugins/workflow/steps/durable_task/DurableTaskStep/help-encoding.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
<div>
Encoding of standard output, if it is being captured.
Encoding of process output.
In the case of <code>returnStdout</code>, applies to the return value of this step;
otherwise, or always for standard error, controls how text is copied to the build log.
If unspecified, uses the system default encoding of the node on which the step is run.
</div>
36 changes: 32 additions & 4 deletions src/test/java/org/jenkinsci/plugins/workflow/steps/durable_task/ShellStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@

import com.google.common.base.Predicate;
import hudson.EnvVars;
import hudson.FilePath;
import hudson.Functions;
import hudson.Launcher;
import hudson.LauncherDecorator;
import hudson.Platform;
import hudson.model.BallColor;
import hudson.model.FreeStyleProject;
import hudson.model.Node;
import hudson.model.Result;
import hudson.model.Slave;
import hudson.slaves.DumbSlave;
import hudson.slaves.EnvironmentVariablesNodeProperty;
import hudson.tasks.BatchFile;
Expand Down Expand Up @@ -39,10 +40,12 @@
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.LoggerRule;
import org.jvnet.hudson.test.SimpleCommandLauncher;
import org.jvnet.hudson.test.TestExtension;
import org.kohsuke.stapler.DataBoundConstructor;

Expand All @@ -52,6 +55,7 @@ public class ShellStepTest {
public static BuildWatcher buildWatcher = new BuildWatcher();

@Rule public JenkinsRule j = new JenkinsRule();
@Rule public TemporaryFolder tmp = new TemporaryFolder();

@Rule public LoggerRule logging = new LoggerRule();

Expand Down Expand Up @@ -197,7 +201,7 @@ public DescriptorImpl() {
s = new StepConfigTester(j).configRoundTrip(s);
assertEquals("echo hello", s.getScript());
assertFalse(s.isReturnStdout());
assertEquals(DurableTaskStep.DurableTaskStepDescriptor.defaultEncoding, s.getEncoding());
assertNull(s.getEncoding());
assertFalse(s.isReturnStatus());
s.setReturnStdout(true);
s.setEncoding("ISO-8859-1");
Expand All @@ -207,12 +211,12 @@ public DescriptorImpl() {
assertEquals("ISO-8859-1", s.getEncoding());
assertFalse(s.isReturnStatus());
s.setReturnStdout(false);
s.setEncoding(DurableTaskStep.DurableTaskStepDescriptor.defaultEncoding);
s.setEncoding("UTF-8");
s.setReturnStatus(true);
s = new StepConfigTester(j).configRoundTrip(s);
assertEquals("echo hello", s.getScript());
assertFalse(s.isReturnStdout());
assertEquals(DurableTaskStep.DurableTaskStepDescriptor.defaultEncoding, s.getEncoding());
assertEquals("UTF-8", s.getEncoding());
assertTrue(s.isReturnStatus());
}

Expand All @@ -233,6 +237,30 @@ public DescriptorImpl() {
j.assertLogContains("truth is 0 but falsity is 1", j.assertBuildStatusSuccess(p.scheduleBuild2(0)));
}

@Issue("JENKINS-31096")
@Test public void encoding() throws Exception {
// Like JenkinsRule.createSlave but passing a system encoding:
Slave remote = new DumbSlave("remote", tmp.getRoot().getAbsolutePath(),
new SimpleCommandLauncher("'" + System.getProperty("java.home") + "/bin/java' -Dfile.encoding=ISO-8859-2 -jar '" + new File(j.jenkins.getJnlpJars("slave.jar").getURL().toURI()) + "'"));
j.jenkins.addNode(remote);
j.waitOnline(remote);
WorkflowJob p = j.createProject(WorkflowJob.class, "p");
FilePath ws;
while ((ws = remote.getWorkspaceFor(p)) == null) {
Thread.sleep(100);
}
ws.child("message").write("Čau ty vole!\n", "ISO-8859-2");
p.setDefinition(new CpsFlowDefinition("node('remote') {if (isUnix()) {sh 'cat message'} else {bat 'type message'}}", true));
j.assertLogContains("Čau ty vole!", j.buildAndAssertSuccess(p));
p.setDefinition(new CpsFlowDefinition("node('remote') {echo(/received: ${isUnix() ? sh(script: 'cat message', returnStdout: true) : bat(script: '@type message', returnStdout: true)}/)}", true)); // http://stackoverflow.com/a/8486061/12916
j.assertLogContains("received: Čau ty vole!", j.buildAndAssertSuccess(p));
p.setDefinition(new CpsFlowDefinition("node('remote') {if (isUnix()) {sh script: 'cat message', encoding: 'US-ASCII'} else {bat script: 'type message', encoding: 'US-ASCII'}}", true));
j.assertLogContains("�au ty vole!", j.buildAndAssertSuccess(p));
ws.child("message").write("¡Čau → there!\n", "UTF-8");
p.setDefinition(new CpsFlowDefinition("node('remote') {if (isUnix()) {sh script: 'cat message', encoding: 'UTF-8'} else {bat script: 'type message', encoding: 'UTF-8'}}", true));
j.assertLogContains("¡Čau → there!", j.buildAndAssertSuccess(p));
}

@Issue("JENKINS-34021")
@Test public void deadStep() throws Exception {
logging.record(DurableTaskStep.class, Level.INFO).record(CpsStepContext.class, Level.INFO).capture(100);
Expand Down