[nrf noup] cmake: Kconfig: strict mode

fail building uppon ineffective Kconfigs

Signed-off-by: Mateusz Michalek <mateusz.michalek@nordicsemi.no>

Author: michalek-no

cmake/modules/kconfig.cmake

@@ -397,6 +397,49 @@ if(DEFINED FORCED_CONF_FILE)
   list(APPEND input_configs_flags --forced-input-configs)
 endif()
 
+# Strict Kconfig checking: an assignment that Kconfig ends up discarding is an
+# error rather than a warning, but only for the configuration the application
+# itself owns. That is the fragments placed directly in the application
+# configuration directory (prj.conf and any overlay next to it) plus the symbols
+# given on the CMake command line as -DCONFIG_<symbol>=<value>. Both are written
+# for one specific build, so an assignment that does not take effect there is a
+# bug in the application.
+#
+# Everything else keeps warning only: board and SoC defconfigs, the application
+# boards/ and socs/ subdirectories, shields, snippets, fragments coming from
+# modules, and fragments a parent image passes to a child image. Those are
+# shared between several build targets, where the same assignment can be both
+# meaningful for one target and discarded for another.
+#
+# Script mode (package_helper.cmake) is exempt as well. It is used to resolve
+# the configuration without building it, most notably by twister to evaluate a
+# testsuite 'filter:' expression on every platform. A test that is about to be
+# filtered out is expected to have assignments that do not take effect there,
+# and the platforms that survive the filter still get the strict checks from
+# the real build that follows.
+zephyr_get(KCONFIG_STRICT SYSBUILD GLOBAL)
+if(NOT DEFINED KCONFIG_STRICT)
+  # Note: set_ifndef() is not usable here, as it overwrites a variable that is
+  # defined but false, which is exactly how the feature is turned off.
+  set(KCONFIG_STRICT ON)
+endif()
+
+set(kconfig_strict_args)
+if(KCONFIG_STRICT AND "--handwritten-input-configs" IN_LIST input_configs_flags
+   AND NOT DEFINED CMAKE_SCRIPT_MODE_FILE)
+  list(APPEND kconfig_strict_args --strict-flip-checks)
+
+  set(kconfig_strict_dirs ${APPLICATION_CONFIG_DIR} ${APPLICATION_SOURCE_DIR})
+  list(REMOVE_DUPLICATES kconfig_strict_dirs)
+  foreach(dir ${kconfig_strict_dirs})
+    list(APPEND kconfig_strict_args --strict-scope-dir ${dir})
+  endforeach()
+
+  if(EXTRA_KCONFIG_OPTIONS_FILE)
+    list(APPEND kconfig_strict_args --strict-scope-file ${EXTRA_KCONFIG_OPTIONS_FILE})
+  endif()
+endif()
+
 cmake_path(GET AUTOCONF_H PARENT_PATH autoconf_h_path)
 if(NOT EXISTS ${autoconf_h_path})
   file(MAKE_DIRECTORY ${autoconf_h_path})
