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: 28f3c4987aacf447643a4f36e4553cc830508b0bdiff --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 {
