Improve GN handling of directory templates.

The previous implementation of the directory-based file templates (like {{source_out_dir}}}) was overly simplistic. It did not allow these templates to be used in the outputs section of a target since the directories were relative to the build directory (it was only useful for actions).

This new implementation changes the expansion of these values depending on where they're used, so they can be absolute when used in the outputs section, but relative to the build directory when used in the args section.

The implementation of outputs was changed to allow the use of these expansions. The previous implementation stored them as SourceFiles which didn't allow the use of directory expansions, since that happens after converting the input to a SourceFile. This new patch stores the outputs as a list of strings to allow this to be late-bound properly (and it turns out that almost all users of this wanted the strings anyway).

Changes the function that checks that files are in the output directory to optionally handle templates. Adds a test for this function.

Fixes a bug in get_target_outputs that it didn't work for copy() targets that contained templates. Fix this and add a test.

BUG=
R=viettrungluu@chromium.org

Review URL: https://codereview.chromium.org/387663003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282639 0039d316-1c4b-4281-b951-d872f2087c98

tools/gn/action_target_generator.cc

@@ -17,10 +17,10 @@ namespace {
 
 // Returns true if the list of files looks like it might have a {{ }} pattern
 // in it. Used for error checking.
-bool FileListHasPattern(const Target::FileList& files) {
+bool StringListHasPattern(const std::vector<std::string>& files) {
   for (size_t i = 0; i < files.size(); i++) {
-    if (files[i].value().find("{{") != std::string::npos &&
-        files[i].value().find("}}") != std::string::npos)
+    if (files[i].find("{{") != std::string::npos &&
+        files[i].find("}}") != std::string::npos)
       return true;
   }
   return false;
@@ -108,10 +108,9 @@ void ActionTargetGenerator::FillScriptArgs() {
   if (!value)
     return;
 
-  std::vector<std::string> args;
-  if (!ExtractListOfStringValues(*value, &args, err_))
+  if (!ExtractListOfStringValues(*value, &target_->action_values().args(),
+                                 err_))
     return;
-  target_->action_values().swap_in_args(&args);
 }
 
 void ActionTargetGenerator::FillDepfile() {
@@ -124,7 +123,7 @@ void ActionTargetGenerator::FillDepfile() {
 }
 
 void ActionTargetGenerator::CheckOutputs() {
-  const Target::FileList& outputs = target_->action_values().outputs();
+  const std::vector<std::string>& outputs = target_->action_values().outputs();
   if (outputs.empty()) {
     *err_ = Err(function_call_, "Action has no outputs.",
         "If you have no outputs, the build system can not tell when your\n"
@@ -134,7 +133,7 @@ void ActionTargetGenerator::CheckOutputs() {
 
   if (output_type_ == Target::ACTION) {
     // Make sure the outputs for an action have no patterns in them.
-    if (FileListHasPattern(outputs)) {
+    if (StringListHasPattern(outputs)) {
       *err_ = Err(function_call_, "Action has patterns in the output.",
           "An action target should have the outputs completely specified. If\n"
           "you want to provide a mapping from source to output, use an\n"
@@ -143,7 +142,7 @@ void ActionTargetGenerator::CheckOutputs() {
     }
   } else if (output_type_ == Target::ACTION_FOREACH) {
     // A foreach target should always have a pattern in the outputs.
-    if (!FileListHasPattern(outputs)) {
+    if (!StringListHasPattern(outputs)) {
       *err_ = Err(function_call_,
           "action_foreach should have a pattern in the output.",
           "An action_foreach target should have a source expansion pattern in\n"

tools/gn/action_values.h

@@ -25,12 +25,11 @@ class ActionValues {
   // Arguments to the script.
   std::vector<std::string>& args() { return args_; }
   const std::vector<std::string>& args() const { return args_; }
-  void swap_in_args(std::vector<std::string>* a) { args_.swap(*a); }
 
-  // Files created by the script.
-  std::vector<SourceFile>& outputs() { return outputs_; }
-  const std::vector<SourceFile>& outputs() const { return outputs_; }
-  void swap_in_outputs(std::vector<SourceFile>* op) { outputs_.swap(*op); }
+  // Files created by the script. These are strings rather than SourceFiles
+  // since they will often contain {{source expansions}}.
+  std::vector<std::string>& outputs() { return outputs_; }
+  const std::vector<std::string>& outputs() const { return outputs_; }
 
   // Depfile generated by the script.
   const SourceFile& depfile() const { return depfile_; }
@@ -40,7 +39,7 @@ class ActionValues {
  private:
   SourceFile script_;
   std::vector<std::string> args_;
-  std::vector<SourceFile> outputs_;
+  std::vector<std::string> outputs_;
   SourceFile depfile_;
 
   DISALLOW_COPY_AND_ASSIGN(ActionValues);

tools/gn/command_desc.cc

@@ -248,6 +248,25 @@ void PrintFileList(const Target::FileList& files,
     OutputString(indent + sorted[i].value() + "\n");
 }
 
+// This sorts the list.
+void PrintStringList(const std::vector<std::string>& strings,
+                     const std::string& header,
+                     bool indent_extra,
+                     bool display_header) {
+  if (strings.empty())
+    return;
+
+  if (display_header)
+    OutputString("\n" + header + ":\n");
+
+  std::string indent = indent_extra ? "    " : "  ";
+
+  std::vector<std::string> sorted = strings;
+  std::sort(sorted.begin(), sorted.end());
+  for (size_t i = 0; i < sorted.size(); i++)
+    OutputString(indent + sorted[i] + "\n");
+}
+
 void PrintSources(const Target* target, bool display_header) {
   PrintFileList(target->sources(), "sources", false, display_header);
 }
@@ -259,16 +278,17 @@ void PrintInputs(const Target* target, bool display_header) {
 void PrintOutputs(const Target* target, bool display_header) {
   if (target->output_type() == Target::ACTION) {
     // Just display the outputs directly.
-    PrintFileList(target->action_values().outputs(), "outputs", false,
+    PrintStringList(target->action_values().outputs(), "outputs", false,
                     display_header);
-  } else if (target->output_type() == Target::ACTION_FOREACH) {
+  } else if (target->output_type() == Target::ACTION_FOREACH ||
+             target->output_type() == Target::COPY_FILES) {
     // Display both the output pattern and resolved list.
     if (display_header)
       OutputString("\noutputs:\n");
 
     // Display the pattern.
     OutputString("  Output pattern:\n");
-    PrintFileList(target->action_values().outputs(), "", true, false);
+    PrintStringList(target->action_values().outputs(), "", true, false);
 
     // Now display what that resolves to given the sources.
     OutputString("\n  Resolved output file list:\n");
@@ -277,11 +297,7 @@ void PrintOutputs(const Target* target, bool display_header) {
     FileTemplate file_template = FileTemplate::GetForTargetOutputs(target);
     for (size_t i = 0; i < target->sources().size(); i++)
       file_template.Apply(target->sources()[i], &output_strings);
-
-    std::sort(output_strings.begin(), output_strings.end());
-    for (size_t i = 0; i < output_strings.size(); i++) {
-      OutputString("    " + output_strings[i] + "\n");
-    }
+    PrintStringList(output_strings, "", true, false);
   }
 }
 

tools/gn/file_template.h

@@ -10,6 +10,7 @@
 #include "base/basictypes.h"
 #include "base/containers/stack_container.h"
 #include "tools/gn/err.h"
+#include "tools/gn/source_dir.h"
 #include "tools/gn/value.h"
 
 struct EscapeOptions;
@@ -30,6 +31,11 @@ extern const char kSourceExpansion_Help[];
 // substitutions.
 class FileTemplate {
  public:
+  enum OutputStyle {
+    OUTPUT_ABSOLUTE,  // Dirs will be absolute "//foo/bar".
+    OUTPUT_RELATIVE,  // Dirs will be relative to a given directory.
+  };
+
   struct Subrange {
     // See the help in the .cc file for what these mean.
     enum Type {
@@ -58,9 +64,19 @@ class FileTemplate {
 
   // Constructs a template from the given value. On error, the err will be
   // set. In this case you should not use this object.
-  FileTemplate(const Settings* settings, const Value& t, Err* err);
-  FileTemplate(const Settings* settings, const std::vector<std::string>& t);
-  FileTemplate(const Settings* settings, const std::vector<SourceFile>& t);
+  FileTemplate(const Settings* settings,
+               const Value& t,
+               OutputStyle output_style,
+               const SourceDir& relative_to,
+               Err* err);
+  FileTemplate(const Settings* settings,
+               const std::vector<std::string>& t,
+               OutputStyle output_style,
+               const SourceDir& relative_to);
+  FileTemplate(const Settings* settings,
+               const std::vector<SourceFile>& t,
+               OutputStyle output_style,
+               const SourceDir& relative_to);
 
   ~FileTemplate();
 
@@ -105,7 +121,6 @@ class FileTemplate {
   // (see GetWithNinjaExpansions).
   void WriteNinjaVariablesForSubstitution(
       std::ostream& out,
-      const Settings* settings,
       const SourceFile& source,
       const EscapeOptions& escape_options) const;
 
@@ -113,13 +128,18 @@ class FileTemplate {
   // substitute for the given type.
   static const char* GetNinjaVariableNameForType(Subrange::Type type);
 
-  // Extracts the given type of substitution from the given source. The source
-  // should be the file name relative to the output directory.
+  // Extracts the given type of substitution from the given source file.
+  // If output_style is RELATIVE, relative_to indicates the directory that the
+  // relative directories should be relative to, otherwise it is ignored.
   static std::string GetSubstitution(const Settings* settings,
                                      const SourceFile& source,
-                                     Subrange::Type type);
+                                     Subrange::Type type,
+                                     OutputStyle output_style,
+                                     const SourceDir& relative_to);
 
-  // Known template types, these include the "{{ }}"
+  // Known template types, these include the "{{ }}".
+  // IF YOU ADD NEW ONES: If the expansion expands to something inside the
+  // output directory, also update EnsureStringIsInOutputDir.
   static const char kSource[];
   static const char kSourceNamePart[];
   static const char kSourceFilePart[];
@@ -138,6 +158,8 @@ class FileTemplate {
   void ParseOneTemplateString(const std::string& str);
 
   const Settings* settings_;
+  OutputStyle output_style_;
+  SourceDir relative_to_;
 
   TemplateVector templates_;
 

tools/gn/file_template_unittest.cc

@@ -14,7 +14,8 @@ TEST(FileTemplate, Static) {
 
   std::vector<std::string> templates;
   templates.push_back("something_static");
-  FileTemplate t(setup.settings(), templates);
+  FileTemplate t(setup.settings(), templates,
+                 FileTemplate::OUTPUT_ABSOLUTE, SourceDir("//"));
   EXPECT_FALSE(t.has_substitutions());
 
   std::vector<std::string> result;
@@ -29,7 +30,8 @@ TEST(FileTemplate, Typical) {
   std::vector<std::string> templates;
   templates.push_back("foo/{{source_name_part}}.cc");
   templates.push_back("foo/{{source_name_part}}.h");
-  FileTemplate t(setup.settings(), templates);
+  FileTemplate t(setup.settings(), templates,
+                 FileTemplate::OUTPUT_ABSOLUTE, SourceDir("//"));
   EXPECT_TRUE(t.has_substitutions());
 
   std::vector<std::string> result;
@@ -44,7 +46,9 @@ TEST(FileTemplate, Weird) {
 
   std::vector<std::string> templates;
   templates.push_back("{{{source}}{{source}}{{");
-  FileTemplate t(setup.settings(), templates);
+  FileTemplate t(setup.settings(), templates,
+                 FileTemplate::OUTPUT_RELATIVE,
+                 setup.settings()->build_settings()->build_dir());
   EXPECT_TRUE(t.has_substitutions());
 
   std::vector<std::string> result;
@@ -62,7 +66,9 @@ TEST(FileTemplate, NinjaExpansions) {
   templates.push_back("--out=foo bar\"{{source_name_part}}\".o");
   templates.push_back("");  // Test empty string.
 
-  FileTemplate t(setup.settings(), templates);
+  FileTemplate t(setup.settings(), templates,
+                 FileTemplate::OUTPUT_RELATIVE,
+                 setup.settings()->build_settings()->build_dir());
 
   std::ostringstream out;
   t.WriteWithNinjaExpansions(out);
@@ -95,13 +101,15 @@ TEST(FileTemplate, NinjaVariables) {
   templates.push_back("{{source_gen_dir}}");
   templates.push_back("{{source_out_dir}}");
 
-  FileTemplate t(setup.settings(), templates);
+  FileTemplate t(setup.settings(), templates,
+                 FileTemplate::OUTPUT_RELATIVE,
+                 setup.settings()->build_settings()->build_dir());
 
   std::ostringstream out;
   EscapeOptions options;
   options.mode = ESCAPE_NINJA_COMMAND;
-  t.WriteNinjaVariablesForSubstitution(out, setup.settings(),
-                                       SourceFile("//foo/bar.txt"), options);
+  t.WriteNinjaVariablesForSubstitution(out, SourceFile("//foo/bar.txt"),
+                                       options);
 
   // Just the variables used above should be written.
   EXPECT_EQ(
@@ -120,27 +128,56 @@ TEST(FileTemplate, NinjaVariables) {
 TEST(FileTemplate, Substitutions) {
   TestWithScope setup;
 
-  #define GetSubst(str, what) \
-      FileTemplate::GetSubstitution(setup.settings(), \
+  // Call to get substitutions relative to the build dir.
+  #define GetRelSubst(str, what) \
+      FileTemplate::GetSubstitution( \
+          setup.settings(), \
           SourceFile(str), \
-                                    FileTemplate::Subrange::what)
+          FileTemplate::Subrange::what, \
+          FileTemplate::OUTPUT_RELATIVE, \
+          setup.settings()->build_settings()->build_dir())
+
+  // Call to get absolute directory substitutions.
+  #define GetAbsSubst(str, what) \
+      FileTemplate::GetSubstitution( \
+          setup.settings(), \
+          SourceFile(str), \
+          FileTemplate::Subrange::what, \
+          FileTemplate::OUTPUT_ABSOLUTE, \
+          SourceDir())
 
   // Try all possible templates with a normal looking string.
-  EXPECT_EQ("../../foo/bar/baz.txt", GetSubst("//foo/bar/baz.txt", SOURCE));
-  EXPECT_EQ("baz", GetSubst("//foo/bar/baz.txt", NAME_PART));
-  EXPECT_EQ("baz.txt", GetSubst("//foo/bar/baz.txt", FILE_PART));
-  EXPECT_EQ("../../foo/bar", GetSubst("//foo/bar/baz.txt", SOURCE_DIR));
-  EXPECT_EQ("foo/bar", GetSubst("//foo/bar/baz.txt", ROOT_RELATIVE_DIR));
-  EXPECT_EQ("gen/foo/bar", GetSubst("//foo/bar/baz.txt", SOURCE_GEN_DIR));
-  EXPECT_EQ("obj/foo/bar", GetSubst("//foo/bar/baz.txt", SOURCE_OUT_DIR));
+  EXPECT_EQ("../../foo/bar/baz.txt", GetRelSubst("//foo/bar/baz.txt", SOURCE));
+  EXPECT_EQ("//foo/bar/baz.txt", GetAbsSubst("//foo/bar/baz.txt", SOURCE));
+
+  EXPECT_EQ("baz", GetRelSubst("//foo/bar/baz.txt", NAME_PART));
+  EXPECT_EQ("baz", GetAbsSubst("//foo/bar/baz.txt", NAME_PART));
+
+  EXPECT_EQ("baz.txt", GetRelSubst("//foo/bar/baz.txt", FILE_PART));
+  EXPECT_EQ("baz.txt", GetAbsSubst("//foo/bar/baz.txt", FILE_PART));
+
+  EXPECT_EQ("../../foo/bar", GetRelSubst("//foo/bar/baz.txt", SOURCE_DIR));
+  EXPECT_EQ("//foo/bar", GetAbsSubst("//foo/bar/baz.txt", SOURCE_DIR));
+
+  EXPECT_EQ("foo/bar", GetRelSubst("//foo/bar/baz.txt", ROOT_RELATIVE_DIR));
+  EXPECT_EQ("foo/bar", GetAbsSubst("//foo/bar/baz.txt", ROOT_RELATIVE_DIR));
+
+  EXPECT_EQ("gen/foo/bar", GetRelSubst("//foo/bar/baz.txt", SOURCE_GEN_DIR));
+  EXPECT_EQ("//out/Debug/gen/foo/bar",
+            GetAbsSubst("//foo/bar/baz.txt", SOURCE_GEN_DIR));
+
+  EXPECT_EQ("obj/foo/bar", GetRelSubst("//foo/bar/baz.txt", SOURCE_OUT_DIR));
+  EXPECT_EQ("//out/Debug/obj/foo/bar",
+            GetAbsSubst("//foo/bar/baz.txt", SOURCE_OUT_DIR));
 
   // Operations on an absolute path.
-  EXPECT_EQ("/baz.txt", GetSubst("/baz.txt", SOURCE));
-  EXPECT_EQ("/.", GetSubst("/baz.txt", SOURCE_DIR));
-  EXPECT_EQ("gen", GetSubst("/baz.txt", SOURCE_GEN_DIR));
-  EXPECT_EQ("obj", GetSubst("/baz.txt", SOURCE_OUT_DIR));
+  EXPECT_EQ("/baz.txt", GetRelSubst("/baz.txt", SOURCE));
+  EXPECT_EQ("/.", GetRelSubst("/baz.txt", SOURCE_DIR));
+  EXPECT_EQ("gen", GetRelSubst("/baz.txt", SOURCE_GEN_DIR));
+  EXPECT_EQ("obj", GetRelSubst("/baz.txt", SOURCE_OUT_DIR));
 
-  EXPECT_EQ(".", GetSubst("//baz.txt", ROOT_RELATIVE_DIR));
+  EXPECT_EQ(".", GetRelSubst("//baz.txt", ROOT_RELATIVE_DIR));
 
-  #undef GetSubst
+  #undef GetAbsSubst
+  #undef GetRelSubst
 }

tools/gn/filesystem_utils.cc

@@ -11,6 +11,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "tools/gn/file_template.h"
 #include "tools/gn/location.h"
 #include "tools/gn/settings.h"
 #include "tools/gn/source_dir.h"
@@ -334,23 +335,28 @@ base::StringPiece FindLastDirComponent(const SourceDir& dir) {
 bool EnsureStringIsInOutputDir(const SourceDir& dir,
                                const std::string& str,
                                const Value& originating,
+                               bool allow_templates,
                                Err* err) {
-  // The last char of the dir will be a slash. We don't care if the input ends
-  // in a slash or not, so just compare up until there.
-  //
   // This check will be wrong for all proper prefixes "e.g. "/output" will
   // match "/out" but we don't really care since this is just a sanity check.
   const std::string& dir_str = dir.value();
-  if (str.compare(0, dir_str.length() - 1, dir_str, 0, dir_str.length() - 1)
-      != 0) {
+  if (str.compare(0, dir_str.length(), dir_str, 0, dir_str.length()) == 0)
+    return true;  // Output directory is hardcoded.
+
+  if (allow_templates) {
+    // Allow the string to begin with any source expansion inside the output
+    // directory.
+    if (StartsWithASCII(str, FileTemplate::kSourceGenDir, true) ||
+        StartsWithASCII(str, FileTemplate::kSourceOutDir, true))
+      return true;
+  }
+
   *err = Err(originating, "File is not inside output directory.",
       "The given file should be in the output directory. Normally you would "
       "specify\n\"$target_out_dir/foo\" or "
       "\"$target_gen_dir/foo\". I interpreted this as\n\""
       + str + "\".");
   return false;
-  }
-  return true;
 }
 
 bool IsPathAbsolute(const base::StringPiece& path) {

tools/gn/filesystem_utils.h

@@ -104,11 +104,16 @@ base::StringPiece FindLastDirComponent(const SourceDir& dir);
 //
 // The originating value will be blamed in the error.
 //
+// Setting allow_templates indicates that the result will be put into a
+// FileTemplate and it will allow the strings to be prefixed with expansions
+// that would include the output path.
+//
 // If the file isn't in the dir, returns false and sets the error. Otherwise
 // returns true and leaves the error untouched.
 bool EnsureStringIsInOutputDir(const SourceDir& dir,
                                const std::string& str,
                                const Value& originating,
+                               bool allow_templates,
                                Err* err);
 
 // ----------------------------------------------------------------------------

tools/gn/filesystem_utils_unittest.cc

@@ -90,6 +90,50 @@ TEST(FilesystemUtils, FindLastDirComponent) {
   EXPECT_EQ("bar", FindLastDirComponent(regular2));
 }
 
+TEST(FilesystemUtils, EnsureStringIsInOutputDir) {
+  SourceDir output_dir("//out/Debug/");
+
+  // Some outside.
+  Err err;
+  EXPECT_FALSE(EnsureStringIsInOutputDir(output_dir, "//foo", Value(), false,
+                                         &err));
+  EXPECT_TRUE(err.has_error());
+  err = Err();
+  EXPECT_FALSE(EnsureStringIsInOutputDir(output_dir, "//out/Debugit", Value(),
+                                         false, &err));
+  EXPECT_TRUE(err.has_error());
+
+  // Some inside.
+  err = Err();
+  EXPECT_TRUE(EnsureStringIsInOutputDir(output_dir, "//out/Debug/", Value(),
+                                        false, &err));
+  EXPECT_FALSE(err.has_error());
+  EXPECT_TRUE(EnsureStringIsInOutputDir(output_dir, "//out/Debug/foo", Value(),
+                                        false, &err));
+  EXPECT_FALSE(err.has_error());
+
+  // Pattern but no template expansions are allowed.
+  EXPECT_FALSE(EnsureStringIsInOutputDir(output_dir, "{{source_gen_dir}}",
+                                         Value(), false, &err));
+  EXPECT_TRUE(err.has_error());
+
+  // Pattern with template expansions allowed.
+  err = Err();
+  EXPECT_TRUE(EnsureStringIsInOutputDir(output_dir, "{{source_gen_dir}}",
+                                        Value(), true, &err));
+  EXPECT_FALSE(err.has_error());
+
+  // Template expansion that doesn't include the absolute directory.
+  EXPECT_FALSE(EnsureStringIsInOutputDir(output_dir, "{{source}}",
+                                         Value(), true, &err));
+  EXPECT_TRUE(err.has_error());
+  err = Err();
+  EXPECT_FALSE(EnsureStringIsInOutputDir(output_dir,
+                                        "{{source_root_relative_dir}}",
+                                        Value(), true, &err));
+  EXPECT_TRUE(err.has_error());
+}
+
 TEST(FilesystemUtils, IsPathAbsolute) {
   EXPECT_TRUE(IsPathAbsolute("/foo/bar"));
   EXPECT_TRUE(IsPathAbsolute("/"));

tools/gn/function_get_target_outputs.cc

@@ -19,19 +19,20 @@ void GetOutputsForTarget(const Settings* settings,
                          const Target* target,
                          std::vector<std::string>* ret) {
   switch (target->output_type()) {
-    case Target::ACTION:
-    case Target::COPY_FILES: {
-      // Actions and copy targets: return the outputs specified.
-      const std::vector<SourceFile>& outs = target->action_values().outputs();
+    case Target::ACTION: {
+      // Actions: return the outputs specified.
+      const std::vector<std::string>& outs = target->action_values().outputs();
       ret->reserve(outs.size());
       for (size_t i = 0; i < outs.size(); i++)
-        ret->push_back(outs[i].value());
+        ret->push_back(outs[i]);
       break;
     }
 
+    case Target::COPY_FILES:
     case Target::ACTION_FOREACH: {
-      // Action_foreach: return the result of the template in the outputs.
-      FileTemplate file_template(settings, target->action_values().outputs());
+      // Copy/action_foreach: return the result of the template in the outputs.
+      FileTemplate file_template(settings, target->action_values().outputs(),
+                                 FileTemplate::OUTPUT_ABSOLUTE, SourceDir());
       const std::vector<SourceFile>& sources = target->sources();
       for (size_t i = 0; i < sources.size(); i++)
         file_template.Apply(sources[i], ret);

tools/gn/function_get_target_outputs_unittest.cc

@@ -82,11 +82,26 @@ TEST_F(GetTargetOutputsTest, SourceSet) {
   AssertSingleStringEquals(result, "//out/Debug/obj/foo/bar.stamp");
 }
 
+TEST_F(GetTargetOutputsTest, Copy) {
+  Target* action = new Target(setup_.settings(), GetLabel("//foo/", "bar"));
+  action->set_output_type(Target::COPY_FILES);
+  action->sources().push_back(SourceFile("//file.txt"));
+  action->action_values().outputs().push_back(
+      "//out/Debug/{{source_file_part}}.one");
+
+  items_.push_back(new scoped_ptr<Item>(action));
+
+  Err err;
+  Value result = GetTargetOutputs("//foo:bar", &err);
+  ASSERT_FALSE(err.has_error());
+  AssertSingleStringEquals(result, "//out/Debug/file.txt.one");
+}
+
 TEST_F(GetTargetOutputsTest, Action) {
   Target* action = new Target(setup_.settings(), GetLabel("//foo/", "bar"));
   action->set_output_type(Target::ACTION);
-  action->action_values().outputs().push_back(SourceFile("//output1.txt"));
-  action->action_values().outputs().push_back(SourceFile("//output2.txt"));
+  action->action_values().outputs().push_back("//output1.txt");
+  action->action_values().outputs().push_back("//output2.txt");
 
   items_.push_back(new scoped_ptr<Item>(action));
 
@@ -101,9 +116,9 @@ TEST_F(GetTargetOutputsTest, ActionForeach) {
   action->set_output_type(Target::ACTION_FOREACH);
   action->sources().push_back(SourceFile("//file.txt"));
   action->action_values().outputs().push_back(
-      SourceFile("//out/Debug/{{source_file_part}}.one"));
+      "//out/Debug/{{source_file_part}}.one");
   action->action_values().outputs().push_back(
-      SourceFile("//out/Debug/{{source_file_part}}.two"));
+      "//out/Debug/{{source_file_part}}.two");
 
   items_.push_back(new scoped_ptr<Item>(action));
 

tools/gn/function_process_file_template.cc

@@ -71,7 +71,8 @@ Value RunProcessFileTemplate(Scope* scope,
     return Value();
   }
 
-  FileTemplate file_template(scope->settings(), args[1], err);
+  FileTemplate file_template(scope->settings(), args[1],
+                             FileTemplate::OUTPUT_ABSOLUTE, SourceDir(), err);
   if (err->has_error())
     return Value();
 

tools/gn/function_write_file.cc

@@ -60,7 +60,7 @@ Value RunWriteFile(Scope* scope,
   SourceFile source_file = cur_dir.ResolveRelativeFile(args[0].string_value());
   if (!EnsureStringIsInOutputDir(
           scope->settings()->build_settings()->build_dir(),
-          source_file.value(), args[0], err))
+          source_file.value(), args[0], false, err))
     return Value();
 
   // Compute output.

tools/gn/ninja_action_target_writer.cc

@@ -23,8 +23,11 @@ NinjaActionTargetWriter::~NinjaActionTargetWriter() {
 }
 
 void NinjaActionTargetWriter::Run() {
-  FileTemplate args_template(target_->settings(),
-                             target_->action_values().args());
+  FileTemplate args_template(
+      target_->settings(),
+      target_->action_values().args(),
+      FileTemplate::OUTPUT_RELATIVE,
+      target_->settings()->build_settings()->build_dir());
   std::string custom_rule_name = WriteRuleDefinition(args_template);
 
   // Collect our deps to pass as "extra hard dependencies" for input deps. This
@@ -58,10 +61,11 @@ void NinjaActionTargetWriter::Run() {
     // Write a rule that invokes the script once with the outputs as outputs,
     // and the data as inputs.
     out_ << "build";
-    const Target::FileList& outputs = target_->action_values().outputs();
+    const std::vector<std::string>& outputs =
+        target_->action_values().outputs();
     for (size_t i = 0; i < outputs.size(); i++) {
       OutputFile output_path(
-          RemovePrefix(outputs[i].value(),
+          RemovePrefix(outputs[i],
                        settings_->build_settings()->build_dir().value()));
       output_files.push_back(output_path);
       out_ << " ";
@@ -141,7 +145,7 @@ void NinjaActionTargetWriter::WriteArgsSubstitutions(
   template_escape_options.mode = ESCAPE_NINJA_COMMAND;
 
   args_template.WriteNinjaVariablesForSubstitution(
-      out_, target_->settings(), source, template_escape_options);
+      out_, source, template_escape_options);
 }
 
 void NinjaActionTargetWriter::WriteSourceRules(
@@ -149,7 +153,7 @@ void NinjaActionTargetWriter::WriteSourceRules(
     const std::string& implicit_deps,
     const FileTemplate& args_template,
     std::vector<OutputFile>* output_files) {
-  FileTemplate output_template(GetOutputTemplate());
+  FileTemplate output_template = FileTemplate::GetForTargetOutputs(target_);
 
   const Target::FileList& sources = target_->sources();
   for (size_t i = 0; i < sources.size(); i++) {
@@ -208,7 +212,11 @@ void NinjaActionTargetWriter::WriteOutputFilesForBuildLine(
   std::vector<std::string> output_template_result;
   output_template.Apply(source, &output_template_result);
   for (size_t out_i = 0; out_i < output_template_result.size(); out_i++) {
-    OutputFile output_path(output_template_result[out_i]);
+    // All output files should be in the build directory, so we can rebase
+    // them just by trimming the prefix.
+    OutputFile output_path(
+        RemovePrefix(output_template_result[out_i],
+                     settings_->build_settings()->build_dir().value()));
     output_files->push_back(output_path);
     out_ << " ";
     path_output_.WriteFile(out_, output_path);
@@ -227,5 +235,6 @@ FileTemplate NinjaActionTargetWriter::GetDepfileTemplate() const {
       RemovePrefix(target_->action_values().depfile().value(),
                    settings_->build_settings()->build_dir().value());
   template_args.push_back(depfile_relative_to_build_dir);
-  return FileTemplate(settings_, template_args);
+  return FileTemplate(settings_, template_args, FileTemplate::OUTPUT_ABSOLUTE,
+                      SourceDir());
 }

tools/gn/ninja_action_target_writer_unittest.cc

@@ -16,14 +16,14 @@ TEST(NinjaActionTargetWriter, WriteOutputFilesForBuildLine) {
   Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
 
   target.action_values().outputs().push_back(
-      SourceFile("//out/Debug/gen/a b{{source_name_part}}.h"));
+      "//out/Debug/gen/a b{{source_name_part}}.h");
   target.action_values().outputs().push_back(
-      SourceFile("//out/Debug/gen/{{source_name_part}}.cc"));
+      "//out/Debug/gen/{{source_name_part}}.cc");
 
   std::ostringstream out;
   NinjaActionTargetWriter writer(&target, setup.toolchain(), out);
 
-  FileTemplate output_template = writer.GetOutputTemplate();
+  FileTemplate output_template = FileTemplate::GetForTargetOutputs(&target);
 
   SourceFile source("//foo/bar.in");
   std::vector<OutputFile> output_files;
@@ -44,7 +44,9 @@ TEST(NinjaActionTargetWriter, WriteArgsSubstitutions) {
   args.push_back("-i");
   args.push_back("{{source}}");
   args.push_back("--out=foo bar{{source_name_part}}.o");
-  FileTemplate args_template(setup.settings(), args);
+  FileTemplate args_template(setup.settings(), args,
+                             FileTemplate::OUTPUT_RELATIVE,
+                             setup.settings()->build_settings()->build_dir());
 
   writer.WriteArgsSubstitutions(SourceFile("//foo/b ar.in"), args_template);
 #if defined(OS_WIN)
@@ -71,8 +73,7 @@ TEST(NinjaActionTargetWriter, ActionWithSources) {
   target.sources().push_back(SourceFile("//foo/source.txt"));
   target.inputs().push_back(SourceFile("//foo/included.txt"));
 
-  target.action_values().outputs().push_back(
-      SourceFile("//out/Debug/foo.out"));
+  target.action_values().outputs().push_back("//out/Debug/foo.out");
 
   // Posix.
   {
@@ -156,7 +157,7 @@ TEST(NinjaActionTargetWriter, ForEach) {
       "--out=foo bar{{source_name_part}}.o");
 
   target.action_values().outputs().push_back(
-      SourceFile("//out/Debug/{{source_name_part}}.out"));
+      "//out/Debug/{{source_name_part}}.out");
 
   target.inputs().push_back(SourceFile("//foo/included.txt"));
 
@@ -265,7 +266,7 @@ TEST(NinjaActionTargetWriter, ForEachWithDepfile) {
       "--out=foo bar{{source_name_part}}.o");
 
   target.action_values().outputs().push_back(
-      SourceFile("//out/Debug/{{source_name_part}}.out"));
+      "//out/Debug/{{source_name_part}}.out");
 
   target.inputs().push_back(SourceFile("//foo/included.txt"));
 

tools/gn/ninja_copy_target_writer.cc

@@ -19,7 +19,7 @@ NinjaCopyTargetWriter::~NinjaCopyTargetWriter() {
 
 void NinjaCopyTargetWriter::Run() {
   CHECK(target_->action_values().outputs().size() == 1);
-  FileTemplate output_template(GetOutputTemplate());
+  FileTemplate output_template = FileTemplate::GetForTargetOutputs(target_);
 
   std::vector<OutputFile> output_files;
 
@@ -35,8 +35,12 @@ void NinjaCopyTargetWriter::Run() {
     std::vector<std::string> template_result;
     output_template.Apply(input_file, &template_result);
     CHECK(template_result.size() == 1);
-    OutputFile output_file(template_result[0]);
 
+    // All output files should be in the build directory, so we can rebase
+    // them just by trimming the prefix.
+    OutputFile output_file(
+        RemovePrefix(template_result[0],
+                     settings_->build_settings()->build_dir().value()));
     output_files.push_back(output_file);
 
     out_ << "build ";

tools/gn/ninja_copy_target_writer_unittest.cc

@@ -22,7 +22,7 @@ TEST(NinjaCopyTargetWriter, Run) {
   target.sources().push_back(SourceFile("//foo/input2.txt"));
 
   target.action_values().outputs().push_back(
-      SourceFile("//out/Debug/{{source_name_part}}.out"));
+      "//out/Debug/{{source_name_part}}.out");
 
   std::ostringstream out;
   NinjaCopyTargetWriter writer(&target, setup.toolchain(), out);
@@ -56,8 +56,7 @@ TEST(NinjaCopyTargetWriter, ToolchainDeps) {
 
   target.sources().push_back(SourceFile("//foo/input1.txt"));
 
-  target.action_values().outputs().push_back(
-      SourceFile("//out/Debug/output.out"));
+  target.action_values().outputs().push_back("//out/Debug/output.out");
 
   std::ostringstream out;
   NinjaCopyTargetWriter writer(&target, setup.toolchain(), out);

tools/gn/ninja_target_writer.cc

@@ -168,15 +168,3 @@ std::string NinjaTargetWriter::WriteInputDepsStampAndGetDep(
   out_ << "\n";
   return " | " + stamp_file_string;
 }
-
-FileTemplate NinjaTargetWriter::GetOutputTemplate() const {
-  const Target::FileList& outputs = target_->action_values().outputs();
-  std::vector<std::string> output_template_args;
-  for (size_t i = 0; i < outputs.size(); i++) {
-    // All outputs should be in the output dir.
-    output_template_args.push_back(
-        RemovePrefix(outputs[i].value(),
-                     settings_->build_settings()->build_dir().value()));
-  }
-  return FileTemplate(target_->settings(), output_template_args);
-}

tools/gn/ninja_target_writer.h

@@ -38,12 +38,6 @@ class NinjaTargetWriter {
   std::string WriteInputDepsStampAndGetDep(
       const std::vector<const Target*>& extra_hard_deps) const;
 
-  // Returns the FileTemplate constructed from the outputs variable. This is
-  // like FileTemplate::GetForTargetOutputs except this additionally trims the
-  // build directory from the front so we can just write the names without
-  // further processing.
-  FileTemplate GetOutputTemplate() const;
-
   const Settings* settings_;  // Non-owning.
   const Target* target_;  // Non-owning.
   const Toolchain* toolchain_;  // Non-owning.

tools/gn/target_generator.cc

@@ -222,20 +222,20 @@ void TargetGenerator::FillOutputs() {
   if (!value)
     return;
 
-  Target::FileList outputs;
-  if (!ExtractListOfRelativeFiles(scope_->settings()->build_settings(), *value,
-                                  scope_->GetSourceDir(), &outputs, err_))
+  std::vector<std::string>& outputs = target_->action_values().outputs();
+  if (!ExtractListOfStringValues(*value, &outputs, err_))
     return;
 
   // Validate that outputs are in the output dir.
+  bool allow_templates = target_->output_type() == Target::ACTION_FOREACH ||
+                         target_->output_type() == Target::COPY_FILES;
   CHECK(outputs.size() == value->list_value().size());
   for (size_t i = 0; i < outputs.size(); i++) {
     if (!EnsureStringIsInOutputDir(
             GetBuildSettings()->build_dir(),
-            outputs[i].value(), value->list_value()[i], err_))
+            outputs[i], value->list_value()[i], allow_templates, err_))
       return;
   }
-  target_->action_values().outputs().swap(outputs);
 }
 
 void TargetGenerator::FillGenericConfigs(const char* var_name,