sysbuild: Image ordering

[57300]

Set the configuration or flashing order for images:

sysbuild_add_dependencies(CONFIGURE my_sample sample_a sample_b)
sysbuild_add_dependencies(FLASH     my_sample sample_c sample_d)

Please review the doc patch for more information.

Fixes #53650

[nordicjm]

Just gave it a quick try and seems to have a problem if you order by images that are not present, i.e. no error is raised.
E.g. putting this in hello_world/sysbuild.cmake:

sysbuild_add_dependencies(IMAGE CONFIGURE hello_world loam)
sysbuild_add_dependencies(IMAGE FLASH     hello_world loam)

Configures and flashes fine like there is no error, but an error should be raised because there is no "loam" image.

[nordicjm]

Just looked at docs thus far

[nordicjm]

A neat addition for the future would be a ninja command that prints out a graphic like this for a configuration showing the flash/configuration order

[57300]

Definitely. So far, I only added a partial printout for when you get a cyclic dependency error.

[tejlmand]

Some initial comments.

If possible, I would prefer if the whole group feature could be made as an independent PR, because I think some proposals in that part of the PR could result in some more discussion.

[tejlmand]

why is this a global property and not a property on the target itself, similar to how CMake itself handles DEPENDS ?
Ref https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#properties-on-targets

As example, the LINK_DEPENDS.
Image targets can then have FLASH_DEPENDS and CONFIGURE_DEPENDS as properties.

[57300]

I used global properties to support using sysbuild_add_dependencies() on images that haven't been added yet, in which case the target wouldn't exist.

See this related comment chain: #57884 (comment) I would appreciate your input there.

[tejlmand]

I used global properties to support using sysbuild_add_dependencies() on images that haven't been added yet, in which case the target wouldn't exist.

Not a good enough reason, cause it also make the rest of the code more complex.
If dependencies are added on targets you can pass the targets directly to your topology search and each target itself will know it's dependencies.

This mean that all code related to loop your global properties and creation of edges are can simply be removed, such as:

zephyr/share/sysbuild/cmake/modules/sysbuild_extensions.cmake

Lines 551 to 566 in 5535dce

	# Generate dependency graph.
	set(V ${SIS_IMAGES})
	set(E)
	foreach(image ${V})
	set(property_name sysbuild_deps_${dependency_type}_${image})
	get_property(image_dependencies GLOBAL PROPERTY ${property_name})
	foreach(dependent ${image_dependencies})
	add_dependencies_to_graph(
	HINT "sysbuild_add_dependencies(${dependency_type} ${image} ... ${dependent} ...)"
	VERTICES_FROM ${dependent}
	VERTICES_TO ${image}
	OUTPUT_EDGES E
	)
	endforeach()
	endforeach()

zephyr/share/sysbuild/cmake/modules/sysbuild_extensions.cmake

Lines 491 to 500 in 5535dce

	macro(add_dependencies_to_graph)
	cmake_parse_arguments(DEP "" "HINT;OUTPUT_EDGES" "VERTICES_FROM;VERTICES_TO" ${ARGN})
	foreach(from ${DEP_VERTICES_FROM})
	foreach(to ${DEP_VERTICES_TO})
	list(APPEND ${DEP_OUTPUT_EDGES} ${from},${to})
	list(APPEND hint_${from}_${to} "${DEP_HINT}")
	endforeach()
	endforeach()
	endmacro()

[tejlmand]

This mean that all code related to loop your global properties and creation of edges are can simply be removed, such as:

For example, the following function will work directly on target, and thus all other surrounding code can be eliminated.

Note: ported from here:

zephyr/scripts/zephyr_module.py

Line 603 in 7457bcf

# This will do a topological sort to ensure the modules are ordered

