twister: add a --flash-command flag

[fabiobaltieri]

Currently twister requires west to flash a board, either directly using west-flash or indirectly through the cmake flash target.

Add an option to specify a custom flash script, this is passed a build-dir and board-id flags so it can be used to implement custom flashing scripts in a system that does not use west.

[marc-hb]

Did you mean?

Suggested change

	help="""Uses a custom flash command instead of ninja or make to flash
	help="""Uses a custom flash command instead of west to flash

Context:

• cmake: flash: add support for using a custom flash command #92406

[fabiobaltieri]

Well by default it uses west through ninja or make, so either works, but also who cares , dropped that whole "instead of bit"

[marc-hb]

At least @tejlmand seems to care; in #92406 (which this supersedes - so it was IMHO worth linking them to each other) he just said ninja flash should die. So it did not seem like a good idea to mention that some --help text.

dropped that whole "instead of bit"

Great!

[fabiobaltieri]

yeah I mean the whole situation is just confusing at this point, even that

if (self.options.west_flash is not None) or runner:

felt like a hack around the current situation, in practice what I found happens is that if you run twister with device testing specifying the serial interface it uses ninja flash by default and west directly if west-flash is specified, but if one specifies a hardware map then runners are always set and this code kicks in again regardless of the west-flash flag.

At this point let's just drop the target and have twister use west directly, what difference does it make anyway. I may followup on it if I don't get sidetracked.

[nashif]

we should first look into implementing this at the cmake level instead of making this yet another twister option. Right now, 'ninja flash' is either using west or erroring and aksing you to create a west workspace, ninja flash in the absense of WEST and based on some definition in the board cmake should be able to run any custom scripts and skip west completely.

something like this, quickly hacked:

diff --git a/boards/nxp/frdm_k64f/board.cmake b/boards/nxp/frdm_k64f/board.cmake
index d2fafeb4a00..e732a1e9bbc 100644
--- a/boards/nxp/frdm_k64f/board.cmake
+++ b/boards/nxp/frdm_k64f/board.cmake
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: Apache-2.0

+set(CUSTOM_FLASHER "/tmp/blah.sh")
 board_runner_args(jlink "--device=MK64FN1M0xxx12")
 board_runner_args(linkserver  "--device=MK64FN1M0xxx12:FRDM-K64F")

diff --git a/cmake/flash/CMakeLists.txt b/cmake/flash/CMakeLists.txt
index 985b7536154..5c609428410 100644
--- a/cmake/flash/CMakeLists.txt
+++ b/cmake/flash/CMakeLists.txt
@@ -186,7 +186,27 @@ foreach(target flash debug debugserver attach rtt)
     set(RUNNER_VERBOSE)
   endif()

-  if(WEST)
+  if(CUSTOM_FLASHER)
+    add_custom_target(${target}
+      # This script will print an error message and fail if <target> has added
+      # dependencies. This is done using dedicated CMake script, as
+      # `cmake -E {true|false}` is not available until CMake 3.16.
+      COMMAND ${CMAKE_COMMAND}
+        -DTARGET=${target}
+        -DDEPENDENCIES="$<TARGET_PROPERTY:${target},MANUALLY_ADDED_DEPENDENCIES>"
+        -P ${CMAKE_CURRENT_LIST_DIR}/check_runner_dependencies.cmake
+      COMMAND
+        ${CMAKE_COMMAND} -E env ZEPHYR_BASE=${ZEPHYR_BASE}
+       ${CUSTOM_FLASHER}
+        ${RUNNER_VERBOSE}
+        ${target}
+      WORKING_DIRECTORY
+        ${APPLICATION_BINARY_DIR}
+      COMMENT
+        ${comment}
+      USES_TERMINAL
+    )
+  elseif(WEST)
     add_custom_target(${target}
       # This script will print an error message and fail if <target> has added
       # dependencies. This is done using dedicated CMake script, as
@@ -212,5 +232,5 @@ foreach(target flash debug debugserver attach rtt)
           '${CMAKE_MAKE_PROGRAM} ${target}', please create a west workspace.\"
       USES_TERMINAL
       )
-  endif(WEST)
+  endif()
 endforeach()

with /tmp/blah.sh:

#!/bin/sh
#
echo "BLAH"

then run ninja flash and ...

...
device_CMSIS component is included.
-- Using ccache: /usr/bin/ccache
-- Found gen_kobject_list: /home/nashif/zephyrproject/zephyr/scripts/build/gen_kobject_list.py
-- Configuring done (5.2s)
-- Generating done (0.2s)
-- Build files have been written to: /home/nashif/zephyrproject/zephyr/build
[0/1] Flashing frdm_k64f
BLAH

[nashif]

so basically whatever you were doing in #92406, making the only way to flash go via west in the build system is not right and we can't push the problem down to the tooling like twister and other test runners.

[fabiobaltieri]

so basically whatever you were doing in #92406, making the only way to flash go via west in the build system is not right and we can't push the problem down to the tooling like twister and other test runners.

Not going to work, turns out I need board_id to support hardware map and that bit has no way of working with the flash target. Even if I was using west and making a custom runner I would need custom support code in there.

[fabiobaltieri]

ping

[marc-hb]

#93944 (comment)

tl;dr, do things like this work fine? They should, just making sure.

Pong.

[fabiobaltieri]