@@ -410,6 +453,7 @@ execute_process(
   ${ZEPHYR_BASE}/scripts/kconfig/kconfig.py
   --zephyr-base=${ZEPHYR_BASE}
   ${input_configs_flags}
+  ${kconfig_strict_args}
   ${KCONFIG_ROOT}
   ${DOTCONFIG}
   ${AUTOCONF_H}

doc/build/kconfig/setting.rst

@@ -111,10 +111,11 @@ Comments use a #:
    # This is a comment
 
 Assignments in configuration files are only respected if the dependencies for
-the symbol are satisfied. A warning is printed otherwise. To figure out what
-the dependencies of a symbol are, use one of the :ref:`interactive
-configuration interfaces <menuconfig>` (you can jump directly to a symbol with
-:kbd:`/`), or look up the symbol in the Kconfig search page.
+the symbol are satisfied. A warning is printed otherwise, or an error for the
+fragments covered by :ref:`kconfig_strict`. To figure out what the dependencies
+of a symbol are, use one of the :ref:`interactive configuration interfaces
+<menuconfig>` (you can jump directly to a symbol with :kbd:`/`), or look up the
+symbol in the Kconfig search page.
 
 
 .. _initial-conf:
@@ -409,6 +410,83 @@ the choice might not be satisfied. In that case, you're setting the default
 selection whenever the user makes the choice visible.
 
 
+.. _kconfig_strict:
+
+Strict checking of ineffective assignments
+==========================================
+
+An assignment that Kconfig cannot honor, because the symbol's dependencies are
+not satisfied or because a ``select`` from another symbol wins, has no effect on
+the build. The value that ends up in :file:`zephyr/.config` is not the one the
+fragment asked for. Such an assignment is usually a leftover from an earlier
+configuration, or a misunderstanding of what the symbol depends on.
+
+Strict checking turns those ineffective assignments into build errors instead of
+warnings. It is enabled by default and only applies to the configuration the
+application itself owns:
+
+* Fragments placed directly in the :ref:`application configuration directory
+  <application-configuration-directory>`, such as :file:`prj.conf` and any
+  overlay fragment next to it.
+
+* Symbols given on the CMake command line as ``-DCONFIG_<symbol>=<value>``.
+
+Both are written for one specific build, so an assignment that does not take
+effect there is a bug in the application.
+
+Every other fragment keeps warning only, because it is shared between several
+build targets and the same assignment can be meaningful for one target and
+discarded for another:
+
+* Board and SoC :file:`*_defconfig` files, and the application
+  :file:`boards/` and :file:`socs/` subdirectories.
+
+* Shield and snippet fragments.
+
+* Fragments coming from a Zephyr module.
+
+* Fragments that a parent image passes to a child image in a
+  :ref:`sysbuild <sysbuild>` build. Note that for a child image, the
+  application configuration directory is
+  :file:`<parent app>/sysbuild/<image>/`, so :file:`prj.conf` there is
+  covered while :file:`<parent app>/sysbuild/<image>/boards/` is not.
+
+When several fragments assign the same symbol, only the fragment that
+established the value Kconfig actually used is reported. A board fragment that
+overrides :file:`prj.conf` therefore takes over responsibility for the symbol,
+and the :file:`prj.conf` assignment is no longer flagged.
+
+The checks are also skipped when Kconfig runs under
+:file:`cmake/package_helper.cmake`, which resolves the configuration without
+building it. Twister uses it to evaluate a testsuite ``filter:`` expression on
+every platform, including the ones the test is about to be filtered out on,
+where assignments are not expected to take effect. The platforms that survive
+the filter are checked by the real build that follows.
+
+The error names the fragment and the line that made the assignment, and all
+findings for an image are reported together:
+
+.. code-block:: none
+
+   error: /path/to/app/prj.conf:12: LOG_BUFFER_SIZE (defined at
+   subsys/logging/Kconfig.processing:38) was assigned the value '2048' but got the
+   value ''. Check these unsatisfied dependencies: (LOG_MODE_DEFERRED) (=n). ...
+
+Fix such an error by assigning a value that Kconfig can honor, by satisfying the
+missing dependencies, or by dropping the assignment. If the assignment is
+intentionally target-dependent, move it to a fragment that is specific to the
+targets it applies to, such as :file:`boards/<board>.conf`.
+
+To downgrade the errors back to warnings for a build, set
+``KCONFIG_STRICT`` to a false value:
+
+.. code-block:: console
+
+   west build -b <board> <app> -- -DKCONFIG_STRICT=n
+
+In a :ref:`sysbuild <sysbuild>` build, this applies to all images.
+
+
 More Kconfig resources
 ======================
 

samples/subsys/mgmt/mcumgr/smp_svr/prj_no_bootloader.conf

@@ -0,0 +1,49 @@
+# Variant of prj.conf for the scenarios that build without a bootloader.
+#
+# prj.conf turns on image management, which only makes sense when MCUboot owns a
+# second slot. Without a bootloader IMG_MANAGER stays off, and with it both
+# MCUMGR_GRP_IMG and the whole MCUBOOT_UTIL log level choice become unreachable,
+# so those assignments could never take effect here. The rest is identical to
+# prj.conf.
+
+# Enable MCUmgr and dependencies.
+CONFIG_NET_BUF=y
+CONFIG_ZCBOR=y
+CONFIG_CRC=y
+CONFIG_MCUMGR=y
+CONFIG_STREAM_FLASH=y
+CONFIG_FLASH_MAP=y
+
+# Some command handlers require a large stack.
+CONFIG_SYSTEM_WORKQUEUE_STACK_SIZE=2304
+CONFIG_MAIN_STACK_SIZE=2176
+
+# Enable flash operations.
+CONFIG_FLASH=y
+
+# Required by the `taskstat` command.
+CONFIG_THREAD_MONITOR=y
+
+# Support for taskstat command
+CONFIG_MCUMGR_GRP_OS_TASKSTAT=y
+
+# Enable statistics and statistic names.
+CONFIG_STATS=y
+CONFIG_STATS_NAMES=y
+
+# Enable most core commands.
+CONFIG_FLASH=y
+# Ineffective without a bootloader: MCUMGR_GRP_IMG depends on IMG_MANAGER.
+# CONFIG_IMG_MANAGER=y
+# CONFIG_MCUMGR_GRP_IMG=y
+CONFIG_MCUMGR_GRP_OS=y
+CONFIG_MCUMGR_GRP_STAT=y
+
+# Enable logging
+CONFIG_LOG=y
+# Ineffective without a bootloader: the MCUBOOT_UTIL log level choice only
+# exists once IMG_MANAGER pulls in MCUBOOT_BOOTUTIL_LIB.
+# CONFIG_MCUBOOT_UTIL_LOG_LEVEL_WRN=y
+
+# Disable debug logging
+CONFIG_LOG_MAX_LEVEL=3

samples/subsys/mgmt/mcumgr/smp_svr/sample.yaml

@@ -37,11 +37,13 @@ tests:
       - frdm_k64f
     integration_platforms:
       - frdm_k64f
+  # Uses prj_no_bootloader.conf instead of prj.conf: this is the only scenario
+  # built without MCUboot, so the image-management settings prj.conf carries for
+  # every other scenario cannot take effect here.
   sample.mcumgr.smp_svr.udp-dtls:
     extra_args:
+      - FILE_SUFFIX="no_bootloader"
       - EXTRA_CONF_FILE="udp-dtls.conf"
-      - CONFIG_IMG_MANAGER=n
-      - SB_CONFIG_BOOTLOADER_NONE=y
     platform_allow:
       - native_sim
     integration_platforms:

samples/subsys/mgmt/mcumgr/smp_svr/sysbuild_no_bootloader.conf

@@ -0,0 +1,2 @@
+# Companion of prj_no_bootloader.conf: build the application on its own.
+SB_CONFIG_BOOTLOADER_NONE=y

scripts/kconfig/kconfig.py

@@ -46,6 +46,9 @@ def main():
     if args.zephyr_base:
         os.environ['ZEPHYR_BASE'] = args.zephyr_base
 
+    strict_scope = StrictScope(args.strict_scope_dir, args.strict_scope_file) \
+        if args.strict_flip_checks else None
+
     print("Parsing " + args.kconfig_file)
     kconf = Kconfig(args.kconfig_file, warn_to_stderr=False,
                     suppress_traceback=True)
@@ -88,8 +91,8 @@ def main():
         # Print warnings for symbols that didn't get the assigned value. Only
         # do this for handwritten input too, to avoid likely unhelpful warnings
         # when using an old configuration and updating Kconfig files.
-        check_assigned_sym_values(kconf)
-        check_assigned_choice_values(kconf)
+        check_assigned_sym_values(kconf, strict_scope)
+        check_assigned_choice_values(kconf, strict_scope)
 
     if kconf.syms.get('WARN_DEPRECATED', kconf.y).tri_value == 2:
         check_deprecated(kconf)
@@ -132,6 +135,9 @@ def main():
         if error_out:
             err("Aborting due to Kconfig warnings")
 
+    if strict_scope:
+        strict_scope.report()
+
     # Write the merged configuration and the C header
     print(kconf.write_config(args.config_out))
     print(kconf.write_autoconf(args.header_out))
@@ -158,7 +164,68 @@ def check_no_promptless_assign(kconf):
 symbols. """ + SYM_INFO_HINT.format(sym))
 
 
-def check_assigned_sym_values(kconf):
+class StrictScope:
+    # Decides which configuration fragments an ineffective assignment is an
+    # error for, rather than just a warning.
+    #
+    # A fragment is in scope when it sits directly in one of 'dirs' (files in
+    # subdirectories such as boards/ or socs/ are not in scope), or when it is
+    # listed in 'files'. The application owns those fragments and knows the
+    # target they are built for, so an assignment that Kconfig discards there
+    # is a bug. Everything else - board and SoC defconfigs, shield, snippet and
+    # module fragments, fragments belonging to another image - is shared
+    # between targets and cannot be expected to "take" everywhere, so those
+    # keep warning only.
+
+    def __init__(self, dirs, files):
+        self.dirs = {os.path.realpath(d) for d in dirs}
+        self.files = {os.path.realpath(f) for f in files}
+        self.errors = []
+
+    def covers(self, loc):
+        if loc is None:
+            return False
+
+        path = os.path.realpath(loc[0])
+        return path in self.files or os.path.dirname(path) in self.dirs
+
+    def add(self, loc, msg):
+        # Records 'msg' as an error and returns True if 'loc' is in scope.
+        # Returns False otherwise, leaving it to the caller to warn instead.
+
+        if not self.covers(loc):
+            return False
+
+        self.errors.append(f"{loc[0]}:{loc[1]}: {msg}")
+        return True
+
+    def report(self):
+        # Prints every recorded error and aborts if there were any. All of them
+        # are reported at once so that a single build lists everything that
+        # needs fixing.
+
+        if not self.errors:
+            return
+
+        for msg in self.errors:
+            print("\n" + textwrap.fill("error: " + msg, 100), file=sys.stderr)
+
+        err(f"aborting due to {len(self.errors)} ineffective Kconfig "
+            "assignment(s) in application-owned configuration. Assign a value "
+            "that Kconfig can honor, satisfy the missing dependencies, or drop "
+            "the assignment. Building with -DKCONFIG_STRICT=n downgrades this "
+            "to a warning.")
+
+
+def report_ineffective(strict_scope, loc, msg):
+    # Turns an ineffective assignment into an error when it comes from a
+    # fragment in the strict scope, and into a warning otherwise.
+
+    if strict_scope is None or not strict_scope.add(loc, msg):
+        warn(msg)
+
+
+def check_assigned_sym_values(kconf, strict_scope=None):
     # Verifies that the values assigned to symbols "took" (matches the value
     # the symbols actually got), printing warnings otherwise. Choice symbols
     # are checked separately, in check_assigned_choice_values().
@@ -197,7 +264,8 @@ def check_assigned_sym_values(kconf):
                 msg += "Check these unsatisfied dependencies: " + \
                     ", ".join(expr_strs) + ". "
 
-            warn(msg + SYM_INFO_HINT.format(sym))
+            report_ineffective(strict_scope, sym.user_loc,
+                               msg + SYM_INFO_HINT.format(sym))
 
 
 def missing_deps(sym):
@@ -225,7 +293,7 @@ def missing_deps(sym):
     return [dep for dep in deps if expr_value(dep) == 0]
 
 
-def check_assigned_choice_values(kconf):
+def check_assigned_choice_values(kconf, strict_scope=None):
     # Verifies that any choice symbols that were selected (by setting them to
     # y) ended up as the selection, printing warnings otherwise.
     #
@@ -242,7 +310,7 @@ def check_assigned_choice_values(kconf):
         if choice.user_selection and \
            choice.user_selection is not choice.selection:
 
-            warn(f"""\
+            report_ineffective(strict_scope, choice.user_selection.user_loc, f"""\
 The choice symbol {choice.user_selection.name_and_loc} was selected (set =y),
 but {choice.selection.name_and_loc if choice.selection else "no symbol"} ended
 up as the choice selection. """ + SYM_INFO_HINT.format(choice.user_selection))
@@ -364,6 +432,25 @@ def parse_args():
                              "set specific configuration settings to a "
                              "pre-defined value and thereby remove any user "
                              " adjustments.")
