Support for excluding variable from forwarding via forward_variables_form.
Some templates wants to forward all their parameters to the underlying
target except a select few they use. Previously they had to list all
the variables the underlying target supported which was error prone and
brittle if the underlying target is changed.
With this change they can just use the following:
forward_variables_from(invoker, "*", ["my_extra_variable"])
# my_extra_variable is not forwarded, all other variables are.
BUG=580293
Review URL: https://codereview.chromium.org/1632573002
Cr-Original-Commit-Position: refs/heads/master@{#371492}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: bf5ff79020d15e683a12b45be08200ab7d857550
diff --git a/tools/gn/docs/reference.md b/tools/gn/docs/reference.md
index 97f12f2..85fba66 100644
--- a/tools/gn/docs/reference.md
+++ b/tools/gn/docs/reference.md
@@ -1327,7 +1327,8 @@
## **forward_variables_from**: Copies variables from a different scope.
```
- forward_variables_from(from_scope, variable_list_or_star)
+ forward_variables_from(from_scope, variable_list_or_star,
+ variable_to_not_forward_list = [])
Copies the given variables from the given scope to the local scope
if they exist. This is normally used in the context of templates to
@@ -1354,6 +1355,10 @@
is never applied by this function. It's assumed than any desired
filtering was already done when sources was set on the from_scope.
+ If variables_to_not_forward_list is non-empty, then it must contains
+ a list of variable names that will not be forwarded. This is mostly
+ useful when variable_list_or_star has a value of "*".
+
```
### **Examples**
@@ -1383,7 +1388,19 @@
target(my_wrapper_target_type, target_name) {
forward_variables_from(invoker, "*")
}
- }
+ }
+
+ # A template that wraps another. It adds behavior based on one
+ # variable, and forwards all others to the nested target.
+ template("my_ios_test_app") {
+ ios_test_app(target_name) {
+ forward_variables_from(invoker, "*", ["test_bundle_name"])
+ if (!defined(extra_substitutions)) {
+ extra_substitutions = []
+ }
+ extra_substitutions += [ "BUNDLE_ID_TEST_NAME=$test_bundle_name" ]
+ }
+ }
```
diff --git a/tools/gn/function_forward_variables_from.cc b/tools/gn/function_forward_variables_from.cc
index 9c97a51..ee14128 100644
--- a/tools/gn/function_forward_variables_from.cc
+++ b/tools/gn/function_forward_variables_from.cc
@@ -14,6 +14,7 @@
void ForwardAllValues(const FunctionCallNode* function,
Scope* source,
Scope* dest,
+ const std::set<std::string>& exclusion_set,
Err* err) {
Scope::MergeOptions options;
// This function needs to clobber existing for it to be useful. It will be
@@ -23,6 +24,7 @@
options.clobber_existing = true;
options.skip_private_vars = true;
options.mark_dest_used = false;
+ options.excluded_values = exclusion_set;
source->NonRecursiveMergeTo(dest, options, function,
"source scope", err);
source->MarkAllUsed();
@@ -31,10 +33,13 @@
void ForwardValuesFromList(Scope* source,
Scope* dest,
const std::vector<Value>& list,
+ const std::set<std::string>& exclusion_set,
Err* err) {
for (const Value& cur : list) {
if (!cur.VerifyTypeIs(Value::STRING, err))
return;
+ if (exclusion_set.find(cur.string_value()) != exclusion_set.end())
+ continue;
const Value* value = source->GetValue(cur.string_value(), true);
if (value) {
// Use the storage key for the original value rather than the string in
@@ -66,7 +71,8 @@
const char kForwardVariablesFrom_Help[] =
"forward_variables_from: Copies variables from a different scope.\n"
"\n"
- " forward_variables_from(from_scope, variable_list_or_star)\n"
+ " forward_variables_from(from_scope, variable_list_or_star,\n"
+ " variable_to_not_forward_list = [])\n"
"\n"
" Copies the given variables from the given scope to the local scope\n"
" if they exist. This is normally used in the context of templates to\n"
@@ -94,6 +100,10 @@
" is never applied by this function. It's assumed than any desired\n"
" filtering was already done when sources was set on the from_scope.\n"
"\n"
+ " If variables_to_not_forward_list is non-empty, then it must contains\n"
+ " a list of variable names that will not be forwarded. This is mostly\n"
+ " useful when variable_list_or_star has a value of \"*\".\n"
+ "\n"
"Examples\n"
"\n"
" # This is a common action template. It would invoke a script with\n"
@@ -121,7 +131,20 @@
" target(my_wrapper_target_type, target_name) {\n"
" forward_variables_from(invoker, \"*\")\n"
" }\n"
- " }\n";
+ " }\n"
+ "\n"
+ " # A template that wraps another. It adds behavior based on one \n"
+ " # variable, and forwards all others to the nested target.\n"
+ " template(\"my_ios_test_app\") {\n"
+ " ios_test_app(target_name) {\n"
+ " forward_variables_from(invoker, \"*\", [\"test_bundle_name\"])\n"
+ " if (!defined(extra_substitutions)) {\n"
+ " extra_substitutions = []\n"
+ " }\n"
+ " extra_substitutions += [ \"BUNDLE_ID_TEST_NAME=$test_bundle_name\" "
+ "]\n"
+ " }\n"
+ " }\n";
// This function takes a ListNode rather than a resolved vector of values
// both avoid copying the potentially-large source scope, and so the variables
@@ -131,9 +154,9 @@
const ListNode* args_list,
Err* err) {
const std::vector<const ParseNode*>& args_vector = args_list->contents();
- if (args_vector.size() != 2) {
+ if (args_vector.size() != 2 && args_vector.size() != 3) {
*err = Err(function, "Wrong number of arguments.",
- "Expecting exactly two.");
+ "Expecting two or three arguments.");
return Value();
}
@@ -157,18 +180,40 @@
return Value();
Scope* source = value->scope_value();
+ // Extract the exclusion list if defined.
+ std::set<std::string> exclusion_set;
+ if (args_vector.size() == 3) {
+ Value exclusion_value = args_vector[2]->Execute(scope, err);
+ if (err->has_error())
+ return Value();
+
+ if (exclusion_value.type() != Value::LIST) {
+ *err = Err(exclusion_value, "Not a valid list of variables to exclude.",
+ "Expecting a list of strings.");
+ return Value();
+ }
+
+ for (const Value& cur : exclusion_value.list_value()) {
+ if (!cur.VerifyTypeIs(Value::STRING, err))
+ return Value();
+
+ exclusion_set.insert(cur.string_value());
+ }
+ }
+
// Extract the list. If all_values is not set, the what_value will be a list.
Value what_value = args_vector[1]->Execute(scope, err);
if (err->has_error())
return Value();
if (what_value.type() == Value::STRING) {
if (what_value.string_value() == "*") {
- ForwardAllValues(function, source, scope, err);
+ ForwardAllValues(function, source, scope, exclusion_set, err);
return Value();
}
} else {
if (what_value.type() == Value::LIST) {
- ForwardValuesFromList(source, scope, what_value.list_value(), err);
+ ForwardValuesFromList(source, scope, what_value.list_value(),
+ exclusion_set, err);
return Value();
}
}
diff --git a/tools/gn/function_forward_variables_from_unittest.cc b/tools/gn/function_forward_variables_from_unittest.cc
index c3dd715..e5137a4 100644
--- a/tools/gn/function_forward_variables_from_unittest.cc
+++ b/tools/gn/function_forward_variables_from_unittest.cc
@@ -32,6 +32,34 @@
setup.print_output().clear();
}
+TEST(FunctionForwardVariablesFrom, ListWithExclusion) {
+ Scheduler scheduler;
+ TestWithScope setup;
+
+ // Defines a template and copy the two x and y, and z values out.
+ TestParseInput input(
+ "template(\"a\") {\n"
+ " forward_variables_from(invoker, [\"x\", \"y\", \"z\"], [\"z\"])\n"
+ " assert(!defined(z))\n" // "z" should still be undefined.
+ " print(\"$target_name, $x, $y\")\n"
+ "}\n"
+ "a(\"target\") {\n"
+ " x = 1\n"
+ " y = 2\n"
+ " z = 3\n"
+ " print(\"$z\")\n"
+ "}\n");
+
+ ASSERT_FALSE(input.has_error());
+
+ Err err;
+ input.parsed()->Execute(setup.scope(), &err);
+ ASSERT_FALSE(err.has_error()) << err.message();
+
+ EXPECT_EQ("3\ntarget, 1, 2\n", setup.print_output());
+ setup.print_output().clear();
+}
+
TEST(FunctionForwardVariablesFrom, ErrorCases) {
Scheduler scheduler;
TestWithScope setup;
@@ -65,19 +93,61 @@
EXPECT_TRUE(err.has_error());
EXPECT_EQ("Not a valid list of variables to copy.", err.message());
- // Programmatic values should error.
- TestParseInput prog(
+ // Type check the exclusion list.
+ TestParseInput invalid_exclusion_list(
"template(\"c\") {\n"
- " forward_variables_from(invoker, [\"root_out_dir\"])\n"
+ " forward_variables_from(invoker, \"*\", 42)\n"
" print(\"$target_name\")\n"
"}\n"
"c(\"target\") {\n"
"}\n");
+ ASSERT_FALSE(invalid_exclusion_list.has_error());
+ err = Err();
+ invalid_exclusion_list.parsed()->Execute(setup.scope(), &err);
+ EXPECT_TRUE(err.has_error());
+ EXPECT_EQ("Not a valid list of variables to exclude.", err.message());
+
+ // Programmatic values should error.
+ TestParseInput prog(
+ "template(\"d\") {\n"
+ " forward_variables_from(invoker, [\"root_out_dir\"])\n"
+ " print(\"$target_name\")\n"
+ "}\n"
+ "d(\"target\") {\n"
+ "}\n");
ASSERT_FALSE(prog.has_error());
err = Err();
prog.parsed()->Execute(setup.scope(), &err);
EXPECT_TRUE(err.has_error());
EXPECT_EQ("This value can't be forwarded.", err.message());
+
+ // Not enough arguments.
+ TestParseInput not_enough_arguments(
+ "template(\"e\") {\n"
+ " forward_variables_from(invoker)\n"
+ " print(\"$target_name\")\n"
+ "}\n"
+ "e(\"target\") {\n"
+ "}\n");
+ ASSERT_FALSE(not_enough_arguments.has_error());
+ err = Err();
+ not_enough_arguments.parsed()->Execute(setup.scope(), &err);
+ EXPECT_TRUE(err.has_error());
+ EXPECT_EQ("Wrong number of arguments.", err.message());
+
+ // Too many arguments.
+ TestParseInput too_many_arguments(
+ "template(\"f\") {\n"
+ " forward_variables_from(invoker, \"*\", [], [])\n"
+ " print(\"$target_name\")\n"
+ "}\n"
+ "f(\"target\") {\n"
+ "}\n");
+ ASSERT_FALSE(too_many_arguments.has_error());
+ err = Err();
+ too_many_arguments.parsed()->Execute(setup.scope(), &err);
+ EXPECT_TRUE(err.has_error());
+ EXPECT_EQ("Wrong number of arguments.", err.message());
}
TEST(FunctionForwardVariablesFrom, Star) {
@@ -106,3 +176,33 @@
EXPECT_EQ("target, 1, 2\n", setup.print_output());
setup.print_output().clear();
}
+
+
+TEST(FunctionForwardVariablesFrom, StarWithExclusion) {
+ Scheduler scheduler;
+ TestWithScope setup;
+
+ // Defines a template and copy all values except z value. The "*" behavior
+ // should clobber existing variables with the same name.
+ TestParseInput input(
+ "template(\"a\") {\n"
+ " x = 1000000\n" // Should be clobbered.
+ " forward_variables_from(invoker, \"*\", [\"z\"])\n"
+ " print(\"$target_name, $x, $y\")\n"
+ "}\n"
+ "a(\"target\") {\n"
+ " x = 1\n"
+ " y = 2\n"
+ " z = 3\n"
+ " print(\"$z\")\n"
+ "}\n");
+
+ ASSERT_FALSE(input.has_error());
+
+ Err err;
+ input.parsed()->Execute(setup.scope(), &err);
+ ASSERT_FALSE(err.has_error()) << err.message();
+
+ EXPECT_EQ("3\ntarget, 1, 2\n", setup.print_output());
+ setup.print_output().clear();
+}
diff --git a/tools/gn/scope.cc b/tools/gn/scope.cc
index 2bf9dd4..dc22367 100644
--- a/tools/gn/scope.cc
+++ b/tools/gn/scope.cc
@@ -25,6 +25,15 @@
} // namespace
+// Defaults to all false, which are the things least likely to cause errors.
+Scope::MergeOptions::MergeOptions()
+ : clobber_existing(false),
+ skip_private_vars(false),
+ mark_dest_used(false) {
+}
+
+Scope::MergeOptions::~MergeOptions() {
+}
Scope::ProgrammaticProvider::~ProgrammaticProvider() {
scope_->RemoveProvider(this);
@@ -249,17 +258,23 @@
Err* err) const {
// Values.
for (const auto& pair : values_) {
- if (options.skip_private_vars && IsPrivateVar(pair.first))
+ const base::StringPiece& current_name = pair.first;
+ if (options.skip_private_vars && IsPrivateVar(current_name))
continue; // Skip this private var.
+ if (!options.excluded_values.empty() &&
+ options.excluded_values.find(current_name.as_string()) !=
+ options.excluded_values.end()) {
+ continue; // Skip this excluded value.
+ }
const Value& new_value = pair.second.value;
if (!options.clobber_existing) {
- const Value* existing_value = dest->GetValue(pair.first);
+ const Value* existing_value = dest->GetValue(current_name);
if (existing_value && new_value != *existing_value) {
// Value present in both the source and the dest.
std::string desc_string(desc_for_err);
*err = Err(node_for_err, "Value collision.",
- "This " + desc_string + " contains \"" + pair.first.as_string() +
+ "This " + desc_string + " contains \"" + current_name.as_string() +
"\"");
err->AppendSubErr(Err(pair.second.value, "defined here.",
"Which would clobber the one in your current scope"));
@@ -269,23 +284,30 @@
return false;
}
}
- dest->values_[pair.first] = pair.second;
+ dest->values_[current_name] = pair.second;
if (options.mark_dest_used)
- dest->MarkUsed(pair.first);
+ dest->MarkUsed(current_name);
}
// Target defaults are owning pointers.
for (const auto& pair : target_defaults_) {
+ const std::string& current_name = pair.first;
+ if (!options.excluded_values.empty() &&
+ options.excluded_values.find(current_name) !=
+ options.excluded_values.end()) {
+ continue; // Skip the excluded value.
+ }
+
if (!options.clobber_existing) {
- if (dest->GetTargetDefaults(pair.first)) {
+ if (dest->GetTargetDefaults(current_name)) {
// TODO(brettw) it would be nice to know the origin of a
// set_target_defaults so we can give locations for the colliding target
// defaults.
std::string desc_string(desc_for_err);
*err = Err(node_for_err, "Target defaults collision.",
"This " + desc_string + " contains target defaults for\n"
- "\"" + pair.first + "\" which would clobber one for the\n"
+ "\"" + current_name + "\" which would clobber one for the\n"
"same target type in your current scope. It's unfortunate that I'm "
"too stupid\nto tell you the location of where the target defaults "
"were set. Usually\nthis happens in the BUILDCONFIG.gn file.");
@@ -294,7 +316,7 @@
}
// Be careful to delete any pointer we're about to clobber.
- Scope** dest_scope = &dest->target_defaults_[pair.first];
+ Scope** dest_scope = &dest->target_defaults_[current_name];
if (*dest_scope)
delete *dest_scope;
*dest_scope = new Scope(settings_);
@@ -320,11 +342,17 @@
// Templates.
for (const auto& pair : templates_) {
- if (options.skip_private_vars && IsPrivateVar(pair.first))
+ const std::string& current_name = pair.first;
+ if (options.skip_private_vars && IsPrivateVar(current_name))
continue; // Skip this private template.
+ if (!options.excluded_values.empty() &&
+ options.excluded_values.find(current_name) !=
+ options.excluded_values.end()) {
+ continue; // Skip the excluded value.
+ }
if (!options.clobber_existing) {
- const Template* existing_template = dest->GetTemplate(pair.first);
+ const Template* existing_template = dest->GetTemplate(current_name);
// Since templates are refcounted, we can check if it's the same one by
// comparing pointers.
if (existing_template && pair.second.get() != existing_template) {
@@ -333,7 +361,7 @@
std::string desc_string(desc_for_err);
*err = Err(node_for_err, "Template collision.",
"This " + desc_string + " contains a template \"" +
- pair.first + "\"");
+ current_name + "\"");
err->AppendSubErr(Err(pair.second->GetDefinitionRange(),
"defined here.",
"Which would clobber the one in your current scope"));
@@ -346,7 +374,7 @@
}
// Be careful to delete any pointer we're about to clobber.
- dest->templates_[pair.first] = pair.second;
+ dest->templates_[current_name] = pair.second;
}
return true;
diff --git a/tools/gn/scope.h b/tools/gn/scope.h
index 7b32941..2554366 100644
--- a/tools/gn/scope.h
+++ b/tools/gn/scope.h
@@ -65,12 +65,8 @@
// Options for configuring scope merges.
struct MergeOptions {
- // Defaults to all false, which are the things least likely to cause errors.
- MergeOptions()
- : clobber_existing(false),
- skip_private_vars(false),
- mark_dest_used(false) {
- }
+ MergeOptions();
+ ~MergeOptions();
// When set, all existing avlues in the destination scope will be
// overwritten.
@@ -92,6 +88,9 @@
// import, for example, or files that don't need a variable from the .gni
// file will throw an error.
bool mark_dest_used;
+
+ // When set, those variables are not merged.
+ std::set<std::string> excluded_values;
};
// Creates an empty toplevel scope.
