intel_adsp: move west sign from west flash to earlier west build

Invoking `west sign` in `west build` accelerates twister because `west
build` is run in parallel, see rationale in superseded and very
different (CMake-based) PR #52942.

To maximize backwards compatibility:
- `west sign` is optional in `west build`
- `west flash` will sign (again) if any rimage --option is passed

Signed-off-by: Marc Herbert <[email protected]>

doc/develop/west/sign.rst

@@ -117,3 +117,54 @@ signing system or extending the default behaviour.
 
 .. _imgtool:
    https://pypi.org/project/imgtool/
+
+
+rimage
+******
+
+rimage configuration uses a different approach that does not rely on Kconfig or CMake
+but on :ref:`west config<west-config>` instead, similar to
+:ref:`west-building-cmake-config`.
+
+Signing involves a number of "wrapper" scripts stacked on top of each other: ``west
+flash`` invokes ``west build`` which invokes ``cmake`` and ``ninja`` which invokes
+``west sign`` which invokes ``imgtool`` or `rimage`_. As long as the signing
+parameters desired are the default ones and fairly static, these indirections are
+not a problem. On the other hand, passing ``imgtool`` or ``rimage`` options through
+all these layers can causes issues typical when the layers don't abstract
+anything. First, this usually requires boilerplate code in each layer. Quoting
+whitespace or other special characters through all the wrappers can be
+difficult. Reproducing a lower ``west sign`` command to debug some build-time issue
+can be very time-consuming: it requires at least enabling and searching verbose
+build logs to find which exact options were used. Copying these options from the
+build logs can be unreliable: it may produce different results because of subtle
+environment differences. Last and worst: new signing feature and options are
+impossible to use until more boilerplate code has been added in each layer.
+
+To avoid these issues, ``rimage`` parameters can bet set in ``west config``
+instead. Here's a ``workspace/.west/config`` example:
+
+.. code-block:: ini
+
+   [sign]
+   # Not needed when invoked from CMake
+   tool = rimage
+
+   [rimage]
+   # Quoting is optional and works like in Unix shells
+   # Not needed when rimage can be found in the default PATH
+   path = "/home/me/zworkspace/build-rimage/rimage"
+
+   # Not needed when using the default development key
+   extra-args = -i 4 -k 'keys/key argument with space.pem'
+
+In order to support quoting, values are parsed by Python's ``shlex.split()`` like in
+:ref:`west-building-cmake-args`.
+
+The ``extra-args`` are passed directly to the ``rimage`` command. The example
+above has the same effect as appending them on command line after ``--`` like this:
+``west sign --tool rimage -- -i 4 -k 'keys/key argument with space.pem'``. In case
+both are used, the command-line arguments go last.
+
+.. _rimage:
+   https://github.com/thesofproject/rimage

scripts/west_commands/runners/intel_adsp.py

@@ -31,7 +31,8 @@ def __init__(self,
                  default_key,
                  key,
                  pty,
-                 tool_opt
+                 tool_opt,
+                 do_sign,
                  ):
         super().__init__(cfg)
 
@@ -50,6 +51,7 @@ def __init__(self,
             self.key = os.path.join(DEFAULT_KEY_DIR, default_key)
 
         self.tool_opt_args = tool_opt
+        self.do_sign = do_sign
 
     @classmethod
     def name(cls):
@@ -82,28 +84,34 @@ def tool_opt_help(cls) -> str:
 
     @classmethod
     def do_create(cls, cfg, args):
+        # We now have `west flash` -> `west build` -> `west sign` so
+        # `west flash` -> `west sign` is not needed anymore; it's also
+        # slower because not concurrent. However, for backwards
+        # compatibility keep signing here if some explicit rimage
+        # --option was passed. Some of these options may differ from the
+        # current `west sign` configuration; we take "precedence" by
+        # running last.
+        do_sign = (
+            args.rimage_tool != DEFAULT_RIMAGE_TOOL or
+            args.config_dir != DEFAULT_CONFIG_DIR or
+            args.key is not None
+        )
         return IntelAdspBinaryRunner(cfg,
                                     remote_host=args.remote_host,
                                     rimage_tool=args.rimage_tool,
                                     config_dir=args.config_dir,
                                     default_key=args.default_key,
                                     key=args.key,
                                     pty=args.pty,
-                                    tool_opt=args.tool_opt
+                                    tool_opt=args.tool_opt,
+                                    do_sign=do_sign,
                                     )
 
     def do_run(self, command, **kwargs):
         self.logger.info('Starting Intel ADSP runner')
 
-        # Always re-sign because here we cannot know whether `west
-        # flash` was invoked with `--skip-rebuild` or not and we have no
-        # way to tell whether there was any code change either.
-        #
-        # Signing does not belong here; it should be invoked either from
-        # some CMakeLists.txt file or an optional hook in some generic
-        # `west flash` code but right now it's in neither so we have to
-        # do this.
-        self.sign(**kwargs)
+        if self.do_sign:
+            self.sign(**kwargs)
 
         if re.search("intel_adsp_cavs", self.platform):
             self.require(self.cavstool)

soc/xtensa/intel_adsp/common/CMakeLists.txt

@@ -118,3 +118,22 @@ add_custom_command(
             $<TARGET_PROPERTY:bintools,strip_flag_final>
 )
 endif()
+
+
+#  west sign
+add_custom_target(zephyr.ri ALL
+  DEPENDS ${CMAKE_BINARY_DIR}/zephyr/zephyr.ri
+)
+
+add_custom_command(
+  OUTPUT ${CMAKE_BINARY_DIR}/zephyr/zephyr.ri
+  COMMENT "west sign --if-tool-available --tool rimage ..."
+  # Use --if-tool-available so we don't force every CI to install
+  # rimage. We don't want to break build-only and other tests that don't
+  # require signing. When rimage is missing, `west flash` fails with a
+  # clear "zephyr.ri missing" error with an "rimage not found" warning
+  # from west sign immediately before it.
+  COMMAND  west sign --if-tool-available --tool rimage --build-dir ${CMAKE_BINARY_DIR}
+  DEPENDS gen_modules
+          ${CMAKE_BINARY_DIR}/zephyr/boot.mod ${CMAKE_BINARY_DIR}/zephyr/main.mod
+)