cmake: add scope support to script mode #83045
Conversation
pillo79
force-pushed
the
pr-cmake-scope-support
branch
from
4b914b7 to
9119750
Compare
Add two new functions that abstract the implementation details of the "scoped property" retrieval, in preparation for script mode support. - zephyr_exists_scope() checks if a scope was defined; - zephyr_get_scoped() retrieves the value of a variable in a specific scope. Refactor current CMake code to use these new functions. Signed-off-by: Luca Burelli <[email protected]>
This patch adds support for scopes in CMake script mode, where the concept of targets is not defined and therefore target properties are not available. To work around this limitation, the patch uses the SOURCE specifier to emulate TARGET functionality in script mode. Since there is no if(SOURCE ...) equivalent in CMake language, a dummy property is created on the virtual source file to support the zephyr_exists_scope() functionality. Signed-off-by: Luca Burelli <[email protected]>
pillo79
force-pushed
the
pr-cmake-scope-support
branch
from
9119750 to
39cb458
Compare
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
force-pushed
the
pr-cmake-scope-support
branch
from
39cb458 to
8a6052c
Compare
zephyrbot
requested review from
57300,
jeremybettis,
nashif,
nordicjm and
tejlmand
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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.
| # 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() |
There was a problem hiding this comment.
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.
| # <output> : Output variable containing the result of the check | ||
| # <scope> : Scope to look up | ||
| # | ||
| macro(zephyr_exists_scope output scope) |
There was a problem hiding this comment.
why is this as macro and not a function ?
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. theyamlmodule) is not usable in script mode.This PR works around this limitation by attaching properties to
SOURCEentities instead, which are still available.Two additional functions are introduced to offer an unified access to the scope functionality:
zephyr_exists_scopemacro to check if a scope exists; andzephyr_get_scopedfunction 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_getAPI 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.