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 = ""