cmake: add scope support to script mode #83045

pillo79 · 2024-12-16T12:05:00Z

In CMake script mode, "targets" are an undefined concept, therefore no properties can be attached to them; those CMake functions are either silently ignored or result in errors. This means that zephyr_set() and any functionality that depends on it (e.g. the yaml module) is not usable in script mode.

This PR works around this limitation as #83199 by using GLOBAL PROPERTY instead of TARGET entities.
Two additional functions are introduced to offer a simplified access to the scope functionality:

a zephyr_exists_scope macro to check if a scope exists; and
a zephyr_get_scoped function to retrieve scoped property values.

These utility functions are then used in the current code to replace direct property retrieval and scope checks.

The current test suite for the zephyr_get API is modified to also run itself in script mode and detect any future issue.

This, along with #80007, is a prerequisite for the LLEXT EDK refactor in #78727.

tejlmand

Biggest concern for now is introduction of different implementation for normal vs script mode, as well as if(CMAKE_SCRIPT_MODE_FILE) starting to creep into various places in CMake code.

The script mode possibility was introduced for support package_helper, see #41302.

Of course purpose can be extended if there is a need, but from the PR description it is not exactly clear to me why we need this change.

Even with this PR, then yaml_save() is only called from <zephyr-base>/CMakeLists.txt and thus never called from the package helper.

I see some references to other llext related PRs, but even with those it is not fully clear who / why / when the yaml output is required in script mode, as twister itself doesn't seem to be needing it.

Please provide some more description of what is failing and when, and why this is problematic compared to the initial purpose of script mode, so that PR can be reviewed in that context and the extended script mode purpose is clear.

tejlmand · 2024-12-17T10:37:32Z

cmake/modules/extensions.cmake

@@ -3359,7 +3359,12 @@ function(zephyr_get_scoped output scope variable)
    message(FATAL_ERROR "zephyr_get_scoped(): scope ${scope} doesn't exists.")
  endif()

-  get_property(value TARGET ${scope}_scope PROPERTY ${variable})
+  if(CMAKE_SCRIPT_MODE_FILE)
+    set(kind SOURCE)


a big concern here is the use of SOURCE.

The SOURCE scope in CMake properties is following the current directory, now in the script mode case then add_subdirectory() cannot be used, meaning the will not be different directories and thus probablt will work,m but I still find it a problematic use, especially because the directory detail will not be clear to many users of CMake.

Thanks for pointing this out, as I was unsure about what using SOURCE implied. It definitely makes sense to add some note in each function comment block detailing that script mode is supported and if additional limitations apply.

However, in this case that limitation has no practical effect as you already identified.

tejlmand · 2024-12-17T10:39:58Z

cmake/modules/extensions.cmake

@@ -3359,7 +3359,12 @@ function(zephyr_get_scoped output scope variable)
    message(FATAL_ERROR "zephyr_get_scoped(): scope ${scope} doesn't exists.")
  endif()

-  get_property(value TARGET ${scope}_scope PROPERTY ${variable})
+  if(CMAKE_SCRIPT_MODE_FILE)


we should not have CMake script mode testing inside functions / throughout the code.

If the code is needed in script mode (thus cannot be stubbed) then it should be written so that there is only one implementation.

The rationale for different implementations is as follows:

Zephyr code heavily relies on current zephyr_set / zephyr_get semantics, so TARGET cannot be changed in project mode without a very significant effort; it would also be a breaking change downstream.

Script mode does not have TARGET support, so an alternative implementation has to be chosen to implement.

I totally agree those SCRIPT_MODE conditionals are ugly, but IMHO this is unavoidable with the current monolithic extensions.cmake module (if this was a collection of separate files it would be easier to cherry-pick and provide alternatives, but that is yet another sizable effort). I personally also think having those in each function is better than having a full alternative implementation at the bottom, because there is only one place to look for.

if this was a collection of separate files it would be easier to cherry-pick and provide alternatives, but that is yet another sizable effort

another one on my wish list, but before moving all those functions to multiple other locations / individual CMake modules, then we must have proper generated docs, else knowing where to find a given function will become even harder.

tejlmand · 2024-12-17T11:52:31Z

cmake/modules/extensions.cmake