function(topological_sort)
  cmake_parse_arguments(TS "" "RESULT;PROPERTY_NAME" "TARGETS" ${ARGN})
  
  set(dep_targets)
  set(start_targets)
  set(sorted_targets)
  
  foreach(target ${TS_TARGETS})
    get_target_property(${target}_dependencies ${target} ${TS_PROPERTY_NAME})
  
    if(${target}_dependencies)
      list(APPEND dep_targets ${target})
    else()
      list(APPEND start_targets ${target})
    endif()
  endforeach()
  
  while(TRUE)
    list(POP_FRONT start_targets node)
    list(APPEND sorted_targets ${node})
    set(to_remove)
    foreach(target ${dep_targets})
      if("${node}" IN_LIST ${target}_dependencies)
        list(REMOVE_ITEM ${target}_dependencies ${node})
        if(NOT ${target}_dependencies)
          list(APPEND start_targets ${target})
          list(APPEND to_remove ${target})
        endif()
      endif()
    endforeach()
  
    foreach(target ${to_remove})
      list(REMOVE_ITEM dep_targets ${target})
    endforeach()
    if(NOT start_targets)
      break()
    endif()
  endwhile()
  
  if(dep_targets)
    foreach(target ${dep_targets})
      get_target_property(deps ${target} ${TS_PROPERTY_NAME})
      list(JOIN deps " " deps)
      list(APPEND dep_string "${target} depends on: ${deps}")
    endforeach()
    list(JOIN dep_string "\n" dep_string)
    message(FATAL_ERROR "Unmet or cyclic dependencies in modules:\n${dep_string}")
  endif()
  
  set(${TS_RESULT} "${sorted_targets}" PARENT_SCOPE)
endfunction()

Function can easily be tested, like this:

add_custom_target(A)
add_custom_target(B)
add_custom_target(C)
add_custom_target(D)

set(IMAGES A B C D)
message("Unsorted images: ${IMAGES}")

set_property(TARGET A APPEND PROPERTY CUSTOM_DEPENDS C)
set_property(TARGET B APPEND PROPERTY CUSTOM_DEPENDS A C)
set_property(TARGET C APPEND PROPERTY CUSTOM_DEPENDS D)

topological_sort(TARGETS ${IMAGES} PROPERTY_NAME CUSTOM_DEPENDS RESULT IMAGES)
message("Sorted images  : ${IMAGES}")
message("==========================\n")

# Add cyclic dep.
set_property(TARGET C APPEND PROPERTY CUSTOM_DEPENDS A)
topological_sort(TARGETS ${IMAGES} PROPERTY_NAME CUSTOM_DEPENDS RESULT IMAGES)
message("Resorted images  : ${IMAGES}")

Result:

Unsorted images: A;B;C;D                                                                                                                                                                                
Sorted images  : D;C;A;B                                                                                                                                                                                
==========================                                                                                                                                                                              

And cyclic errors:

CMake Error at CMakeLists.txt:51 (message):                                                                                                                                                             
  Unmet or cyclic dependencies in modules:                                                                                                                                                              
                                                                                                                                                                                                        
  C depends on: D A                                                                                                                                                                                     
                                                                                                                                                                                                        
  A depends on: C                                                                                                                                                                                       
                                                                                                                                                                                                        
  B depends on: A C                                                                                                                                                                                     
Call Stack (most recent call first):                                                                                                                                                                    
  CMakeLists.txt:76 (topological_sort)       

The topological sort is very similar to your current proposal, and feel free to rework that one into use targets (or images) instead of using above proposal.

The main point is to avoid all the additional overhead / maintenance of extra global properties, processing of those properties into additional ,-separated strings and then pass this on to the sort function when everything can be done directly using target properties.

[57300]

Sorry, zephyr_topological_sort() needs to stay generic in order to support groups: #58174

Please have a look at how I extended the sysbuild_images_order() function in 314fcf8. There, I construct a single dependency graph based on both image-wise and group-wise dependencies. The target-based sorting you wrote would work nicely with image-wise dependencies only.

Thanks for the effort, though... I should have replied sooner.

[tejlmand]

Sorry, zephyr_topological_sort() needs to stay generic in order to support groups: #58174

Groups are handled outside of topological_sort() in a foreach() here:

zephyr/share/sysbuild/cmake/modules/sysbuild_extensions.cmake

Line 680 in 2b791a1

foreach(group ${V})

zephyr/share/sysbuild/cmake/modules/sysbuild_extensions.cmake

Line 816 in 2b791a1

foreach(group ${SIS_GROUPS})

so I don't see the relevance in relation to the groups PR.

Instead, the use of ,-separated strings just makes it more complex to use the topological sort function, as every caller must now generate a list of such strings.

