pw_log: Migrate pw_log facade to use link deps like pw_assert Migrates pw_log to use pw_build_LINK_DEPS w/ require_link_deps. Updates the upstream log backends to refactor them accordingly. Also updates the targets to pull in both assert and log through pw_build_LINK_DEPS. Note this does not yet move the actual impls, which is blocked by migrating all users to use pw_build_LINK_DEPS for logging. Change-Id: I17a4647e2ce2341b8cea6ee5c11cd1227a324c83 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/44147 Reviewed-by: Armando Montanez <amontanez@google.com> Reviewed-by: Wyatt Hepler <hepler@google.com> Commit-Queue: Ewout van Bekkum <ewout@google.com>
diff --git a/pw_log/BUILD.gn b/pw_log/BUILD.gn index 913e840..d94926b 100644 --- a/pw_log/BUILD.gn +++ b/pw_log/BUILD.gn
@@ -34,6 +34,28 @@ "public/pw_log/short.h", "public/pw_log/shorter.h", ] + + # TODO(pwbug/372): Update projects to properly list pw_log:impl. + # require_link_deps = [ ":impl" ] +} + +# pw_log 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_log:impl") must listed in pw_build_LINK_DEPS if +# pw_log_BACKEND is set. +# +# pw_log 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_log_BACKEND != "") { + public_deps += + [ get_label_info(pw_log_BACKEND, "label_no_toolchain") + ".impl" ] + } } pw_test_group("tests") {
diff --git a/pw_log/docs.rst b/pw_log/docs.rst index 2a2b0a9..159d9b6 100644 --- a/pw_log/docs.rst +++ b/pw_log/docs.rst
@@ -269,6 +269,31 @@ between call site code size, call site run time, wire format size, logging complexity, and more. +.. _module-pw_log-circular-deps: + +Avoiding circular dependencies with ``PW_LOG`` +------------------------------------------------- +Because logs are so widely used, including in low-level libraries, it is +common for the ``pw_log`` backend to cause circular dependencies. Because of +this, log backends may avoid declaring explicit dependencies, instead relying +on include paths to access header files. + +In GN, the ``pw_log`` backend's full implementation with true dependencies is +made available through the ``$dir_pw_log:impl`` group. When ``pw_log_BACKEND`` +is set, ``$dir_pw_log:impl`` must be listed in the ``pw_build_LINK_DEPS`` +variable. See :ref:`module-pw_build-link-deps`. + +In the ``pw_log``, the backend's full implementation is placed in the +``$pw_log_BACKEND.impl`` target. ``$dir_pw_log:impl`` depends on this +backend target. The ``$pw_log_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_log_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``. + Design discussion -----------------
diff --git a/pw_log_basic/BUILD.gn b/pw_log_basic/BUILD.gn index 5269f09..a82331f 100644 --- a/pw_log_basic/BUILD.gn +++ b/pw_log_basic/BUILD.gn
@@ -25,7 +25,7 @@ pw_log_basic_CONFIG = pw_build_DEFAULT_MODULE_CONFIG } -config("default_config") { +config("public_include_path") { include_dirs = [ "public" ] } @@ -33,25 +33,25 @@ include_dirs = [ "public_overrides" ] } +# pw_log_basic only provides the backend's interface. The implementation is +# pulled in through pw_build_LINK_DEPS. pw_source_set("pw_log_basic") { public_configs = [ ":backend_config", - ":default_config", + ":public_include_path", ] - public = [ "public_overrides/pw_log_backend/log_backend.h" ] - public_deps = [ ":core" ] -} - -pw_source_set("core") { - public_configs = [ ":default_config" ] + public = [ + "public/pw_log_basic/log_basic.h", + "public_overrides/pw_log_backend/log_backend.h", + ] public_deps = [ dir_pw_preprocessor ] + deps = [ "$dir_pw_log:facade", dir_pw_string, dir_pw_sys_io, pw_log_basic_CONFIG, ] - public = [ "public/pw_log_basic/log_basic.h" ] # Use emoji log levels if they've been enabled. _use_emoji = getenv("PW_EMOJI") @@ -66,6 +66,13 @@ ] } +# TODO(ewout): Fill this out once all users are using link deps. +# The log backend deps that might cause circular dependencies, since +# pw_log is so ubiquitous. These deps are kept separate so they can be +# depended on from elsewhere. +pw_source_set("pw_log_basic.impl") { +} + pw_doc_group("docs") { sources = [ "docs.rst" ] }
diff --git a/pw_log_tokenized/BUILD.gn b/pw_log_tokenized/BUILD.gn index d51a510..0d3831b 100644 --- a/pw_log_tokenized/BUILD.gn +++ b/pw_log_tokenized/BUILD.gn
@@ -38,20 +38,12 @@ visibility = [ ":*" ] } -# This target provides the log_tokenized.h header, which implements the pw_log -# backend. -# -# This target depends on the pw_tokenizer facade target -# (dir_pw_tokenizer:global_handler_with_payload.facade) to avoid circular -# dependencies. The dependency graph for pw_log_tokenized is the following: -# -# pw_log:facade <--- :pw_log_tokenized -# ^ \ ^ ^ -# | \ / \ -# -> pw_log -> :log_backend --> pw_tokenizer:global_handler_with_payload -# +# This target provides the backend for pw_log. pw_source_set("pw_log_tokenized") { - public_configs = [ ":public_includes" ] + public_configs = [ + ":public_includes", + ":backend_config", + ] public_deps = [ "$dir_pw_log:facade", "$dir_pw_tokenizer:global_handler_with_payload.facade", @@ -64,7 +56,8 @@ ] } -# This target provides the backend for pw_log. +# TODO(ewout): remove this once all users are migrated off this target, using +# link deps for the log impl instead. pw_source_set("log_backend") { public_configs = [ ":backend_config" ] public_deps = [ ":pw_log_tokenized" ] @@ -72,6 +65,19 @@ deps = [ "$dir_pw_tokenizer:global_handler_with_payload" ] } +# TODO(ewout): remove this once all users are migrated off this target, using +# pw_log_tokenized and its pw_log_tokenized.impl instead. +pw_source_set("log_backend.impl") { +} + +# TODO(ewout): Fill this out once all users are using link deps. +# The log backend deps that might cause circular dependencies, since +# pw_log is so ubiquitous. These deps are kept separate so they can be +# depended on from elsewhere. +pw_source_set("pw_log_tokenized.impl") { + public_deps = [ ":log_backend" ] +} + # This target provides a backend for pw_tokenizer that encodes tokenized logs as # Base64, encodes them into HDLC frames, and writes them over sys_io. pw_source_set("base64_over_hdlc") {
diff --git a/targets/arduino/target_toolchains.gni b/targets/arduino/target_toolchains.gni index 167c185..cba6db8 100644 --- a/targets/arduino/target_toolchains.gni +++ b/targets/arduino/target_toolchains.gni
@@ -51,7 +51,10 @@ "$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:impl" ] + pw_build_LINK_DEPS = [ + "$dir_pw_assert:impl", + "$dir_pw_log:impl", + ] current_cpu = "arm" current_os = ""
diff --git a/targets/host/target_toolchains.gni b/targets/host/target_toolchains.gni index 5103a5b..240f111 100644 --- a/targets/host/target_toolchains.gni +++ b/targets/host/target_toolchains.gni
@@ -65,7 +65,10 @@ 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:impl" ] + pw_build_LINK_DEPS = [ + "$dir_pw_assert:impl", + "$dir_pw_log: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 3d2f120..b0138af 100644 --- a/targets/lm3s6965evb-qemu/target_toolchains.gni +++ b/targets/lm3s6965evb-qemu/target_toolchains.gni
@@ -61,7 +61,10 @@ "PW_BOOT_VECTOR_TABLE_SIZE=512", ] - pw_build_LINK_DEPS = [ "$dir_pw_assert:impl" ] + pw_build_LINK_DEPS = [ + "$dir_pw_assert:impl", + "$dir_pw_log:impl", + ] current_cpu = "arm" current_os = ""
diff --git a/targets/stm32f429i-disc1/target_toolchains.gni b/targets/stm32f429i-disc1/target_toolchains.gni index 6acf09c..8117109 100644 --- a/targets/stm32f429i-disc1/target_toolchains.gni +++ b/targets/stm32f429i-disc1/target_toolchains.gni
@@ -73,7 +73,10 @@ "PW_BOOT_VECTOR_TABLE_SIZE=512", ] - pw_build_LINK_DEPS = [ "$dir_pw_assert:impl" ] + pw_build_LINK_DEPS = [ + "$dir_pw_assert:impl", + "$dir_pw_log:impl", + ] current_cpu = "arm" current_os = ""