Allow multiple set_default calls in GN.
Allow multipe set_defaults calls in GN to override one another, and allow
imported .gni files with set_defaults calls in them to not collide with
existing defaults settings as long as the settings are the same.
This will allow us to be more flexible with defaults, in particular, to
allow the defaults for templates to move to the template declaration location
rather than having to reside in BUILDCONFIG.
Add an android condition around an android-specific config in base I noticed
when testing this patch.
BUG=627978
Review-Url: https://codereview.chromium.org/2148993002
Cr-Original-Commit-Position: refs/heads/master@{#405539}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: af8a964ba300ff17d304c982a0e98605c41637c5
diff --git a/tools/gn/function_set_defaults.cc b/tools/gn/function_set_defaults.cc
index 4efb929..7f26b44 100644
--- a/tools/gn/function_set_defaults.cc
+++ b/tools/gn/function_set_defaults.cc
@@ -27,9 +27,14 @@
"\n"
" set_defaults can be used for built-in target types (\"executable\",\n"
" \"shared_library\", etc.) and custom ones defined via the \"template\"\n"
- " command.\n"
+ " command. It can be called more than once and the most recent call in\n"
+ " any scope will apply, but there is no way to refer to the previous\n"
+ " defaults and modify them (each call to set_defaults must supply a\n"
+ " complete list of all defaults it wants). If you want to share\n"
+ " defaults, store them in a separate variable.\n"
"\n"
- "Example:\n"
+ "Example\n"
+ "\n"
" set_defaults(\"static_library\") {\n"
" configs = [ \"//tools/mything:settings\" ]\n"
" }\n"
@@ -49,27 +54,6 @@
return Value();
const std::string& target_type(args[0].string_value());
- // Ensure there aren't defaults already set.
- //
- // It might be nice to allow multiple calls set mutate the defaults. The
- // main case for this is where some local portions of the code want
- // additional defaults they specify in an imported file.
- //
- // Currently, we don't allow imports to clobber anything, so this wouldn't
- // work. Additionally, allowing this would be undesirable since we don't
- // want multiple imports to each try to set defaults, since it might look
- // like the defaults are modified by each one in sequence, while in fact
- // imports would always clobber previous values and it would be confusing.
- //
- // If we wanted this, the solution would be to allow imports to overwrite
- // target defaults set up by the default build config only. That way there
- // are no ordering issues, but this would be more work.
- if (scope->GetTargetDefaults(target_type)) {
- *err = Err(function->function(),
- "This target type defaults were already set.");
- return Value();
- }
-
if (!block) {
FillNeedsBlockError(function, err);
return Value();
diff --git a/tools/gn/scope.cc b/tools/gn/scope.cc
index 3869f3b..8e9230d 100644
--- a/tools/gn/scope.cc
+++ b/tools/gn/scope.cc
@@ -298,18 +298,27 @@
}
if (!options.clobber_existing) {
- 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"
- "\"" + 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.");
- return false;
+ const Scope* dest_defaults = dest->GetTargetDefaults(current_name);
+ if (dest_defaults) {
+ if (RecordMapValuesEqual(pair.second->values_,
+ dest_defaults->values_)) {
+ // Values of the two defaults are equivalent, just ignore the
+ // collision.
+ continue;
+ } else {
+ // 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"
+ "\"" + 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 or in a related .gni file.\n");
+ return false;
+ }
}
}
@@ -404,14 +413,7 @@
}
Scope* Scope::MakeTargetDefaults(const std::string& target_type) {
- if (GetTargetDefaults(target_type))
- return nullptr;
-
std::unique_ptr<Scope>& dest = target_defaults_[target_type];
- if (dest) {
- NOTREACHED(); // Already set.
- return dest.get();
- }
dest = base::WrapUnique(new Scope(settings_));
return dest.get();
}
@@ -514,3 +516,17 @@
DCHECK(programmatic_providers_.find(p) != programmatic_providers_.end());
programmatic_providers_.erase(p);
}
+
+// static
+bool Scope::RecordMapValuesEqual(const RecordMap& a, const RecordMap& b) {
+ if (a.size() != b.size())
+ return false;
+ for (const auto& pair : a) {
+ const auto& found_b = b.find(pair.first);
+ if (found_b == b.end())
+ return false; // Item in 'a' but not 'b'.
+ if (pair.second.value != found_b->second.value)
+ return false; // Values for variable in 'a' and 'b' are different.
+ }
+ return true;
+}
diff --git a/tools/gn/scope.h b/tools/gn/scope.h
index e9bd139..685f840 100644
--- a/tools/gn/scope.h
+++ b/tools/gn/scope.h
@@ -229,8 +229,7 @@
// change, we don't have to copy its values).
std::unique_ptr<Scope> MakeClosure() const;
- // Makes an empty scope with the given name. Returns NULL if the name is
- // already set.
+ // Makes an empty scope with the given name. Overwrites any existing one.
Scope* MakeTargetDefaults(const std::string& target_type);
// Gets the scope associated with the given target name, or null if it hasn't
@@ -311,9 +310,16 @@
Value value;
};
+ typedef base::hash_map<base::StringPiece, Record, base::StringPieceHash>
+ RecordMap;
+
void AddProvider(ProgrammaticProvider* p);
void RemoveProvider(ProgrammaticProvider* p);
+ // Returns true if the two RecordMaps contain the same values (the origins
+ // of the values may be different).
+ static bool RecordMapValuesEqual(const RecordMap& a, const RecordMap& b);
+
// Scopes can have no containing scope (both null), a mutable containing
// scope, or a const containing scope. The reason is that when we're doing
// a new target, we want to refer to the base_config scope which will be read
@@ -329,8 +335,6 @@
// for more.
unsigned mode_flags_;
- typedef base::hash_map<base::StringPiece, Record, base::StringPieceHash>
- RecordMap;
RecordMap values_;
// Note that this can't use string pieces since the names are constructed from