A core part of CMake are targets and handling of those, so sticking to targets and properties will remove need for construction of ,-separated strings.

And groups are already defined as targets: https://github.com/zephyrproject-rtos/zephyr/blob/2b791a1eb2dc489001b6bc244504dadd726e750f/share/sysbuild/cmake/modules/sysbuild_extensions.cmake#LL433C5-L433C22

[57300]

I guess I could use target-based sorting after all, if I rewrite the loop to recreate group-wise dependencies among image targets, then sort the image targets. My only concern now is losing information about where each dependency originates for error reporting, but I'll figure it out.

Alright then, moving to draft.

[57300]

I decided to copy your target-based toposort and credit you as the author, if you don't mind. Thanks again for the effort!

[tejlmand]

be very careful on using macros.

Especially when combining it with cmake_parse_arguments() where you use: ARGN.

In a function, ARGN, ARGC, ARGV and ARGV0, ARGV1, ... are true variables in the usual CMake sense. In a macro, they are not, they are string replacements much like the C preprocessor would do with a macro.

Ref: https://cmake.org/cmake/help/latest/command/macro.html

[57300]

Thanks for the heads up!

Does it make a difference in this case? From what I can see, I shouldn't use something like if(ARGC GREATER 2) because ARGC is not a variable. Here, I'm using ${ARGN} instead of ARGN, so that should be fine, right?

[tejlmand]

are IMAGES_CONFIGURATION_ORDER and IMAGES_FLASHING_ORDER still to be considered global variables ?

[57300]

Sure, why not? IMAGES_FLASHING_ORDER is reused in domains.cmake.

[tejlmand]

Sure, why not?

because IMAGES_CONFIGURATION_ORDER doesn't seem to be used as a global variable, it is being (correctly) passed as argument to the sysbuild_module_call(PRE_CMAKE ...) function.
Secondly, the IMAGES_CONFIGURATION_ORDER is correctly used for the foreach() loop, but we don't promise a specific image order being passed to modules, ref: https://docs.zephyrproject.org/latest/develop/modules.html#sysbuild-modules-hooks

Meaning users should not start to depend on a given order.
Unless there is a strong use-case for guaranteeing a specific image order for sysbuild_module_call(...) calls, I would prefer we don't start to give such gurantees as it might put restrictions on restructuring sysbuild code.

Also correct that IMAGES_FLASHING_ORDER is used by domains.cmake, but why doesn't domains.cmake do the sorting itself ?

So far I see no specific argument as to why sysbuild_module_call(PRE_DOMAINS ...) needs the images to be ordered.

[tejlmand]

as followup, is this the order we want to execute ?

zephyr/share/sysbuild/CMakeLists.txt

Lines 118 to 122 in e7b66b5

	sysbuild_images_order(IMAGES_CONFIGURATION_ORDER CONFIGURE IMAGES ${IMAGES})
	sysbuild_images_order(IMAGES_FLASHING_ORDER FLASH IMAGES ${IMAGES})
	sysbuild_module_call(PRE_CMAKE MODULES ${SYSBUILD_MODULE_NAMES} IMAGES ${IMAGES_CONFIGURATION_ORDER})
	foreach(image ${IMAGES_CONFIGURATION_ORDER})

The PRE_CMAKE and hook was introduced in order to allow modules to adjust images / image properties before CMake configure was executed for the image.

One such adjustment could be dependency adjustment, but in order to allow the hook to adjust dependencies, then the following order would be needed instead:

 sysbuild_module_call(PRE_CMAKE MODULES ${SYSBUILD_MODULE_NAMES} IMAGES ${IMAGES}) 

 sysbuild_images_order(IMAGES_CONFIGURATION_ORDER CONFIGURE IMAGES ${IMAGES}) 
 foreach(image ${IMAGES_CONFIGURATION_ORDER}) 

@nordicjm any thoughts on this ?

[57300]

Unless there is a strong use-case for guaranteeing a specific image order for sysbuild_module_call(...) calls, I would prefer we don't start to give such gurantees as it might put restrictions on restructuring sysbuild code.

That's fair. I didn't have a strong reason. It just seemed logical for PRE_<op> and POST_<op> hooks to be aware of the image order for <op>, but it's an unnecessary detail, so I will revert it.

