cmake: refactor zephyr_get_*_for_lang functions
#98377
Conversation
The 'convert_list_of_flags_to_string_of_flags' function was added back in 2017 with commit 89516fb at the time language support was added to the CMake build system. The same commit also added all users of that function; they were directly passed the result of get_property() calls, so a simple replace was sufficient. This code was later expanded with the use of generator expressions to manage delimiters and prefixes, which required replacing the ';' separator with '$<SEMICOLON>' to properly preserve the list entries. This made the conversion function invalid, since no ';' can be found in any 'zephyr_get_*' output, so the conversion function was effectively a no-op; setting a space as a delimiter is already sufficient to get the desired result since circa 2020. Remove this function and 'get_property_and_add_prefix', a macro that was made redundant by the same value-to-genex conversion. Signed-off-by: Luca Burelli <[email protected]>
pillo79
changed the title
zephyr_get_*_for lang functionszephyr_get_*_for_lang functions
zephyrbot
requested review from
57300,
jeremybettis,
nashif,
nordicjm and
tejlmand
cmake/modules/extensions.cmake
Outdated
| # - STRIP_PREFIX: Omit the compiler flag prefix (-I, -D, etc.) | ||
| # - DELIMITER <delimiter>: Specify the output delimiter to use | ||
|
|
||
| function(zephyr_get_build_prop_for_lang prop lang i) |
There was a problem hiding this comment.
I think cleaning up the current use of zephyr_get_*_for_lang.. is valuable, but if we anyway change current style, then perhaps align closer to CMake.
Like we have zephyr_string and zephyr_get, then perhaps a
zephyr_get_property(<out-var> PROPERTY <prop> LANG <C|CXX|ASM>)
alternative zephyr_get_build_property().
Optional, the _as_string could even be a flag to the function which would default the delimiter to <space> instead of $<SEMICOLON>, as to not need to remember that " " does exactly that.
There was a problem hiding this comment.
I dropped AS_STRING once I understood it could be done with existing options. Looks a lot cleaner with it though, so I'll re-add that.
cmake/modules/extensions.cmake
Outdated
| macro(generate_build_prop_wrappers item prop) | ||
| function(zephyr_get_${item}_for_lang lang i) | ||
| zephyr_get_build_prop_for_lang(${prop} ${lang} result_output_list ${ARGN}) | ||
| set(${i} ${result_output_list} PARENT_SCOPE) | ||
| endfunction() | ||
| function(zephyr_get_${item}_for_lang_as_string lang i) | ||
| zephyr_get_build_prop_for_lang(${prop} ${lang} result_output_list DELIMITER " " ${ARGN}) | ||
| set(${i} ${result_output_list} PARENT_SCOPE) | ||
| endfunction() | ||
| endmacro() |
There was a problem hiding this comment.
No, we should not wrap function definition in a macro like this.
It might make it easy for us, but it will make it impossible for ordinary developers to find out where a function like:
zephyr_get_include_directories_for_lang() and friends are defined.
Not to mention the impact this will have on tasks like this: #87083
When we have the property function in place, then let's just deprecate the old style.
There was a problem hiding this comment.
I really hate copying code that should be written once only, so this was basically the first thing I did. 😅
I did consider the issue of making the definitions opaque, so I left comments with the generated function names next to each macro invocation for easy grepping. But if #87083 performs actual indexing, I will gladly revert to the verbose sequence of commands. 👍
cmake/modules/extensions.cmake
Outdated
| # | ||
| # Options: | ||
| # - STRIP_PREFIX: Omit the compiler flag prefix (-I, -D, etc.) | ||
| # - DELIMITER <delimiter>: Specify the output delimiter to use |
There was a problem hiding this comment.
Perhaps specify that default delimiter is $<SEMICOLON> to allow the result to be used in generator expressions.
cmake/modules/extensions.cmake
Outdated
| macro(generate_build_prop_wrappers item prop) | ||
| function(zephyr_get_${item}_for_lang lang i) | ||
| zephyr_get_build_prop_for_lang(${prop} ${lang} result_output_list ${ARGN}) | ||
| set(${i} ${result_output_list} PARENT_SCOPE) | ||
| endfunction() | ||
| function(zephyr_get_${item}_for_lang_as_string lang i) | ||
| zephyr_get_build_prop_for_lang(${prop} ${lang} result_output_list DELIMITER " " ${ARGN}) | ||
| set(${i} ${result_output_list} PARENT_SCOPE) | ||
| endfunction() | ||
| endmacro() |
There was a problem hiding this comment.
I really hate copying code that should be written once only, so this was basically the first thing I did. 😅
I did consider the issue of making the definitions opaque, so I left comments with the generated function names next to each macro invocation for easy grepping. But if #87083 performs actual indexing, I will gladly revert to the verbose sequence of commands. 👍
cmake/modules/extensions.cmake
Outdated
| # - STRIP_PREFIX: Omit the compiler flag prefix (-I, -D, etc.) | ||
| # - DELIMITER <delimiter>: Specify the output delimiter to use | ||
|
|
||
| function(zephyr_get_build_prop_for_lang prop lang i) |
There was a problem hiding this comment.
I dropped AS_STRING once I understood it could be done with existing options. Looks a lot cleaner with it though, so I'll re-add that.
pillo79
force-pushed
the
pr-cmake-prop-refact
branch
2 times, most recently
from
e0fb12f to
6b4ff3f
Compare
Refactored the various 'zephyr_get_*_for_lang*' functions into a single 'zephyr_get_property' function that takes the property to get and the specific output formats as arguments. This avoids differences between each function, reduces code duplication, and makes it easier to support additional features in the future. Signed-off-by: Luca Burelli <[email protected]>
Extended the zephyr_get_property() CMake function with several
convenience options:
- AS_STRING:
Returns the result as a string rather than a list. Equivalent to
specifying DELIMITER " ".
- GENEX:
Uses generator expressions to get the property entries at build
time, for targets that are not yet fully configured.
Cannot be used with LANG. Also, CMake will complain if the
requested property includes $<COMPILE_OPTIONS:...> expressions.
- TARGET <target>:
Specifies the target to get the property from. Defaults to
"zephyr_interface" as before.
Signed-off-by: Luca Burelli <[email protected]>
pillo79
force-pushed
the
pr-cmake-prop-refact
branch
from
6b4ff3f to
2fa4231
Compare
|
|
v2, addressing all comments:
|
3 participants
Not for 4.3.
This PR unifies the implementation of the various
zephyr_get_*_for_langfunctions and their string variants with a genericzephyr_get_property, so that the same feature set is available to all wrappers. It also adds several new options that expand the possible use cases:AS_STRING: Return the result as a string rather than a list. Equivalent to specifyingDELIMITER " ".DELIMITER <delim>: Specify the output delimiter to use for the list entries. Defaults to$<SEMICOLON>.GENEX: Use generator expressions to get the property entries at build time, for targets that are not yet fully configured. Cannot be used withLANG. Also, CMake will complain if the requested property includes$<COMPILE_OPTIONS:...>expressions.LANG <C|CXX|ASM>: When set, the property value is filtered for$<COMPILE_LANGUAGE:lang>entries, and only those matching the given language are kept. Cannot be used withGENEX.PREFIX <prefix>: Specify a prefix to use for each entry in the output list.STRIP_PREFIX: Omit the prefix string, even if given byPREFIX.TARGET <target>: Specify the target to get the property from. Defaults tozephyr_interface.This work was done while experimenting with alternatives to #98348. It is no longer useful for the EDK, but I think it is still worth considering for merge.