+    parser.add_argument("--strict-flip-checks",
+                        action="store_true",
+                        help="Treat an assignment that Kconfig does not honor "
+                             "as an error instead of a warning, for the "
+                             "fragments selected by --strict-scope-dir and "
+                             "--strict-scope-file")
+    parser.add_argument("--strict-scope-dir",
+                        action="append",
+                        default=[],
+                        help="Directory whose configuration fragments the "
+                             "strict checks apply to. Only files directly in "
+                             "the directory are covered, not files in its "
+                             "subdirectories. May be given multiple times.")
+    parser.add_argument("--strict-scope-file",
+                        action="append",
+                        default=[],
+                        help="Configuration fragment the strict checks apply "
+                             "to, regardless of its location. May be given "
+                             "multiple times.")
     parser.add_argument("--zephyr-base",
                         help="Path to current Zephyr installation")
     parser.add_argument("kconfig_file",

tests/drivers/timer/nrf_grtc_timer/prj.conf

@@ -2,7 +2,13 @@ CONFIG_ZTEST=y
 CONFIG_NRF_GRTC_TIMER=y
 CONFIG_COUNTER=y
 CONFIG_TEST_RANDOM_GENERATOR=y
-CONFIG_XOSHIRO_RANDOM_GENERATOR=y
-CONFIG_LOG_PRINTK=y
+# Ineffective here: XOSHIRO_RANDOM_GENERATOR depends on ENTROPY_HAS_DRIVER, which no board this
+# test runs on enables, so the RNG choice falls back to TIMER_RANDOM_GENERATOR.
+# CONFIG_XOSHIRO_RANDOM_GENERATOR=y
+# Ineffective here: LOG_PRINTK depends on !LOG_MODE_MINIMAL, and the minimal logging mode is what
+# these boards default to. Where it is available it already defaults to y.
+# CONFIG_LOG_PRINTK=y
 CONFIG_CPU_LOAD=y
-CONFIG_CPU_LOAD_USE_COUNTER=y
+# Ineffective here: CPU_LOAD_USE_COUNTER depends on the zephyr,cpu-load-counter chosen node, which
+# most of these cores do not have. On the cores that do it already defaults to y.
+# CONFIG_CPU_LOAD_USE_COUNTER=y