I might still keep IMAGES_CONFIGURATION_ORDER and IMAGES_FLASHING_ORDER as global (read-only) variables, in case there is a use case for them, but they won't necessarily be set at PRE_CMAKE and PRE_DOMAINS time anymore respectively.

The PRE_CMAKE and hook was introduced in order to allow modules to adjust images / image properties before CMake configure was executed for the image.

One such adjustment could be dependency adjustment

That would make sense, and it would provide a good reason for sysbuild_add_dependencies() to start throwing errors for non-existent images, since we'd have a reliable place to call it where the set of images can no longer change.

However, I have some concerns. One of them relates to another one of your questions above:

why doesn't domains.cmake do the sorting itself ?

For simplicity, I wanted to have a single cut-off point after which it's no longer valid to add more images or update their properties. I ensured this by keeping both sysbuild_images_order() calls next to each other, right underneath the sysbuild.cmake processing loop. Since you're proposing to move them around, the result would be to have three cut-off points for the following:

1. When you're allowed to add images.
2. When you're allowed to add CONFIGURE dependencies.
3. When you're allowed to add FLASH dependencies.

It's not a big deal, but I'm a little worried that having multiple cut-off points would be viewed as unintuitive. Either way, it should be documented.

I will also note that I didn't really expect hooks to allow tweaking image-specific properties, because my impression was that they were mainly added to support downstream functionality like signing and such.

Another concern I have is that hooks are only available for modules and not for individual sysbuild.cmake files. Suppose I want to add an extra image and some dependencies between it and some other, conditionally added images. My ExternalZephyrProject_Add() and sysbuild_add_dependencies() calls may have to be spread out between two locations - adding the image in a sysbuild.cmake file, then putting dependencies in a module hook (where plenty of dependencies could eventually be lumped together).

It might become a mess, especially for images added in-tree. Assuming that Zephyr itself is not a module, it cannot have its own hooks, so system-wide dependencies might have to be sort of baked into sysbuild.

If we want that information to be more self-contained, then one option would be to support adding EXTRA_ZEPHYR_MODULES in sysbuild context, but that would require more work.

[tejlmand]

For simplicity, I wanted to have a single cut-off point after which it's no longer valid to add more images or update their properties.

Understandable, but that's not even the reality today.
User's can adjust any image property / cache setting until CMake configure has been invoked.
So I don't see why the image dependency should have special treatment, as it's just one setting among others.

Regarding the PRE_DOMAINS you may argue that we don't offer any special handling / properties dedicated to this.
Today the PRE/POST_DOMAINS hooks are provided to allow downstream users to perform custom tasks.
A use-case is merging / post processing of images before they are flashed.

However, if there is no technical reasons why users should be prevented from adjusting properties available, then I don't think we should introduce such limitation.

because my impression was that they were mainly added to support downstream functionality like signing and such.

That is one use-case, but we don't put restrictions as to what users want to do in those hooks.

Another concern I have is that hooks are only available for modules and not for individual sysbuild.cmake files.

Correct, because the sysbuild.cmake serves a different purpose.
There are two kind of sysbuild.cmake files.

• Board specific
• Image specific

For the board specific one, it allows a board to adjust CMake variables, add board required images, etc.
You seem to be considering the image part, and one such image could be a board which needs to have a specific hex / bin flashed, one such example could be the Raspberry Pi Pico, ref:

zephyr/modules/hal_rpi_pico/CMakeLists.txt

Lines 13 to 15 in 3104ad0

	# The Second Stage Bootloader is only linked to the app that resides
	# at 0x100. Therefore, only if the app's offset is 0x100, the second
	# stage bootloader should be compiled.

If a board specific image is depending on user controlled images, then it's probably not so board specific, but should instead be a generally defined image which is described in Kconfig.

Similar with a sample specific image. If using <sample>/sysbuild.cmake to add extra images, then it should be core images to the sample. If they are general purpose samples they should be available in the modules CMakeLists txt files.
And with relevant Kconfig settings and dependencies.

So the hooks are purposely available to Zephyr modules only to help guide users into a proper CMake / Kconfig structure.
The sysbuild.cmake files are more specific fine-tuning / specific handling as just described.

