Runner name deprecation + deprecate 'misc-flasher' to prefer 'misc' #61243
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbolivar-ampere
requested review from
iuliana-prodan,
dbaluta,
nashif,
dcpleung,
enjiamai,
kgugala,
pgielda,
MaureenHelm,
galak and
andyross
as code owners
mbolivar-ampere
changed the title
zephyrbot
added
area: RISCV
|
Looks like CI is flagging some real issues; I'll update when I've fixed them. |
Comment on lines
+3
to
+4
| board_set_flasher_ifnset(misc) | ||
| board_finalize_runner_args(misc) |
There was a problem hiding this comment.
want to to dedup these in the template while you are at it?
There was a problem hiding this comment.
@fabiobaltieri sorry, I don't know what you mean
There was a problem hiding this comment.
There's load of these in board.cmake, all other runners have a template file for the board_set_flasher_ifnset and board_finalize_runner_args pair, that's what I was trying to get rid of with my PR.
The do_run_common_image() function responsible for calling west flash, debug, etc. is doing runner-related work before setting up the logging for the runners module. This means messages aren't formatted properly. Fix it. Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
The 'if' statement was added in commit 8408af6 ("scripts: west commands to support --domain"), because do_run_common_image() was called once per domain as a result of this commit. The 'if' is no longer necessary now that this code has been moved back into do_run_common(), which should only be called once per 'west <flash|debug|...>' process. Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
The dump_runner_option_help() function is breaking an argparse
abstraction in a way that is not working properly. The attempt to
remove the 'REMOVE ME' string mentioned in the function is not
working for the 'misc-flasher' runner, which has additional positional
argument groups that this function did not contemplate.
(The function was implemented before the misc-flasher runner, so probably
this regression has existed since misc-flasher was introduced.)
Fix it by replacing it with an implementation that sticks to
argparse abstractions. With this patch, 'west flash -H -r misc-flasher'
results look like:
misc-flasher options:
usage: [--elf-file FILE] [--hex-file FILE] [--bin-file FILE] cmd ...
positional arguments:
cmd command to run; it will be passed the build
directory as its first argument
args additional arguments to pass after the build
directory
options:
--elf-file FILE path to zephyr.elf
--hex-file FILE path to zephyr.hex
--bin-file FILE path to zephyr.bin
Instead of how they appear now:
misc-flasher options:
cmd command to run; it will be passed the build
directory as its first argument
args additional arguments to pass after the build
directory
REMOVE ME:
--elf-file FILE path to zephyr.elf
--hex-file FILE path to zephyr.hex
--bin-file FILE path to zephyr.bin
Output for other runners is similar. For instance, (abbreviated) jlink
output looks like this now:
jlink options:
usage: [-i DEV_ID] [--dt-flash {Y,y,N,n,yes,no,YES,NO}] [...]
options:
-i DEV_ID, --dev-id DEV_ID
Device identifier. Use it to select the
J-Link Serial Number of the device
connected over USB.
--dt-flash {Y,y,N,n,yes,no,YES,NO}
If 'yes', try to use flash address
information from devicetree when flash
addresses are unknown (e.g. when flashing
a .bin)
[...]
Including the usage is arguably better anyway.
Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
The location of the runners.yaml file is a hard-coded implementation detail. Its path is also still present in the cmake cache, but we don't look for it there. Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
The only caller of dump_all_runner_context() already has the path to runners.yaml, so pass it down to the function instead of recomputing it. Additionally, the content of that file is already loaded, so do not load it again. This avoids unnecessary work. Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
Over time, we may want to deprecate and phase out old runner names. There is currently no way to do this. Add one by allowing ZephyrBinaryRunner subclasses to override a new deprecated_names() classmethod. Subclasses can move their current name into the return value of this list and return their new names from the name() classmethod. Make the framework support this by falling back on deprecated names. Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
Make this runner name match the file names used to refer to it (e.g. misc.board.cmake and misc.py). Use the new name deprecation framework to avoid compatibility breaks. Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
The preferred name is 'misc' and 'misc-flasher' has been deprecated. The current board files and related documentation still work, but they generate warnings. Use the new preferred name to avoid the warnings. Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
mbolivar-ampere
force-pushed
the
runner-name-deprecation
branch
from
01eceb2
to
6ff9efb
Compare
|
@fabiobaltieri please revisit |
|
hey 6ff9efb#r1294803186 is still relevant, asking if this should have something like c63f621, that's how every other runner is used in the board files |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An alternative to #60894. As pointed out there, the 'misc-flasher' ZephyrBinaryRunner name is inconsistent with the names of these associated files:
The runner probably should have been named 'misc' all along. I added it in a hurry to get an issue report to go away a long time ago, and I wasn't thinking about how it would evolve. Right now it only "flashes", but it would be trivial to extend this to add miscellaneous debug support as well.
Clean things up so that the flasher's name is "misc".
To avoid breaking out of tree boards, though, preserve backwards compatibility by allowing runners to declare deprecated names as well, and keep the old 'misc-flasher' name around, but deprecated.