pw_build: pw_facade require_link_deps arg

- Make the pw_build_LINK_DEPS check in pw_assert a generic feature in
  pw_facade.
- Use "impl" instead of "deps" for the pw_assert dependencies and
  restructure the impl / backend split.

Change-Id: I75c0f7e67b3b97bfe333760897223ab4601649c0
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/43980
Pigweed-Auto-Submit: Wyatt Hepler <[email protected]>
Commit-Queue: Auto-Submit <[email protected]>
Reviewed-by: Ewout van Bekkum <[email protected]>
Reviewed-by: Armando Montanez <[email protected]>

pw_assert/BUILD.gn

@@ -60,6 +60,9 @@ pw_facade("check") {
     "public/pw_assert/short.h",
   ]
   public_deps = [ dir_pw_preprocessor ]
+
+  # TODO(pwbug/372): Update projects to properly list pw_assert:impl.
+  # require_link_deps = [ ":impl" ]
 }
 
 # Provide "pw_assert/assert.h" in its own source set, so it can be used without
@@ -87,39 +90,18 @@ pw_source_set("assert") {
 # circular dependencies. This target collects dependencies from the backend that
 # cannot be used because they would cause circular deps.
 #
-# This group ("$dir_pw_assert:deps") must listed in pw_build_LINK_DEPS if
+# This group ("$dir_pw_assert:impl") must listed in pw_build_LINK_DEPS if
 # pw_assert_BACKEND is set.
 #
-# pw_assert backends must provide their own "deps" group that collects their
-# actual dependencies. The backend "deps" group may be empty.
-group("deps") {
+# pw_assert backends must provide their own "impl" target that collects their
+# actual dependencies. The backend "impl" group may be empty if everything can
+# go directly in the backend target without causing circular dependencies.
+group("impl") {
   public_deps = []
 
   if (pw_assert_BACKEND != "") {
-    public_deps += [ get_label_info(pw_assert_BACKEND, "dir") + ":deps" ]
-
-    # Make sure this target is listed in pw_build_LINK_DEPS. This
-    # ensures these dependencies are available during linking, even if nothing
-    # else in the build depends on them.
-    _deps_label = get_label_info(":$target_name", "label_no_toolchain")
-    _deps_is_in_link_dependencies = false
-
-    foreach(label, pw_build_LINK_DEPS) {
-      if (get_label_info(label, "label_no_toolchain") == _deps_label) {
-        _deps_is_in_link_dependencies = true
-      }
-    }
-
-    # TODO(pwbug/372): Update projects with pw_assert to link the :deps target.
-    _disable_this_check_for_now = true
-    not_needed([
-                 "_deps_is_in_link_dependencies",
-                 "_deps_label",
-               ])
-
-    assert(_disable_this_check_for_now || _deps_is_in_link_dependencies,
-           "\$dir_pw_assert:$target_name must be listed in " +
-               "pw_build_LINK_DEPS when pw_assert_BACKEND is set")
+    public_deps +=
+        [ get_label_info(pw_assert_BACKEND, "label_no_toolchain") + ".impl" ]
   }
 }
 

pw_assert/docs.rst

@@ -481,17 +481,21 @@ common for the ``pw_assert`` backend to cause circular dependencies. Because of
 this, assert backends may avoid declaring explicit dependencies, instead relying
 on include paths to access header files.
 
-In GN, the ``pw_assert`` backend's true dependencies are made available through
-the ``$dir_pw_assert:deps`` group. When ``pw_assert_BACKEND`` is set,
-``$dir_pw_assert:deps`` must be listed in the ``pw_build_LINK_DEPS`` variable.
-See :ref:`module-pw_build-link-deps`.
-
-If necessary, ``pw_assert`` backends can access dependencies from include paths
-rather than GN ``deps``. In this case, the may disable GN header checking with
-``check_includes = false``. The true build dependencies must be listed in a
-``deps`` group, which the ``pw_assert`` facade depends on. The ``deps`` group
-may be empty if the backend can use its dependencies directly without causing
-circular dependencies.
+In GN, the ``pw_assert`` backend's full implementation with true dependencies is
+made available through the ``$dir_pw_assert:impl`` group. When
+``pw_assert_BACKEND`` is set, ``$dir_pw_assert:impl`` must be listed in the
+``pw_build_LINK_DEPS`` variable. See :ref:`module-pw_build-link-deps`.
+
+In the ``pw_assert``, the backend's full implementation is placed in the
+``$pw_assert_BACKEND.impl`` target. ``$dir_pw_assert:impl`` depends on this
+backend target. The ``$pw_assert_BACKEND.impl`` target may be an empty group if
+the backend target can use its dependencies directly without causing circular
+dependencies.
+
+In order to break dependency cycles, the ``pw_assert_BACKEND`` target may need
+to directly provide dependencies through include paths only, rather than GN
+``public_deps``. In this case, GN header checking can be disabled with
+``check_includes = false``.
 
 .. _module-pw_assert-backend_api:
 
@@ -582,11 +586,13 @@ header, but instead is in a ``.cc`` file.
 
 Backend build targets
 ---------------------
-In GN, the backend must provide a ``deps`` build target in the same directory as
-the backend target. The ``deps`` target contains the backend's dependencies that
-could result in a dependency cycle. In the simplest case, it can be an empty
-group. Circular dependencies are a common problem with ``pw_assert`` because it
-is so widely used. See :ref:`module-pw_assert-circular-deps`.
+In GN, the backend must provide a ``pw_assert.impl`` build target in the same
+directory as the backend target. If the main backend target's dependencies would
+cause dependency cycles, the actual backend implementation with its full
+dependencies is placed in the ``pw_assert.impl`` target. If this is not
+necessary, ``pw_assert.impl`` can be an empty group. Circular dependencies are a
+common problem with ``pw_assert`` because it is so widely used. See
+:ref:`module-pw_assert-circular-deps`.
 
 --------------------------
 Frequently asked questions

pw_assert_basic/BUILD.gn

@@ -32,7 +32,8 @@ config("backend_config") {
 # These assert backend deps that might cause circular dependencies, since
 # pw_assert is so ubiquitous. These deps are kept separate so they can be
 # depended on from elsewhere.
-group("deps") {
+# TODO(pwbug/372): Reorganize this.
+group("pw_assert_basic.impl") {
   public_deps = [
     dir_pw_string,
     dir_pw_sys_io,

pw_assert_log/BUILD.gn

@@ -46,9 +46,10 @@ pw_source_set("core") {
   sources = [ "assert_log.cc" ]
 }
 
+# TODO(pwbug/372): Reorganize this.
 # pw_assert_log doesn't have deps with potential circular dependencies, so
 # "deps" can be empty.
-group("deps") {
+group("pw_assert_log.impl") {
 }
 
 pw_doc_group("docs") {

pw_build/docs.rst

@@ -125,6 +125,14 @@ The ``pw_facade`` template declares two targets:
     public = [ "public/pw_foo/foo.h" ]
   }
 
+Low-level facades like ``pw_assert`` cannot express all of their dependencies
+due to the potential for dependency cycles. Facades with this issue may require
+backends to place their implementations in a separate build target to be listed
+in ``pw_build_LINK_DEPS`` (see :ref:`module-pw_build-link-deps`). The
+``require_link_deps`` variable in ``pw_facade`` asserts that all specified build
+targets are present in ``pw_build_LINK_DEPS`` if the facade's backend variable
+is set.
+
 .. _module-pw_build-python-action:
 
 pw_python_action

pw_build/facade.gni

@@ -72,6 +72,9 @@ import("$dir_pw_build/target_types.gni")
 #    include headers from the facade itself. If the facade doesn't expose any
 #    headers, it's basically the same as just depending directly on the build
 #    arg that `backend` is set to.
+#  require_link_deps: A list of build targets that must be included in
+#    pw_build_LINK_DEPS if this facade is used. This mechanism is used to
+#    avoid circular dependencies in low-level facades like pw_assert.
 #
 template("pw_facade") {
   assert(defined(invoker.backend),
@@ -99,7 +102,7 @@ template("pw_facade") {
     "public",
   ]
   pw_source_set(_facade_name) {
-    forward_variables_from(invoker, _facade_vars)
+    forward_variables_from(invoker, _facade_vars, [ "require_link_deps" ])
   }
 
   if (invoker.backend == "") {
@@ -140,7 +143,10 @@ template("pw_facade") {
   # correctly expressed for gn check.
   pw_source_set(target_name) {
     # The main library contains everything else specified in the template.
-    _ignore_vars = [ "backend" ] + _facade_vars
+    _ignore_vars = _facade_vars + [
+                     "backend",
+                     "require_link_deps",
+                   ]
     forward_variables_from(invoker, "*", _ignore_vars)
 
     public_deps = [ ":$_facade_name" ]
@@ -153,4 +159,28 @@ template("pw_facade") {
       public_deps += [ ":$target_name.NO_BACKEND_SET" ]
     }
   }
+
+  if (defined(invoker.require_link_deps) && invoker.backend != "") {
+    # Check that the specified labels are listed in pw_build_LINK_DEPS. This
+    # ensures these dependencies are available during linking, even if nothing
+    # else in the build depends on them.
+    foreach(label, invoker.require_link_deps) {
+      _required_dep = get_label_info(label, "label_no_toolchain")
+      _dep_is_in_link_dependencies = false
+
+      foreach(link_dep, pw_build_LINK_DEPS) {
+        if (get_label_info(link_dep, "label_no_toolchain") == _required_dep) {
+          _dep_is_in_link_dependencies = true
+        }
+      }
+
+      _facade_name = get_label_info(":$target_name", "label_no_toolchain")
+      assert(_dep_is_in_link_dependencies,
+             "$_required_dep must be listed in the pw_build_LINK_DEPS build " +
+                 "arg when the $_facade_name facade is in use. Please update " +
+                 "your toolchain configuration.")
+    }
+  } else {
+    not_needed(invoker, [ "require_link_deps" ])
+  }
 }

targets/arduino/target_toolchains.gni

@@ -51,7 +51,7 @@ _target_config = {
       "$dir_pigweed/targets/arduino:system_rpc_server"
   pw_arduino_build_INIT_BACKEND = "$dir_pigweed/targets/arduino:pre_init"
 
-  pw_build_LINK_DEPS = [ "$dir_pw_assert:deps" ]
+  pw_build_LINK_DEPS = [ "$dir_pw_assert:impl" ]
 
   current_cpu = "arm"
   current_os = ""

targets/host/target_toolchains.gni

@@ -65,7 +65,7 @@ _host_common = {
   pw_thread_THREAD_BACKEND = "$dir_pw_thread_stl:thread"
 
   pw_build_LINK_DEPS = []  # Explicit list overwrite required by GN
-  pw_build_LINK_DEPS = [ "$dir_pw_assert:deps" ]
+  pw_build_LINK_DEPS = [ "$dir_pw_assert:impl" ]
 
   # Specify builtin GN variables.
   current_os = host_os

targets/lm3s6965evb-qemu/target_toolchains.gni

@@ -61,7 +61,7 @@ _target_config = {
     "PW_BOOT_VECTOR_TABLE_SIZE=512",
   ]
 
-  pw_build_LINK_DEPS = [ "$dir_pw_assert:deps" ]
+  pw_build_LINK_DEPS = [ "$dir_pw_assert:impl" ]
 
   current_cpu = "arm"
   current_os = ""

targets/stm32f429i-disc1/target_toolchains.gni

@@ -73,7 +73,7 @@ _target_config = {
     "PW_BOOT_VECTOR_TABLE_SIZE=512",
   ]
 
-  pw_build_LINK_DEPS = [ "$dir_pw_assert:deps" ]
+  pw_build_LINK_DEPS = [ "$dir_pw_assert:impl" ]
 
   current_cpu = "arm"
   current_os = ""