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 <hepler@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
Reviewed-by: Ewout van Bekkum <ewout@google.com>
Reviewed-by: Armando Montanez <amontanez@google.com>
diff --git a/pw_assert/BUILD.gn b/pw_assert/BUILD.gn
index 0362da6..7196831 100644
--- a/pw_assert/BUILD.gn
+++ b/pw_assert/BUILD.gn
@@ -60,6 +60,9 @@
"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 @@
# 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" ]
}
}
diff --git a/pw_assert/docs.rst b/pw_assert/docs.rst
index cd36d07..9a03d64 100644
--- a/pw_assert/docs.rst
+++ b/pw_assert/docs.rst
@@ -481,17 +481,21 @@
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`.
+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`.
-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 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 @@
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
diff --git a/pw_assert_basic/BUILD.gn b/pw_assert_basic/BUILD.gn
index c2e48d1..34125d9 100644
--- a/pw_assert_basic/BUILD.gn
+++ b/pw_assert_basic/BUILD.gn
@@ -32,7 +32,8 @@
# 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,
diff --git a/pw_assert_log/BUILD.gn b/pw_assert_log/BUILD.gn
index f6726b0..46ce4c8 100644
--- a/pw_assert_log/BUILD.gn
+++ b/pw_assert_log/BUILD.gn
@@ -46,9 +46,10 @@
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") {
diff --git a/pw_build/docs.rst b/pw_build/docs.rst
index ad43bff..a5d38da 100644
--- a/pw_build/docs.rst
+++ b/pw_build/docs.rst
@@ -125,6 +125,14 @@
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
diff --git a/pw_build/facade.gni b/pw_build/facade.gni
index 4cb422e..0b188a1 100644
--- a/pw_build/facade.gni
+++ b/pw_build/facade.gni
@@ -72,6 +72,9 @@
# 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 @@
"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 @@
# 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 @@
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" ])
+ }
}
diff --git a/targets/arduino/target_toolchains.gni b/targets/arduino/target_toolchains.gni
index 0ee8322..167c185 100644
--- a/targets/arduino/target_toolchains.gni
+++ b/targets/arduino/target_toolchains.gni
@@ -51,7 +51,7 @@
"$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 = ""
diff --git a/targets/host/target_toolchains.gni b/targets/host/target_toolchains.gni
index f0fa57e..5103a5b 100644
--- a/targets/host/target_toolchains.gni
+++ b/targets/host/target_toolchains.gni
@@ -65,7 +65,7 @@
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
diff --git a/targets/lm3s6965evb-qemu/target_toolchains.gni b/targets/lm3s6965evb-qemu/target_toolchains.gni
index dc18b3f..3d2f120 100644
--- a/targets/lm3s6965evb-qemu/target_toolchains.gni
+++ b/targets/lm3s6965evb-qemu/target_toolchains.gni
@@ -61,7 +61,7 @@
"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 = ""
diff --git a/targets/stm32f429i-disc1/target_toolchains.gni b/targets/stm32f429i-disc1/target_toolchains.gni
index 6c690b8..6acf09c 100644
--- a/targets/stm32f429i-disc1/target_toolchains.gni
+++ b/targets/stm32f429i-disc1/target_toolchains.gni
@@ -73,7 +73,7 @@
"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 = ""
