Add more documentation for GN header checking

tools/gn/variables.cc

Index: tools/gn/variables.cc
diff --git a/tools/gn/variables.cc b/tools/gn/variables.cc
index fd7800f841b80e3f2163d4f72fab98a69dc1533f..81bb7e3ee535c495ed80f318b5591eb6a3ea250b 100644
--- a/tools/gn/variables.cc
+++ b/tools/gn/variables.cc
@@ -354,29 +354,68 @@ const char kAllowCircularIncludesFrom_Help[] =
     "  These targets will be permitted to include headers from the current\n"
     "  target despite the dependency going in the opposite direction.\n"
     "\n"
-    "Tedious exposition\n"
+    "  When you use this, both targets must be included in a final binary\n"
+    "  for it to link. To keep linker errors from happening, it is good\n"
+    "  practice to have all external dependencies depend only on one of\n"
+    "  the two targets, and to set the visibility on the other to enforce\n"
+    "  this. Thus the targets will always be linked together in any output.\n"
+    "\n"
+    "Details\n"
     "\n"
     "  Normally, for a file in target A to include a file from target B,\n"
     "  A must list B as a dependency. This invariant is enforced by the\n"
-    "  \"gn check\" command (and the --check flag to \"gn gen\").\n"
+    "  \"gn check\" command (and the --check flag to \"gn gen\" -- see\n"
+    "  \"gn help check\").\n"
     "\n"
     "  Sometimes, two targets might be the same unit for linking purposes\n"
     "  (two source sets or static libraries that would always be linked\n"
-    "  together in a final executable or shared library). In this case,\n"
-    "  you want A to be able to include B's headers, and B to include A's\n"
-    "  headers.\n"
+    "  together in a final executable or shared library) and they each\n"
+    "  include headers from the other: you want A to be able to include B's\n"
+    "  headers, and B to include A's headers. This is not an ideal situation\n"
+    "  but is sometimes unavoidable.\n"
     "\n"
     "  This list, if specified, lists which of the dependencies of the\n"
     "  current target can include header files from the current target.\n"
     "  That is, if A depends on B, B can only include headers from A if it is\n"
-    "  in A's allow_circular_includes_from list.\n"
+    "  in A's allow_circular_includes_from list. Normally includes must\n"
+    "  follow the direction of dependencies, this flag allows them to go\n"
+    "  in the opposite direction.\n"
+    "\n"
+    "Danger\n"
+    "\n"
+    "  In the above example, A's headers are likely to include headers from\n"
+    "  A's dependencies. Those dependencies may have public_configs that\n"
+    "  apply flags, defines, and include paths that make those headers work\n"
+    "  properly.\n"
+    "\n"
+    "  With allow_circular_includes_from, B can include A's headers, and\n"
+    "  transitively from A's dependencies, without having the dependencies\n"
+    "  that would bring in the public_configs those headers need. The result\n"
+    "  may be errors or inconsistent builds.\n"
+    "\n"
+    "  So when you use allow_circular_includes_from, make sure that any\n"
+    "  compiler settings, flags, and include directories are the same between\n"
+    "  both targets (consider putting such things in a shared config they can\n"
+    "  both reference). Make sure the dependencies are also the same (you\n"
+    "  might consider a group to collect such dependencies they both\n"
+    "  depend on).\n"
     "\n"
     "Example\n"
     "\n"
     "  source_set(\"a\") {\n"
-    "    deps = [ \":b\", \":c\" ]\n"
+    "    deps = [ \":b\", \":a_b_shared_deps\" ]\n"
     "    allow_circular_includes_from = [ \":b\" ]\n"
     "    ...\n"
+    "  }\n"
+    "\n"
+    "  source_set(\"b\") {\n"
+    "    deps = [ \":a_b_shared_deps\" ]\n"
+    "    # Sources here can include headers from a despite lack of deps.\n"
+    "    ...\n"
+    "  }\n"
+    "\n"
+    "  group(\"a_b_shared_deps\") {\n"
+    "    public_deps = [ \":c\" ]\n"
     "  }\n";
 
 const char kArgs[] = "args";
@@ -480,7 +519,7 @@ const char kBundleResourcesDir_Help[] =
 const char kBundleExecutableDir[] = "bundle_executable_dir";
 const char kBundleExecutableDir_HelpShort[] =
     "bundle_executable_dir: "
-        "Expansion of {{bundle_executable_dir}} in create_bundle.";

[brettw, 2016/03/18 20:15:05]

This was on purpose, this line was 81 cols and wrapped funny.

+        "Expansion of {{bundle_executable_dir}} in create_bundle";
 const char kBundleExecutableDir_Help[] =
     "bundle_executable_dir: "
         "Expansion of {{bundle_executable_dir}} in create_bundle.\n"
@@ -575,26 +614,12 @@ const char kCheckIncludes_Help[] =
     "  This does not affect other targets that depend on the current target,\n"
     "  it just skips checking the includes of the current target's files.\n"
     "\n"
-    "Controlling includes individually\n"
-    "\n"
-    "  If only certain includes are problematic, you can annotate them\n"
-    "  individually rather than disabling header checking on an entire\n"
-    "  target. Add the string \"nogncheck\" to the include line:\n"
+    "  If there are a few conditionally included headers that trip up\n"
+    "  checking, you can exclude headers individually by annotating them with\n"
+    "  \"nogncheck\" (see \"gn help nogncheck\").\n"
     "\n"
-    "    #include \"foo/something_weird.h\"  // nogncheck (bug 12345)\n"
-    "\n"
-    "  It is good form to include a reference to a bug (if the include is\n"
-    "  improper, or some other comment expressing why the header checker\n"
-    "  doesn't work for this particular case.\n"
-    "\n"
-    "  The most common reason to need \"nogncheck\" is conditional includes.\n"
-    "  The header checker does not understand the preprocessor, so may flag\n"
-    "  some includes as improper even if the dependencies and #defines are\n"
-    "  always matched correctly:\n"
-    "\n"
-    "    #if defined(ENABLE_DOOM_MELON)\n"
-    "    #include \"doom_melon/beam_controller.h\"  // nogncheck\n"
-    "    #endif\n"
+    "  The topic \"gn help check\" has general information on how checking\n"
+    "  works and advice on how to pass a check in problematic cases.\n"
     "\n"
     "Example\n"
     "\n"
@@ -623,6 +648,9 @@ const char kCompleteStaticLib_Help[] =
     "  for all dependencies in one complete package. Since GN does not unpack\n"
     "  static libraries to forward their contents up the dependency chain,\n"
     "  it is an error for complete static libraries to depend on other static\n"
+    "\n"
+    "  In rare cases it makes sense to list a header in more than one\n"
+    "  target if it could be considered conceptually a member of both.\n"
     "  libraries.\n"
     "\n"
     "Example\n"