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