+  # Script mode version, using SOURCE properties
+  function(zephyr_create_scope scope)
+    zephyr_exists_scope(scope_exists ${scope})
+    if(scope_exists)
+      message(FATAL_ERROR "zephyr_create_scope(${scope}) already exists.")
+    endif()
+
+    set_property(SOURCE ${scope}_scope PROPERTY ZEPHYR_SCOPE TRUE)
+  endfunction()
+
+  # Script mode version, using SOURCE properties
+  macro(zephyr_exists_scope output scope)
+    get_property(zephyr_exists_scope_value SOURCE ${scope}_scope PROPERTY ZEPHYR_SCOPE)
+    if(DEFINED zephyr_exists_scope_value)
+      unset(zephyr_exists_scope_value)
+      set(${output} TRUE)
+    else()
+      set(${output} FALSE)
+    endif()
+  endmacro()


we should not embark on a path where we have different behavior / implementation in script mode and normal mode.

Current stubbing is to silence warnings which are not required in script mode.
The script mode was introduced for package_helper, #41302, for example for twister to do a partial (and faster) run for filtering purposes, and thereby remove the need for a full CMake build generation.

As detailed in my comment above, the LLEXT EDK will become the second case where the extensions module, which is being required by yaml, has to be used in script mode. I personally would leave this last area only to actual stubs.

tejlmand · 2024-12-17T14:54:20Z

cmake/modules/extensions.cmake

+# <output>   : Output variable containing the result of the check
+# <scope>    : Scope to look up
+#
+macro(zephyr_exists_scope output scope)


why is this as macro and not a function ?

No real need, will convert to set(... PARENT_SCOPE) instead.

pillo79

from the PR description it is not exactly clear to me why we need this change.

@tejlmand I feel like I'm on a goose chase 🙂 but here's a recap of the full story:

In September I have proposed PR llext-edk: rework, add Python export and more optional information #78727, that limited the rework to the EDK files and the call site in the main CMakelists.txt. This was blocked twice, the last time in favor of using the just-introduced YAML support. That lacked a way to expand generator expressions, which is extremely important for the EDK. So I worked on that.
I have been working since October to add genex support to the YAML module in PR cmake: support generator expression in yaml module #80007, before reworking the above PR to use it. As you well know, the yaml module heavily depends on the scope functionality in zephyr_set, and since I now need to use it from llext-edk.cmake (which is called in script mode), I need to also add that support. So I worked on that.
I submitted this PR as the third attempt at moving forward with something, mentioning the relevant threads, and yet this was blocked again because -among other things- there is no obvious need for it.

I do have a working implementation here, of course. I just pushed an update to the YAML PR yesterday, and I was rewording that PR's description at the time you commented here. Updating production-quality commits and rationales for 3 cascading PRs on every change takes me some time... 😰

I sincerely appreciate your reviews and insightful comments 🙇, and try to apply all of them. In this instance however I really feel it's unfair to dismiss this PR without providing any guidance on how you think the issue should be properly addressed.

Do you see any alternative to adding script mode support for zephyr_set et al, to be able to use the yaml module in the LLEXT EDK?
If we agree on adding this functionality, modifying the existing functions to use SOURCE and TARGET in the appropriate contexts minimizes code and semantic changes IMO. Do you have any advice on how else to approach this?

Thanks in advance for your time, but please advise how to proceed.

pillo79 · 2024-12-17T16:22:26Z

cmake/modules/extensions.cmake

+# <output>   : Output variable containing the result of the check
+# <scope>    : Scope to look up
+#
+macro(zephyr_exists_scope output scope)


No real need, will convert to set(... PARENT_SCOPE) instead.

pillo79 · 2024-12-17T16:37:36Z

cmake/modules/extensions.cmake

@@ -3359,7 +3359,12 @@ function(zephyr_get_scoped output scope variable)
    message(FATAL_ERROR "zephyr_get_scoped(): scope ${scope} doesn't exists.")
  endif()

-  get_property(value TARGET ${scope}_scope PROPERTY ${variable})
+  if(CMAKE_SCRIPT_MODE_FILE)


The rationale for different implementations is as follows:

Zephyr code heavily relies on current zephyr_set / zephyr_get semantics, so TARGET cannot be changed in project mode without a very significant effort; it would also be a breaking change downstream.

Script mode does not have TARGET support, so an alternative implementation has to be chosen to implement.

