diff options
author | Florian Weikert <fwe@google.com> | 2015-08-01 20:03:46 +0000 |
---|---|---|
committer | Damien Martin-Guillerez <dmarting@google.com> | 2015-08-04 09:07:14 +0000 |
commit | 75037576b6bb97fa970ace22d72ddc3d5d0d61bb (patch) | |
tree | 31fdf763a23bdefbe718faaecf138eb2f072df8e /src | |
parent | d75bf4da5748b251f09d73e0f977e35698e779df (diff) |
TemplateExpansionAction now consistently uses UTF-8 instead of mixing UTF-8 with Latin-1
--
MOS_MIGRATED_REVID=99651466
Diffstat (limited to 'src')
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; } |