TemplateExpansionAction now consistently uses UTF-8 instead of mixing UTF-8 with Latin-1

--
MOS_MIGRATED_REVID=99651466

4 files changed, 77 insertions, 22 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java
index 0a806825c5..30ff2fb453 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java
@@ -14,8 +14,6 @@
 
 package com.google.devtools.build.lib.analysis.actions;
 
-import static java.nio.charset.StandardCharsets.UTF_8;
-
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.base.Objects;
@@ -33,6 +31,8 @@ import com.google.devtools.build.lib.vfs.Path;
 
 import java.io.IOException;
 import java.io.OutputStream;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
 import java.util.Collection;
 import java.util.List;
 
@@ -145,6 +145,8 @@ public class TemplateExpansionAction extends AbstractFileWriteAction {
    */
   public abstract static class Template {
 
+    private static final Charset DEFAULT_CHARSET = StandardCharsets.UTF_8;
+
     /**
      * We only allow subclasses in this file.
      */
@@ -218,7 +220,7 @@ public class TemplateExpansionAction extends AbstractFileWriteAction {
         protected String getContent() throws IOException {
           Path templatePath = templateArtifact.getPath();
           try {
-            return new String(FileSystemUtils.readContentAsLatin1(templatePath));
+            return FileSystemUtils.readContent(templatePath, DEFAULT_CHARSET);
           } catch (IOException e) {
             throw new IOException("failed to load template file '" + templatePath.getPathString()
                 + "' due to I/O error: " + e.getMessage(), e);
@@ -320,7 +322,7 @@ public class TemplateExpansionAction extends AbstractFileWriteAction {
   @Override
   public DeterministicWriter newDeterministicWriter(EventHandler eventHandler,
                                                     Executor executor) throws IOException {
-    final byte[] bytes = getFileContents().getBytes(UTF_8);
+    final byte[] bytes = getFileContents().getBytes(Template.DEFAULT_CHARSET);
     return new DeterministicWriter() {
       @Override
       public void writeOutputFile(OutputStream out) throws IOException {
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
index 6ce67aeac2..840433a11e 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
@@ -936,6 +936,13 @@ public class FileSystemUtils {
   }
 
   /**
+   * Reads the entire file using the given charset and returns the contents as a string
+   */
+  public static String readContent(Path inputFile, Charset charset) throws IOException {
+    return asByteSource(inputFile).asCharSource(charset).read();
+  }
+
+  /**
    * Reads at most {@code limit} bytes from {@code inputFile} and returns it as a byte array.
    *
    * @throws IOException if there was an error.
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java
index 500c35fe48..2576c729ac 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 
+import java.nio.charset.StandardCharsets;
 import java.util.List;
 
 /**
@@ -42,6 +43,7 @@ import java.util.List;
 public class TemplateExpansionActionTest extends FoundationTestCase {
 
   private static final String TEMPLATE = Joiner.on('\n').join("key=%key%", "value=%value%");
+  private static final String SPECIAL_CHARS = "Š©±½_strøget";
 
   private Root outputRoot;
   private Artifact inputArtifact;
@@ -54,12 +56,8 @@ public class TemplateExpansionActionTest extends FoundationTestCase {
   @Override
   protected void setUp() throws Exception {
     super.setUp();
-    Root workspace = Root.asSourceRoot(scratch.dir("/workspace"));
-    outputRoot = Root.asDerivedRoot(scratch.dir("/workspace"), scratch.dir("/workspace/out"));
-    Path input = scratch.file("/workspace/input.txt", TEMPLATE);
-    inputArtifact = new Artifact(input, workspace);
-    output = scratch.resolve("/workspace/out/destination.txt");
-    outputArtifact = new Artifact(output, outputRoot);
+    createArtifacts(TEMPLATE);
+
     substitutions = Lists.newArrayList();
     substitutions.add(Substitution.of("%key%", "foo"));
     substitutions.add(Substitution.of("%value%", "bar"));
@@ -70,6 +68,15 @@ public class TemplateExpansionActionTest extends FoundationTestCase {
     binTools = BinTools.empty(directories);
   }
 
+  private void createArtifacts(String template) throws Exception {
+    Root workspace = Root.asSourceRoot(scratch.dir("/workspace"));
+    outputRoot = Root.asDerivedRoot(scratch.dir("/workspace"), scratch.dir("/workspace/out"));
+    Path input = scratch.overwriteFile("/workspace/input.txt", StandardCharsets.UTF_8, template);
+    inputArtifact = new Artifact(input, workspace);
+    output = scratch.resolve("/workspace/out/destination.txt");
+    outputArtifact = new Artifact(output, outputRoot);
+  }
+
   private TemplateExpansionAction create() {
     TemplateExpansionAction result = new TemplateExpansionAction(NULL_ACTION_OWNER,
          outputArtifact, Template.forString(TEMPLATE), substitutions, false);
@@ -141,11 +148,31 @@ public class TemplateExpansionActionTest extends FoundationTestCase {
   }
 
   private TemplateExpansionAction createWithArtifact() {
-    TemplateExpansionAction result = new TemplateExpansionAction(NULL_ACTION_OWNER,
-         inputArtifact, outputArtifact, substitutions, false);
+    return createWithArtifact(substitutions);
+  }
+
+  private TemplateExpansionAction createWithArtifact(List<Substitution> substitutions) {
+    TemplateExpansionAction result = new TemplateExpansionAction(
+        NULL_ACTION_OWNER, inputArtifact, outputArtifact, substitutions, false);
     return result;
   }
 
+  private ActionExecutionContext createContext(Executor executor) {
+    return new ActionExecutionContext(executor, null, null, new FileOutErr(), null);
+  }
+
+  private void executeTemplateExpansion(String expected) throws Exception {
+    executeTemplateExpansion(expected, substitutions);
+  }
+
+  private void executeTemplateExpansion(String expected, List<Substitution> substitutions)
+      throws Exception {
+    Executor executor = new TestExecutorBuilder(directories, binTools).build();
+    createWithArtifact(substitutions).execute(createContext(executor));
+    String actual = FileSystemUtils.readContent(output, StandardCharsets.UTF_8);
+    assertThat(actual).isEqualTo(expected);
+  }
+
   public void testArtifactTemplateHasInput() {
     assertEquals(ImmutableList.of(inputArtifact), createWithArtifact().getInputs());
   }
@@ -155,15 +182,18 @@ public class TemplateExpansionActionTest extends FoundationTestCase {
   }
 
   public void testArtifactTemplateExpansion() throws Exception {
-    Executor executor = new TestExecutorBuilder(directories, binTools).build();
-    createWithArtifact().execute(createContext(executor));
-    String content = new String(FileSystemUtils.readContentAsLatin1(output));
-    // The trailing "" is needed because scratch.file implicitly appends "\n".
+    // The trailing "" is needed because scratch.overwriteFile implicitly appends "\n".
     String expected = Joiner.on('\n').join("key=foo", "value=bar", "");
-    assertEquals(expected, content);
+    executeTemplateExpansion(expected);
   }
 
-  private ActionExecutionContext createContext(Executor executor) {
-    return new ActionExecutionContext(executor, null, null, new FileOutErr(), null);
+  public void testWithSpecialCharacters() throws Exception {
+    // We have to overwrite the artifacts since we need our template in "inputs"
+    createArtifacts(SPECIAL_CHARS + "%key%");
+
+    // scratch.overwriteFile appends a newline, so we need an additional %n here
+    String expected = String.format("%s%s%n", SPECIAL_CHARS, SPECIAL_CHARS);
+
+    executeTemplateExpansion(expected, ImmutableList.of(Substitution.of("%key%", SPECIAL_CHARS)));
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/testutil/Scratch.java b/src/test/java/com/google/devtools/build/lib/testutil/Scratch.java
index dda097959c..93402d7845 100644
--- a/src/test/java/com/google/devtools/build/lib/testutil/Scratch.java
+++ b/src/test/java/com/google/devtools/build/lib/testutil/Scratch.java
@@ -22,11 +22,15 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 
 import java.io.IOException;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
 
 /**
  * Allow tests to easily manage scratch files in a FileSystem.
  */
 public final class Scratch {
+  
+  private static final Charset DEFAULT_CHARSET = StandardCharsets.ISO_8859_1;
 
   private final FileSystem fileSystem;
   private Path workingDir = null;
@@ -111,14 +115,18 @@ public final class Scratch {
     return dir;
   }
 
+  public Path file(String pathName, String... lines) throws IOException {
+    return file(pathName, DEFAULT_CHARSET, lines);
+  }
+  
   /**
    * Create a scratch file in the scratch filesystem, with the given pathName,
    * consisting of a set of lines. The method returns a Path instance for the
    * scratch file.
    */
-  public Path file(String pathName, String... lines) throws IOException {
+  public Path file(String pathName, Charset charset, String... lines) throws IOException {
     Path file = newFile(pathName);
-    FileSystemUtils.writeContentAsLatin1(file, linesAsString(lines));
+    FileSystemUtils.writeContent(file, charset, linesAsString(lines));
     file.setLastModifiedTime(-1L);
     return file;
   }
@@ -128,10 +136,18 @@ public final class Scratch {
    * exists.
    */
   public Path overwriteFile(String pathName, String... lines) throws IOException {
+    return overwriteFile(pathName, DEFAULT_CHARSET, lines);
+  }
+
+  /**
+   * Like {@code scratch.file}, but the file is first deleted if it already
+   * exists.
+   */
+  public Path overwriteFile(String pathName, Charset charset, String... lines) throws IOException {
     Path oldFile = resolve(pathName);
     long newMTime = oldFile.exists() ? oldFile.getLastModifiedTime() + 1 : -1;
     oldFile.delete();
-    Path newFile = file(pathName, lines);
+    Path newFile = file(pathName, charset, lines);
     newFile.setLastModifiedTime(newMTime);
     return newFile;
   }