[JEP-206] Define API for gathering command output in a local encoding

.mvn/extensions.xml

@@ -2,6 +2,6 @@
   <extension>
     <groupId>io.jenkins.tools.incrementals</groupId>
     <artifactId>git-changelist-maven-extension</artifactId>
-    <version>1.0-beta-3</version>
+    <version>1.0-beta-4</version>
   </extension>
 </extensions>

pom.xml

@@ -3,11 +3,11 @@
     <parent>
         <groupId>org.jenkins-ci.plugins</groupId>
         <artifactId>plugin</artifactId>
-        <version>3.14</version>
+        <version>3.19</version>
         <relativePath />
     </parent>
     <artifactId>durable-task</artifactId>
-    <version>1.24-SNAPSHOT</version>
+    <version>${revision}${changelist}</version>
     <packaging>hpi</packaging>
     <name>Durable Task Plugin</name>
     <description>Library offering an extension point for processes which can run outside of Jenkins yet be monitored.</description>
@@ -20,7 +20,7 @@
     </licenses>
 
     <properties>
-        <revision>1.23</revision>
+        <revision>1.24</revision>
         <changelist>-SNAPSHOT</changelist>
         <jenkins.version>2.7.3</jenkins.version>
         <java.level>7</java.level>

src/main/java/org/jenkinsci/plugins/durabletask/Controller.java

