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
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: f0bbcdc29da3d55178958982724a9a3bc45c2eec
diff --git a/tools/gn/action_target_generator.cc b/tools/gn/action_target_generator.cc
index 8840f16..0ad3928 100644
--- a/tools/gn/action_target_generator.cc
+++ b/tools/gn/action_target_generator.cc
@@ -17,10 +17,10 @@
// 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 @@
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::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 @@
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 @@
}
} 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"
diff --git a/tools/gn/action_values.h b/tools/gn/action_values.h
index 8b883c7..7a70d33 100644
--- a/tools/gn/action_values.h
+++ b/tools/gn/action_values.h
@@ -25,12 +25,11 @@
// 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 @@
private:
SourceFile script_;
std::vector<std::string> args_;
- std::vector<SourceFile> outputs_;
+ std::vector<std::string> outputs_;
SourceFile depfile_;
DISALLOW_COPY_AND_ASSIGN(ActionValues);
diff --git a/tools/gn/command_desc.cc b/tools/gn/command_desc.cc
index 4491920..16aee40 100644
--- a/tools/gn/command_desc.cc
+++ b/tools/gn/command_desc.cc
@@ -248,6 +248,25 @@
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 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,
- display_header);
- } else if (target->output_type() == Target::ACTION_FOREACH) {
+ PrintStringList(target->action_values().outputs(), "outputs", false,
+ display_header);
+ } 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 @@
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);
}
}
diff --git a/tools/gn/file_template.cc b/tools/gn/file_template.cc
index 2809920..6ad37e5 100644
--- a/tools/gn/file_template.cc
+++ b/tools/gn/file_template.cc
@@ -44,10 +44,9 @@
"Placeholders\n"
"\n"
" {{source}}\n"
- " The name of the source file relative to the root build output\n"
- " directory (which is the current directory when running compilers\n"
- " and scripts). This will generally be used for specifying inputs\n"
- " to a script in the \"args\" variable.\n"
+ " The name of the source file including directory (*). This will\n"
+ " generally be used for specifying inputs to a script in the\n"
+ " \"args\" variable.\n"
" \"//foo/bar/baz.txt\" => \"../../foo/bar/baz.txt\"\n"
"\n"
" {{source_file_part}}\n"
@@ -62,34 +61,48 @@
" \"//foo/bar/baz.txt\" => \"baz\"\n"
"\n"
" {{source_dir}}\n"
- " The directory containing the source file, relative to the build\n"
- " directory, with no trailing slash.\n"
+ " The directory (*) containing the source file with no\n"
+ " trailing slash.\n"
" \"//foo/bar/baz.txt\" => \"../../foo/bar\"\n"
"\n"
" {{source_root_relative_dir}}\n"
" The path to the source file's directory relative to the source\n"
" root, with no leading \"//\" or trailing slashes. If the path is\n"
" system-absolute, (beginning in a single slash) this will just\n"
- " return the path with no trailing slash.\n"
+ " return the path with no trailing slash. This value will always\n"
+ " be the same, regardless of whether it appears in the \"outputs\"\n"
+ " or \"args\" section.\n"
" \"//foo/bar/baz.txt\" => \"foo/bar\"\n"
"\n"
" {{source_gen_dir}}\n"
- " The generated file directory corresponding to the source file's\n"
- " path, relative to the build directory. This will be different than\n"
- " the target's generated file directory if the source file is in a\n"
- " different directory than the build.gn file. If the input path is\n"
- " system absolute, this will return the root generated file\n"
- " directory."
+ " The generated file directory (*) corresponding to the source\n"
+ " file's path. This will be different than the target's generated\n"
+ " file directory if the source file is in a different directory\n"
+ " than the BUILD.gn file.\n"
" \"//foo/bar/baz.txt\" => \"gen/foo/bar\"\n"
"\n"
" {{source_out_dir}}\n"
- " The object file directory corresponding to the source file's\n"
+ " The object file directory (*) corresponding to the source file's\n"
" path, relative to the build directory. this us be different than\n"
" the target's out directory if the source file is in a different\n"
- " directory than the build.gn file. if the input path is system\n"
- " absolute, this will return the root generated file directory.\n"
+ " directory than the build.gn file.\n"
" \"//foo/bar/baz.txt\" => \"obj/foo/bar\"\n"
"\n"
+ "(*) Note on directories\n"
+ "\n"
+ " Paths containing directories (except the source_root_relative_dir)\n"
+ " will be different depending on what context the expansion is evaluated\n"
+ " in. Generally it should \"just work\" but it means you can't\n"
+ " concatenate strings containing these values with reasonable results.\n"
+ "\n"
+ " Details: source expansions can be used in the \"outputs\" variable,\n"
+ " the \"args\" variable, and in calls to \"process_file_template\". The\n"
+ " \"args\" are passed to a script which is run from the build directory,\n"
+ " so these directories will relative to the build directory for the\n"
+ " script to find. In the other cases, the directories will be source-\n"
+ " absolute (begin with a \"//\") because the results of those expansions\n"
+ " will be handled by GN internally.\n"
+ "\n"
"Examples\n"
"\n"
" Non-varying outputs:\n"
@@ -103,8 +116,8 @@
" Varying outputs:\n"
" action_foreach(\"varying_outputs\") {\n"
" sources = [ \"input1.idl\", \"input2.idl\" ]\n"
- " outputs = [ \"$target_out_dir/{{source_name_part}}.h\",\n"
- " \"$target_out_dir/{{source_name_part}}.cc\" ]\n"
+ " outputs = [ \"{{source_gen_dir}}/{{source_name_part}}.h\",\n"
+ " \"{{source_gen_dir}}/{{source_name_part}}.cc\" ]\n"
" }\n"
" Performing source expansion will result in the following output names:\n"
" //out/Debug/obj/mydirectory/input1.h\n"
@@ -112,16 +125,26 @@
" //out/Debug/obj/mydirectory/input2.h\n"
" //out/Debug/obj/mydirectory/input2.cc\n";
-FileTemplate::FileTemplate(const Settings* settings, const Value& t, Err* err)
+FileTemplate::FileTemplate(const Settings* settings,
+ const Value& t,
+ OutputStyle output_style,
+ const SourceDir& relative_to,
+ Err* err)
: settings_(settings),
+ output_style_(output_style),
+ relative_to_(relative_to),
has_substitutions_(false) {
std::fill(types_required_, &types_required_[Subrange::NUM_TYPES], false);
ParseInput(t, err);
}
FileTemplate::FileTemplate(const Settings* settings,
- const std::vector<std::string>& t)
+ const std::vector<std::string>& t,
+ OutputStyle output_style,
+ const SourceDir& relative_to)
: settings_(settings),
+ output_style_(output_style),
+ relative_to_(relative_to),
has_substitutions_(false) {
std::fill(types_required_, &types_required_[Subrange::NUM_TYPES], false);
for (size_t i = 0; i < t.size(); i++)
@@ -129,8 +152,12 @@
}
FileTemplate::FileTemplate(const Settings* settings,
- const std::vector<SourceFile>& t)
+ const std::vector<SourceFile>& t,
+ OutputStyle output_style,
+ const SourceDir& relative_to)
: settings_(settings),
+ output_style_(output_style),
+ relative_to_(relative_to),
has_substitutions_(false) {
std::fill(types_required_, &types_required_[Subrange::NUM_TYPES], false);
for (size_t i = 0; i < t.size(); i++)
@@ -142,11 +169,12 @@
// static
FileTemplate FileTemplate::GetForTargetOutputs(const Target* target) {
- const Target::FileList& outputs = target->action_values().outputs();
+ const std::vector<std::string>& outputs = target->action_values().outputs();
std::vector<std::string> output_template_args;
for (size_t i = 0; i < outputs.size(); i++)
- output_template_args.push_back(outputs[i].value());
- return FileTemplate(target->settings(), output_template_args);
+ output_template_args.push_back(outputs[i]);
+ return FileTemplate(target->settings(), output_template_args,
+ OUTPUT_ABSOLUTE, SourceDir());
}
bool FileTemplate::IsTypeUsed(Subrange::Type type) const {
@@ -161,8 +189,9 @@
std::string subst[Subrange::NUM_TYPES];
for (int i = 1; i < Subrange::NUM_TYPES; i++) {
if (types_required_[i]) {
- subst[i] =
- GetSubstitution(settings_, source, static_cast<Subrange::Type>(i));
+ subst[i] = GetSubstitution(settings_, source,
+ static_cast<Subrange::Type>(i),
+ output_style_, relative_to_);
}
}
@@ -227,15 +256,16 @@
void FileTemplate::WriteNinjaVariablesForSubstitution(
std::ostream& out,
- const Settings* settings,
const SourceFile& source,
const EscapeOptions& escape_options) const {
for (int i = 1; i < Subrange::NUM_TYPES; i++) {
if (types_required_[i]) {
Subrange::Type type = static_cast<Subrange::Type>(i);
out << " " << GetNinjaVariableNameForType(type) << " = ";
- EscapeStringToStream(out, GetSubstitution(settings, source, type),
- escape_options);
+ EscapeStringToStream(
+ out,
+ GetSubstitution(settings_, source, type, output_style_, relative_to_),
+ escape_options);
out << std::endl;
}
}
@@ -268,13 +298,16 @@
// static
std::string FileTemplate::GetSubstitution(const Settings* settings,
const SourceFile& source,
- Subrange::Type type) {
+ Subrange::Type type,
+ OutputStyle output_style,
+ const SourceDir& relative_to) {
+ std::string to_rebase;
switch (type) {
case Subrange::SOURCE:
if (source.is_system_absolute())
return source.value();
- return RebaseSourceAbsolutePath(source.value(),
- settings->build_settings()->build_dir());
+ to_rebase = source.value();
+ break;
case Subrange::NAME_PART:
return FindFilenameNoExtension(&source.value()).as_string();
@@ -285,9 +318,8 @@
case Subrange::SOURCE_DIR:
if (source.is_system_absolute())
return DirectoryWithNoLastSlash(source.GetDir());
- return RebaseSourceAbsolutePath(
- DirectoryWithNoLastSlash(source.GetDir()),
- settings->build_settings()->build_dir());
+ to_rebase = DirectoryWithNoLastSlash(source.GetDir());
+ break;
case Subrange::ROOT_RELATIVE_DIR:
if (source.is_system_absolute())
@@ -296,21 +328,26 @@
DirectoryWithNoLastSlash(source.GetDir()), SourceDir("//"));
case Subrange::SOURCE_GEN_DIR:
- return RebaseSourceAbsolutePath(
- DirectoryWithNoLastSlash(
- GetGenDirForSourceDir(settings, source.GetDir())),
- settings->build_settings()->build_dir());
+ to_rebase = DirectoryWithNoLastSlash(
+ GetGenDirForSourceDir(settings, source.GetDir()));
+ break;
case Subrange::SOURCE_OUT_DIR:
- return RebaseSourceAbsolutePath(
- DirectoryWithNoLastSlash(
- GetOutputDirForSourceDir(settings, source.GetDir())),
- settings->build_settings()->build_dir());
+ to_rebase = DirectoryWithNoLastSlash(
+ GetOutputDirForSourceDir(settings, source.GetDir()));
+ break;
default:
NOTREACHED();
+ return std::string();
}
- return std::string();
+
+ // If we get here, the result is a path that should be made relative or
+ // absolute according to the output_style. Other cases (just file name or
+ // extension extraction) will have been handled via early return above.
+ if (output_style == OUTPUT_ABSOLUTE)
+ return to_rebase;
+ return RebaseSourceAbsolutePath(to_rebase, relative_to);
}
void FileTemplate::ParseInput(const Value& value, Err* err) {
diff --git a/tools/gn/file_template.h b/tools/gn/file_template.h
index b04a6c7..ddbd67f 100644
--- a/tools/gn/file_template.h
+++ b/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 @@
// 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 @@
// 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 @@
// (see GetWithNinjaExpansions).
void WriteNinjaVariablesForSubstitution(
std::ostream& out,
- const Settings* settings,
const SourceFile& source,
const EscapeOptions& escape_options) const;
@@ -113,13 +128,18 @@
// 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 @@
void ParseOneTemplateString(const std::string& str);
const Settings* settings_;
+ OutputStyle output_style_;
+ SourceDir relative_to_;
TemplateVector templates_;
diff --git a/tools/gn/file_template_unittest.cc b/tools/gn/file_template_unittest.cc
index d10791e..eab5285 100644
--- a/tools/gn/file_template_unittest.cc
+++ b/tools/gn/file_template_unittest.cc
@@ -14,7 +14,8 @@
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 @@
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 @@
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 @@
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 @@
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, Substitutions) {
TestWithScope setup;
- #define GetSubst(str, what) \
- FileTemplate::GetSubstitution(setup.settings(), \
- SourceFile(str), \
- FileTemplate::Subrange::what)
+ // Call to get substitutions relative to the build dir.
+ #define GetRelSubst(str, what) \
+ FileTemplate::GetSubstitution( \
+ setup.settings(), \
+ SourceFile(str), \
+ 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
}
diff --git a/tools/gn/filesystem_utils.cc b/tools/gn/filesystem_utils.cc
index ca45625..17fec9f 100644
--- a/tools/gn/filesystem_utils.cc
+++ b/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 @@
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) {
- *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;
+ 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;
}
- 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;
}
bool IsPathAbsolute(const base::StringPiece& path) {
diff --git a/tools/gn/filesystem_utils.h b/tools/gn/filesystem_utils.h
index 71e7057..3864ecc 100644
--- a/tools/gn/filesystem_utils.h
+++ b/tools/gn/filesystem_utils.h
@@ -104,11 +104,16 @@
//
// 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);
// ----------------------------------------------------------------------------
diff --git a/tools/gn/filesystem_utils_unittest.cc b/tools/gn/filesystem_utils_unittest.cc
index 906d672..4eaf038 100644
--- a/tools/gn/filesystem_utils_unittest.cc
+++ b/tools/gn/filesystem_utils_unittest.cc
@@ -90,6 +90,50 @@
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("/"));
diff --git a/tools/gn/function_get_target_outputs.cc b/tools/gn/function_get_target_outputs.cc
index be014f2..aeb057f 100644
--- a/tools/gn/function_get_target_outputs.cc
+++ b/tools/gn/function_get_target_outputs.cc
@@ -19,19 +19,20 @@
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);
diff --git a/tools/gn/function_get_target_outputs_unittest.cc b/tools/gn/function_get_target_outputs_unittest.cc
index 2833fdd..7c1b80c 100644
--- a/tools/gn/function_get_target_outputs_unittest.cc
+++ b/tools/gn/function_get_target_outputs_unittest.cc
@@ -82,11 +82,26 @@
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 @@
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));
diff --git a/tools/gn/function_process_file_template.cc b/tools/gn/function_process_file_template.cc
index 061c901..0c56134 100644
--- a/tools/gn/function_process_file_template.cc
+++ b/tools/gn/function_process_file_template.cc
@@ -71,7 +71,8 @@
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();
diff --git a/tools/gn/function_write_file.cc b/tools/gn/function_write_file.cc
index 36bcbc5..4a70175 100644
--- a/tools/gn/function_write_file.cc
+++ b/tools/gn/function_write_file.cc
@@ -60,7 +60,7 @@
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.
diff --git a/tools/gn/ninja_action_target_writer.cc b/tools/gn/ninja_action_target_writer.cc
index 6a632e7..6fec079 100644
--- a/tools/gn/ninja_action_target_writer.cc
+++ b/tools/gn/ninja_action_target_writer.cc
@@ -23,8 +23,11 @@
}
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 @@
// 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 @@
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 @@
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 @@
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 @@
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());
}
diff --git a/tools/gn/ninja_action_target_writer_unittest.cc b/tools/gn/ninja_action_target_writer_unittest.cc
index c6aa5f8..18ae7e4 100644
--- a/tools/gn/ninja_action_target_writer_unittest.cc
+++ b/tools/gn/ninja_action_target_writer_unittest.cc
@@ -16,14 +16,14 @@
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 @@
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 @@
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 @@
"--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 @@
"--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"));
diff --git a/tools/gn/ninja_copy_target_writer.cc b/tools/gn/ninja_copy_target_writer.cc
index 06c72df..3facd62 100644
--- a/tools/gn/ninja_copy_target_writer.cc
+++ b/tools/gn/ninja_copy_target_writer.cc
@@ -19,7 +19,7 @@
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 @@
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 ";
diff --git a/tools/gn/ninja_copy_target_writer_unittest.cc b/tools/gn/ninja_copy_target_writer_unittest.cc
index 0b1618e..e9f2437 100644
--- a/tools/gn/ninja_copy_target_writer_unittest.cc
+++ b/tools/gn/ninja_copy_target_writer_unittest.cc
@@ -22,7 +22,7 @@
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 @@
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);
diff --git a/tools/gn/ninja_target_writer.cc b/tools/gn/ninja_target_writer.cc
index df0412b..4003a3f 100644
--- a/tools/gn/ninja_target_writer.cc
+++ b/tools/gn/ninja_target_writer.cc
@@ -168,15 +168,3 @@
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);
-}
diff --git a/tools/gn/ninja_target_writer.h b/tools/gn/ninja_target_writer.h
index 51e3ba1..6cd0a0a 100644
--- a/tools/gn/ninja_target_writer.h
+++ b/tools/gn/ninja_target_writer.h
@@ -38,12 +38,6 @@
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.
diff --git a/tools/gn/target_generator.cc b/tools/gn/target_generator.cc
index 252d982..d38dbd9 100644
--- a/tools/gn/target_generator.cc
+++ b/tools/gn/target_generator.cc
@@ -222,20 +222,20 @@
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,