I totally agree those SCRIPT_MODE conditionals are ugly, but IMHO this is unavoidable with the current monolithic extensions.cmake module (if this was a collection of separate files it would be easier to cherry-pick and provide alternatives, but that is yet another sizable effort). I personally also think having those in each function is better than having a full alternative implementation at the bottom, because there is only one place to look for.

pillo79 · 2024-12-17T16:57:03Z

cmake/modules/extensions.cmake

@@ -3359,7 +3359,12 @@ function(zephyr_get_scoped output scope variable)
    message(FATAL_ERROR "zephyr_get_scoped(): scope ${scope} doesn't exists.")
  endif()

-  get_property(value TARGET ${scope}_scope PROPERTY ${variable})
+  if(CMAKE_SCRIPT_MODE_FILE)
+    set(kind SOURCE)


Thanks for pointing this out, as I was unsure about what using SOURCE implied. It definitely makes sense to add some note in each function comment block detailing that script mode is supported and if additional limitations apply.

However, in this case that limitation has no practical effect as you already identified.

pillo79 · 2024-12-18T09:48:48Z

cmake/modules/extensions.cmake

+  # Script mode version, using SOURCE properties
+  function(zephyr_create_scope scope)
+    zephyr_exists_scope(scope_exists ${scope})
+    if(scope_exists)
+      message(FATAL_ERROR "zephyr_create_scope(${scope}) already exists.")
+    endif()
+
+    set_property(SOURCE ${scope}_scope PROPERTY ZEPHYR_SCOPE TRUE)
+  endfunction()
+
+  # Script mode version, using SOURCE properties
+  macro(zephyr_exists_scope output scope)
+    get_property(zephyr_exists_scope_value SOURCE ${scope}_scope PROPERTY ZEPHYR_SCOPE)
+    if(DEFINED zephyr_exists_scope_value)
+      unset(zephyr_exists_scope_value)
+      set(${output} TRUE)
+    else()
+      set(${output} FALSE)
+    endif()
+  endmacro()


As detailed in my comment above, the LLEXT EDK will become the second case where the extensions module, which is being required by yaml, has to be used in script mode. I personally would leave this last area only to actual stubs.

tejlmand · 2024-12-18T11:09:26Z

I now need to use it from llext-edk.cmake (which is called in script mode), I need to also add that support.

That was the missing piece of info I needed, for some reason I had missed the combo that llext-edk which is now being updated to use the yaml parser is actually called in script mode. :)

Guess I was to focused on the Zephyr build system itself to remember that the yaml module itself would be re-used.

tejlmand · 2024-12-18T11:12:59Z

In this instance however I really feel it's unfair to dismiss this PR without providing any guidance on how you think the issue should be properly addressed.

Sorry, I didn't mean any offense in the nack, I think the work done is great, but to provide the best guidance I needed the actual use-case on where the script mode entered the loop.

So it was with a genuine desire to fully understand the flow and not having to review this code bty traversing the other PRs.
I could see they were connected, but not the exact connection point (llext-edk.cmake)

tejlmand · 2024-12-19T09:18:40Z

@pillo79 I made a quick draft here #83199 where I switched from using targets and target properties to instead using global properties.

To still support scope creation / checking, which I think is quite important for the ability to catch mistake like scope name typos / naming refactory, then I've kept the feature of tracking created scopes.

Take a look, I think the principle used there can be merged into this work.
And a big bonus is that we avoid script mode checking as the scoping feature now works in both normal and script mode.

Feel free to take and modify #83199 anyway you like so that it suits your needs.

pillo79 · 2024-12-19T10:16:34Z

Thanks @tejlmand, I did not dare touch how zephyr_set worked 😁

I amended your commit with the zephyr_get_scoped functionality.
I also left the build_info script mode stub because the yaml module is not loaded by extensions.cmake (should it?)
Ran the cmake.zephyr_get test suite again so this should be good now!

tejlmand · 2024-12-19T10:50:36Z

I also left the build_info script mode stub because the yaml module is not loaded by extensions.cmake (should it?)

Good point. I tested my work locally with package helper and some extra local mods for build_info and discovered that we could avoid the stub in those cases 😄

But you're right, we should not load yaml module in extensions.cmake. I would defer that until next step cleanup #83045 (comment)

