cmake: flash/debug: refactor runner configuration

This commit message is a bit of a novel mostly:

- because the issues involved are longstanding
- as evidence this is not a capricious refactoring

The runners.core.RunnerConfig Python class holds common configuration
values used by multiple runners, such as the location of the build
outputs and board directory.

The runners code, first written in 2017-ish, replaced various shell
scripts that got this information from the environment. Avoiding
environment variables was a requirement, however. It's ghastly to set
environment variables for a single command invocation on Windows, and
the whole thing was part of a larger push to make Zephyr development
on Windows better.

I had a hammer (the argparse module). Finding a replacement naturally
looked like a nail, so the information that ends up in RunnerConfig
got shunted from the build system to Python in the form of 'west
flash' / 'west debug' command line options like '--board-dir',
'--elf-file', etc.

I initially stored the options and their values in the CMake cache.
This was chosen in hopes the build system maintainer would like
the strategy (which worked).

I knew the command line arguments approach was a bit hacky (this
wasn't a nail), but I also honestly didn't have a better idea at the
time.

It did indeed cause issues:

- users don't know that just because they specify --bin-file on the
  command line doesn't mean that their runner respects the option, and
  have gotten confused trying to flash alternate files, usually for
  chain-loading by MCUboot (for example, see #15961)

- common options weren't possible to pass via board.cmake files
  (#22563, fixed partly via introduction of runners.yaml and the west
  flash/debug commands no longer relying on the cache)

- it is confusing that "west flash --help" prints information about
  openocd related options even when the user's board has no openocd
  support. The same could be said about gdb in potential future use
  cases where debugging occurs via some other tool.

Over time, they've caused enough users enough problems that
improvements are a priority.

To work towards this, put these values into runners.yaml using a new
'config: ...' key/value instead of command line options.

For example, instead of this in the generated runners.yaml file:

args:
  common:
  - --hex-file=.../zephyr.hex

we now have:

config:
  hex_file: zephyr.hex

and similarly for other values.

In Python, we still support the command line options, but they are not
generated by the build system for any in-tree boards. Further work is
needed to deprecate the confusing ones (like --hex-file) and move the
runner-specific host tool related options (like --openocd) to the
runners that need them.

Individual board.cmake files should now influence these values by
overriding the relevant target properties of the
runners_yaml_props_target.

For example, instead of:

  board_runner_args(foo "--hex-file=bar.hex")

Do this:

  set_target_properties(runners_yaml_props_target PROPERTIES
                        hex_file bar.hex)

This change additionally allows us to stitch cmake/mcuboot.cmake and
the runners together easily by having mcuboot.cmake override the
properties that set the hex or bin file to flash. (The command line
arguments are still supported as-is.)

Combined with 98e0c95d91ae16f14e4997fb64ccdf0956595712 ("build:
auto-generate signed mcuboot binaries"), this will allow users to
build and flash images to be chain loaded by mcuboot in a way that
avoids calling 'west sign' and passing 'west flash' its output files
entirely.

While we are here, rename runner_yml_write to runners_yaml_append().
This function doesn't actually write anything, and we're here
refactoring this file anyway, so we might as well improve the
situation while we're at it.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

cmake/flash/CMakeLists.txt

@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: Apache-2.0
 
-function(runner_yml_write content)
+function(runners_yaml_append content)
   # Append ${content}\n to a target property which is later evaluated as a
   # generator expression when writing the flash runner yaml file.
   # We define this function here to have access to the `flash` target.
@@ -13,6 +13,52 @@ function(runner_yml_write content)
     )
 endfunction()
 
+function(get_runners_prop prop out_var default_value)
+  # Get property 'prop' from runners_yaml_props_target, storing its
+  # value in 'out_var'. If the property is not found (value is
+  # ...-NOTFOUND), 'out_var' is set to 'default_value'.
+
+  get_target_property(out runners_yaml_props_target "${prop}")
+
+  if("${out}" STREQUAL "out-NOTFOUND")
+    set("${out_var}" "${default_value}" PARENT_SCOPE)
+  else()
+    set("${out_var}" "${out}" PARENT_SCOPE)
+  endif()
+endfunction()
+
+function(runners_yaml_append_config)
+  # Append the common configuration values to the relevant property target.
+
+  runners_yaml_append("\n# Common runner configuration values.")
+  runners_yaml_append("config:")
+  runners_yaml_append("  board_dir: ${BOARD_DIR}")
+  get_runners_prop(elf_file elf "${KERNEL_ELF_NAME}")
+  runners_yaml_append("  # Build outputs:")
+  runners_yaml_append("  elf_file: ${elf}")
+  if(CONFIG_BUILD_OUTPUT_HEX)
+    get_runners_prop(hex_file hex "${KERNEL_HEX_NAME}")
+    runners_yaml_append("  hex_file: ${hex}")
+  endif()
+  if(CONFIG_BUILD_OUTPUT_BIN)
+    get_runners_prop(bin_file bin "${KERNEL_BIN_NAME}")
+    runners_yaml_append("  bin_file: ${bin}")
+  endif()
+
+  if (DEFINED CMAKE_GDB OR DEFINED OPENOCD OR DEFINED OPENOCD_DEFAULT_PATH)
+    runners_yaml_append("  # Host tools:")
+  endif()
+  if (DEFINED CMAKE_GDB)
+    runners_yaml_append("  gdb: ${CMAKE_GDB}")
+  endif()
+  if (DEFINED OPENOCD)
+    runners_yaml_append("  openocd: ${OPENOCD}")
+  endif()
+  if(DEFINED OPENOCD_DEFAULT_PATH)
+    runners_yaml_append("  openocd_search: ${OPENOCD_DEFAULT_PATH}")
+  endif()
+  runners_yaml_append("")
+endfunction()
 
 # Save runner state in a YAML file, and put that YAML file's location
 # in the cache.
@@ -21,57 +67,39 @@ function(create_runners_yaml)
 
   set(runners_yaml "${PROJECT_BINARY_DIR}/runners.yaml")
 
-  runner_yml_write("# Available runners configured by board.cmake.\nrunners:")
+  runners_yaml_append("# Available runners configured by board.cmake.\nrunners:")
   foreach(runner ${runners})
-    runner_yml_write("- ${runner}")
+    runners_yaml_append("- ${runner}")
   endforeach()
 
   if(DEFINED BOARD_FLASH_RUNNER)
-    runner_yml_write("\n# Default flash runner if --runner is not given.")
+    runners_yaml_append("\n# Default flash runner if --runner is not given.")
-    runner_yml_write("flash-runner: ${BOARD_FLASH_RUNNER}")
+    runners_yaml_append("flash-runner: ${BOARD_FLASH_RUNNER}")
   endif()
   if(DEFINED BOARD_DEBUG_RUNNER)
-    runner_yml_write("\n# Default debug runner if --runner is not given.")
+    runners_yaml_append("\n# Default debug runner if --runner is not given.")
-    runner_yml_write("debug-runner: ${BOARD_DEBUG_RUNNER}")
+    runners_yaml_append("debug-runner: ${BOARD_DEBUG_RUNNER}")
   endif()
 
-  runner_yml_write("\n# Default command line arguments. The ones in \"common\" are always given.\n# The other sub-keys give runner-specific arguments.")
+  # Sets up common runner configuration values.
-  runner_yml_write("args:\n  common:")
+  runners_yaml_append_config()
-
-  # Get default settings for common arguments.
-  #
-  # TODO: clean up the host tools arguments. These are really runner
-  # specific; they only continue to exist here out of inertia.
-
-  runner_yml_write("\
-  - --board-dir=${BOARD_DIR}
-  - --elf-file=${PROJECT_BINARY_DIR}/${KERNEL_ELF_NAME}
-  - --hex-file=${PROJECT_BINARY_DIR}/${KERNEL_HEX_NAME}
-  - --bin-file=${PROJECT_BINARY_DIR}/${KERNEL_BIN_NAME}")
-  if (DEFINED CMAKE_GDB)
-    runner_yml_write("  - --gdb=${CMAKE_GDB}")
-  endif()
-  if (DEFINED OPENOCD)
-    runner_yml_write("  - --openocd=${OPENOCD}")
-  endif()
-  if(DEFINED OPENOCD_DEFAULT_PATH)
-    runner_yml_write("  - --openocd-search=${OPENOCD_DEFAULT_PATH}")
-  endif()
 
   # Get runner-specific arguments set in the board files.
+  runners_yaml_append("# Runner specific arguments")
+  runners_yaml_append("args:")
   foreach(runner ${runners})
     string(MAKE_C_IDENTIFIER ${runner} runner_id)
-    runner_yml_write("  ${runner}:")
+    runners_yaml_append("  ${runner}:")
     get_property(args GLOBAL PROPERTY "BOARD_RUNNER_ARGS_${runner_id}")
     if(args)
       # Usually, the runner has arguments. Append them to runners.yaml,
       # one per line.
       foreach(arg ${args})
-        runner_yml_write("    - ${arg}")
+        runners_yaml_append("    - ${arg}")
       endforeach()
     else()
       # If the runner doesn't need any arguments, just use an empty list.
-      runner_yml_write("    []\n")
+      runners_yaml_append("    []\n")
     endif()
   endforeach()
 

scripts/west_commands/run_common.py

@@ -7,7 +7,7 @@
 
 import argparse
 import logging
-from os import close, getcwd, path
+from os import close, getcwd, path, fspath
 from pathlib import Path
 from subprocess import CalledProcessError
 import sys
@@ -109,6 +109,7 @@ def add_parser_common(command, parser_adder=None, parser=None):
         Not all runners respect --elf-file / --hex-file / --bin-file, nor use
         gdb or openocd.'''))
 
+    # Options used to override RunnerConfig values in runners.yaml.
     # TODO: is this actually useful?
     group.add_argument('--board-dir', metavar='DIR', help='board directory')
     # FIXME: we should just have a single --file argument. The variation
@@ -142,7 +143,8 @@ def do_run_common(command, user_args, user_runner_args):
         rebuild(command, build_dir, user_args)
 
     # Load runners.yaml.
-    runners_yaml = load_runners_yaml(runners_yaml_path(cache), user_args)
+    yaml_path = runners_yaml_path(build_dir, board)
+    runners_yaml = load_runners_yaml(yaml_path)
 
     # Get a concrete ZephyrBinaryRunner subclass to use based on
     # runners.yaml and command line arguments.
@@ -159,15 +161,11 @@ def do_run_common(command, user_args, user_runner_args):
     # parsing, it will show up here, and needs to be filtered out.
     runner_args = [arg for arg in user_runner_args if arg != '--']
 
-    # Arguments are provided in this order to allow the specific to
+    # Arguments in this order to allow specific to override general:
-    # override the general:
     #
-    # - common runners.yaml arguments
     # - runner-specific runners.yaml arguments
-    # - command line arguments
+    # - user-provided command line arguments
-    final_argv = (runners_yaml['args']['common'] +
+    final_argv = runners_yaml['args'][runner_name] + runner_args
-                  runners_yaml['args'][runner_name] +
-                  runner_args)
 
     # 'user_args' contains parsed arguments which are:
     #
@@ -200,16 +198,15 @@ def do_run_common(command, user_args, user_runner_args):
         if v is not None:
             setattr(args, a, v)
 
-    # Create the RunnerConfig from the values assigned to common
+    # Create the RunnerConfig from runners.yaml and any command line
-    # arguments. This is a hacky way to go about this; probably
+    # overrides.
-    # ZephyrBinaryRunner should define what it needs to make this
+    runner_config = get_runner_config(build_dir, yaml_path, runners_yaml, args)
-    # happen by itself. That would be a larger refactoring of the
+    log.dbg(f'runner_config: {runner_config}', level=log.VERBOSE_VERY)
-    # runners package than there's time for right now, though.
+
-    #
     # Use that RunnerConfig to create the ZephyrBinaryRunner instance
     # and call its run().
     try:
-        runner = runner_cls.create(runner_cfg_from_args(args, build_dir), args)
+        runner = runner_cls.create(runner_config, args)
         runner.run(command_name)
     except ValueError as ve:
         log.err(str(ve), fatal=True)
@@ -266,18 +263,16 @@ def rebuild(command, build_dir, args):
         else:
             log.die(f're-build in {build_dir} failed (no --build-dir given)')
 
-def runners_yaml_path(cache):
+def runners_yaml_path(build_dir, board):
-    try:
+    ret = Path(build_dir) / 'zephyr' / 'runners.yaml'
-        return cache['ZEPHYR_RUNNERS_YAML']
+    if not ret.is_file():
-    except KeyError:
-        board = cache.get('CACHED_BOARD')
         log.die(f'either a pristine build is needed, or board {board} '
                 "doesn't support west flash/debug "
                 '(no ZEPHYR_RUNNERS_YAML in CMake cache)')
+    return ret
 
-def load_runners_yaml(path, args):
+def load_runners_yaml(path):
-    # Load runners.yaml using its location in the CMake cache,
+    # Load runners.yaml and convert to Python object.
-    # allowing a command line override for the cache file location.
 
     try:
         with open(path, 'r') as f:
@@ -319,11 +314,41 @@ def use_runner_cls(command, board, args, runners_yaml, cache):
 
     return runner_cls
 
-def runner_cfg_from_args(args, build_dir):
+def get_runner_config(build_dir, yaml_path, runners_yaml, args=None):
-    return RunnerConfig(build_dir, args.board_dir, args.elf_file,
+    # Get a RunnerConfig object for the current run. yaml_config is
-                        args.hex_file, args.bin_file,
+    # runners.yaml's config: map, and args are the command line arguments.
-                        gdb=args.gdb, openocd=args.openocd,
+    yaml_config = runners_yaml['config']
-                        openocd_search=args.openocd_search)
+    yaml_dir = yaml_path.parent
+    if args is None:
+        args = argparse.Namespace()
+
+    def output_file(filetype):
+
+        from_args = getattr(args, f'{filetype}_file', None)
+        if from_args is not None:
+            return from_args
+
+        from_yaml = yaml_config.get(f'{filetype}_file')
+        if from_yaml is not None:
+            # Output paths in runners.yaml are relative to the
+            # directory containing the runners.yaml file.
+            return fspath(yaml_dir / from_yaml)
+
+        # FIXME these RunnerConfig values really ought to be
+        # Optional[str], but some runners rely on them being str.
+        return ''
+
+    def config(attr):
+        return getattr(args, attr, None) or yaml_config.get(attr)
+
+    return RunnerConfig(build_dir,
+                        yaml_config['board_dir'],
+                        output_file('elf'),
+                        output_file('hex'),
+                        output_file('bin'),
+                        config('gdb'),
+                        config('openocd'),
+                        config('openocd_search'))
 
 def dump_traceback():
     # Save the current exception to a file and return its path.
@@ -345,8 +370,8 @@ def dump_context(command, args, unknown_args):
     else:
         cache = load_cmake_cache(build_dir, args)
         board = cache['CACHED_BOARD']
-        yaml_path = runners_yaml_path(cache)
+        yaml_path = runners_yaml_path(build_dir, board)
-        runners_yaml = load_runners_yaml(yaml_path, args)
+        runners_yaml = load_runners_yaml(yaml_path)
 
     # Re-build unless asked not to, to make sure the output is up to date.
     if build_dir and not args.skip_rebuild:
@@ -447,6 +472,8 @@ def dump_all_runner_context(command, runners_yaml, board, build_dir):
     available = runners_yaml['runners']
     available_cls = {r: all_cls[r] for r in available if r in all_cls}
     default_runner = runners_yaml[command.runner_key]
+    yaml_path = runners_yaml_path(build_dir, board)
+    runners_yaml = load_runners_yaml(yaml_path)
 
     log.inf(f'zephyr runners which support "west {command.name}":',
             colorize=True)
@@ -461,7 +488,10 @@ def dump_all_runner_context(command, runners_yaml, board, build_dir):
     dump_wrapped_lines(', '.join(available), INDENT)
     log.inf(f'default runner in runners.yaml:', colorize=True)
     log.inf(INDENT + default_runner)
-    dump_runner_args('common', runners_yaml)
+    log.inf('common runner configuration:', colorize=True)
+    runner_config = get_runner_config(build_dir, yaml_path, runners_yaml)
+    for field, value in zip(runner_config._fields, runner_config):
+        log.inf(f'{INDENT}- {field}: {value}')
     log.inf('runner-specific context:', colorize=True)
     for cls in available_cls.values():
         dump_runner_context(command, cls, runners_yaml, INDENT)