#93944 (comment)

Oh cool you were waiting for me the whole time, hey GitHub did you forget to send me the notification email for that comment? Again? Yes, yes you did. Thanks GitHub.

Pong.

Pong indeed.

[fabiobaltieri]

• tweaked the harness commit to do the parsing in the same way as the twister flag, i.e. pass an array, not sure why I did not do it in like that the first time
• added a patch to switch the extra arguments parsing from split to csv, that handles correctly quotes and allows passing comma arguments, which sounds like a not so unreasonable use case, this is both for the new flag as well as the west one

[marc-hb]

twister: use csv to split extra argument arguments

I'm sorry to ask this but I think this deserves a separate, preliminary PR for both 1) visibility and 2) CI reasons.

\2) give upstream and downstream CIs at least a 24h opportunity for one "nightly" build to catch any test failure caused by this. This change is for the greater good but it is technically backwards-incompatible. Maybe some tests already had some "creative" quoting solution that is obviously going to be incompatible with this.

\1) Give people an opportunity to notice this change in their flood of GitHub notifications before it gets merged. And make it easily to spot even after merge when scanning what changed (and possibly broke some test of theirs)

Should be really quick
https://github.com/zephyrproject-rtos/zephyr/pull/83839/files#diff-d84ff1b1f7f14c50055618e1e0f6a0986cae639ab168881fc493de64ebd76971

[fabiobaltieri]

@marc-hb cool, dropped the last commit, let's merge the others once they are in I'll open a separate PR for the quotes magic stuff.

[marc-hb]

I've changed the code for parsing these to use the cvs parser which seems to handle these correctly and no doubt will introduce another bunch of problems down the road, and I've updated the documentation with an examples with commas, which I think is the interesting one

let's merge the others once they are in I'll open a separate PR for the quotes magic stuff.

So the example with commas does not work yet?

In general I think it's better to 1) merge bug fixes first 2) wait at least 24h to make sure there's no serious regression 3) add the new feature relying on the fixes.

Considering the older #92406, you started working on this almost 2 months ago. Surely, waiting for 24 more hours is not that big of a deal?

[fabiobaltieri]

Yes I'm happy to merge this with the extra flag quotes behaving in the same way as the twister flag, and I'd rather merge this first on the basis of the quote flag fix pr having a decent likelihood of causing a regression and having to be reverted, if we merge it first, then we merge this, then that gets reverted and conflict, whoever does the rollback may (rightfully so) not care enough to figure what's going on and end up reverting both.

So yeah, no. This first. Please.

[marc-hb]

Respectfully but strongly disagree with the "process" question here.

1. I think it's unlikely, but if the backwards-incompatible quoting fix needs to be reverted, then we have IMHO discovered a "bigger" and more important problem in the existing codebase, bigger and more urgent than adding this new feature that is not there yet and has been under review for almost 2 months. The former should be dealt with before the latter.

whoever does the rollback may (rightfully so) not care enough to figure what's going on and end up reverting both.

2. If there's at least 24h distance between the two things and they don't git conflict with each other, AND there is a regression with the first thing AND it does not get caught in the first 24h... then there is still zero chance of both being reverted at the same time by accident. Reverts get code reviews too.

[marc-hb]

then there is still zero chance of both being reverted at the same time by accident. Reverts get code reviews too.

BTW people sometimes get emotional about reverts but I think a small number of infrequent reverts is perfectly fine.

"If you never break anything, then you're not trying hard enough" (a former team lead).

[fabiobaltieri]

The quote bug is the existing behavior, it's been there from the start, great job finding it but fixing it is not urgent, this one has been well discussed and tested by multiple people. Let's wrap it and then fix the quotes.

[fabiobaltieri]

Or I can add it back to the stack of this PR, you tell me.

[marc-hb]

The "urgent" problem is not fixing the quoting bug itself; I understand it's been there since forever. The urgent questions are: 1) finding whether anything depends on the current behavior 2) if anything, figuring out what to do about it/them. Can they be updated easily? Or is that too much branching hassle? Other unexpected problem?

Knowing where we stand and what the next steps are is IMHO higher priority than adding a brand new feature interacting with that very same, likely changing behavior.

But again: in all likelihood this small and isolated quoting fix will just sail through and not break anything. So I don't understand the rush (after 2 months) to get this new feature first.

[fabiobaltieri]

Ok how about this, I'll make this work correctly for the new flag, leave the west flag untouched. No regression chances, win win.

[fabiobaltieri]

Done, quote handled properly, no changes in behavior for any current flag.

[marc-hb]

Ok how about this, I'll make this work correctly for the new flag, leave the west flag untouched. No regression chances, win win.

Not my preference but since you isolated decoupled the two things from each other then fine by me. Please don't waste your bug fix though; please submit it in a separate PR - which can now be merged either after or before this one; does not matter anymore. Thanks!

[fabiobaltieri]

Sure opened the followup pr in draft already. Honestly I could probably let both run and whatever gets merged first wins and I'll suck up the merge conflict I'm just a bit tired of the long iteration cycles on this, not pointing fingers really, last one was my fault the rest was people on vacation, I just want this off my radar and not have to think about it during ZDS or let this drag to month three.

[marc-hb]

1st commit LGTM. I don't know the test harness.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[nashif]

#95298 should fix those test failures