Targets are not available in script mode. To support the Zephyr scoping feature used by snippets and yaml module then this commit moves from using custom targets to use GLOBAL properties for scopes. A scope property is prefixed with `<scope>:<property>` to avoid naming collisions. A `scope:<scope-name>` global property is used to track created scopes. Tracking valid scopes ensure that properties are only set on known scopes and thus catches typos / naming errors. Add zephyr_scope_exists() and zephyr_get_scoped() to abstract the implementation details of the scoped property retrieval and refactor current code to use them. Signed-off-by: Torsten Rasmussen <[email protected]> Signed-off-by: Luca Burelli <[email protected]>

Re-run the zephyr_get() testsuite in script mode after the project mode testsuite has been executed. This is to ensure that the zephyr_get() function works correctly in script mode as well. Signed-off-by: Luca Burelli <[email protected]>

pillo79 · 2024-12-19T12:58:59Z

this should be good now!

^-- only slightly less effective than "what can possibly go wrong"
💣

Fixed an issue with APPEND property to zephyr_set.
EDIT: also rebased to use the same PR base as #80007 - the only change is this line.

tejlmand

Nice catch on the argument order swap 🙈

pillo79 force-pushed the pr-cmake-scope-support branch 3 times, most recently from 39cb458 to 8a6052c Compare December 16, 2024 16:45

pillo79 marked this pull request as ready for review December 17, 2024 08:54

zephyrbot added the area: Build System label Dec 17, 2024

zephyrbot requested review from 57300, jeremybettis, nashif, nordicjm and tejlmand December 17, 2024 08:55

zephyrbot assigned tejlmand Dec 17, 2024

tejlmand requested changes Dec 17, 2024

View reviewed changes

pillo79 commented Dec 18, 2024

View reviewed changes

pillo79 force-pushed the pr-cmake-scope-support branch from 8a6052c to 5ab8546 Compare December 19, 2024 10:02

tejlmand previously approved these changes Dec 19, 2024

View reviewed changes

tejlmand and others added 2 commits December 19, 2024 13:54

pillo79 dismissed tejlmand’s stale review via 40a7224 December 19, 2024 12:55

pillo79 force-pushed the pr-cmake-scope-support branch from 5ab8546 to 40a7224 Compare December 19, 2024 12:55

tejlmand approved these changes Dec 20, 2024

View reviewed changes

pillo79 mentioned this pull request Dec 20, 2024

cmake: support generator expression in yaml module #80007

Open

kartben approved these changes Dec 26, 2024

View reviewed changes

kartben merged commit d492c84 into zephyrproject-rtos:main Dec 26, 2024
24 checks passed

tejlmand mentioned this pull request Jan 8, 2025

cmake: use GLOBAL property instead TARGET properties for scoping #83199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: add scope support to script mode #83045

cmake: add scope support to script mode #83045

pillo79 commented Dec 16, 2024 •

edited

Loading

tejlmand left a comment

tejlmand Dec 17, 2024

pillo79 Dec 17, 2024

tejlmand Dec 17, 2024

pillo79 Dec 17, 2024

tejlmand Dec 18, 2024

tejlmand Dec 17, 2024

pillo79 Dec 18, 2024

tejlmand Dec 17, 2024

pillo79 Dec 17, 2024

pillo79 left a comment

pillo79 Dec 17, 2024

pillo79 Dec 17, 2024

pillo79 Dec 17, 2024

pillo79 Dec 18, 2024

tejlmand commented Dec 18, 2024

tejlmand commented Dec 18, 2024

tejlmand commented Dec 19, 2024 •

edited

Loading

pillo79 commented Dec 19, 2024

tejlmand commented Dec 19, 2024

pillo79 commented Dec 19, 2024 •

edited

Loading

tejlmand left a comment

cmake: add scope support to script mode #83045

cmake: add scope support to script mode #83045

Conversation

pillo79 commented Dec 16, 2024 • edited Loading

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pillo79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand commented Dec 18, 2024

tejlmand commented Dec 18, 2024

tejlmand commented Dec 19, 2024 • edited Loading

pillo79 commented Dec 19, 2024

tejlmand commented Dec 19, 2024

pillo79 commented Dec 19, 2024 • edited Loading

tejlmand left a comment

Choose a reason for hiding this comment

pillo79 commented Dec 16, 2024 •

edited

Loading

tejlmand commented Dec 19, 2024 •

edited

Loading

pillo79 commented Dec 19, 2024 •

edited

Loading