Validate GN substitutions in args and rsp files.
Previously GN did not validate the substitution types used in the args or
response_file_contents for action_foreach targets. If a caller used an
incorrect one (as was happening in the nocompile test template), GN would
assert in debug mode and skip the substitution in release mode.
This didn't seem to cause a problem for the nocompile test because the output
file is not used.
This adds the necessary validation for these types so GN will give a proper
error message on misuse, and adds some additional unit tests for other aspects
of action_foreach parameter validation.
Review-Url: https://codereview.chromium.org/2627703002
Cr-Original-Commit-Position: refs/heads/master@{#444507}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: bb57bea1642b294af0578ab0e50d1c63bb5491b2
diff --git a/tools/gn/action_target_generator.cc b/tools/gn/action_target_generator.cc
index 6f962b6..5c7cbf6 100644
--- a/tools/gn/action_target_generator.cc
+++ b/tools/gn/action_target_generator.cc
@@ -117,15 +117,32 @@
bool ActionTargetGenerator::FillScriptArgs() {
const Value* value = scope_->GetValue(variables::kArgs, true);
if (!value)
- return true;
- return target_->action_values().args().Parse(*value, err_);
+ return true; // Nothing to do.
+
+ if (!target_->action_values().args().Parse(*value, err_))
+ return false;
+ if (!EnsureValidSubstitutions(
+ target_->action_values().args().required_types(),
+ &IsValidScriptArgsSubstitution,
+ value->origin(), err_))
+ return false;
+
+ return true;
}
bool ActionTargetGenerator::FillResponseFileContents() {
const Value* value = scope_->GetValue(variables::kResponseFileContents, true);
if (!value)
- return true;
- return target_->action_values().rsp_file_contents().Parse(*value, err_);
+ return true; // Nothing to do.
+
+ if (!target_->action_values().rsp_file_contents().Parse(*value, err_))
+ return false;
+ if (!EnsureValidSubstitutions(
+ target_->action_values().rsp_file_contents().required_types(),
+ &IsValidSourceSubstitution, value->origin(), err_))
+ return false;
+
+ return true;
}
bool ActionTargetGenerator::FillDepfile() {
diff --git a/tools/gn/action_target_generator_unittest.cc b/tools/gn/action_target_generator_unittest.cc
index 8f3e4d4..5a45932 100644
--- a/tools/gn/action_target_generator_unittest.cc
+++ b/tools/gn/action_target_generator_unittest.cc
@@ -15,11 +15,11 @@
// First test one with no substitutions, this should be valid.
TestParseInput input_good(
- "action(\"foo\") {\n"
- " script = \"//foo.py\"\n"
- " sources = [ \"//bar.txt\" ]\n"
- " outputs = [ \"//out/Debug/one.txt\" ]\n"
- "}");
+ R"(action("foo") {
+ script = "//foo.py"
+ sources = [ "//bar.txt" ]
+ outputs = [ "//out/Debug/one.txt" ]
+ })");
ASSERT_FALSE(input_good.has_error());
// This should run fine.
@@ -29,14 +29,93 @@
// Same thing with a pattern in the output should fail.
TestParseInput input_bad(
- "action(\"foo\") {\n"
- " script = \"//foo.py\"\n"
- " sources = [ \"//bar.txt\" ]\n"
- " outputs = [ \"//out/Debug/{{source_name_part}}.txt\" ]\n"
- "}");
+ R"(action("foo") {
+ script = "//foo.py"
+ sources = [ "//bar.txt" ]
+ outputs = [ "//out/Debug/{{source_name_part}}.txt" ]
+ })");
ASSERT_FALSE(input_bad.has_error());
// This should run fine.
input_bad.parsed()->Execute(setup.scope(), &err);
ASSERT_TRUE(err.has_error());
}
+
+// Tests that arg and response file substitutions are validated for
+// action_foreach targets.
+TEST(ActionTargetGenerator, ActionForeachSubstitutions) {
+ Scheduler scheduler;
+ TestWithScope setup;
+ Scope::ItemVector items_;
+ setup.scope()->set_item_collector(&items_);
+
+ // Args listing a response file but missing a response file definition should
+ // fail.
+ TestParseInput input_missing_resp_file(
+ R"(action_foreach("foo") {
+ script = "//foo.py"
+ sources = [ "//bar.txt" ]
+ outputs = [ "//out/Debug/{{source_name_part}}" ]
+ args = [ "{{response_file_name}}" ]
+ })");
+ ASSERT_FALSE(input_missing_resp_file.has_error());
+ Err err;
+ input_missing_resp_file.parsed()->Execute(setup.scope(), &err);
+ ASSERT_TRUE(err.has_error());
+
+ // Adding a response file definition should pass.
+ err = Err();
+ TestParseInput input_resp_file(
+ R"(action_foreach("foo") {
+ script = "//foo.py"
+ sources = [ "//bar.txt" ]
+ outputs = [ "//out/Debug/{{source_name_part}}" ]
+ args = [ "{{response_file_name}}" ]
+ response_file_contents = [ "{{source_name_part}}" ]
+ })");
+ ASSERT_FALSE(input_resp_file.has_error());
+ input_resp_file.parsed()->Execute(setup.scope(), &err);
+ ASSERT_FALSE(err.has_error()) << err.message();
+
+ // Defining a response file but not referencing it should fail.
+ err = Err();
+ TestParseInput input_missing_rsp_args(
+ R"(action_foreach("foo") {
+ script = "//foo.py"
+ sources = [ "//bar.txt" ]
+ outputs = [ "//out/Debug/{{source_name_part}}" ]
+ args = [ "{{source_name_part}}" ]
+ response_file_contents = [ "{{source_name_part}}" ]
+ })");
+ ASSERT_FALSE(input_missing_rsp_args.has_error());
+ input_missing_rsp_args.parsed()->Execute(setup.scope(), &err);
+ ASSERT_TRUE(err.has_error()) << err.message();
+
+ // Bad substitutions in args.
+ err = Err();
+ TestParseInput input_bad_args(
+ R"(action_foreach("foo") {
+ script = "//foo.py"
+ sources = [ "//bar.txt" ]
+ outputs = [ "//out/Debug/{{source_name_part}}" ]
+ args = [ "{{response_file_name}} {{ldflags}}" ]
+ response_file_contents = [ "{{source_name_part}}" ]
+ })");
+ ASSERT_FALSE(input_bad_args.has_error());
+ input_bad_args.parsed()->Execute(setup.scope(), &err);
+ ASSERT_TRUE(err.has_error()) << err.message();
+
+ // Bad substitutions in response file contents.
+ err = Err();
+ TestParseInput input_bad_rsp(
+ R"(action_foreach("foo") {
+ script = "//foo.py"
+ sources = [ "//bar.txt" ]
+ outputs = [ "//out/Debug/{{source_name_part}}" ]
+ args = [ "{{response_file_name}}" ]
+ response_file_contents = [ "{{source_name_part}} {{ldflags}}" ]
+ })");
+ ASSERT_FALSE(input_bad_rsp.has_error());
+ input_bad_rsp.parsed()->Execute(setup.scope(), &err);
+ ASSERT_TRUE(err.has_error()) << err.message();
+}
diff --git a/tools/gn/ninja_action_target_writer.cc b/tools/gn/ninja_action_target_writer.cc
index 77ee2b4..9e56362 100644
--- a/tools/gn/ninja_action_target_writer.cc
+++ b/tools/gn/ninja_action_target_writer.cc
@@ -180,7 +180,7 @@
// The required types is the union of the args and response file. This
// might theoretically duplicate a definition if the same substitution is
- // used in both the args and the reponse file. However, this should be
+ // used in both the args and the response file. However, this should be
// very unusual (normally the substitutions will go in one place or the
// other) and the redundant assignment won't bother Ninja.
SubstitutionWriter::WriteNinjaVariablesForSource(
diff --git a/tools/gn/substitution_type.cc b/tools/gn/substitution_type.cc
index c6ea385..99b4e68 100644
--- a/tools/gn/substitution_type.cc
+++ b/tools/gn/substitution_type.cc
@@ -166,6 +166,11 @@
type == SUBSTITUTION_SOURCE_TARGET_RELATIVE;
}
+bool IsValidScriptArgsSubstitution(SubstitutionType type) {
+ return IsValidSourceSubstitution(type) ||
+ type == SUBSTITUTION_RSP_FILE_NAME;
+}
+
bool IsValidToolSubstitution(SubstitutionType type) {
return type == SUBSTITUTION_LITERAL ||
type == SUBSTITUTION_OUTPUT ||
@@ -236,14 +241,14 @@
type == SUBSTITUTION_BUNDLE_PRODUCT_TYPE;
}
-bool EnsureValidSourcesSubstitutions(
- const std::vector<SubstitutionType>& types,
- const ParseNode* origin,
- Err* err) {
- for (size_t i = 0; i < types.size(); i++) {
- if (!IsValidSourceSubstitution(types[i])) {
+bool EnsureValidSubstitutions(const std::vector<SubstitutionType>& types,
+ bool (*is_valid_subst)(SubstitutionType),
+ const ParseNode* origin,
+ Err* err) {
+ for (SubstitutionType type : types) {
+ if (!is_valid_subst(type)) {
*err = Err(origin, "Invalid substitution type.",
- "The substitution " + std::string(kSubstitutionNames[types[i]]) +
+ "The substitution " + std::string(kSubstitutionNames[type]) +
" isn't valid for something\n"
"operating on a source file such as this.");
return false;
diff --git a/tools/gn/substitution_type.h b/tools/gn/substitution_type.h
index 74ab507..df7f8b7 100644
--- a/tools/gn/substitution_type.h
+++ b/tools/gn/substitution_type.h
@@ -118,6 +118,8 @@
// Returns true if the given substitution is valid for the named purpose.
bool IsValidBundleDataSubstitution(SubstitutionType type);
bool IsValidSourceSubstitution(SubstitutionType type);
+bool IsValidScriptArgsSubstitution(SubstitutionType type);
+
// Both compiler and linker tools.
bool IsValidToolSubstitution(SubstitutionType type);
bool IsValidCompilerSubstitution(SubstitutionType type);
@@ -128,10 +130,12 @@
bool IsValidCopySubstitution(SubstitutionType type);
bool IsValidCompileXCassetsSubstitution(SubstitutionType type);
-// Like the "IsValid..." version above but checks a list of types and sets a
-// an error blaming the given source if the test fails.
-bool EnsureValidSourcesSubstitutions(
+// Validates that each substitution type in the vector passes the given
+// is_valid_subst predicate. Returns true on success. On failure, fills in the
+// error object with an appropriate message and returns false.
+bool EnsureValidSubstitutions(
const std::vector<SubstitutionType>& types,
+ bool (*is_valid_subst)(SubstitutionType),
const ParseNode* origin,
Err* err);
diff --git a/tools/gn/target_generator.cc b/tools/gn/target_generator.cc
index 7fe64fd..ca193b6 100644
--- a/tools/gn/target_generator.cc
+++ b/tools/gn/target_generator.cc
@@ -312,8 +312,9 @@
}
// Check the substitutions used are valid for this purpose.
- if (!EnsureValidSourcesSubstitutions(outputs.required_types(),
- value->origin(), err_))
+ if (!EnsureValidSubstitutions(outputs.required_types(),
+ &IsValidSourceSubstitution,
+ value->origin(), err_))
return false;
// Validate that outputs are in the output dir.
