Skip to content

scripts: west_commands: Support out-of-tree runners #83190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

pdgendt
Copy link
Contributor

@pdgendt pdgendt commented Dec 19, 2024

Add runners entry to the module schema and import the file(s) specified.

Every class that inherits from ZephyrBinaryRunner and implements all abstract methods will be discovered and added to runners list.

Add an entry to zephyr/module.yml to test:

runners:
  - file: scripts/runners.py # relative to the project's root

Example added here: zephyrproject-rtos/example-application#78

Fixes #83145

@pdgendt pdgendt requested review from kartben and nordicjm December 19, 2024 06:36
@pdgendt pdgendt force-pushed the oot-runners branch 2 times, most recently from 037bc5e to 0187bf7 Compare December 19, 2024 06:59
Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't approve directly because I would like to hear from @tejlmand first. But this sounds to me like a great solution to a long standing problem. Thank you @pdgendt!

@pdgendt
Copy link
Contributor Author

pdgendt commented Dec 19, 2024

I'd like to add some test/demo for this feature, any suggestion how to do this or where to add it?

@pdgendt
Copy link
Contributor Author

pdgendt commented Dec 20, 2024

@mbolivar I would appreciate your input on this PR

@tejlmand
Copy link
Contributor

Example added here: zephyrproject-rtos/example-application#78

Fixes #23452

This PR solves a problem, but afaict not the problem described by #23452.

Don't get me wrong, I think it's valuable to be able to define new runners out-of-tree, but the problem described in #23452, where details can be seen here: #23336 (comment) was that the app cannot define an extra runner and have that runner used by a board (by referring to it from the <app>/CMakeLists.txt).

Even with this PR, it's still the board which describes runners to use, ref: https://github.com/zephyrproject-rtos/example-application/blob/502c540b63003fc1eddcd71791ba1b2cb94d74aa/boards/vendor/custom_plank/board.cmake#L14

@henrikbrixandersen feel free to add more details.

@pdgendt
Copy link
Contributor Author

pdgendt commented Dec 20, 2024

This PR solves a problem, but afaict not the problem described by #23452.

Reading up on the issue, I does indeed not match. I've set the "fixes" to my own issue #83145

Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM code wise, but before giving a +1 I think we should try discuss the general topic raised by @pdgendt here: #83190 (comment).

@pdgendt pdgendt force-pushed the oot-runners branch 2 times, most recently from 0be4be9 to 3cad258 Compare December 20, 2024 14:49
Copy link
Contributor

@mbolivar mbolivar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to implement this long-asked-for feature.

Two things come to mind:

  1. The runners package is actually independent from west
  2. Allowing external dependencies makes at least the runners.core module API, not just shared code

If you look inside scripts/west_commands/runners, you'll see there is actually no import west going on: all of the west-specific pieces are in files like scripts/west_commands/flash.py.

I did this separation intentionally to keep the possibility open of using the runners outside of west itself, since we try to keep west 'optional' in zephyr and I think someone might want to take the runners outside of west some day.

This hints at a runners: key instead of west: runners: since they're actually not west runners -- they're zephyr runners. Make sense?

As for 2., I think it would be a good idea to add comments to the core.py file making clear it's API and that backwards compatible changes should not be allowed without following the usual deprecation policy, and updating the documentation to match, especially around here:

doc/develop/west/build-flash-debug.rst
791:   These APIs are provided for reference, but they are more "shared code" used

The core module has been stable for years so I don't think this will be a problem in practice, but I think it's important to establish now that it's being promoted for outside use.

Great work!

Add runners entry to the module schema and import the file specified.

Every class that inherits from ZephyrBinaryRunner will be discovered and
added to runners list.

Signed-off-by: Pieter De Gendt <[email protected]>
@pdgendt
Copy link
Contributor Author

pdgendt commented Dec 20, 2024

Thanks @mbolivar and @tejlmand for your comments, I've reverted to a top-level runners entry.

I also update the documentation notes to indicate the new public nature of the runners.core module.

Add an example for the `runners` section in the zephyr/module.yml file,
and a paragraph to the west "flash and debug runners" section.

Signed-off-by: Pieter De Gendt <[email protected]>
@henrikbrixandersen
Copy link
Member

Example added here: zephyrproject-rtos/example-application#78
Fixes #23452

This PR solves a problem, but afaict not the problem described by #23452.

Don't get me wrong, I think it's valuable to be able to define new runners out-of-tree, but the problem described in #23452, where details can be seen here: #23336 (comment) was that the app cannot define an extra runner and have that runner used by a board (by referring to it from the <app>/CMakeLists.txt).

Even with this PR, it's still the board which describes runners to use, ref: zephyrproject-rtos/example-application@502c540/boards/vendor/custom_plank/board.cmake#L14

@henrikbrixandersen feel free to add more details.

You are correct. Next step could be to allow board extensions to add new runners (in-tree as well as out-of-tree). Being able to add application-specific runners seems much more difficult, but at least board extension runners + this PR would solve most of my original needs as raised in #23452

@henrikbrixandersen henrikbrixandersen merged commit 751f3a9 into zephyrproject-rtos:main Jan 8, 2025
42 checks passed
@pdgendt pdgendt deleted the oot-runners branch January 8, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support out-of-tree runners
6 participants