Implement "friend" for GN header checking. This allows a target to include private headers it wouldn't otherwise be able to include. A target can provide a list of patterns of friend targets that allow dependent targets to bypass public header checking. A proper dependency path is still required, this applies only to the concept of public/private headers on a target. Without this, it's difficult to make a target be explicit about its public headers while also allowing that target's own tests to use internal headers. BUG=732993 Change-Id: I1cb94c7ac0db6f74cdcd49d08bcabedf70ea3eb0 Reviewed-on: https://chromium-review.googlesource.com/994453 Commit-Queue: Brett Wilson <brettw@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#548279} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 28f3c4987aacf447643a4f36e4553cc830508b0b
diff --git a/tools/gn/binary_target_generator.cc b/tools/gn/binary_target_generator.cc index 8659306..4ff9366 100644 --- a/tools/gn/binary_target_generator.cc +++ b/tools/gn/binary_target_generator.cc
@@ -47,6 +47,9 @@ if (!FillPublic()) return; + if (!FillFriends()) + return; + if (!FillCheckIncludes()) return; @@ -79,6 +82,15 @@ return true; } +bool BinaryTargetGenerator::FillFriends() { + const Value* value = scope_->GetValue(variables::kFriend, true); + if (value) { + return ExtractListOfLabelPatterns(*value, scope_->GetSourceDir(), + &target_->friends(), err_); + } + return true; +} + bool BinaryTargetGenerator::FillOutputName() { const Value* value = scope_->GetValue(variables::kOutputName, true); if (!value)
diff --git a/tools/gn/binary_target_generator.h b/tools/gn/binary_target_generator.h index 0788a20..40fc314 100644 --- a/tools/gn/binary_target_generator.h +++ b/tools/gn/binary_target_generator.h
@@ -25,6 +25,7 @@ private: bool FillCompleteStaticLib(); + bool FillFriends(); bool FillOutputName(); bool FillOutputPrefixOverride(); bool FillOutputDir();
diff --git a/tools/gn/functions_target.cc b/tools/gn/functions_target.cc index 0e20367..0ceaba0 100644 --- a/tools/gn/functions_target.cc +++ b/tools/gn/functions_target.cc
@@ -17,9 +17,9 @@ " Dependent configs: all_dependent_configs, public_configs\n" #define DEPS_VARS \ " Deps: data_deps, deps, public_deps\n" -#define GENERAL_TARGET_VARS \ - " General: check_includes, configs, data, inputs, output_name,\n" \ - " output_extension, public, sources, testonly, visibility\n" +#define GENERAL_TARGET_VARS \ + " General: check_includes, configs, data, friend, inputs, output_name,\n" \ + " output_extension, public, sources, testonly, visibility\n" namespace functions {
diff --git a/tools/gn/header_checker.cc b/tools/gn/header_checker.cc index c47e6f1..57d2642 100644 --- a/tools/gn/header_checker.cc +++ b/tools/gn/header_checker.cc
@@ -111,6 +111,14 @@ a->label().name() == b->label().name(); } +// Returns true if the target |annotation_on| includes a friend annotation +// that allows |is_marked_friend| as a friend. +bool FriendMatches(const Target* annotation_on, + const Target* is_marked_friend) { + return LabelPattern::VectorMatches(annotation_on->friends(), + is_marked_friend->label()); +} + } // namespace HeaderChecker::HeaderChecker(const BuildSettings* build_settings, @@ -354,8 +362,9 @@ return true; // For all targets containing this file, we require that at least one be - // a direct or public dependency of the current target, and that the header - // is public within the target. + // a direct or public dependency of the current target, and either (1) the + // header is public within the target, or (2) there is a friend definition + // whitelisting the includor. // // If there is more than one target containing this header, we may encounter // some error cases before finding a good one. This error stores the previous @@ -378,14 +387,17 @@ found_dependency = true; - if (target.is_public && is_permitted_chain) { + bool effectively_public = + target.is_public || FriendMatches(to_target, from_target); + + if (effectively_public && is_permitted_chain) { // This one is OK, we're done. last_error = Err(); break; } // Diagnose the error. - if (!target.is_public) { + if (!effectively_public) { // Danger: must call CreatePersistentRange to put in Err. last_error = Err(CreatePersistentRange(source_file, range), "Including a private header.",
diff --git a/tools/gn/header_checker.h b/tools/gn/header_checker.h index 3645d7a..b1d0f79 100644 --- a/tools/gn/header_checker.h +++ b/tools/gn/header_checker.h
@@ -71,6 +71,8 @@ FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, SourceFileForInclude); FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, SourceFileForInclude_FileNotFound); + FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, Friend); + ~HeaderChecker(); struct TargetInfo {
diff --git a/tools/gn/header_checker_unittest.cc b/tools/gn/header_checker_unittest.cc index 5bb5c70..dcfa7bb 100644 --- a/tools/gn/header_checker_unittest.cc +++ b/tools/gn/header_checker_unittest.cc
@@ -348,3 +348,35 @@ EXPECT_TRUE(source_file.is_null()); EXPECT_FALSE(err.has_error()); } + +TEST_F(HeaderCheckerTest, Friend) { + // Note: we have a public dependency chain A -> B -> C set up already. + InputFile input_file(SourceFile("//some_file.cc")); + input_file.SetContents(std::string()); + LocationRange range; // Dummy value. + + // Add a private header on C. + SourceFile c_private("//c_private.h"); + c_.sources().push_back(c_private); + c_.set_all_headers_public(false); + + // List A as a friend of C. + Err err; + c_.friends().push_back( + LabelPattern::GetPattern(SourceDir("//"), Value(nullptr, "//a:*"), &err)); + ASSERT_FALSE(err.has_error()); + + // Must be after setting everything up for it to find the files. + scoped_refptr<HeaderChecker> checker( + new HeaderChecker(setup_.build_settings(), targets_)); + + // B should not be allowed to include C's private header. + err = Err(); + EXPECT_FALSE(checker->CheckInclude(&b_, input_file, c_private, range, &err)); + EXPECT_TRUE(err.has_error()); + + // A should be able to because of the friend declaration. + err = Err(); + EXPECT_TRUE(checker->CheckInclude(&a_, input_file, c_private, range, &err)); + EXPECT_FALSE(err.has_error()) << err.message(); +}
diff --git a/tools/gn/label_pattern.cc b/tools/gn/label_pattern.cc index c4e9bb6..b2568e7 100644 --- a/tools/gn/label_pattern.cc +++ b/tools/gn/label_pattern.cc
@@ -242,6 +242,16 @@ } } +// static +bool LabelPattern::VectorMatches(const std::vector<LabelPattern>& patterns, + const Label& label) { + for (const auto& pattern : patterns) { + if (pattern.Matches(label)) + return true; + } + return false; +} + std::string LabelPattern::Describe() const { std::string result;
diff --git a/tools/gn/label_pattern.h b/tools/gn/label_pattern.h index 7d0768c..774b81a 100644 --- a/tools/gn/label_pattern.h +++ b/tools/gn/label_pattern.h
@@ -46,6 +46,10 @@ // Returns true if this pattern matches the given label. bool Matches(const Label& label) const; + // Returns true if any of the patterns in the vector match the label. + static bool VectorMatches(const std::vector<LabelPattern>& patterns, + const Label& label); + // Returns a string representation of this pattern. std::string Describe() const;
diff --git a/tools/gn/target.h b/tools/gn/target.h index b286518..568bea5 100644 --- a/tools/gn/target.h +++ b/tools/gn/target.h
@@ -243,9 +243,10 @@ return recursive_hard_deps_; } - std::vector<LabelPattern>& assert_no_deps() { - return assert_no_deps_; - } + std::vector<LabelPattern>& friends() { return friends_; } + const std::vector<LabelPattern>& friends() const { return friends_; } + + std::vector<LabelPattern>& assert_no_deps() { return assert_no_deps_; } const std::vector<LabelPattern>& assert_no_deps() const { return assert_no_deps_; } @@ -374,6 +375,7 @@ // target is marked resolved. This will not include the current target. std::set<const Target*> recursive_hard_deps_; + std::vector<LabelPattern> friends_; std::vector<LabelPattern> assert_no_deps_; // Used for all binary targets, and for inputs in regular targets. The
diff --git a/tools/gn/variables.cc b/tools/gn/variables.cc index 80c6b36..591a945 100644 --- a/tools/gn/variables.cc +++ b/tools/gn/variables.cc
@@ -1100,17 +1100,67 @@ See also "public_deps". )"; -const char kXcodeExtraAttributes[] = "xcode_extra_attributes"; -const char kXcodeExtraAttributes_HelpShort[] = - "xcode_extra_attributes: [scope] Extra attributes for Xcode projects."; -const char kXcodeExtraAttributes_Help[] = - R"(xcode_extra_attributes: [scope] Extra attributes for Xcode projects. +const char kFriend[] = "friend"; +const char kFriend_HelpShort[] = + "friend: [label pattern list] Allow targets to include private headers."; +const char kFriend_Help[] = + R"(friend: Allow targets to include private headers. - The value defined in this scope will be copied to the EXTRA_ATTRIBUTES - property of the generated Xcode project. They are only meaningful when - generating with --ide=xcode. + A list of label patterns (see "gn help label_pattern") that allow dependent + targets to include private headers. Applies to all binary targets. - See "gn help create_bundle" for more information. + Normally if a target lists headers in the "public" list (see "gn help + public"), other headers are implicitly marked as private. Private headers + can not be included by other targets, even with a public dependency path. + The "gn check" function performs this validation. + + A friend declaration allows one or more targets to include private headers. + This is useful for things like unit tests that are closely associated with a + target and require internal knowledge without opening up all headers to be + included by all dependents. + + A friend target does not allow that target to include headers when no + dependency exists. A public dependency path must still exist between two + targets to include any headers from a destination target. The friend + annotation merely allows the use of headers that would otherwise be + prohibited because they are private. + + The friend annotation is matched only against the target containing the file + with the include directive. Friend annotations are not propagated across + public or private dependencies. Friend annotations do not affect visibility. + +Example + + static_library("lib") { + # This target can include our private headers. + friend = [ ":unit_tests" ] + + public = [ + "public_api.h", # Normal public API for dependent targets. + ] + + # Private API and sources. + sources = [ + "a_source_file.cc", + + # Normal targets that depend on this one won't be able to include this + # because this target defines a list of "public" headers. Without the + # "public" list, all headers are implicitly public. + "private_api.h", + ] + } + + executable("unit_tests") { + sources = [ + # This can include "private_api.h" from the :lib target because it + # depends on that target and because of the friend annotation. + "my_test.cc", + ] + + deps = [ + ":lib", # Required for the include to be allowed. + ] + } )"; const char kIncludeDirs[] = "include_dirs"; @@ -1593,7 +1643,8 @@ If no public files are declared, other targets (assuming they have visibility to depend on this target) can include any file in the sources list. If this variable is defined on a target, dependent targets may only include files on - this whitelist. + this whitelist unless that target is marked as a friend (see "gn help + friend"). Header file permissions are also subject to visibility. A target must be visible to another target to include any files from it at all and the public @@ -1609,6 +1660,11 @@ targets. If a file is included that is not known to the build, it will be allowed. + It is common for test targets to need to include private headers for their + associated code. In this case, list the test target in the "friend" list of + the target that owns the private header to allow the inclusion. See + "gn help friend" for more. + Examples These exact files are public: @@ -1901,6 +1957,19 @@ help --runtime-deps-list-file"). )"; +const char kXcodeExtraAttributes[] = "xcode_extra_attributes"; +const char kXcodeExtraAttributes_HelpShort[] = + "xcode_extra_attributes: [scope] Extra attributes for Xcode projects."; +const char kXcodeExtraAttributes_Help[] = + R"(xcode_extra_attributes: [scope] Extra attributes for Xcode projects. + + The value defined in this scope will be copied to the EXTRA_ATTRIBUTES + property of the generated Xcode project. They are only meaningful when + generating with --ide=xcode. + + See "gn help create_bundle" for more information. +)"; + // ----------------------------------------------------------------------------- VariableInfo::VariableInfo() @@ -1971,7 +2040,7 @@ INSERT_VARIABLE(Defines) INSERT_VARIABLE(Depfile) INSERT_VARIABLE(Deps) - INSERT_VARIABLE(XcodeExtraAttributes) + INSERT_VARIABLE(Friend) INSERT_VARIABLE(IncludeDirs) INSERT_VARIABLE(Inputs) INSERT_VARIABLE(Ldflags) @@ -1998,6 +2067,7 @@ INSERT_VARIABLE(Testonly) INSERT_VARIABLE(Visibility) INSERT_VARIABLE(WriteRuntimeDeps) + INSERT_VARIABLE(XcodeExtraAttributes) } return info_map; }
diff --git a/tools/gn/variables.h b/tools/gn/variables.h index 0e3891d..80615f9 100644 --- a/tools/gn/variables.h +++ b/tools/gn/variables.h
@@ -195,9 +195,9 @@ extern const char kDeps_HelpShort[]; extern const char kDeps_Help[]; -extern const char kXcodeExtraAttributes[]; -extern const char kXcodeExtraAttributes_HelpShort[]; -extern const char kXcodeExtraAttributes_Help[]; +extern const char kFriend[]; +extern const char kFriend_HelpShort[]; +extern const char kFriend_Help[]; extern const char kIncludeDirs[]; extern const char kIncludeDirs_HelpShort[]; @@ -303,6 +303,10 @@ extern const char kWriteRuntimeDeps_HelpShort[]; extern const char kWriteRuntimeDeps_Help[]; +extern const char kXcodeExtraAttributes[]; +extern const char kXcodeExtraAttributes_HelpShort[]; +extern const char kXcodeExtraAttributes_Help[]; + // ----------------------------------------------------------------------------- struct VariableInfo {
diff --git a/tools/gn/visibility.cc b/tools/gn/visibility.cc index 54d7280..ec96824 100644 --- a/tools/gn/visibility.cc +++ b/tools/gn/visibility.cc
@@ -54,11 +54,7 @@ } bool Visibility::CanSeeMe(const Label& label) const { - for (const auto& pattern : patterns_) { - if (pattern.Matches(label)) - return true; - } - return false; + return LabelPattern::VectorMatches(patterns_, label); } std::string Visibility::Describe(int indent, bool include_brackets) const {