Feel free to extend / improve our documentation, as it could be valuable to make the difference / intentions clear for the sysbuild.cmake files.

Assuming that Zephyr itself is not a module, it cannot have its own hooks,

Zephyr itself is not expected to be using those hooks.
If sysbuild has special needs, then it should create a clear step, for example a built-in signing step between CMake and domains, and then one such step can provide dedicated pre/post hooks.

This is also why there is both a PRE_DOMAINS and POST_CMAKE even though POST_DOMAINS is called right after POST_CMAKE, cause that ensure users can place their processing at correct location even if more steps are added later.

we want that information to be more self-contained, then one option would be to support adding EXTRA_ZEPHYR_MODULES in sysbuild context, but that would require more work.

That is already supported in sysbuild, unless you refer to the NCS misuse of having samples adding themselves as a Zephyr module (which was never the intention of that setting), but guess that's another topic.

[57300]

Thanks for the detailed reply. What you wrote gave me more clarity on how this new feature should fit into sysbuild. Much of this info is indeed worth capturing in the documentation; I wouldn't mind looking into it on a separate occasion.

I've made it possible to add CONFIGURE deps in PRE_CMAKE and FLASH deps in PRE_DOMAINS.

If using <sample>/sysbuild.cmake to add extra images, then it should be core images to the sample. If they are general purpose samples they should be available in the modules CMakeLists txt files. And with relevant Kconfig settings and dependencies.

That works for me, because those general purpose samples from modules are added early on (and in a well-defined order, as you mentioned elsewhere), so adding dependencies to them should be easy.

The only leftover concern I had was: what if a general purpose sample had its own core image added via sysbuild.cmake? Then, adding dependencies to that second image would be trickier, since it wouldn't be added at the same time as the first image. For this reason, I chose to add another patch which adjusts the image processing order.

[tejlmand]

This should not be added here.

The existing code at those places:

zephyr/share/sysbuild/CMakeLists.txt

Line 88 in 7d898ed

list(APPEND IMAGES "${app_name}")

zephyr/share/sysbuild/bootloader/CMakeLists.txt

Line 13 in 7d898ed

set(IMAGES "mcuboot" ${IMAGES} PARENT_SCOPE)

are exactly done outside of ExternalZephyrProject_Add() in order to keep building of Zephyr based projects independent of flashing.
A use-case is where a sysbuild module / extension want to build a Zephyr based project but merge the output of such project into another image (instead of flashing said Zephyr image directly).

Also, not all images may be Zephyr project. There can be other forms of external projects which can be added to sysbuild image list. Of course flashing of such images requires that they generate a runners.yml file, but such file is quite simple.
Such a use-case could be porting of the espressif bootloader to sysbuild, ref:

zephyr/soc/xtensa/esp32s3/CMakeLists.txt

