Add an assert_no_deps variable to GN. This asserts that there is no dependency path to a given target. BUG= Review URL: https://codereview.chromium.org/1621053002 Cr-Original-Commit-Position: refs/heads/master@{#371539} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 81aa4e8f010454835da6caa14507bcee73578e83
diff --git a/tools/gn/setup.cc b/tools/gn/setup.cc index 459b87b..79814e6 100644 --- a/tools/gn/setup.cc +++ b/tools/gn/setup.cc
@@ -31,6 +31,7 @@ #include "tools/gn/tokenizer.h" #include "tools/gn/trace.h" #include "tools/gn/value.h" +#include "tools/gn/value_extractors.h" #if defined(OS_WIN) #include <windows.h> @@ -655,21 +656,13 @@ const Value* check_targets_value = dotfile_scope_.GetValue("check_targets", true); if (check_targets_value) { - // Fill the list of targets to check. - if (!check_targets_value->VerifyTypeIs(Value::LIST, &err)) { + check_patterns_.reset(new std::vector<LabelPattern>); + ExtractListOfLabelPatterns(*check_targets_value, current_dir, + check_patterns_.get(), &err); + if (err.has_error()) { err.PrintToStdout(); return false; } - - check_patterns_.reset(new std::vector<LabelPattern>); - for (const auto& item : check_targets_value->list_value()) { - check_patterns_->push_back( - LabelPattern::GetPattern(current_dir, item, &err)); - if (err.has_error()) { - err.PrintToStdout(); - return false; - } - } } // Fill exec_script_whitelist.
diff --git a/tools/gn/target.cc b/tools/gn/target.cc index 905ee4f..7dd1921 100644 --- a/tools/gn/target.cc +++ b/tools/gn/target.cc
@@ -129,6 +129,61 @@ return false; } +// check_this indicates if the given target should be matched against the +// patterns. It should be set to false for the first call since assert_no_deps +// shouldn't match the target itself. +// +// visited should point to an empty set, this will be used to prevent +// multiple visits. +// +// *failure_path_str will be filled with a string describing the path of the +// dependency failure, and failure_pattern will indicate the pattern in +// assert_no that matched the target. +// +// Returns true if everything is OK. failure_path_str and failure_pattern_index +// will be unchanged in this case. +bool RecursiveCheckAssertNoDeps(const Target* target, + bool check_this, + const std::vector<LabelPattern>& assert_no, + std::set<const Target*>* visited, + std::string* failure_path_str, + const LabelPattern** failure_pattern) { + static const char kIndentPath[] = " "; + + if (visited->find(target) != visited->end()) + return true; // Already checked this target. + visited->insert(target); + + if (check_this) { + // Check this target against the given list of patterns. + for (const LabelPattern& pattern : assert_no) { + if (pattern.Matches(target->label())) { + // Found a match. + *failure_pattern = &pattern; + *failure_path_str = + kIndentPath + target->label().GetUserVisibleName(false); + return false; + } + } + } + + // Recursively check dependencies. + for (const auto& pair : target->GetDeps(Target::DEPS_ALL)) { + if (pair.ptr->output_type() == Target::EXECUTABLE) + continue; + if (!RecursiveCheckAssertNoDeps(pair.ptr, true, assert_no, visited, + failure_path_str, failure_pattern)) { + // To reconstruct the path, prepend the current target to the error. + std::string prepend_path = + kIndentPath + target->label().GetUserVisibleName(false) + " ->\n"; + failure_path_str->insert(0, prepend_path); + return false; + } + } + + return true; +} + } // namespace Target::Target(const Settings* settings, const Label& label) @@ -234,6 +289,8 @@ return false; if (!CheckNoNestedStaticLibs(err)) return false; + if (!CheckAssertNoDeps(err)) + return false; CheckSourcesGenerated(); } @@ -604,6 +661,27 @@ return true; } +bool Target::CheckAssertNoDeps(Err* err) const { + if (assert_no_deps_.empty()) + return true; + + std::set<const Target*> visited; + std::string failure_path_str; + const LabelPattern* failure_pattern = nullptr; + + if (!RecursiveCheckAssertNoDeps(this, false, assert_no_deps_, &visited, + &failure_path_str, &failure_pattern)) { + *err = Err(defined_from(), "assert_no_deps failed.", + label().GetUserVisibleName(false) + + " has an assert_no_deps entry:\n " + + failure_pattern->Describe() + + "\nwhich fails for the dependency path:\n" + + failure_path_str); + return false; + } + return true; +} + void Target::CheckSourcesGenerated() const { // Checks that any inputs or sources to this target that are in the build // directory are generated by a target that this one transitively depends on
diff --git a/tools/gn/target.h b/tools/gn/target.h index cbdab35..18e5fb8 100644 --- a/tools/gn/target.h +++ b/tools/gn/target.h
@@ -16,6 +16,7 @@ #include "tools/gn/config_values.h" #include "tools/gn/inherited_libraries.h" #include "tools/gn/item.h" +#include "tools/gn/label_pattern.h" #include "tools/gn/label_ptr.h" #include "tools/gn/lib_file.h" #include "tools/gn/ordered_set.h" @@ -209,6 +210,13 @@ return recursive_hard_deps_; } + std::vector<LabelPattern>& assert_no_deps() { + return assert_no_deps_; + } + const std::vector<LabelPattern>& assert_no_deps() const { + return assert_no_deps_; + } + // The toolchain is only known once this target is resolved (all if its // dependencies are known). They will be null until then. Generally, this can // only be used during target writing. @@ -284,6 +292,7 @@ bool CheckVisibility(Err* err) const; bool CheckTestonly(Err* err) const; bool CheckNoNestedStaticLibs(Err* err) const; + bool CheckAssertNoDeps(Err* err) const; void CheckSourcesGenerated() const; void CheckSourceGenerated(const SourceFile& source) const; @@ -324,6 +333,8 @@ // target is marked resolved. This will not include the current target. std::set<const Target*> recursive_hard_deps_; + std::vector<LabelPattern> assert_no_deps_; + // Used for all binary targets. The precompiled header values in this struct // will be resolved to the ones to use for this target, if precompiled // headers are used.
diff --git a/tools/gn/target_generator.cc b/tools/gn/target_generator.cc index 3734ea9..b1ed1ea 100644 --- a/tools/gn/target_generator.cc +++ b/tools/gn/target_generator.cc
@@ -50,6 +50,9 @@ if (!FillTestonly()) return; + if (!FillAssertNoDeps()) + return; + if (!Visibility::FillItemVisibility(target_, scope_, err_)) return; @@ -266,6 +269,15 @@ return true; } +bool TargetGenerator::FillAssertNoDeps() { + const Value* value = scope_->GetValue(variables::kAssertNoDeps, true); + if (value) { + return ExtractListOfLabelPatterns(*value, scope_->GetSourceDir(), + &target_->assert_no_deps(), err_); + } + return true; +} + bool TargetGenerator::FillOutputs(bool allow_substitutions) { const Value* value = scope_->GetValue(variables::kOutputs, true); if (!value)
diff --git a/tools/gn/target_generator.h b/tools/gn/target_generator.h index 1fd055b..4fa51f6 100644 --- a/tools/gn/target_generator.h +++ b/tools/gn/target_generator.h
@@ -70,6 +70,7 @@ bool FillData(); bool FillDependencies(); // Includes data dependencies. bool FillTestonly(); + bool FillAssertNoDeps(); // Reads configs/deps from the given var name, and uses the given setting on // the target to save them.
diff --git a/tools/gn/target_unittest.cc b/tools/gn/target_unittest.cc index c59f44c..c99ebe8 100644 --- a/tools/gn/target_unittest.cc +++ b/tools/gn/target_unittest.cc
@@ -668,3 +668,58 @@ " source: //pcs2.cc", err.help_text()); } + +TEST(Target, AssertNoDeps) { + TestWithScope setup; + Err err; + + // A target. + TestTarget a(setup, "//a", Target::SHARED_LIBRARY); + ASSERT_TRUE(a.OnResolved(&err)); + + // B depends on A and has an assert_no_deps for a random dir. + TestTarget b(setup, "//b", Target::SHARED_LIBRARY); + b.private_deps().push_back(LabelTargetPair(&a)); + b.assert_no_deps().push_back(LabelPattern( + LabelPattern::RECURSIVE_DIRECTORY, SourceDir("//disallowed/"), + std::string(), Label())); + ASSERT_TRUE(b.OnResolved(&err)); + + LabelPattern disallow_a(LabelPattern::RECURSIVE_DIRECTORY, SourceDir("//a/"), + std::string(), Label()); + + // C depends on B and disallows depending on A. This should fail. + TestTarget c(setup, "//c", Target::EXECUTABLE); + c.private_deps().push_back(LabelTargetPair(&b)); + c.assert_no_deps().push_back(disallow_a); + ASSERT_FALSE(c.OnResolved(&err)); + + // Validate the error message has the proper path. + EXPECT_EQ( + "//c:c has an assert_no_deps entry:\n" + " //a/*\n" + "which fails for the dependency path:\n" + " //c:c ->\n" + " //b:b ->\n" + " //a:a", + err.help_text()); + err = Err(); + + // Add an intermediate executable with: exe -> b -> a + TestTarget exe(setup, "//exe", Target::EXECUTABLE); + exe.private_deps().push_back(LabelTargetPair(&b)); + ASSERT_TRUE(exe.OnResolved(&err)); + + // D depends on the executable and disallows depending on A. Since there is + // an intermediate executable, this should be OK. + TestTarget d(setup, "//d", Target::EXECUTABLE); + d.private_deps().push_back(LabelTargetPair(&exe)); + d.assert_no_deps().push_back(disallow_a); + ASSERT_TRUE(d.OnResolved(&err)); + + // A2 disallows depending on anything in its own directory, but the + // assertions should not match the target itself so this should be OK. + TestTarget a2(setup, "//a:a2", Target::EXECUTABLE); + a2.assert_no_deps().push_back(disallow_a); + ASSERT_TRUE(a2.OnResolved(&err)); +}
diff --git a/tools/gn/value_extractors.cc b/tools/gn/value_extractors.cc index 23c5a27..ff009ce 100644 --- a/tools/gn/value_extractors.cc +++ b/tools/gn/value_extractors.cc
@@ -144,6 +144,17 @@ const Label& current_toolchain; }; +struct LabelPatternResolver { + LabelPatternResolver(const SourceDir& current_dir_in) + : current_dir(current_dir_in) { + } + bool operator()(const Value& v, LabelPattern* out, Err* err) const { + *out = LabelPattern::GetPattern(current_dir, v, err); + return !err->has_error(); + } + const SourceDir& current_dir; +}; + } // namespace bool ExtractListOfStringValues(const Value& value, @@ -236,3 +247,11 @@ RelativeFileConverter converter(build_settings, current_dir); return converter(value, file, err); } + +bool ExtractListOfLabelPatterns(const Value& value, + const SourceDir& current_dir, + std::vector<LabelPattern>* patterns, + Err* err) { + return ListValueExtractor(value, patterns, err, + LabelPatternResolver(current_dir)); +}
diff --git a/tools/gn/value_extractors.h b/tools/gn/value_extractors.h index 06d64ce..1e42650 100644 --- a/tools/gn/value_extractors.h +++ b/tools/gn/value_extractors.h
@@ -15,6 +15,7 @@ class BuildSettings; class Err; class Label; +class LabelPattern; class SourceDir; class SourceFile; class Value; @@ -80,4 +81,9 @@ SourceFile* file, Err* err); +bool ExtractListOfLabelPatterns(const Value& value, + const SourceDir& current_dir, + std::vector<LabelPattern>* patterns, + Err* err); + #endif // TOOLS_GN_VALUE_EXTRACTORS_H_
diff --git a/tools/gn/variables.cc b/tools/gn/variables.cc index a835b22..c88a182 100644 --- a/tools/gn/variables.cc +++ b/tools/gn/variables.cc
@@ -391,6 +391,49 @@ "\n" " See also \"gn help action\" and \"gn help action_foreach\".\n"; +const char kAssertNoDeps[] = "assert_no_deps"; +const char kAssertNoDeps_HelpShort[] = + "assert_no_deps: [label pattern list] Ensure no deps on these targets."; +const char kAssertNoDeps_Help[] = + "assert_no_deps: Ensure no deps on these targets.\n" + "\n" + " A list of label patterns.\n" + "\n" + " This list is a list of patterns that must not match any of the\n" + " transitive dependencies of the target. These include all public,\n" + " private, and data dependencies, and cross shared library boundaries.\n" + " This allows you to express that undesirable code isn't accidentally\n" + " added to downstream dependencies in a way that might otherwise be\n" + " difficult to notice.\n" + "\n" + " Checking does not cross executable boundaries. If a target depends on\n" + " an executable, it's assumed that the executable is a tool that is\n" + " producing part of the build rather than something that is linked and\n" + " distributed. This allows assert_no_deps to express what is distributed\n" + " in the final target rather than depend on the internal build steps\n" + " (which may include non-distributable code).\n" + "\n" + " See \"gn help label_pattern\" for the format of the entries in the\n" + " list. These patterns allow blacklisting individual targets or whole\n" + " directory hierarchies.\n" + "\n" + " Sometimes it is desirable to enforce that many targets have no\n" + " dependencies on a target or set of targets. One efficient way to\n" + " express this is to create a group with the assert_no_deps rule on\n" + " it, and make that group depend on all targets you want to apply that\n" + " assertion to.\n" + "\n" + "Example\n" + "\n" + " executable(\"doom_melon\") {\n" + " deps = [ \"//foo:bar\" ]\n" + " ...\n" + " assert_no_deps = [\n" + " \"//evil/*\", # Don't link any code from the evil directory.\n" + " \"//foo:test_support\", # This target is also disallowed.\n" + " ]\n" + " }\n"; + const char kCflags[] = "cflags"; const char kCflags_HelpShort[] = "cflags: [string list] Flags passed to all C compiler variants."; @@ -1400,6 +1443,7 @@ INSERT_VARIABLE(AllowCircularIncludesFrom) INSERT_VARIABLE(Args) INSERT_VARIABLE(Asmflags) + INSERT_VARIABLE(AssertNoDeps) INSERT_VARIABLE(Cflags) INSERT_VARIABLE(CflagsC) INSERT_VARIABLE(CflagsCC)
diff --git a/tools/gn/variables.h b/tools/gn/variables.h index d02d2c6..ee68bd5 100644 --- a/tools/gn/variables.h +++ b/tools/gn/variables.h
@@ -87,6 +87,10 @@ extern const char kAsmflags_HelpShort[]; extern const char* kAsmflags_Help; +extern const char kAssertNoDeps[]; +extern const char kAssertNoDeps_HelpShort[]; +extern const char kAssertNoDeps_Help[]; + extern const char kCflags[]; extern const char kCflags_HelpShort[]; extern const char* kCflags_Help;