@@ -49,6 +49,8 @@ public abstract class Controller implements Serializable {
      * @param workspace the workspace in use
      * @param sink where to send new log output
      * @return true if something was written and the controller should be resaved, false if everything is idle
+     * @see DurableTask#charset
+     * @see DurableTask#defaultCharset
      */
     public abstract boolean writeLog(FilePath workspace, OutputStream sink) throws IOException, InterruptedException;
 
@@ -88,6 +90,8 @@ public abstract class Controller implements Serializable {
      * @param workspace the workspace in use
      * @param launcher a way to start processes
      * @return the output of the process as raw bytes (may be empty but not null)
+     * @see DurableTask#charset
+     * @see DurableTask#defaultCharset
      */
     public @Nonnull byte[] getOutput(@Nonnull FilePath workspace, @Nonnull Launcher launcher) throws IOException, InterruptedException {
         throw new IOException("Did not implement getOutput in " + getClass().getName());

src/main/java/org/jenkinsci/plugins/durabletask/DurableTask.java

@@ -31,13 +31,19 @@
 import hudson.model.AbstractDescribableImpl;
 import hudson.model.TaskListener;
 import java.io.IOException;
+import java.nio.charset.Charset;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import javax.annotation.Nonnull;
 
 /**
  * A task which may be run asynchronously on a build node and withstand disconnection of the slave agent.
  * Should have a descriptor, and a {@code config.jelly} for form data binding.
  */
 public abstract class DurableTask extends AbstractDescribableImpl<DurableTask> implements ExtensionPoint {
 
+    private static final Logger LOGGER = Logger.getLogger(DurableTask.class.getName());
+
     @Override public DurableTaskDescriptor getDescriptor() {
         return (DurableTaskDescriptor) super.getDescriptor();
     }
@@ -63,4 +69,23 @@ public void captureOutput() throws UnsupportedOperationException {
         throw new UnsupportedOperationException("Capturing of output is not implemented in " + getClass().getName());
     }
 
+    /**
+     * Requests that a specified charset be used to transcode process output.
+     * The encoding of {@link Controller#writeLog} and {@link Controller#getOutput} is then presumed to be UTF-8.
+     * If not called, no translation is performed.
+     * @param cs the character set in which process output is expected to be
+     */
+    public void charset(@Nonnull Charset cs) {
+        LOGGER.log(Level.WARNING, "The charset method should be overridden in {0}", getClass().getName());
+    }
+
+    /**
+     * Requests that the node’s system charset be used to transcode process output.
+     * The encoding of {@link Controller#writeLog} and {@link Controller#getOutput} is then presumed to be UTF-8.
+     * If not called, no translation is performed.
+     */
+    public void defaultCharset() {
+        LOGGER.log(Level.WARNING, "The defaultCharset method should be overridden in {0}", getClass().getName());
+    }
+
 }

src/main/java/org/jenkinsci/plugins/durabletask/FileMonitoringTask.java

@@ -37,22 +37,29 @@
 import hudson.util.StreamTaskListener;
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.OutputStream;
+import java.io.OutputStreamWriter;
 import java.io.RandomAccessFile;
 import java.io.StringWriter;
+import java.nio.ByteBuffer;
 import java.nio.charset.Charset;
+import java.nio.charset.CharsetDecoder;
+import java.nio.charset.CodingErrorAction;
+import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.UUID;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 import jenkins.MasterToSlaveFileCallable;
-import org.apache.commons.io.IOUtils;
+import jenkins.security.MasterToSlaveCallable;
+import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.output.CountingOutputStream;
-
-import javax.annotation.CheckForNull;
+import org.apache.commons.io.output.WriterOutputStream;
 
 /**
  * A task which forks some external command and then waits for log and status files to be updated/created.
@@ -63,19 +70,37 @@ public abstract class FileMonitoringTask extends DurableTask {
 
     private static final String COOKIE = "JENKINS_SERVER_COOKIE";
 
+    /** Value of {@link #charset} used to mean the node’s system default. */
+    private static final String SYSTEM_DEFAULT_CHARSET = "SYSTEM_DEFAULT";
+
+    /**
+     * Charset name to use for transcoding, or {@link #SYSTEM_DEFAULT_CHARSET}, or null for no transcoding.
+     */
+    private @CheckForNull String charset;
+
     private static String cookieFor(FilePath workspace) {
         return "durable-" + Util.getDigestOf(workspace.getRemote());
     }
 
     @Override public final Controller launch(EnvVars env, FilePath workspace, Launcher launcher, TaskListener listener) throws IOException, InterruptedException {
-        return launchWithCookie(workspace, launcher, listener, env, COOKIE, cookieFor(workspace));
+        FileMonitoringController controller = launchWithCookie(workspace, launcher, listener, env, COOKIE, cookieFor(workspace));
+        controller.charset = charset;
+        return controller;
     }
 
     protected FileMonitoringController launchWithCookie(FilePath workspace, Launcher launcher, TaskListener listener, EnvVars envVars, String cookieVariable, String cookieValue) throws IOException, InterruptedException {
         envVars.put(cookieVariable, cookieValue); // ensure getCharacteristicEnvVars does not match, so Launcher.killAll will leave it alone
         return doLaunch(workspace, launcher, listener, envVars);
     }
 
+    @Override public final void charset(Charset cs) {
+        charset = cs.name();
+    }
+
+    @Override public final void defaultCharset() {
+        charset = SYSTEM_DEFAULT_CHARSET;
+    }
+
     /**
      * Should start a process which sends output to {@linkplain FileMonitoringController#getLogFile(FilePath) log file}
      * in the workspace and finally writes its exit code to {@linkplain FileMonitoringController#getResultFile(FilePath) result file}.
@@ -115,6 +140,15 @@ protected static class FileMonitoringController extends Controller {
          */
         private long lastLocation;
 
+        /** @see FileMonitoringTask#charset */
+        private @CheckForNull String charset;
+
+        /**
+         * {@link #transcodingCharset} on the remote side when using {@link #writeLog}.
+         * May be a wrapper for null; initialized on demand.
+         */
+        private transient volatile AtomicReference<Charset> writeLogCs;
+
         protected FileMonitoringController(FilePath ws) throws IOException, InterruptedException {
             // can't keep ws reference because Controller is expected to be serializable
             ws.mkdirs();
@@ -124,12 +158,31 @@ protected FileMonitoringController(FilePath ws) throws IOException, InterruptedE
         }
 
         @Override public final boolean writeLog(FilePath workspace, OutputStream sink) throws IOException, InterruptedException {
+            if (writeLogCs == null) {
+                if (SYSTEM_DEFAULT_CHARSET.equals(charset)) {
+                    String cs = workspace.act(new TranscodingCharsetForSystemDefault());
+                    writeLogCs = new AtomicReference<>(cs == null ? null : Charset.forName(cs));
+                } else {
+                    // Does not matter what system default charset on the remote side is, so save the Remoting call.
+                    writeLogCs = new AtomicReference<>(transcodingCharset(charset));
+                }
+                LOGGER.log(Level.FINE, "remote transcoding charset: {0}", writeLogCs);
+            }
             FilePath log = getLogFile(workspace);
-            CountingOutputStream cos = new CountingOutputStream(sink);
+            OutputStream transcodedSink;
+            if (writeLogCs.get() == null) {
+                transcodedSink = sink;
+            } else {
+                // WriterOutputStream constructor taking Charset calls .replaceWith("?") which we do not want:
+                CharsetDecoder decoder = writeLogCs.get().newDecoder().onMalformedInput(CodingErrorAction.REPLACE).onUnmappableCharacter(CodingErrorAction.REPLACE);
+                transcodedSink = new WriterOutputStream(new OutputStreamWriter(sink, StandardCharsets.UTF_8), decoder, 1024, true);
+            }
+            CountingOutputStream cos = new CountingOutputStream(transcodedSink);
             try {
                 log.act(new WriteLog(lastLocation, new RemoteOutputStream(cos)));
                 return cos.getByteCount() > 0;
             } finally { // even if RemoteOutputStream write was interrupted, record what we actually received
+                transcodedSink.flush(); // writeImmediately flag does not seem to work
                 long written = cos.getByteCount();
                 if (written > 0) {
                     LOGGER.log(Level.FINE, "copied {0} bytes from {1}", new Object[] {written, log});
@@ -162,6 +215,13 @@ private static class WriteLog extends MasterToSlaveFileCallable<Void> {
             }
         }
 
+        private static class TranscodingCharsetForSystemDefault extends MasterToSlaveCallable<String, RuntimeException> {
+            @Override public String call() throws RuntimeException {
+                Charset cs = transcodingCharset(SYSTEM_DEFAULT_CHARSET);
+                return cs != null ? cs.name() : null;
+            }
+        }
+
         /** Avoids excess round-tripping when reading status file. */
         static class StatusCheck extends MasterToSlaveFileCallable<Integer> {
             @Override
@@ -196,9 +256,47 @@ public Integer invoke(File f, VirtualChannel channel) throws IOException, Interr
         }
 
         @Override public byte[] getOutput(FilePath workspace, Launcher launcher) throws IOException, InterruptedException {
-            // TODO could perhaps be more efficient for large files to send a MasterToSlaveFileCallable<byte[]>
-            try (InputStream is = getOutputFile(workspace).read()) {
-                return IOUtils.toByteArray(is);
+            return getOutputFile(workspace).act(new MasterToSlaveFileCallable<byte[]>() {
+                @Override public byte[] invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
+                    byte[] buf = FileUtils.readFileToByteArray(f);
+                    ByteBuffer transcoded = maybeTranscode(buf, charset);
+                    if (transcoded == null) {
+                        return buf;
+                    } else {
+                        byte[] buf2 = new byte[transcoded.remaining()];
+                        transcoded.get(buf2);
+                        return buf2;
+                    }
+                }
+            });
+        }
+
+        /**
+         * Transcode process output to UTF-8 if necessary.
+         * @param data output presumed to be in local encoding
+         * @param charset a particular encoding name, or the empty string for the system default encoding, or null to skip transcoding
+         * @return a buffer of UTF-8 encoded data ({@link CodingErrorAction#REPLACE} is used),
+         *         or null if not performing transcoding because it was not requested or the data was already thought to be in UTF-8
+         */
+        private static @CheckForNull ByteBuffer maybeTranscode(@Nonnull byte[] data, @CheckForNull String charset) {
+            Charset cs = transcodingCharset(charset);
+            if (cs == null) {
+                return null;
+            } else {
+                return StandardCharsets.UTF_8.encode(cs.decode(ByteBuffer.wrap(data)));
+            }
+        }
+
+        private static @CheckForNull Charset transcodingCharset(@CheckForNull String charset) {
+            if (charset == null) {
+                return null;
+            } else {
+                Charset cs = charset.equals(SYSTEM_DEFAULT_CHARSET) ? Charset.defaultCharset() : Charset.forName(charset);
+                if (cs.equals(StandardCharsets.UTF_8)) { // transcoding unnecessary as output was already UTF-8
+                    return null;
+                } else { // decode output in specified charset and reëncode in UTF-8
+                    return cs;
+                }
             }
         }
 

src/test/java/org/jenkinsci/plugins/durabletask/BourneShellScriptTest.java

@@ -34,8 +34,12 @@
 import hudson.util.VersionNumber;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
+import java.io.OutputStream;
+import java.nio.charset.Charset;
 import java.util.Collections;
+import java.util.Locale;
 import java.util.logging.Level;
+import jenkins.security.MasterToSlaveCallable;
 import org.apache.commons.io.output.TeeOutputStream;
 import static org.hamcrest.Matchers.*;
 import org.jenkinsci.test.acceptance.docker.Docker;
@@ -67,7 +71,7 @@ public class BourneShellScriptTest {
         assumeThat("Docker must be at least 1.13.0 for this test (uses --init)", new VersionNumber(baos.toString().trim()), greaterThanOrEqualTo(new VersionNumber("1.13.0")));
     }
 
-    @Rule public LoggerRule logging = new LoggerRule().record(BourneShellScript.class, Level.FINE);
+    @Rule public LoggerRule logging = new LoggerRule().recordPackage(BourneShellScript.class, Level.FINE);
 
     private StreamTaskListener listener;
     private FilePath ws;
@@ -236,4 +240,64 @@ private void runOnDocker(DumbSlave s) throws Exception {
         runOnDocker(new DumbSlave("docker", "/home/jenkins/agent", new SimpleCommandLauncher("docker run -i --rm --name agent --init jenkinsci/slave:3.7-1 java -jar /usr/share/jenkins/slave.jar")));
     }
 
+    @Issue("JENKINS-31096")
+    @Test public void encoding() throws Exception {
+        JavaContainer container = dockerUbuntu.get();
+        DumbSlave s = new DumbSlave("docker", "/home/test", new SSHLauncher(container.ipBound(22), container.port(22), "test", "test", "", "-Dfile.encoding=ISO-8859-1"));
+        j.jenkins.addNode(s);
+        j.waitOnline(s);
+        assertEquals("ISO-8859-1", s.getChannel().call(new DetectCharset()));
+        FilePath dockerWS = s.getWorkspaceRoot();
+        dockerWS.child("latin").write("¡Ole!\n", "ISO-8859-1");
+        dockerWS.child("eastern").write("Čau!\n", "ISO-8859-2");
+        dockerWS.child("mixed").write("¡Čau → there!\n", "UTF-8");
+        Launcher dockerLauncher = s.createLauncher(listener);
+        assertEncoding("control: no transcoding", "latin", null, "¡Ole!", "ISO-8859-1", dockerWS, dockerLauncher);
+        assertEncoding("test: specify particular charset (UTF-8)", "mixed", "UTF-8", "¡Čau → there!", "UTF-8", dockerWS, dockerLauncher);
+        assertEncoding("test: specify particular charset (unrelated)", "eastern", "ISO-8859-2", "Čau!", "UTF-8", dockerWS, dockerLauncher);
+        assertEncoding("test: specify agent default charset", "latin", "", "¡Ole!", "UTF-8", dockerWS, dockerLauncher);
+        assertEncoding("test: inappropriate charset, some replacement characters", "mixed", "US-ASCII", "����au ��� there!", "UTF-8", dockerWS, dockerLauncher);
+        s.toComputer().disconnect(new OfflineCause.UserCause(null, null));
+    }
+    private static class DetectCharset extends MasterToSlaveCallable<String, RuntimeException> {
+        @Override public String call() throws RuntimeException {
+            return Charset.defaultCharset().name();
+        }
+    }
+    private void assertEncoding(String description, String file, String charset, String expected, String expectedEncoding, FilePath dockerWS, Launcher dockerLauncher) throws Exception {
+        assertEncoding(description, file, charset, expected, expectedEncoding, false, dockerWS, dockerLauncher);
+        assertEncoding(description, file, charset, expected, expectedEncoding, true, dockerWS, dockerLauncher);
+    }
+    private void assertEncoding(String description, String file, String charset, String expected, String expectedEncoding, boolean output, FilePath dockerWS, Launcher dockerLauncher) throws Exception {
+        System.err.println(description + " (output=" + output + ")"); // TODO maybe this should just be moved into a new class and @RunWith(Parameterized.class) for clarity
+        BourneShellScript dt = new BourneShellScript("set +x; cat " + file + "; sleep 1; tr '[a-z]' '[A-Z]' < " + file);
+        if (charset != null) {
+            if (charset.isEmpty()) {
+                dt.defaultCharset();
+            } else {
+                dt.charset(Charset.forName(charset));
+            }
+        }
+        if (output) {
+            dt.captureOutput();
+        }
+        Controller c = dt.launch(new EnvVars(), dockerWS, dockerLauncher, listener);
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        OutputStream tee = new TeeOutputStream(baos, System.err);
+        while (c.exitStatus(dockerWS, dockerLauncher, listener) == null) {
+            c.writeLog(dockerWS, tee);
+            Thread.sleep(100);
+        }
+        c.writeLog(dockerWS, tee);
+        assertEquals(description, 0, c.exitStatus(dockerWS, dockerLauncher, listener).intValue());
+        String fullExpected = expected + "\n" + expected.toUpperCase(Locale.ENGLISH) + "\n";
+        if (output) {
+            assertEquals(description, fullExpected, new String(c.getOutput(dockerWS, launcher), expectedEncoding));
+        } else {
+            assertThat(description, baos.toString(expectedEncoding), containsString(fullExpected));
+        }
+        c.cleanup(dockerWS);
+
+    }
+
 }