Lines 29 to 30 in 7d898ed

	ExternalProject_Add(
	EspIdfBootloader

So please keep the list of sysbuild images to flash independent of the ExternalZephyrProject_Add().

Of course this comment naturally also impact the later GROUP argument proposal, cause if ExternalZephyrProject_Add() is decoupled from sysbuild image list, then it should also be decoupled from sysbuild image groups.

[57300]

The existing code at those places (...) are exactly done outside of ExternalZephyrProject_Add() in order to keep building of Zephyr based projects independent of flashing. A use-case is where a sysbuild module / extension want to build a Zephyr based project but merge the output of such project into another image (instead of flashing said Zephyr image directly).

You mean to support building an image without flashing it? I was not aware of this and I'm pretty sure this is undocumented. I would propose specifying that as an argument to ExternalZephyrProject_Add() as well, so that the IMAGES variable can be removed.

Also, not all images may be Zephyr project. There can be other forms of external projects which can be added to sysbuild image list.

My impression was that non-Zephyr projects were not really closely tied to sysbuild. Here's the documentation:

zephyr/doc/build/sysbuild/index.rst

Lines 622 to 635 in 2f718dd

	Adding non-Zephyr applications to sysbuild
	******************************************
	You can include non-Zephyr applications in a multi-image build using the
	standard CMake module `ExternalProject`_. Please refer to the CMake
	documentation for usage details.
	When using ``ExternalProject``, the non-Zephyr application will be built as
	part of the sysbuild build invocation, but ``west flash`` or ``west debug``
	will not be aware of the application. Instead, you must manually flash and
	debug the application.
	.. _MCUboot with Zephyr: https://mcuboot.com/documentation/readme-zephyr/
	.. _ExternalProject: https://cmake.org/cmake/help/latest/module/ExternalProject.html

I wouldn't presume it necessary to populate IMAGES when using standard ExternalProject_Add(), because I could call it with BUILD_ALWAYS TRUE to ensure that it builds. For Zephyr projects, sysbuild will ignore them raise an error unless I append to IMAGES. Also, I'm not sure if there are any side effects of calling ExternalZephyrProject_Cmake() on a target not added using ExternalZephyrProject_Add(). All of this should probably be documented.

By the way, is there a reason why the espressif bootloader won't be able to use ExternalZephyrProject_Add()?

[tejlmand]

You mean to support building an image without flashing it?

Correct.

I was not aware of this and I'm pretty sure this is undocumented.

It's been discussed as part of a potential wish for systems to be able to build Zephyr samples, but decide to merge to single / fewer combined images instead of flashing each image independently. That would currently be a very advanced use, but is the reason why building and flashing is not intermixed, as that could make it harder to split / make flashing more independent of building later.
Also, it's not directly described that ExternalZephyrProject_Add() will also do flashing:

zephyr/share/sysbuild/cmake/modules/sysbuild_extensions.cmake

Lines 112 to 113 in 7e23e73

	# This function includes a Zephyr based build system into the multiimage
	# build system

But it's a very valid point that we should improve the doc details on this.
Especially after the split of adding the project and running CMake in this commit: 631fa63 it is neccesary to ensure CMake is still executed, which is also done based on the IMAGES variable.

I would propose specifying that as an argument to ExternalZephyrProject_Add() as well, so that the IMAGES variable can be removed.

Need some careful consideration. Adding an argument to ExternalZephyrProject_Add() would indicate it's the caller of the function who decides this, where in some cases it could actually be a downstream project who would like to control this.
It's a very valid discussion you're opening, and perhaps it's time to do some more consideration on this topic.

[tejlmand]

By the way, is there a reason why the espressif bootloader won't be able to use ExternalZephyrProject_Add()?

The fact that the espressif bootloader is not a Zephyr project 😉

[tejlmand]

Also, I'm not sure if there are any side effects of calling ExternalZephyrProject_Cmake() on a target not added using ExternalZephyrProject_Add().

Handled here:

zephyr/share/sysbuild/cmake/modules/sysbuild_extensions.cmake

Lines 298 to 302 in 215f180

	if(NOT TARGET ${ZCMAKE_APPLICATION})
	message(FATAL_ERROR
	"${ZCMAKE_APPLICATION} does not exists. Remember to call "
	"ExternalZephyrProject_Add(APPLICATION ${ZCMAKE_APPLICATION} ...) first."
	)

[nordicjm]

The fact that the espressif bootloader is not a Zephyr project wink

I don't believe that this is true anymore, I think it has been switched to mcuboot

[tejlmand]

I don't believe that this is true anymore, I think it has been switched to mcuboot

both MCUboot and Espressif IDF bootloaders are supported as 2nd stage bootloader for esp32.

And the support of MCUboot on esp32 still doesn't make the IDF bootloader a Zephyr project 😉

[57300]

It's a very valid discussion you're opening, and perhaps it's time to do some more consideration on this topic.

Sounds good. I'm looking forward to continuing the discussion in a separate forum.

For now, I would still like to proceed with removing the IMAGES variable, on account of the build/flash split not working right now, as pointed out above:

For Zephyr projects, sysbuild will ignore them raise an error unless I append to IMAGES.

Maybe it was broken by the commit you linked, but I haven't checked.

Also, I'm not sure if there are any side effects of calling ExternalZephyrProject_Cmake() on a target not added using ExternalZephyrProject_Add().

Handled here:

But this check is not specific enough to external Zephyr projects, is it? It just checks whether there exists a target with a given name. It could've been added using ExternalProject_Add() (which it is, under the hood) or using add_library() for that matter.

[57300]

If possible, I would prefer if the whole group feature could be made as an independent PR, because I think some proposals in that part of the PR could result in some more discussion.

No problem. I made a separate PR for the grouping feature and will address relevant comments there: #58174

It's a draft, so please add yourself as a reviewer if you're interested.

[nordicjm]

Seems OK, but there are users already requesting ways of building images without flashing them (including zephyr projects), and this seems to prevent being able to do that anymore, so would be good if there was a flag added that could be used when adding an image which would not configure it for flashing.

[57300]

Seems OK, but there are users already requesting ways of building images without flashing them (including zephyr projects), and this seems to prevent being able to do that anymore, so would be good if there was a flag added that could be used when adding an image which would not configure it for flashing.

@nordicjm I assume you're referring to the removal of IMAGES as a hindrance here. That was also brought up in this thread: #57884 (comment). Now that this PR is rebased on top of 93b1fb2, it shouldn't be a problem.

By the way, I also added an optimization for build-only images in the latest commit.

[tejlmand]

Thanks for this huge improvement.

The general impression is very positive, but a couple of must fixes, mostly to avoid surprises.

Regarding the looping itself and comments on i / j, IMAGES local var and include(), then I believe we could improve the solution even further wrt. loop counters and robustness with the following approach.

1. Have a custom property on each image identifying if it has already had its <image_source_dir>/sysbuild.cmake file processed or not.
2. Whenever a call is made to ExternalZephyrProject_Add() , then not only do we add to the global IMAGES property, we also add the image to the IMAGE property of the directory. To allow nested calls of sysbuild_add_directory() combined with regular add_directory() then we define SYSBUILD_CURRENT_SOURCE_DIR on the scope.
   This will allow fetching the IMAGES property and thus obtain all images added in SYSBUILD_CURRENT_SOURCE_DIR. Meaning all images in the current sysbuild_add_subdirectory() and loop those in a foreach() without the need for i and j counters. This should result in more robust code.

Note, such improvement is not a requirement for this PR to be approved, but can come as followup. Feel free to make direct ping for extra details.

[tejlmand]

this comment may be considered a followup comment to: #61210

The argument BUILD_ONLY for ExternalZephyrProject_Add() is very reasonable, but internally we should consider if it would be better to use property names: BUILD, and FLASH.
That way the argument BUILD_ONLY will set BUILD=True;FLASH=False, and code that needs to handle flashing later may just consider just the flash property.
That will make it easier later to extend the image properties, for example if we want to introduce SIGN, ENCRYPT, or MERGE properties.
Then each property can be set to true/false, with default being BUILD and FLASH as true.
A BUILD_ONLY would then mean BUILD=True, rest are false (unless explicit enabled).

That would allow this code to only consider FLASH property and disregard BUILD_ONLY or similar flags.

Which again would also be cleaner if we need to support pre-built images which could be identified with following properties IMPORT=True;BUILD=False;FLASH=True.
In such example the BUILD_ONLY property makes less sense.

[57300]

No objections from me, as long as this can be considered for a later PR. We can also discuss how pre-built images should be represented.

[tejlmand]

it can be considered for later PR.
Reason for raising it here was because the use of the flag in this new code made me notice it.

[57300]

Regarding the looping itself and comments on i / j, IMAGES local var and include(), then I believe we could improve the solution even further wrt. loop counters and robustness with the following approach.

1. Have a custom property on each image identifying if it has already had its <image_source_dir>/sysbuild.cmake file processed or not.

2. Whenever a call is made to ExternalZephyrProject_Add() , then not only do we add to the global IMAGES property, we also add the image to the IMAGE property of the directory. To allow nested calls of sysbuild_add_directory() combined with regular add_directory() then we define SYSBUILD_CURRENT_SOURCE_DIR on the scope.
   This will allow fetching the IMAGES property and thus obtain all images added in SYSBUILD_CURRENT_SOURCE_DIR. Meaning all images in the current sysbuild_add_subdirectory() and loop those in a foreach() without the need for i and j counters. This should result in more robust code.

Note, such improvement is not a requirement for this PR to be approved, but can come as followup. Feel free to make direct ping for extra details.

Thank you for this proposal @tejlmand. We talked about it more offline, and I chose to implement sysbuild_add_subdirectory() right away, because I think it's a great way to tackle the local variable problem that you rightfully pointed out.

The latest revision includes an updated directory structure to accommodate this function.

[nordicjm]

Is this line still needed?

[57300]

I guess so, if it's meant to control the order in which those images are built (not configured).

[tejlmand]

Thanks for this huge improvement 🎉

I understand the location of sysbuild modules processing code, but concerned that in future this location might be illogical.
Would like a reply to my comment on this before final approval, as moving this code later may have an impacted / changed behavior on Sysbuild module's processing order.

Other comments to this PR may be addressed later, as addressing those bring no restrictions to use of sysbuild.

[tejlmand]

IMAGES was named upper case because it could be used further down the tree of CMake files.

With the new approach of sysbuild_add_subdirectory() combined with the global property then we could consider to use a new (lower case) name for this variable. If so, we should probably take a closer look at domains.cmake

No need for changing in this PR, but worth considering when extending sysbuild, or if refactoring domains.cmake in future.

[tejlmand]

why this limitation ?

What we need in sysbuild is of course the source_dir, but remaining args are d/c seen from sysbuild and can simply be passed to CMake's own add_subdirectory().

Suggested change

	if(ARGC GREATER 2)
	message(FATAL_ERROR
	"sysbuild_add_subdirectory(...) called with incorrect number of arguments"
	" (expected at most 2, got ${ARGC})"
	)
	endif()

[57300]

Will remove. I like your other suggestions for this function.

[tejlmand]

removing source_dir allows you to just pass on ARGN to CMake's add_subdirectory() later.

ARGV0 can be used as source_dir.

Suggested change

	function(sysbuild_add_subdirectory source_dir)
	function(sysbuild_add_subdirectory)

[tejlmand]

Suggested change

	cmake_path(ABSOLUTE_PATH source_dir NORMALIZE OUTPUT_VARIABLE SYSBUILD_CURRENT_SOURCE_DIR)
	cmake_path(ABSOLUTE_PATH ARGV0 NORMALIZE OUTPUT_VARIABLE SYSBUILD_CURRENT_SOURCE_DIR)

[tejlmand]

Suggested change

	add_subdirectory(${source_dir} ${binary_dir})
	add_subdirectory(${ARGV})

[tejlmand]

I understand why this is moved here, as today's main benefit of Sysbuild modules is to include extra images to the build.

But as sysbuild evolves in future, then a Sysbuild module may do more / others things than image adding, in which case having this processing placed under images may seem unlogical.

I do believe we should be careful to move this under the images folder.

[57300]

Well, the only reason this is moved here is for the processing order. But you have a good point about placing this under images and what this might communicate. I don't mean to imply that the sole purpose of sysbuild modules (or sysbuild.cmake files for that matter) is to add more images.

I see this as a naming issue. The directory doesn't need to be named images, but I don't know what else to call it. It could be more along the lines of main, root, or core, but those don't sit right with me.

That directory only exists to be the outermost one included with sysbuild_add_subdirectory() - other than that, it's completely artificial. Once it's renamed, I think the sysbuild modules should stay here.

[tejlmand]

That directory only exists to be the outermost one included with sysbuild_add_subdirectory()

if that's the reason then we could also consider to just create app and boards folders with a CMakeLists.txt file in each, and then at the top-level sysbuild/CMakeLists.txt have:

# sysbuild/CMakeLists.txt
...
sysbuild_add_subdirectory(app)
sysbuild_add_subdirectory(bootloader)

# sysbuild modules loop
...
foreach(SYSBUILD_CURRENT_MODULE_NAME ${SYSBUILD_MODULE_NAMES})
  ...
  sysbuild_add_subdirectory(${SYSBUILD_CURRENT_CMAKE_DIR} ...)
endforeach()
...

sysbuild_add_subdirectory(boards)

and then remove the images folder.

But regardless of whether we do that, or find a better name for the images folder, then that is a simple refactoring and can be done after this PR has been merged.

Thanks for sharing the intentions behind the images folder.
As we are clear on the purpose of the folder and the naming issues, then let's get this PR, as the functionality is more important than the small refactoring, which can always be done.

[tejlmand]

lgtm 🎉