Improve GN templates use of "visibility" and "testonly"

Many spots using forward_variables_from(invoker, "*") were not
allowing these two variables to be set in outer scopes.

* Introduce a helper variable "EXPLICIT_FORWARDS" for use with the
recommended pattern of:
  forward_variables_from(invoker, "*", [ "testonly", "visibility" ])
  forward_variables_from(invoker, [ "testonly", "visibility" ])
* Ensures this pattern is used in android templates, test.gni,
  and BUILDCONFIG.gn
* Documents this pattern in writing_gn_templates.md
* Adds a PRESUBMIT.py for it
* Fixes visibility of a few blink targets now that test()
  respects it.

Bug: 862232
Change-Id: Ib71dbf34be76131fc749c721aea856e1146bc69a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2454427
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@google.com>
Cr-Commit-Position: refs/heads/master@{#830678}

PRESUBMIT.py

@@ -3995,6 +3995,35 @@ def CheckWATCHLISTS(input_api, output_api):
   return []
 
 
+def CheckGnGlobForward(input_api, output_api):
+  """Checks that forward_variables_from(invoker, "*") follows best practices.
+
+  As documented at //build/docs/writing_gn_templates.md
+  """
+  def gn_files(f):
+    return input_api.FilterSourceFile(f, files_to_check=(r'.+\.gni', ))
+
+  problems = []
+  for f in input_api.AffectedSourceFiles(gn_files):
+    for line_num, line in f.ChangedContents():
+      if 'forward_variables_from(invoker, "*")' in line:
+        problems.append(
+            'Bare forward_variables_from(invoker, "*") in %s:%d' % (
+                f.LocalPath(), line_num))
+
+  if problems:
+    return [output_api.PresubmitPromptWarning(
+        'forward_variables_from("*") without exclusions',
+        items=sorted(problems),
+        long_text=('The variables "visibilty" and "test_only" should be '
+                   'explicitly listed in forward_variables_from(). For more '
+                   'details, see:\n'
+                   'https://chromium.googlesource.com/chromium/src/+/HEAD/'
+                   'build/docs/writing_gn_templates.md'
+                   '#Using-forward_variables_from'))]
+  return []
+
+
 def CheckNewHeaderWithoutGnChangeOnUpload(input_api, output_api):
   """Checks that newly added header files have corresponding GN changes.
   Note that this is only a heuristic. To be precise, run script:

PRESUBMIT_test.py

@@ -1824,6 +1824,37 @@ class CCIncludeTest(unittest.TestCase):
     self.assertEqual(1, len(errors))
 
 
+class GnGlobForwardTest(unittest.TestCase):
+  def testAddBareGlobs(self):
+    mock_input_api = MockInputApi()
+    mock_input_api.files = [
+      MockAffectedFile('base/stuff.gni', [
+          'forward_variables_from(invoker, "*")']),
+      MockAffectedFile('base/BUILD.gn', [
+          'forward_variables_from(invoker, "*")']),
+    ]
+    warnings = PRESUBMIT.CheckGnGlobForward(mock_input_api, MockOutputApi())
+    self.assertEqual(1, len(warnings))
+    msg = '\n'.join(warnings[0].items)
+    self.assertIn('base/stuff.gni', msg)
+    # Should not check .gn files. Local templates don't need to care about
+    # visibility / testonly.
+    self.assertNotIn('base/BUILD.gn', msg)
+
+  def testValidUses(self):
+    mock_input_api = MockInputApi()
+    mock_input_api.files = [
+      MockAffectedFile('base/stuff.gni', [
+          'forward_variables_from(invoker, "*", [])']),
+      MockAffectedFile('base/stuff2.gni', [
+          'forward_variables_from(invoker, "*", TESTONLY_AND_VISIBILITY)']),
+      MockAffectedFile('base/stuff3.gni', [
+          'forward_variables_from(invoker, [ "testonly" ])']),
+    ]
+    warnings = PRESUBMIT.CheckGnGlobForward(mock_input_api, MockOutputApi())
+    self.assertEqual([], warnings)
+
+
 class NewHeaderWithoutGnChangeTest(unittest.TestCase):
   def testAddHeaderWithoutGn(self):
     mock_input_api = MockInputApi()

build/config/BUILDCONFIG.gn

@@ -447,6 +447,18 @@ set_defaults("loadable_module") {
   }
 }
 
+# A helper for forwarding testonly and visibility.
+# Forwarding "*" does not include variables from outer scopes (to avoid copying
+# all globals into each template invocation), so it will not pick up
+# file-scoped or outer-template-scoped variables. Normally this behavior is
+# desired, but "visibility" and "testonly" are commonly defined in outer scopes.
+# Explicitly forwarding them in forward_variables_from() works around this
+# nuance. See //build/docs/writing_gn_templates.md#using-forward_variables_from
+TESTONLY_AND_VISIBILITY = [
+  "testonly",
+  "visibility",
+]
+
 # Sets default dependencies for executable and shared_library targets.
 #
 # Variables
@@ -461,9 +473,13 @@ foreach(_target_type,
           "shared_library",
         ]) {
   template(_target_type) {
+    # Alias "target_name" because it is clobbered by forward_variables_from().
     _target_name = target_name
     target(_target_type, _target_name) {
-      forward_variables_from(invoker, "*", [ "no_default_deps" ])
+      forward_variables_from(invoker,
+                             "*",
+                             TESTONLY_AND_VISIBILITY + [ "no_default_deps" ])
+      forward_variables_from(invoker, TESTONLY_AND_VISIBILITY)
       if (!defined(deps)) {
         deps = []
       }
@@ -543,15 +559,8 @@ template("component") {
     _component_mode = "static_library"
   }
   target(_component_mode, target_name) {
-    # Explicitly forward visibility, implicitly forward everything else.
-    # Forwarding "*" doesn't recurse into nested scopes (to avoid copying all
-    # globals into each template invocation), so won't pick up file-scoped
-    # variables. Normally this isn't too bad, but visibility is commonly
-    # defined at the file scope. Explicitly forwarding visibility and then
-    # excluding it from the "*" set works around this problem.
-    # See http://crbug.com/594610
-    forward_variables_from(invoker, [ "visibility" ])
-    forward_variables_from(invoker, "*", [ "visibility" ])
+    forward_variables_from(invoker, TESTONLY_AND_VISIBILITY)
+    forward_variables_from(invoker, "*", TESTONLY_AND_VISIBILITY)
   }
 }
 

build/config/android/extract_unwind_tables.gni

@@ -10,9 +10,7 @@ template("unwind_table_asset") {
   _unwind_action = "${target_name}__extract"
 
   action(_unwind_action) {
-    if (defined(invoker.testonly)) {
-      testonly = invoker.testonly
-    }
+    forward_variables_from(invoker, TESTONLY_AND_VISIBILITY)
 
     _root_dir = "$root_out_dir"
     if (defined(android_secondary_abi_cpu)) {
@@ -38,9 +36,7 @@ template("unwind_table_asset") {
     deps += [ "//third_party/breakpad:dump_syms" ]
   }
   android_assets(target_name) {
-    if (defined(invoker.testonly)) {
-      testonly = invoker.testonly
-    }
+    forward_variables_from(invoker, TESTONLY_AND_VISIBILITY)
     sources = [ _asset_path ]
     disable_compression = true
     deps = [ ":$_unwind_action" ]

build/config/fuchsia/generate_runner_scripts.gni

@@ -45,7 +45,7 @@ declare_args() {
 #   is_test_exe: If true, the generated script will run the command under
 #       test_env.py and add arguments expected to be passed to test exes.
 template("fuchsia_package_runner") {
-  forward_variables_from(invoker, [ "runner_script" ])
+  forward_variables_from(invoker, TESTONLY_AND_VISIBILITY + [ "runner_script" ])
 
   if (defined(invoker.package_name_override)) {
     _pkg_shortname = invoker.package_name_override
@@ -66,11 +66,7 @@ template("fuchsia_package_runner") {
 
   # Generates a script which installs and runs a test.
   generate_wrapper(_generate_runner_target) {
-    forward_variables_from(invoker,
-                           [
-                             "target",
-                             "testonly",
-                           ])
+    forward_variables_from(invoker, [ "target" ])
 
     _is_test_exe = defined(invoker.is_test_exe) && invoker.is_test_exe
 
@@ -212,8 +208,6 @@ template("fuchsia_package_runner") {
   # Produces a script which installs a package and its dependencies into the
   # Amber repository of a pre-existing Fuchsia build directory.
   generate_wrapper(_generate_installer_target) {
-    forward_variables_from(invoker, [ "testonly" ])
-
     executable = rebase_path("//build/fuchsia/deploy_to_amber_repo.py")
     wrapper_script = generated_install_pkg_script_path
 
@@ -255,7 +249,6 @@ template("fuchsia_package_runner") {
   }
 
   group(target_name) {
-    forward_variables_from(invoker, [ "testonly" ])
     deps = [ ":${_generate_installer_target}" ]
 
     if (!defined(invoker.install_only) || invoker.install_only == false) {

build/docs/writing_gn_templates.md

@@ -178,6 +178,33 @@ template("my_template") {
 This scheme ensures that subtargets defined in templates do not conflict with
 top-level targets.
 
+### Visibility for Intermediate Targets
+
+You can restrict what targets can depend on one another using [visibility].
+When writing templates, with multiple intermediate targets, `visibility` should
+only be applied to the final target (the one named `target_name`). Applying only
+to the final target ensures that the invoker-provided visibility does not
+prevent intermediate targets from depending on each other.
+
+[visibility]: https://gn.googlesource.com/gn/+/master/docs/reference.md#var_visibility
+
+Example:
+```python
+template("my_template") {
+  # Do not forward visibility here.
+  action("${target_name}__helper") {
+    # Do not forward visibility here.
+    ...
+  }
+  action(target_name) {
+    # Forward visibility here.
+    forward_variables_from(invoker, [ "visibility" ])
+    deps = [ ":${target_name}__helper" ]
+    ...
+  }
+}
+```
+
 ### Variables
 Prefix variables within templates and targets with an underscore. For example:
 
@@ -235,22 +262,52 @@ template("hello") {
 ```
 
 ### Using forward_variables_from()
-Using `forward_variables_from()` is encouraged, but `testonly` and `visibility`
-should always be listed explicitly in case they are assigned in an enclosing
-scope (applies to the `"*"` variant of `forward_variables_from()`).
-See [this bug](https://bugs.chromium.org/p/chromium/issues/detail?id=862232)
-for more context.
+Using [forward_variables_from()] is encouraged, but special care needs to be
+taken when forwarding `"*"`. The variables `testonly` and `visibility` should
+always be listed explicitly in case they are assigned in an enclosing
+scope.
+See [this bug] for more a full example.
 
+To make this easier, `//build/config/BUILDCONFIG.gn` defines:
+```python
+TESTONLY_AND_VISIBILITY = [ "testonly", "visibility" ]
+```
+
+Example usage:
 ```python
 template("action_wrapper") {
   action(target_name) {
-    forward_variables_from(invoker, "*", [ "testonly", "visibility" ])
-    forward_variables_from(invoker, [ "testonly", "visibility" ])
+    forward_variables_from(invoker, "*", TESTONLY_AND_VISIBILITY)
+    forward_variables_from(invoker, TESTONLY_AND_VISIBILITY)
+    ...
+  }
+}
+```
+
+If your template defines multiple targets, be careful to apply `testonly` to
+both, but `visibility` only to the primary one (so that the primary one is not
+prevented from depending on the other ones).
+
+Example:
+```python
+template("template_with_multiple_targets") {
+  action("${target_name}__helper) {
+    forward_variables_from(invoker, [ "testonly" ])
+    ...
+  }
+  action(target_name) {
+    forward_variables_from(invoker, TESTONLY_AND_VISIBILITY)
     ...
   }
 }
 ```
 
+An alternative would be to explicitly set `visibility` on all inner targets,
+but doing so tends to be tedious and has little benefit.
+
+[this bug]: https://bugs.chromium.org/p/chromium/issues/detail?id=862232
+[forward_variables_from]: https://gn.googlesource.com/gn/+/master/docs/reference.md#func_forward_variables_from
+
 ## Useful Ninja Flags
 Useful ninja flags when developing build rules:
 * `ninja -v` - log the full command-line of every target.

build/util/generate_wrapper.gni

@@ -58,13 +58,12 @@ template("generate_wrapper") {
 
   action(target_name) {
     forward_variables_from(invoker,
-                           [
-                             "data",
-                             "data_deps",
-                             "deps",
-                             "sources",
-                             "testonly",
-                           ])
+                           TESTONLY_AND_VISIBILITY + [
+                                 "data",
+                                 "data_deps",
+                                 "deps",
+                                 "sources",
+                               ])
     script = _generator_script
     if (!defined(data)) {
       data = []

testing/test.gni

@@ -41,6 +41,7 @@ if (is_android) {
 #      Similar to the GN arg 'enable_run_ios_unittests_with_xctest' but
 #      for build targets.
 template("test") {
+  testonly = true
   if (!is_ios) {
     assert(!defined(invoker.is_xctest) || !invoker.is_xctest,
            "is_xctest can be set only for iOS builds")
@@ -78,8 +79,8 @@ template("test") {
         data_deps = []
         forward_variables_from(invoker,
                                "*",
-                               _wrapper_script_vars + [ "extra_dist_files" ])
-        testonly = true
+                               TESTONLY_AND_VISIBILITY + _wrapper_script_vars +
+                                   [ "extra_dist_files" ])
 
         # Thanks to the set_defaults() for test(), configs are initialized with
         # the default shared_library configs rather than executable configs.
@@ -94,7 +95,6 @@ template("test") {
       }
 
       create_native_executable_dist(_dist_target) {
-        testonly = true
         dist_dir = "$root_out_dir/$target_name"
         binary = _exec_output
         deps = [ ":$_exec_target" ]
@@ -126,7 +126,6 @@ template("test") {
           invoker.add_unwind_tables_in_apk) {
         _unwind_table_asset_name = "${target_name}_unwind_assets"
         unwind_table_asset(_unwind_table_asset_name) {
-          testonly = true
           library_target = _library_target
           deps = [ ":$_library_target" ]
         }
@@ -136,13 +135,12 @@ template("test") {
         # Configs will always be defined since we set_defaults in BUILDCONFIG.gn.
         configs = []  # Prevent list overwriting warning.
         configs = invoker.configs
-        testonly = true
 
         deps = []
         forward_variables_from(
             invoker,
             "*",
-            _apk_specific_vars + _wrapper_script_vars + [ "visibility" ])
+            _apk_specific_vars + _wrapper_script_vars + TESTONLY_AND_VISIBILITY)
 
         if (!defined(invoker.use_default_launcher) ||
             invoker.use_default_launcher) {
@@ -198,7 +196,7 @@ template("test") {
     # directory with the same $target_name.
     # Also - bots run this script directly for "components_perftests".
     generate_wrapper(target_name) {
-      testonly = true
+      forward_variables_from(invoker, [ "visibility" ])
       executable = "$root_build_dir/bin/run_$_output_name"
       wrapper_script = "$root_build_dir/$_output_name"
       deps = [ ":$_test_runner_target" ]
@@ -221,12 +219,12 @@ template("test") {
     _exec_target = "${_output_name}__exec"
 
     fuchsia_package_runner(target_name) {
-      testonly = true
       is_test_exe = true
       forward_variables_from(invoker,
                              [
                                "use_test_server",
                                "package_deps",
+                               "visibility",
                              ])
       runner_script = "//build/fuchsia/test_runner.py"
       package = ":$_pkg_target"
@@ -236,15 +234,13 @@ template("test") {
     }
 
     cr_fuchsia_package(_pkg_target) {
-      testonly = true
       forward_variables_from(invoker, [ "manifest" ])
       binary = ":$_exec_target"
       package_name_override = _output_name
     }
 
     executable(_exec_target) {
-      forward_variables_from(invoker, "*")
-      testonly = true
+      forward_variables_from(invoker, "*", TESTONLY_AND_VISIBILITY)
       output_name = _exec_target
     }
   } else if (is_ios) {
@@ -309,7 +305,7 @@ template("test") {
         xctest_module_target = "//base/test:google_test_runner"
       }
 
-      forward_variables_from(invoker, "*", [ "testonly" ])
+      forward_variables_from(invoker, "*", TESTONLY_AND_VISIBILITY)
 
       # Provide sensible defaults in case invoker did not define any of those
       # required variables.
@@ -350,14 +346,14 @@ template("test") {
         "/" + get_label_info(target_name, "name") + ".runtime_deps"
 
     generate_runner_script(_gen_runner_target) {
-      testonly = true
       generated_script = "$root_build_dir/bin/run_" + invoker.target_name
       test_exe = invoker.target_name
       runtime_deps_file = _runtime_deps_file
     }
 
     executable(target_name) {
-      forward_variables_from(invoker, "*")
+      forward_variables_from(invoker, "*", TESTONLY_AND_VISIBILITY)
+      forward_variables_from(invoker, [ "visibility" ])
       if (!defined(deps)) {
         deps = []
       }
@@ -368,7 +364,6 @@ template("test") {
       # We use a special trigger script for CrOS hardware tests.
       data += [ "//testing/trigger_scripts/chromeos_device_trigger.py" ]
 
-      testonly = true
       output_name = target_name
       write_runtime_deps = _runtime_deps_file
       data += [ _runtime_deps_file ]
@@ -393,7 +388,6 @@ template("test") {
     _use_ash_chrome = _use_xvfb
 
     generate_wrapper(_gen_runner_target) {
-      testonly = true
       wrapper_script = "$root_build_dir/bin/run_" + _executable
 
       data = []
@@ -436,7 +430,8 @@ template("test") {
     }
 
     executable(target_name) {
-      forward_variables_from(invoker, "*")
+      forward_variables_from(invoker, "*", TESTONLY_AND_VISIBILITY)
+      forward_variables_from(invoker, [ "visibility" ])
       if (!defined(deps)) {
         deps = []
       }
@@ -445,7 +440,6 @@ template("test") {
         data_deps = []
       }
 
-      testonly = true
       write_runtime_deps = _runtime_deps_file
       deps += [ ":$_gen_runner_target" ]
       if (_use_ash_chrome && also_build_ash_chrome) {
@@ -469,7 +463,6 @@ template("test") {
     }
 
     generate_wrapper(_gen_runner_target) {
-      testonly = true
       wrapper_script = "$root_build_dir/bin/run_" + _executable
 
       data = []
@@ -502,12 +495,14 @@ template("test") {
     }
 
     executable(target_name) {
-      forward_variables_from(invoker, "*", [ "use_xvfb" ])
+      forward_variables_from(invoker,
+                             "*",
+                             TESTONLY_AND_VISIBILITY + [ "use_xvfb" ])
+      forward_variables_from(invoker, [ "visibility" ])
       if (!defined(deps)) {
         deps = []
       }
 
-      testonly = true
       deps += [
         # Give tests the default manifest on Windows (a no-op elsewhere).
         "//build/win:default_exe_manifest",
@@ -523,12 +518,11 @@ template("test") {
     assert(!defined(invoker.use_xvfb) || !invoker.use_xvfb,
            "use_xvfb should not be defined for a non-linux configuration")
     executable(target_name) {
-      forward_variables_from(invoker, "*")
+      forward_variables_from(invoker, "*", TESTONLY_AND_VISIBILITY)
+      forward_variables_from(invoker, [ "visibility" ])
       if (!defined(deps)) {
         deps = []
       }
-
-      testonly = true
     }
   }
 }
@@ -581,13 +575,13 @@ template("script_test") {
 
     forward_variables_from(invoker,
                            [ "*" ],
-                           [
-                             "data",
-                             "data_deps",
-                             "script",
-                             "script_args",
-                             "testonly",
-                           ])
+                           TESTONLY_AND_VISIBILITY + [
+                                 "data",
+                                 "data_deps",
+                                 "script",
+                                 "script_args",
+                               ])
+    forward_variables_from(invoker, [ "visibility" ])
 
     write_runtime_deps = "${root_build_dir}/${target_name}.runtime_deps"
   }

third_party/blink/renderer/controller/BUILD.gn

@@ -9,7 +9,10 @@ import("//third_party/blink/renderer/config.gni")
 import("//third_party/blink/renderer/core/core.gni")
 import("//third_party/blink/renderer/modules/modules.gni")
 
-visibility = [ "//third_party/blink/*" ]
+visibility = [
+  "//:gn_all",
+  "//third_party/blink/*",
+]
 
 component("controller") {
   output_name = "blink_controller"

third_party/blink/renderer/platform/BUILD.gn

@@ -21,6 +21,8 @@ import("//v8/gni/v8.gni")
 # Most targets in this file are private actions so use that as the default.
 visibility = [
   ":*",
+  "//:gn_all",
+  "//third_party/blink/public:all_blink",
   "//third_party/blink/renderer/platform/heap:test_support",
 ]
 

third_party/blink/renderer/platform/wtf/BUILD.gn

@@ -19,6 +19,7 @@ buildflag_header("buildflags") {
 
 visibility = [
   ":*",
+  "//:gn_all",
   "//mojo/public/cpp/bindings/*",
   "//third_party/blink/*",
 ]