diff options
| -rw-r--r-- | pw_assert/BUILD.gn | 40 | ||||
| -rw-r--r-- | pw_assert/docs.rst | 36 | ||||
| -rw-r--r-- | pw_assert_basic/BUILD.gn | 30 | ||||
| -rw-r--r-- | pw_assert_log/BUILD.gn | 5 | ||||
| -rw-r--r-- | pw_build/BUILD.gn | 13 | ||||
| -rw-r--r-- | pw_build/docs.rst | 15 | ||||
| -rw-r--r-- | pw_build/target_types.gni | 23 | ||||
| -rw-r--r-- | pw_string/BUILD.gn | 4 | ||||
| -rw-r--r-- | pw_sys_io/BUILD.gn | 4 | ||||
| -rw-r--r-- | targets/arduino/target_toolchains.gni | 2 | ||||
| -rw-r--r-- | targets/host/target_toolchains.gni | 3 | ||||
| -rw-r--r-- | targets/lm3s6965evb-qemu/target_toolchains.gni | 2 | ||||
| -rw-r--r-- | targets/stm32f429i-disc1/target_toolchains.gni | 2 |
13 files changed, 149 insertions, 30 deletions
diff --git a/pw_assert/BUILD.gn b/pw_assert/BUILD.gn index 2b78c00a3..0362da628 100644 --- a/pw_assert/BUILD.gn +++ b/pw_assert/BUILD.gn @@ -83,6 +83,46 @@ pw_source_set("assert") { public_deps = [ dir_pw_preprocessor ] } +# pw_assert is low-level and ubiquitous. Because of this, it can often cause +# 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 +# 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") { + 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") + } +} + # Note: While this is technically a test, doesn't verify any of the output and # is more of a compile test. The results can be visually verified if desired. pw_test("assert_test") { diff --git a/pw_assert/docs.rst b/pw_assert/docs.rst index 6a6576ee5..a66c3f7e1 100644 --- a/pw_assert/docs.rst +++ b/pw_assert/docs.rst @@ -472,20 +472,26 @@ The ``PW_ASSERT`` API ultimately calls the C function ``pw_assert_HandleFailure()``, which must be provided by the ``pw_assert`` backend. +.. _module-pw_assert-circular-deps: + Avoiding circular dependencies with ``PW_ASSERT`` ------------------------------------------------- Because asserts are so widely used, including in low-level libraries, it is -common for the ``pw_assert`` backend to cause circular dependencies. These can -be avoided by only using the ``PW_ASSERT`` macro and depending directly on the -library that provides it, rather than the whole ``pw_assert`` module. The -``pw_assert`` backend, which defines ``pw_assert_HandleFailure()``, will need to -be linked in elsewhere in the build, so only depend directly on the -``pw_assert`` build target when necessary. - -In GN, depend on ``"$dir_pw_assert:assert"`` rather than on ``dir_pw_assert`` to -access only ``PW_ASSERT`` without risking circular dependencies. The -``pw_assert_HandleFailure()`` function is provided by the -``$dir_pw_assert:pw_assert`` or ``"$dir_pw_assert:check"`` target. +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. .. _module-pw_assert-backend_api: @@ -574,6 +580,14 @@ header, but instead is in a ``.cc`` file. file, expression, or other rich assert information. Backends should do something reasonable in this case; typically, capturing the stack is useful. +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`. + -------------------------- Frequently asked questions -------------------------- diff --git a/pw_assert_basic/BUILD.gn b/pw_assert_basic/BUILD.gn index a8682f171..c2e48d14f 100644 --- a/pw_assert_basic/BUILD.gn +++ b/pw_assert_basic/BUILD.gn @@ -19,17 +19,29 @@ import("$dir_pw_build/target_types.gni") import("$dir_pw_docgen/docs.gni") import("backend.gni") -config("default_config") { +config("public_include_path") { include_dirs = [ "public" ] + visibility = [ ":*" ] } config("backend_config") { include_dirs = [ "public_overrides" ] + visibility = [ ":*" ] +} + +# 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") { + public_deps = [ + dir_pw_string, + dir_pw_sys_io, + ] } pw_facade("handler") { backend = pw_assert_basic_HANDLER_BACKEND - public_configs = [ ":default_config" ] + public_configs = [ ":public_include_path" ] public_deps = [ "$dir_pw_preprocessor" ] public = [ "public/pw_assert_basic/handler.h" ] } @@ -37,7 +49,7 @@ pw_facade("handler") { pw_source_set("pw_assert_basic") { public_configs = [ ":backend_config", - ":default_config", + ":public_include_path", ] deps = [ "$dir_pw_assert:facade", @@ -52,14 +64,20 @@ pw_source_set("pw_assert_basic") { sources = [ "assert_basic.cc" ] } -# A basic handler backend using sysio. +# A basic handler backend using pw_sys_io. pw_source_set("basic_handler") { + # Turn off GN check since this target intentionally leaves out deps to avoid + # circular dependencies. + check_includes = false + + # Depend on the include path instead of the library to avoid circular deps. + configs = [ "$dir_pw_string:public_include_path" ] deps = [ ":handler.facade", "$dir_pw_assert:facade", "$dir_pw_preprocessor", - "$dir_pw_string", - "$dir_pw_sys_io", + "$dir_pw_sys_io:facade", # Only pull in the facade to avoid circular deps + dir_pw_status, ] sources = [ "basic_handler.cc" ] } diff --git a/pw_assert_log/BUILD.gn b/pw_assert_log/BUILD.gn index 083c5e8e4..f6726b078 100644 --- a/pw_assert_log/BUILD.gn +++ b/pw_assert_log/BUILD.gn @@ -46,6 +46,11 @@ pw_source_set("core") { sources = [ "assert_log.cc" ] } +# pw_assert_log doesn't have deps with potential circular dependencies, so +# "deps" can be empty. +group("deps") { +} + pw_doc_group("docs") { sources = [ "docs.rst" ] } diff --git a/pw_build/BUILD.gn b/pw_build/BUILD.gn index 706b8d6e9..bb8361717 100644 --- a/pw_build/BUILD.gn +++ b/pw_build/BUILD.gn @@ -16,6 +16,7 @@ import("//build_overrides/pigweed.gni") import("$dir_pw_build/python.gni") import("$dir_pw_docgen/docs.gni") +import("target_types.gni") # IMPORTANT: The compilation flags in this file must be kept in sync with # the CMake flags pw_build/CMakeLists.txt. @@ -121,6 +122,18 @@ config("cpp17") { ] } +# This group is linked into all pw_executable, pw_static_library, and +# pw_shared_library targets. This makes it possible to ensure symbols are +# defined without a dependency on them. +# +# pw_build_LINK_DEPS should only be used when necessary. For example, +# pw_assert relies on pw_build_LINK_DEPS to avoid circular dependencies +# in GN. In almost all other cases, build targets should explicitly depend on +# the other targets they use. +group("link_deps") { + deps = pw_build_LINK_DEPS +} + # This empty target is used as the default value for module configurations. # Projects may set pw_build_DEFAULT_MODULE_CONFIG to a different GN target that # overrides modules' configuration options via macro definitions or a header diff --git a/pw_build/docs.rst b/pw_build/docs.rst index 61357556b..ad43bffd7 100644 --- a/pw_build/docs.rst +++ b/pw_build/docs.rst @@ -72,6 +72,21 @@ template for a project. All of the ``pw_*`` target type overrides accept any arguments, as they simply forward them through to the underlying target. +.. _module-pw_build-link-deps: + +Link-only deps +-------------- +It may be necessary to specify additional link-time dependencies that may not be +explicitly depended on elsewhere in the build. One example of this is a +``pw_assert`` backend, which may need to leave out dependencies to avoid +circular dependencies. Its dependencies need to be linked for executables and +libraries, even if they aren't pulled in elsewhere. + +The ``pw_build_LINK_DEPS`` build arg is a list of dependencies to add to all +``pw_executable``, ``pw_static_library``, and ``pw_shared_library`` targets. +This should only be used as a last resort when dependencies cannot be properly +expressed in the build. + Python packages --------------- GN templates for :ref:`Python build automation <docs-python-build>` are diff --git a/pw_build/target_types.gni b/pw_build/target_types.gni index 7590f2d0d..fb1709e28 100644 --- a/pw_build/target_types.gni +++ b/pw_build/target_types.gni @@ -27,6 +27,14 @@ declare_args() { # If pw_build_EXECUTABLE_TARGET_TYPE is not the default of `executable`, this # .gni file is imported to provide the template definition. pw_build_EXECUTABLE_TARGET_TYPE_FILE = "" + + # Additional build targets to add as dependencies for pw_executable, + # pw_static_library, and pw_shared_library targets. The + # $dir_pw_build:link_deps target pulls in these libraries. + # + # pw_build_LINK_DEPS can be used to break circular dependencies for low-level + # libraries such as pw_assert. + pw_build_LINK_DEPS = [] } if (pw_build_EXECUTABLE_TARGET_TYPE != "executable" && @@ -112,10 +120,9 @@ template("pw_static_library") { configs += invoker.configs } + public_deps = [ "$dir_pw_build:link_deps" ] if (defined(pw_build_defaults.public_deps)) { - public_deps = pw_build_defaults.public_deps - } else { - public_deps = [] + public_deps += pw_build_defaults.public_deps } if (defined(remove_public_deps)) { if (remove_public_deps != [] && remove_public_deps[0] == "*") { @@ -161,10 +168,9 @@ template("pw_shared_library") { configs += invoker.configs } + public_deps = [ "$dir_pw_build:link_deps" ] if (defined(pw_build_defaults.public_deps)) { - public_deps = pw_build_defaults.public_deps - } else { - public_deps = [] + public_deps += pw_build_defaults.public_deps } if (defined(remove_public_deps)) { if (remove_public_deps != [] && remove_public_deps[0] == "*") { @@ -213,10 +219,9 @@ template("pw_executable") { configs += invoker.configs } + public_deps = [ "$dir_pw_build:link_deps" ] if (defined(pw_build_defaults.public_deps)) { - public_deps = pw_build_defaults.public_deps - } else { - public_deps = [] + public_deps += pw_build_defaults.public_deps } if (defined(remove_public_deps)) { if (remove_public_deps != [] && remove_public_deps[0] == "*") { diff --git a/pw_string/BUILD.gn b/pw_string/BUILD.gn index bf59ff28e..5aa0134d7 100644 --- a/pw_string/BUILD.gn +++ b/pw_string/BUILD.gn @@ -19,12 +19,12 @@ import("$dir_pw_build/target_types.gni") import("$dir_pw_docgen/docs.gni") import("$dir_pw_unit_test/test.gni") -config("default_config") { +config("public_include_path") { include_dirs = [ "public" ] } pw_source_set("pw_string") { - public_configs = [ ":default_config" ] + public_configs = [ ":public_include_path" ] public = [ "public/pw_string/format.h", "public/pw_string/string_builder.h", diff --git a/pw_sys_io/BUILD.gn b/pw_sys_io/BUILD.gn index 47feb4377..a15e99907 100644 --- a/pw_sys_io/BUILD.gn +++ b/pw_sys_io/BUILD.gn @@ -19,13 +19,13 @@ import("$dir_pw_build/target_types.gni") import("$dir_pw_docgen/docs.gni") import("backend.gni") -config("default_config") { +config("public_include_path") { include_dirs = [ "public" ] } pw_facade("pw_sys_io") { backend = pw_sys_io_BACKEND - public_configs = [ ":default_config" ] + public_configs = [ ":public_include_path" ] public_deps = [ dir_pw_status ] public = [ "public/pw_sys_io/sys_io.h" ] } diff --git a/targets/arduino/target_toolchains.gni b/targets/arduino/target_toolchains.gni index 469a95f70..0ee832224 100644 --- a/targets/arduino/target_toolchains.gni +++ b/targets/arduino/target_toolchains.gni @@ -51,6 +51,8 @@ _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" ] + current_cpu = "arm" current_os = "" } diff --git a/targets/host/target_toolchains.gni b/targets/host/target_toolchains.gni index 8980c4549..f0fa57eed 100644 --- a/targets/host/target_toolchains.gni +++ b/targets/host/target_toolchains.gni @@ -64,6 +64,9 @@ _host_common = { pw_thread_YIELD_BACKEND = "$dir_pw_thread_stl:yield" 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" ] + # Specify builtin GN variables. current_os = host_os current_cpu = host_cpu diff --git a/targets/lm3s6965evb-qemu/target_toolchains.gni b/targets/lm3s6965evb-qemu/target_toolchains.gni index c3fba371b..dc18b3f23 100644 --- a/targets/lm3s6965evb-qemu/target_toolchains.gni +++ b/targets/lm3s6965evb-qemu/target_toolchains.gni @@ -61,6 +61,8 @@ _target_config = { "PW_BOOT_VECTOR_TABLE_SIZE=512", ] + pw_build_LINK_DEPS = [ "$dir_pw_assert:deps" ] + current_cpu = "arm" current_os = "" } diff --git a/targets/stm32f429i-disc1/target_toolchains.gni b/targets/stm32f429i-disc1/target_toolchains.gni index e87245e3e..6c690b86d 100644 --- a/targets/stm32f429i-disc1/target_toolchains.gni +++ b/targets/stm32f429i-disc1/target_toolchains.gni @@ -73,6 +73,8 @@ _target_config = { "PW_BOOT_VECTOR_TABLE_SIZE=512", ] + pw_build_LINK_DEPS = [ "$dir_pw_assert:deps" ] + current_cpu = "arm" current_os = "" } |