sysbuild: Add support for snippets

[nordicjm]

Adds support for sysbuild loading snippets, these can be included by using e.g.: cmake ... -DSB_SNIPPET=blah

Fixes #73250

[tejlmand]

I see the need for extending the snippet functionality to cover sysbuild, but I think we should look into how snippets themselves can be extended to also support sysbuild.

For example by supporting a sysbuild field.
A loose thought could be:

name: foo
append:
  EXTRA_DTC_OVERLAY_FILE: foo.overlay # Setting still goes to main app.
sysbuild:
  append:
    SB_EXTRA_CONF_FILE: sysbuild_foo.conf  # Setting goes to sysbuild.

this could further allow sysbuild section in the snippet to pass additional settings to images when sysbuild is used, for example:

name: foo
append:
  EXTRA_DTC_OVERLAY_FILE: foo.overlay # Setting still goes to main app.
sysbuild:
  append:
    SB_EXTRA_CONF_FILE: sysbuild_foo.conf  # Setting goes to sysbuild.
  images:
    - name: bar
      append:
          EXTRA_CONF_FILE: bar_foo.conf  # Setting goes to bar image, when using sysbuild.

all of this can of course still be applied on a per-board basis as allowed in snippets today.

A benefit of such approach is that a common snippet name can be used and applied with -DSNIPPET=<snippet> or -S <snippet> when using sysbuild with west.

Has such approach been considered ?

[nordicjm]

Has such approach been considered ?

It was the first method I tried, it did not work because you need to validate the file and it failed

[tejlmand]

It was the first method I tried, it did not work because you need to validate the file and it failed

which validation failed ?
I assume you already updated the snippets schema, so curious what you mean by failed.

[nordicjm]

It was the first method I tried, it did not work because you need to validate the file and it failed

which validation failed ? I assume you already updated the snippets schema, so curious what you mean by failed.

I don't remember, it was 2 weeks ago

[tejlmand]

It was the first method I tried, it did not work because you need to validate the file and it failed

which validation failed ? I assume you already updated the snippets schema, so curious what you mean by failed.

I don't remember, it was 2 weeks ago

If it was the first method you tried, then it sounds like you also consider it to be a better approach.
I believe that if we want to extend snippets to sysbuild, then it should be done in a way so that snippets writers and users don't have to consider: is this snippet for an image or for sysbuild.

So please try to post the old code, if lucky you have it in git reflog, unless you never did a local commit.
Then we can work together to solve the problems you see in initial approach.
I would much prefer such approach instead of just accepting this proposal without a clearer reason as to what was not working.

[nordicjm]

They changes were never commited

[nordicjm]

The snippets are applied to a specific image, snippets do not know what sysbuild is, it just knows what a non-specific image is, and if a snippet is used on an image, applies those variables to it in addition, therefore SB_* variables cannot be used

[tejlmand]

it just knows what a non-specific image is, and if a snippet is used on an image, applies those variables to it in addition

actually it doesn't even know that.
The snippets infrastructure simply creates a snippets_generated.cmake file with contents like:

# Create variable scope for snippets build variables
zephyr_create_scope(snippets)
...
zephyr_set(<VAR> "<val>" SCOPE snippets APPEND)
...

which in CMake is then retrieved by calling zephyr_get().

This means that snippets parsing can simply write:

zephyr_set(<image>_<VAR> "<val>" SCOPE snippets APPEND)

if the section within the snippet itself relates to a specific image.
Then zephyr_get() can handle the retrieval of image specific settings because it already knows about sysbuild and image name.
zephyr_get() is able to distinguish between main app build and image scope builds when sysbuild is used, so that principle can be expanded to cover snippets.

What should be considered is whether the sysbuild generated snippets_generated.cmake should be used by all images when -DSNIPPET=<snippet> is used, or if each image should generate its own snippets_generated.cmake.
I haven't considered all pros / cons on this part.

I think image specific SNIPPET arguments, like -Dfoo_SNIPPET=<foosnip> should be treated as the internal fields, for example EXTRA_CONF_FILE, would apply directly to foo image, just like -Dfoo_EXTRA_CONF_FILE does, which initially tells me that images should probably re-use the sysbuild generated snippets_generated.cmake to distinguish values provided by SNIPPET and <image>_SNIPPET in the CMake handling. But this can be further evaluated as part of implementation.

stale, needs re-review

[henrikbrixandersen]

LGTM

[tejlmand]

Nice work.

We should try to keep the code under cmake/modules/ free from sysbuild specific implementation and instead have such adjustments under share/sysbuild/cmake/modules.

[tejlmand]

sysbuild_SNIPPET should be SB_SNIPPET to follow same naming pattern as SB_CONF_FILE, SB_EXTRA_CONF_FILE, SB_CONFIG_<kconfig_setting>

[nordicjm]

Fixed

[tejlmand]

Two comments.

First, the code in <zephyr_base>/cmake/modules/ should be free of sysbuild specific handling.
Base code should be re-usable and sysbuild specifics should go to <zephyr_base>/share/sysbuild/cmake/modules/ in a sysbuild_snippets.cmake which can then include the snippets.cmake.

Like we do for other CMake modules.

Second comment is for opening a discussion on which behavior we want ?
This code means that if user does -DSNIPPET=foo -DSB_SNIPPET=bar, then both foo and bar will be considered by sysbuild.

Are we sure that's really the behavior we want ?
It makes it harder for users to request sysbuild to opt-out of foo.

Whereas, if SB_SNIPPET supersede values given by SNIPPET, then a user wanting

• All images to use foo and sysbuild to use bar, can do:
  -DSNIPPET=foo -DSB_SNIPPET=bar
• All images to use foo and sysbuild to use both foo and bar, can do
  -DSNIPPET=foo -DSB_SNIPPET=foo,bar

The latter also has the pros of allowing the user to define the order, foo before bar or bar before foo.
The current implementation has decided the order for the user.
In this case, always foo before bar.

[nordicjm]

Reworked to support this

[tejlmand]

if we move SNIPPET_ROOT handling into the common root handling https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/modules/root.cmake and similar for the sysbuild part https://github.com/zephyrproject-rtos/zephyr/blob/main/share/sysbuild/cmake/modules/sysbuild_root.cmake then it follows the same principle as other roots and we avoid sysbuild specifics in the common snippet module.

[nordicjm]

Fixed

[tejlmand]

perhaps SB_EXTRA_CONF_FILE ?

[nordicjm]

Fixed

[kartben]

@nordicjm this is causing failures in main, please have a look
https://github.com/zephyrproject-rtos/zephyr/actions/runs/12512656490/job/34906265875#step:11:1

[swift-tk]

@nordicjm

zephyr/tests/cmake/sysbuild_snippets/Kconfig

Line 13 in bc6eded

This option's value should be overridden by sysbuild.

Typo? The app kconfig did not get overidden.